Skip to content

feat(pci,virtio): read x86 legacy IRQ via PCI config space#19

Open
sunhaosheng wants to merge 7 commits intoarceos-org:dev_oldfrom
sunhaosheng:for-tcp-fix-pr1
Open

feat(pci,virtio): read x86 legacy IRQ via PCI config space#19
sunhaosheng wants to merge 7 commits intoarceos-org:dev_oldfrom
sunhaosheng:for-tcp-fix-pr1

Conversation

@sunhaosheng
Copy link
Copy Markdown

@sunhaosheng sunhaosheng commented Mar 10, 2026

Related Issue

Starry-OS/StarryOS#129

On x86_64, a host-side TCP client could not complete a TCP echo exchange with a server running inside the system. The server never reported an accepted connection, the client blocked indefinitely, and the server could not be terminated with Ctrl+C. See the issue for full reproduction steps.

Description

This PR fixes x86 legacy IRQ discovery for VirtIO PCI devices.

Previously, the x86 path relied on derived or hardcoded IRQ assumptions, which do not reliably match the actual platform-assigned legacy INTx routing. On x86, the correct legacy IRQ should be taken from the PCI configuration space Interrupt Line register instead.

To support this, this PR adds a small PCI config-space accessor in axdriver_pci and uses it in axdriver_virtio to fetch the IRQ from PCI config space during device probing.

Implementation

This PR includes two changes:

  • Add PciConfigAccess to axdriver_pci

    • This provides read/write access to arbitrary PCI configuration space registers through the existing ECAM/MMIO mapping.
    • It is intended as a minimal helper for drivers that need config-space fields not exposed by virtio-drivers.
  • Update axdriver_virtio::probe_pci_device() on x86

    • Accept a PciConfigAccess reference.
    • Read the legacy IRQ from PCI config offset 0x3C (Interrupt Line).
    • Reject invalid/unassigned values (0 and 0xff) on x86 instead of treating them as usable IRQs.

Additional Context

If this PR is merged, PR arceos-org/arceos#354 should be also merged to match the new axdriver_virtio::probe_pci_device() interface.

This change is based on the same fix direction used in x-kernel commit, where x86 VirtIO probing was changed to use the PCI Interrupt Line register instead of hardcoded IRQ derivation.

I kept this PR intentionally small so it can be reviewed independently from any follow-up work around MSI-X or higher-level interrupt handling.

Copy link
Copy Markdown
Contributor

@AsakuraMizu AsakuraMizu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.rs/virtio-drivers/0.8.0/virtio_drivers/transport/pci/bus/trait.ConfigurationAccess.html

So if we upgrade virtio-drivers to at least version 0.8.0, it will provide an interface for accessing the configuration, and we will no longer need to write PciConfigAccess.

There is already a proposal about upgrading virtio-drivers: #17

@guoweikang What do you think? I suggest upgrading virtio-drivers to a recent version (depends on your requirement) first and then working on this PR.

@sunhaosheng
Copy link
Copy Markdown
Author

sunhaosheng commented Mar 16, 2026

https://docs.rs/virtio-drivers/0.8.0/virtio_drivers/transport/pci/bus/trait.ConfigurationAccess.html

So if we upgrade virtio-drivers to at least version 0.8.0, it will provide an interface for accessing the configuration, and we will no longer need to write PciConfigAccess.

There is already a proposal about upgrading virtio-drivers: #17

@guoweikang What do you think? I suggest upgrading virtio-drivers to a recent version (depends on your requirement) first and then working on this PR.

Thanks, that makes sense.

This PR was implemented against virtio-drivers 0.7.x, where I added PciConfigAccess as a local workaround to read the PCI Interrupt Line register on x86. If we upgrade to virtio-drivers >= 0.8.0, I agree it would be better to reuse the upstream configuration access interface and rework this PR on top of that.

Please see this pr:
#20

Add PciConfigAccess to axdriver_pci so drivers can read arbitrary
PCI config registers, and use it in axdriver_virtio to fetch the
legacy IRQ from the Interrupt Line register on x86.

This avoids hardcoded IRQ derivation and matches the platform-assigned
legacy INTx routing.
Comment on lines +74 to +75
root: &mut PciRoot<C>,
config: &C,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I haven't think about this before in #20... These arguments are definitely redundant, and for the caller, it's hard to have a PciRoot and ConfigurationAccess at the same time (PciRoot::new takes ownership). I think we may send a PR to virtio-drivers to add a Deref<Target = C> impl for PciRoot<C>.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the configuration_access field was made public after the 0.13.0 crate was published to crates.io, a [patch.crates-io] entry pointing to the git HEAD is needed temporarily. This will be removed once a new version is released.

- Remove the redundant  parameter from ,
  since  now exposes  as a public field
  (rcore-os/virtio-drivers#233).
- Use  directly on x86_64 instead.
- Add  to .
- Bump  dependency to 0.13.0 (patch via git for the
  public  field).

#[cfg(not(target_arch = "x86_64"))]
let irq = {
#[cfg(target_arch = "loongarch64")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not read Interrupt Line on no-x86 architectures?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We intentionally only read PCI Interrupt Line on x86_64.

Although the register exists in standard PCI config space, on our non-x86 targets we do not treat it as a reliable IRQ source. Those platforms use board/host-specific INTx routing, and in our current environments the IRQ is derived from the fixed platform mapping instead of the Interrupt Line register.

///
/// If the device is recognized, returns the device type and a transport object
/// for later operations. Otherwise, returns [`None`].
pub fn probe_pci_device<H: VirtIoHal, C: ConfigurationAccess>(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of this function now includes irq, which is inconsistent with probe_mmio_device. Why not let probe_mmio_device also return the IRQ number? Or use another method to obtain the IRQ number without breaking the interface?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The irq in the return type is not introduced by this change; it has been part of probe_pci_device since commit 59a8d70 (feat(virtio,net): irq).

This patch only changes how the PCI IRQ is derived on x86_64, not the function interface itself.

If you think we should also clean up the interface inconsistency with probe_mmio_device, I can send a follow-up change for that.

@sunhaosheng sunhaosheng requested a review from AsakuraMizu April 3, 2026 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants