Re: [kvm-unit-tests PATCH] x86: pci: Support unaligned register access in PCI config read/write

On Thu, Jul 30, 2026 at 1:54 PM Jim Mattson <[email protected]> wrote:
>
> On Thu, Jul 30, 2026 at 11:31 AM Irene Wang <[email protected]> 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.
> >
> > Assisted-by: Gemini:gemini-3.6-flash
> > Signed-off-by: Irene Wang <[email protected]>
> > ---
> >  lib/x86/asm/pci.h | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/x86/asm/pci.h b/lib/x86/asm/pci.h
> > index 03e55c27..09fd2372 100644
> > --- a/lib/x86/asm/pci.h
> > +++ b/lib/x86/asm/pci.h
> > @@ -9,18 +9,18 @@
> >  #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))
>
> Nit: ((0x1 << 31) | ((dev) << 8) | ((reg) & ~3)) is more future-proof.

Will fix in v2, thanks for the catch!

>
> >  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)
> >  {
> >      outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
> > -    return inw(0xCFC);
> > +    return inw(0xCFC + (reg & 3));
>
> What happens here if (reg & 3) == 3? I don't think inw(0xCFF) is legal.

Yes, you are right that inw(0xCFF) is invalid since it would attempt a 16-bit
access spanning 0xCFF and 0xD00, which falls outside the CONFIG_DATA
(0xCFC–0xCFF) window and would return corrupted data.

Looking through current kvm-unit-tests, existing usages of 16-bit PCI
accesses use naturally aligned offsets (PCI_VENDOR_ID, PCI_DEVICE_ID,
PCI_COMMAND, PCI_MSI_FLAGS), where `(reg & 3)` is always 0 or 2, so this
condition is currently never triggered.

Since crossing DWORD boundaries is also not spec-compliant (PCI Local
Bus Spec Section 6.1 requires registers to be naturally aligned), I'm
thinking it makes sense to add an assertion here (e.g.,
`assert((reg & 3) != 3)`) to catch any invalid test cases added in the
future. What do you think?

>
> >  }
> >
> >  static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
> > @@ -33,14 +33,14 @@ 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)
> >  {
> >      outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
> > -    outw(val, 0xCFC);
> > +    outw(val, 0xCFC + (reg & 3));
>
> Same as above. I don't think outw(val, 0XCFF) is legal.
>
> >  }
> >
> >  static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg,
> > --
> > 2.55.0.508.g3f0d502094-goog
> >
> >