On Thu, 14 Oct 2021, Bertrand Marquis wrote: > PCI standard is using ECAM and not MCFG which is coming from ACPI[1]. > Use ECAM/ecam instead of MCFG in common code and in new functions added > in common vpci code by this patch. > > Rename vpci_access_allowed to vpci_ecam_access_allowed and move it > from arch/x86/hvm/io.c to drivers/vpci/vpci.c. > > Create vpci_ecam_mmio_{read,write} in drivers/vpci/vpci.c that > contains the common code to perform these operations, changed > vpci_mmcfg_{read,write} accordingly to make use of these functions. > > The vpci_ecam_mmio_{read,write} functions are returning 0 on error and 1 > on success. As the x86 code was previously always returning X86EMUL_OKAY > the return code is ignored. A comment has been added in the code to show > that this is intentional. > > Those functions will be used in a following patch inside by arm vpci > implementation. > > Rename MMCFG_SBDF to ECAM_SBDF. > > Not functional change intended with this patch. > > [1] https://wiki.osdev.org/PCI_Express > > Suggested-by: Roger Pau Monné > Signed-off-by: Bertrand Marquis Everything checks out: Reviewed-by: Stefano Stabellini FYI I like the new naming scheme and I think it is the right one for us to use, but either way for me it wouldn't be a blocker for acceptance. > --- > Changes in v6: Patch added > --- > xen/arch/x86/hvm/io.c | 50 +++++--------------------------- > xen/drivers/vpci/vpci.c | 60 +++++++++++++++++++++++++++++++++++++++ > xen/include/asm-x86/pci.h | 2 +- > xen/include/xen/vpci.h | 10 +++++++ > 4 files changed, 78 insertions(+), 44 deletions(-) > > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c > index 046a8eb4ed..340b8c8b0e 100644 > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -260,20 +260,6 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr, > return CF8_ADDR_LO(cf8) | (addr & 3); > } > > -/* Do some sanity checks. */ > -static bool vpci_access_allowed(unsigned int reg, unsigned int len) > -{ > - /* Check access size. */ > - if ( len != 1 && len != 2 && len != 4 && len != 8 ) > - return false; > - > - /* Check that access is size aligned. */ > - if ( (reg & (len - 1)) ) > - return false; > - > - return true; > -} > - > /* vPCI config space IO ports handlers (0xcf8/0xcfc). */ > static bool vpci_portio_accept(const struct hvm_io_handler *handler, > const ioreq_t *p) > @@ -305,7 +291,7 @@ static int vpci_portio_read(const struct hvm_io_handler *handler, > > reg = hvm_pci_decode_addr(cf8, addr, &sbdf); > > - if ( !vpci_access_allowed(reg, size) ) > + if ( !vpci_ecam_access_allowed(reg, size) ) > return X86EMUL_OKAY; > > *data = vpci_read(sbdf, reg, size); > @@ -335,7 +321,7 @@ static int vpci_portio_write(const struct hvm_io_handler *handler, > > reg = hvm_pci_decode_addr(cf8, addr, &sbdf); > > - if ( !vpci_access_allowed(reg, size) ) > + if ( !vpci_ecam_access_allowed(reg, size) ) > return X86EMUL_OKAY; > > vpci_write(sbdf, reg, size, data); > @@ -394,7 +380,7 @@ static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg, > paddr_t addr, pci_sbdf_t *sbdf) > { > addr -= mmcfg->addr; > - sbdf->bdf = MMCFG_BDF(addr); > + sbdf->bdf = ECAM_BDF(addr); > sbdf->bus += mmcfg->start_bus; > sbdf->seg = mmcfg->segment; > > @@ -434,25 +420,8 @@ static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr, > reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf); > read_unlock(&d->arch.hvm.mmcfg_lock); > > - if ( !vpci_access_allowed(reg, len) || > - (reg + len) > PCI_CFG_SPACE_EXP_SIZE ) > - return X86EMUL_OKAY; > - > - /* > - * According to the PCIe 3.1A specification: > - * - Configuration Reads and Writes must usually be DWORD or smaller > - * in size. > - * - Because Root Complex implementations are not required to support > - * accesses to a RCRB that cross DW boundaries [...] software > - * should take care not to cause the generation of such accesses > - * when accessing a RCRB unless the Root Complex will support the > - * access. > - * Xen however supports 8byte accesses by splitting them into two > - * 4byte accesses. > - */ > - *data = vpci_read(sbdf, reg, min(4u, len)); > - if ( len == 8 ) > - *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32; > + /* Ignore return code */ > + vpci_ecam_mmio_read(sbdf, reg, len, data); > > return X86EMUL_OKAY; > } > @@ -476,13 +445,8 @@ static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr, > reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf); > read_unlock(&d->arch.hvm.mmcfg_lock); > > - if ( !vpci_access_allowed(reg, len) || > - (reg + len) > PCI_CFG_SPACE_EXP_SIZE ) > - return X86EMUL_OKAY; > - > - vpci_write(sbdf, reg, min(4u, len), data); > - if ( len == 8 ) > - vpci_write(sbdf, reg + 4, 4, data >> 32); > + /* Ignore return code */ > + vpci_ecam_mmio_write(sbdf, reg, len, data); > > return X86EMUL_OKAY; > } > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index cbd1bac7fc..c0853176d7 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -478,6 +478,66 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, > spin_unlock(&pdev->vpci->lock); > } > > +/* Helper function to check an access size and alignment on vpci space. */ > +bool vpci_ecam_access_allowed(unsigned int reg, unsigned int len) > +{ > + /* > + * Check access size. > + * > + * On arm32 or for 32bit guests on arm, 64bit accesses should be forbidden > + * but as for those platform ISV register, which gives the access size, > + * cannot have a value 3, checking this would just harden the code. > + */ > + if ( len != 1 && len != 2 && len != 4 && len != 8 ) > + return false; > + > + /* Check that access is size aligned. */ > + if ( (reg & (len - 1)) ) > + return false; > + > + return true; > +} > + > +int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > + unsigned long data) > +{ > + if ( !vpci_ecam_access_allowed(reg, len) || > + (reg + len) > PCI_CFG_SPACE_EXP_SIZE ) > + return 0; > + > + vpci_write(sbdf, reg, min(4u, len), data); > + if ( len == 8 ) > + vpci_write(sbdf, reg + 4, 4, data >> 32); > + > + return 1; > +} > + > +int vpci_ecam_mmio_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > + unsigned long *data) > +{ > + if ( !vpci_ecam_access_allowed(reg, len) || > + (reg + len) > PCI_CFG_SPACE_EXP_SIZE ) > + return 0; > + > + /* > + * According to the PCIe 3.1A specification: > + * - Configuration Reads and Writes must usually be DWORD or smaller > + * in size. > + * - Because Root Complex implementations are not required to support > + * accesses to a RCRB that cross DW boundaries [...] software > + * should take care not to cause the generation of such accesses > + * when accessing a RCRB unless the Root Complex will support the > + * access. > + * Xen however supports 8byte accesses by splitting them into two > + * 4byte accesses. > + */ > + *data = vpci_read(sbdf, reg, min(4u, len)); > + if ( len == 8 ) > + *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32; > + > + return 1; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h > index edd7c3e71a..a0df5c1279 100644 > --- a/xen/include/asm-x86/pci.h > +++ b/xen/include/asm-x86/pci.h > @@ -6,7 +6,7 @@ > #define CF8_ADDR_HI(cf8) ( ((cf8) & 0x0f000000) >> 16) > #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000)) > > -#define MMCFG_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > +#define ECAM_BDF(addr) ( ((addr) & 0x0ffff000) >> 12) > > #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \ > || id == 0x01268086 || id == 0x01028086 \ > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h > index 9f5b5d52e1..4a0c3d77c9 100644 > --- a/xen/include/xen/vpci.h > +++ b/xen/include/xen/vpci.h > @@ -208,6 +208,16 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix, > { > return entry - msix->entries; > } > + > +/* ECAM mmio read/write helpers */ > +bool vpci_ecam_access_allowed(unsigned int reg, unsigned int len); > + > +int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > + unsigned long data); > + > +int vpci_ecam_mmio_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len, > + unsigned long *data); > + > #endif /* __XEN__ */ > > #else /* !CONFIG_HAS_VPCI */ > -- > 2.25.1 > >