* [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write @ 2026-08-04 23:23 Irene Wang 2026-08-05 15:36 ` Jim Mattson 2026-08-07 22:47 ` David Matlack 0 siblings, 2 replies; 5+ messages in thread From: Irene Wang @ 2026-08-04 23:23 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: Andrew Jones, Thomas Huth, kvm, Yosry Ahmed, David Matlack, Jim Mattson, Irene Wang Per the PCI Local Bus Specification (Section 3.2.2.3.2, "Configuration Mechanism #1"), port 0xCF8 (CONFIG_ADDRESS) requires a DWORD-aligned register offset (bits [1:0] = 00b), while byte and word offsets within the DWORD must be selected via data port 0xCFC + (reg & 3). Previously, PCI_CONF1_ADDRESS did not clear bits [1:0], and 8-bit / 16-bit helpers always accessed base port 0xCFC directly. This worked in QEMU because its PCI host bridge emulation preserves unaligned bits in CONFIG_ADDRESS and uses them during CONFIG_DATA accesses. However, in strictly spec-compliant VMMs (and potentially real hardware), bits [1:0] of 0xCF8 are ignored, causing non-aligned reads/writes at port 0xCFC to erroneously target byte 0 of the DWORD. In practice, this causes pci_find_dev() to read the vendor ID twice instead of reading the vendor ID and device ID. Fix this by masking `reg` with `~3` in PCI_CONF1_ADDRESS and adding the `(reg & 3)` offset to the CONFIG_DATA port for 8-bit and 16-bit accessors, ensuring compatibility across QEMU and other VMMs. Additionally, assert that 16-bit accesses do not use an offset of 3, as reading/writing a word across DWORD boundaries (port 0xCFC + 3) is invalid per spec. Assisted-by: Gemini:gemini-3.6-flash Signed-off-by: Irene Wang <yiranirenewang@gmail.com> --- v1 -> v2: - Enclose macro parameter `dev` in parentheses in PCI_CONF1_ADDRESS. - Add assert((reg & 3) != 3) in pci_config_readw() and pci_config_writew() to guard against illegal 16-bit accesses crossing DWORD boundaries. Link to v1: https://lore.kernel.org/kvm/20260730182053.905335-1-yiranirenewang@gmail.com/ lib/x86/asm/pci.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h index 03e55c27..da0b59a3 100644 --- a/lib/x86/asm/pci.h +++ b/lib/x86/asm/pci.h @@ -9,18 +9,19 @@ #include "pci.h" #include "x86/asm/io.h" -#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | (dev << 8) | reg) +#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | ((dev) << 8) | ((reg) & ~3)) static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg) { outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - return inb(0xCFC); + return inb(0xCFC + (reg & 3)); } static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg) { + assert((reg & 3) != 3); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - return inw(0xCFC); + return inw(0xCFC + (reg & 3)); } static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg) @@ -33,14 +34,15 @@ static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg, uint8_t val) { outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - outb(val, 0xCFC); + outb(val, 0xCFC + (reg & 3)); } static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg, uint16_t val) { + assert((reg & 3) != 3); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - outw(val, 0xCFC); + outw(val, 0xCFC + (reg & 3)); } static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, -- 2.55.0.571.g244d577d93-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write 2026-08-04 23:23 [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write Irene Wang @ 2026-08-05 15:36 ` Jim Mattson 2026-08-07 22:47 ` David Matlack 1 sibling, 0 replies; 5+ messages in thread From: Jim Mattson @ 2026-08-05 15:36 UTC (permalink / raw) To: Irene Wang Cc: Paolo Bonzini, Sean Christopherson, Andrew Jones, Thomas Huth, kvm, Yosry Ahmed, David Matlack On Tue, Aug 4, 2026 at 4:28 PM Irene Wang <yiranirenewang@gmail.com> wrote: > > Per the PCI Local Bus Specification (Section 3.2.2.3.2, "Configuration > Mechanism #1"), port 0xCF8 (CONFIG_ADDRESS) requires a DWORD-aligned > register offset (bits [1:0] = 00b), while byte and word offsets within > the DWORD must be selected via data port 0xCFC + (reg & 3). > > Previously, PCI_CONF1_ADDRESS did not clear bits [1:0], and 8-bit / > 16-bit helpers always accessed base port 0xCFC directly. This worked > in QEMU because its PCI host bridge emulation preserves unaligned bits > in CONFIG_ADDRESS and uses them during CONFIG_DATA accesses. However, > in strictly spec-compliant VMMs (and potentially real hardware), bits > [1:0] of 0xCF8 are ignored, causing non-aligned reads/writes at port > 0xCFC to erroneously target byte 0 of the DWORD. In practice, this > causes pci_find_dev() to read the vendor ID twice instead of reading > the vendor ID and device ID. > > Fix this by masking `reg` with `~3` in PCI_CONF1_ADDRESS and adding > the `(reg & 3)` offset to the CONFIG_DATA port for 8-bit and 16-bit > accessors, ensuring compatibility across QEMU and other VMMs. Additionally, > assert that 16-bit accesses do not use an offset of 3, as reading/writing a > word across DWORD boundaries (port 0xCFC + 3) is invalid per spec. > > Assisted-by: Gemini:gemini-3.6-flash > Signed-off-by: Irene Wang <yiranirenewang@gmail.com> Reviewed-by: Jim Mattson <jmattson@google.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write 2026-08-04 23:23 [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write Irene Wang 2026-08-05 15:36 ` Jim Mattson @ 2026-08-07 22:47 ` David Matlack 2026-08-13 20:57 ` [kvm-unit-tests PATCH v3] " Irene Wang 1 sibling, 1 reply; 5+ messages in thread From: David Matlack @ 2026-08-07 22:47 UTC (permalink / raw) To: Irene Wang Cc: Paolo Bonzini, Sean Christopherson, Andrew Jones, Thomas Huth, kvm, Yosry Ahmed, Jim Mattson On 2026-08-04 11:23 PM, Irene Wang wrote: > Per the PCI Local Bus Specification (Section 3.2.2.3.2, "Configuration > Mechanism #1"), port 0xCF8 (CONFIG_ADDRESS) requires a DWORD-aligned > register offset (bits [1:0] = 00b), while byte and word offsets within > the DWORD must be selected via data port 0xCFC + (reg & 3). > > Previously, PCI_CONF1_ADDRESS did not clear bits [1:0], and 8-bit / > 16-bit helpers always accessed base port 0xCFC directly. This worked > in QEMU because its PCI host bridge emulation preserves unaligned bits > in CONFIG_ADDRESS and uses them during CONFIG_DATA accesses. However, > in strictly spec-compliant VMMs (and potentially real hardware), bits > [1:0] of 0xCF8 are ignored, causing non-aligned reads/writes at port > 0xCFC to erroneously target byte 0 of the DWORD. In practice, this > causes pci_find_dev() to read the vendor ID twice instead of reading > the vendor ID and device ID. > > Fix this by masking `reg` with `~3` in PCI_CONF1_ADDRESS and adding > the `(reg & 3)` offset to the CONFIG_DATA port for 8-bit and 16-bit > accessors, ensuring compatibility across QEMU and other VMMs. Additionally, > assert that 16-bit accesses do not use an offset of 3, as reading/writing a > word across DWORD boundaries (port 0xCFC + 3) is invalid per spec. > > Assisted-by: Gemini:gemini-3.6-flash > Signed-off-by: Irene Wang <yiranirenewang@gmail.com> > --- > v1 -> v2: > - Enclose macro parameter `dev` in parentheses in PCI_CONF1_ADDRESS. > - Add assert((reg & 3) != 3) in pci_config_readw() and pci_config_writew() > to guard against illegal 16-bit accesses crossing DWORD boundaries. > > Link to v1: https://lore.kernel.org/kvm/20260730182053.905335-1-yiranirenewang@gmail.com/ > > lib/x86/asm/pci.h | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h > index 03e55c27..da0b59a3 100644 > --- a/lib/x86/asm/pci.h > +++ b/lib/x86/asm/pci.h > @@ -9,18 +9,19 @@ > #include "pci.h" > #include "x86/asm/io.h" > > -#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | (dev << 8) | reg) > +#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | ((dev) << 8) | ((reg) & ~3)) A comment here would be useful. Spec-compliant host bridges (including emulations) should ignore the lower 2 bits so clearing it should technically be unnecessary. The reason we have to clear it is because QEMU is not spec-compliant and will consume those 2 bits. > > static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg) > { > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - return inb(0xCFC); > + return inb(0xCFC + (reg & 3)); > } > > static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg) > { > + assert((reg & 3) != 3); > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - return inw(0xCFC); > + return inw(0xCFC + (reg & 3)); > } > > static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg) > @@ -33,14 +34,15 @@ static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg, > uint8_t val) > { > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - outb(val, 0xCFC); > + outb(val, 0xCFC + (reg & 3)); > } > > static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg, > uint16_t val) > { > + assert((reg & 3) != 3); Should assert((reg & 3) == 0) be added to the readl/writel routines as well? > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - outw(val, 0xCFC); > + outw(val, 0xCFC + (reg & 3)); > } > > static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, > -- > 2.55.0.571.g244d577d93-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [kvm-unit-tests PATCH v3] x86: pci: Support unaligned register access in PCI config read/write 2026-08-07 22:47 ` David Matlack @ 2026-08-13 20:57 ` Irene Wang 2026-08-13 21:30 ` David Matlack 0 siblings, 1 reply; 5+ messages in thread From: Irene Wang @ 2026-08-13 20:57 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: Andrew Jones, Thomas Huth, kvm, Yosry Ahmed, David Matlack, Jim Mattson, Irene Wang Per the PCI Local Bus Specification (Section 3.2.2.3.2, "Configuration Mechanism #1"), port 0xCF8 (CONFIG_ADDRESS) requires a DWORD-aligned register offset (bits [1:0] = 00b), while byte and word offsets within the DWORD must be selected via data port 0xCFC + (reg & 3). Previously, PCI_CONF1_ADDRESS did not clear bits [1:0], and 8-bit / 16-bit helpers always accessed base port 0xCFC directly. This worked in QEMU because its PCI host bridge emulation preserves unaligned bits in CONFIG_ADDRESS and uses them during CONFIG_DATA accesses. However, in strictly spec-compliant VMMs (and potentially real hardware), bits [1:0] of 0xCF8 are ignored, causing non-aligned reads/writes at port 0xCFC to erroneously target byte 0 of the DWORD. In practice, this causes pci_find_dev() to read the vendor ID twice instead of reading the vendor ID and device ID. Fix this by masking `reg` with `~3` in PCI_CONF1_ADDRESS and adding the `(reg & 3)` offset to the CONFIG_DATA port for 8-bit and 16-bit accessors, ensuring compatibility across QEMU and other VMMs. Additionally, add an ASSERT_PCI_CONF1_VALID macro to guard against illegal accesses that cross DWORD boundaries. Assisted-by: Gemini:gemini-3.6-flash Reviewed-by: Jim Mattson <jmattson@google.com> Signed-off-by: Irene Wang <yiranirenewang@gmail.com> --- v2 -> v3: - Add comment in PCI_CONF1_ADDRESS explicitly explaining that clearing bits [1:0] prevents QEMU from double-offsetting accesses (David) - Introduce ASSERT_PCI_CONF1_VALID macro checking that access offset + access size do not read/write past 4-byte window (David) Link to v2: https://lore.kernel.org/kvm/20260804232344.2976694-2-yiranirenewang@gmail.com/ Link to v1: https://lore.kernel.org/kvm/20260730182053.905335-1-yiranirenewang@gmail.com/ --- lib/x86/asm/pci.h | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h index 03e55c27..144a4459 100644 --- a/lib/x86/asm/pci.h +++ b/lib/x86/asm/pci.h @@ -9,22 +9,35 @@ #include "pci.h" #include "x86/asm/io.h" -#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | (dev << 8) | reg) +/* + * Bits [1:0] offset into data port, not address port. Spec compliant + * host bridges ignore them in CONFIG_ADDRESS, but QEMU does not; mask them out + * here so QEMU doesn't double-offset the access. + */ +#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | ((dev) << 8) | ((reg) & ~3)) + +/* + * Ensure access offset + size stays within the 4-byte (DWORD) boundary. + */ +#define ASSERT_PCI_CONF1_VALID(reg, type) \ + assert(((reg) & 3) + sizeof(type) <= 4) static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg) { outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - return inb(0xCFC); + return inb(0xCFC + (reg & 3)); } static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg) { + ASSERT_PCI_CONF1_VALID(reg, uint16_t); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - return inw(0xCFC); + return inw(0xCFC + (reg & 3)); } static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg) { + ASSERT_PCI_CONF1_VALID(reg, uint32_t); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); return inl(0xCFC); } @@ -33,19 +46,21 @@ static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg, uint8_t val) { outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - outb(val, 0xCFC); + outb(val, 0xCFC + (reg & 3)); } static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg, uint16_t val) { + ASSERT_PCI_CONF1_VALID(reg, uint16_t); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); - outw(val, 0xCFC); + outw(val, 0xCFC + (reg & 3)); } static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val) { + ASSERT_PCI_CONF1_VALID(reg, uint32_t); outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); outl(val, 0xCFC); } -- 2.55.0.691.gc56d675ccc-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH v3] x86: pci: Support unaligned register access in PCI config read/write 2026-08-13 20:57 ` [kvm-unit-tests PATCH v3] " Irene Wang @ 2026-08-13 21:30 ` David Matlack 0 siblings, 0 replies; 5+ messages in thread From: David Matlack @ 2026-08-13 21:30 UTC (permalink / raw) To: Irene Wang Cc: Paolo Bonzini, Sean Christopherson, Andrew Jones, Thomas Huth, kvm, Yosry Ahmed, Jim Mattson On 2026-08-13 08:57 PM, Irene Wang wrote: > Per the PCI Local Bus Specification (Section 3.2.2.3.2, "Configuration > Mechanism #1"), port 0xCF8 (CONFIG_ADDRESS) requires a DWORD-aligned > register offset (bits [1:0] = 00b), while byte and word offsets within > the DWORD must be selected via data port 0xCFC + (reg & 3). > > Previously, PCI_CONF1_ADDRESS did not clear bits [1:0], and 8-bit / > 16-bit helpers always accessed base port 0xCFC directly. This worked > in QEMU because its PCI host bridge emulation preserves unaligned bits > in CONFIG_ADDRESS and uses them during CONFIG_DATA accesses. However, > in strictly spec-compliant VMMs (and potentially real hardware), bits > [1:0] of 0xCF8 are ignored, causing non-aligned reads/writes at port > 0xCFC to erroneously target byte 0 of the DWORD. In practice, this > causes pci_find_dev() to read the vendor ID twice instead of reading > the vendor ID and device ID. > > Fix this by masking `reg` with `~3` in PCI_CONF1_ADDRESS and adding > the `(reg & 3)` offset to the CONFIG_DATA port for 8-bit and 16-bit > accessors, ensuring compatibility across QEMU and other VMMs. Additionally, > add an ASSERT_PCI_CONF1_VALID macro to guard against illegal accesses that > cross DWORD boundaries. > > Assisted-by: Gemini:gemini-3.6-flash > Reviewed-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Irene Wang <yiranirenewang@gmail.com> Looks good aside from the comment nits. Reviewed-by: David Matlack <dmatlack@google.com> > --- > v2 -> v3: > - Add comment in PCI_CONF1_ADDRESS explicitly explaining that clearing > bits [1:0] prevents QEMU from double-offsetting accesses (David) > - Introduce ASSERT_PCI_CONF1_VALID macro checking that access offset + > access size do not read/write past 4-byte window (David) > > Link to v2: https://lore.kernel.org/kvm/20260804232344.2976694-2-yiranirenewang@gmail.com/ > Link to v1: https://lore.kernel.org/kvm/20260730182053.905335-1-yiranirenewang@gmail.com/ > --- > lib/x86/asm/pci.h | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h > index 03e55c27..144a4459 100644 > --- a/lib/x86/asm/pci.h > +++ b/lib/x86/asm/pci.h > @@ -9,22 +9,35 @@ > #include "pci.h" > #include "x86/asm/io.h" > > -#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | (dev << 8) | reg) > +/* > + * Bits [1:0] offset into data port, not address port. Spec compliant > + * host bridges ignore them in CONFIG_ADDRESS, but QEMU does not; mask them out > + * here so QEMU doesn't double-offset the access. > + */ nit: Wrap at 80 chars. /* * Bits [1:0] offset into data port, not address port. Spec compliant host * bridges ignore them in CONFIG_ADDRESS, but QEMU does not; mask them out here * so QEMU doesn't double-offset the access. */ > +#define PCI_CONF1_ADDRESS(dev, reg) ((0x1 << 31) | ((dev) << 8) | ((reg) & ~3)) > + > +/* > + * Ensure access offset + size stays within the 4-byte (DWORD) boundary. > + */ nit: Use /* ... */ style for one-line comments. /* Ensure access offset + size stays within the 4-byte (DWORD) boundary. */ > +#define ASSERT_PCI_CONF1_VALID(reg, type) \ > + assert(((reg) & 3) + sizeof(type) <= 4) > > static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg) > { > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - return inb(0xCFC); > + return inb(0xCFC + (reg & 3)); > } > > static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg) > { > + ASSERT_PCI_CONF1_VALID(reg, uint16_t); > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - return inw(0xCFC); > + return inw(0xCFC + (reg & 3)); > } > > static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg) > { > + ASSERT_PCI_CONF1_VALID(reg, uint32_t); > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > return inl(0xCFC); > } > @@ -33,19 +46,21 @@ static inline void pci_config_writeb(pcidevaddr_t dev, uint8_t reg, > uint8_t val) > { > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - outb(val, 0xCFC); > + outb(val, 0xCFC + (reg & 3)); > } > > static inline void pci_config_writew(pcidevaddr_t dev, uint8_t reg, > uint16_t val) > { > + ASSERT_PCI_CONF1_VALID(reg, uint16_t); > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > - outw(val, 0xCFC); > + outw(val, 0xCFC + (reg & 3)); > } > > static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, > uint32_t val) > { > + ASSERT_PCI_CONF1_VALID(reg, uint32_t); > outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8); > outl(val, 0xCFC); > } > -- > 2.55.0.691.gc56d675ccc-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-13 21:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-04 23:23 [kvm-unit-tests PATCH v2] x86: pci: Support unaligned register access in PCI config read/write Irene Wang 2026-08-05 15:36 ` Jim Mattson 2026-08-07 22:47 ` David Matlack 2026-08-13 20:57 ` [kvm-unit-tests PATCH v3] " Irene Wang 2026-08-13 21:30 ` David Matlack
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox