On Tue, Dec 14, 2021 at 10:45:17AM +0000, Rahul Singh wrote: > vpci/msix.c file will be used for arm architecture when vpci msix > support will be added to ARM, but there is x86 specific code in this > file. > > Move x86 specific code to the x86_msix.c file to make sure common code > will be used for other architecture. > > No functional change intended. > > Signed-off-by: Rahul Singh <[email protected]> > --- > xen/arch/x86/msi.c | 2 +- > xen/drivers/passthrough/amd/iommu_init.c | 1 + > xen/drivers/vpci/Makefile | 1 + > xen/drivers/vpci/msi.c | 3 +- > xen/drivers/vpci/msix.c | 134 +++++--------------- > xen/drivers/vpci/x86_msix.c | 155 +++++++++++++++++++++++
This should go into xen/arch/x86/hvm/vmsi.c there's already vPCI MSI specific code in there. > xen/include/asm-x86/msi.h | 28 ---- > xen/include/xen/msi.h | 28 ++++ > xen/include/xen/vpci.h | 21 +++ > 9 files changed, 239 insertions(+), 134 deletions(-) > create mode 100644 xen/drivers/vpci/x86_msix.c > > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c > index 5febc0ea4b..2b120f897f 100644 > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -23,7 +23,7 @@ > #include <asm/io.h> > #include <asm/smp.h> > #include <asm/desc.h> > -#include <asm/msi.h> > +#include <xen/msi.h> You likely need to move this up to the xen/ prefixed include block. > #include <asm/fixmap.h> > #include <asm/p2m.h> > #include <mach_apic.h> > diff --git a/xen/drivers/passthrough/amd/iommu_init.c > b/xen/drivers/passthrough/amd/iommu_init.c > index 559a734bda..fc385959c7 100644 > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -20,6 +20,7 @@ > #include <xen/acpi.h> > #include <xen/delay.h> > #include <xen/keyhandler.h> > +#include <xen/msi.h> > > #include "iommu.h" Might be better to replace the asm/msi.h in include in iommu.h with xen/msi.h instead (or just add the xen/msi.h include instead of replace). > > diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile > index 1a1413b93e..543c265199 100644 > --- a/xen/drivers/vpci/Makefile > +++ b/xen/drivers/vpci/Makefile > @@ -1,2 +1,3 @@ > obj-y += vpci.o header.o > obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o > +obj-$(CONFIG_X86) += x86_msix.o > diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c > index 5757a7aed2..8fc82a9b8d 100644 > --- a/xen/drivers/vpci/msi.c > +++ b/xen/drivers/vpci/msi.c > @@ -16,12 +16,11 @@ > * License along with this program; If not, see > <http://www.gnu.org/licenses/>. > */ > > +#include <xen/msi.h> > #include <xen/sched.h> > #include <xen/softirq.h> > #include <xen/vpci.h> > > -#include <asm/msi.h> > - > static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg, > void *data) > { > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c > index 846f1b8d70..7a9b02f1a5 100644 > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -17,15 +17,24 @@ > * License along with this program; If not, see > <http://www.gnu.org/licenses/>. > */ > > +#include <xen/msi.h> > #include <xen/sched.h> > #include <xen/vpci.h> > > -#include <asm/msi.h> > #include <asm/p2m.h> > > -#define VMSIX_ADDR_IN_RANGE(addr, vpci, nr) \ > - ((addr) >= vmsix_table_addr(vpci, nr) && \ > - (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr)) > +/* > + * The return value is different for the MMIO handler on ARM and x86 > + * architecture. To make the code common for both architectures create > + * generic return code with architecture dependent values. > + */ > +#ifdef CONFIG_X86 > +#define VPCI_EMUL_OKAY X86EMUL_OKAY > +#define VPCI_EMUL_RETRY X86EMUL_RETRY > +#else > +#define VPCI_EMUL_OKAY 1 > +#define VPCI_EMUL_RETRY VPCI_EMUL_OKAY > +#endif Since msix_{read/write} are no longer directly used by the MMIO handlers you might as well just return an error code (or a boolean) and let the caller translate that into the per-arch return code. > > static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg, > void *data) > @@ -138,29 +147,6 @@ static void control_write(const struct pci_dev *pdev, > unsigned int reg, > pci_conf_write16(pdev->sbdf, reg, val); > } > > -static struct vpci_msix *msix_find(const struct domain *d, unsigned long > addr) > -{ > - struct vpci_msix *msix; > - > - list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next ) > - { > - const struct vpci_bar *bars = msix->pdev->vpci->header.bars; > - unsigned int i; > - > - for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ ) > - if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled && > - VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) ) > - return msix; > - } > - > - return NULL; > -} > - > -static int msix_accept(struct vcpu *v, unsigned long addr) > -{ > - return !!msix_find(v->domain, addr); > -} > - > static bool access_allowed(const struct pci_dev *pdev, unsigned long addr, > unsigned int len) > { > @@ -182,21 +168,19 @@ static struct vpci_msix_entry *get_entry(struct > vpci_msix *msix, > return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE]; > } > > -static int msix_read(struct vcpu *v, unsigned long addr, unsigned int len, > - unsigned long *data) > +int msix_read(struct vpci_msix *msix, unsigned long addr, unsigned int len, This now requires a vpci_ prefix, since it's a global function. Plain msix_{read,write} is way to generic. > + unsigned long *data) > { > - const struct domain *d = v->domain; > - struct vpci_msix *msix = msix_find(d, addr); > const struct vpci_msix_entry *entry; > unsigned int offset; > > *data = ~0ul; > > if ( !msix ) > - return X86EMUL_RETRY; > + return VPCI_EMUL_RETRY; > > if ( !access_allowed(msix->pdev, addr, len) ) > - return X86EMUL_OKAY; > + return VPCI_EMUL_OKAY; > > if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) ) > { > @@ -210,11 +194,11 @@ static int msix_read(struct vcpu *v, unsigned long > addr, unsigned int len, > switch ( len ) > { > case 4: > - *data = readl(addr); > + *data = vpci_arch_readl(addr); Why do you need a vpci wrapper around the read/write handlers? AFAICT arm64 also has {read,write}{l,q}. And you likely want to protect the 64bit read with CONFIG_64BIT if this code is to be made available to arm32. > break; > > case 8: > - *data = readq(addr); > + *data = vpci_arch_readq(addr); > break; > > default: > @@ -222,7 +206,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, > unsigned int len, > break; > } > > - return X86EMUL_OKAY; > + return VPCI_EMUL_OKAY; > } > > spin_lock(&msix->pdev->vpci->lock); > @@ -256,22 +240,20 @@ static int msix_read(struct vcpu *v, unsigned long > addr, unsigned int len, > } > spin_unlock(&msix->pdev->vpci->lock); > > - return X86EMUL_OKAY; > + return VPCI_EMUL_OKAY; > } > > -static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len, > - unsigned long data) > +int msix_write(const struct domain *d, struct vpci_msix *msix, > + unsigned long addr, unsigned int len, unsigned long data) > { > - const struct domain *d = v->domain; > - struct vpci_msix *msix = msix_find(d, addr); > struct vpci_msix_entry *entry; > unsigned int offset; > > if ( !msix ) > - return X86EMUL_RETRY; > + return VPCI_EMUL_RETRY; > > if ( !access_allowed(msix->pdev, addr, len) ) > - return X86EMUL_OKAY; > + return VPCI_EMUL_OKAY; > > if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) ) > { > @@ -281,11 +263,11 @@ static int msix_write(struct vcpu *v, unsigned long > addr, unsigned int len, > switch ( len ) > { > case 4: > - writel(data, addr); > + vpci_arch_writel(data, addr); > break; > > case 8: > - writeq(data, addr); > + vpci_arch_writeq(data, addr); > break; > > default: > @@ -294,7 +276,7 @@ static int msix_write(struct vcpu *v, unsigned long addr, > unsigned int len, > } > } > > - return X86EMUL_OKAY; > + return VPCI_EMUL_OKAY; > } > > spin_lock(&msix->pdev->vpci->lock); > @@ -372,60 +354,7 @@ static int msix_write(struct vcpu *v, unsigned long > addr, unsigned int len, > } > spin_unlock(&msix->pdev->vpci->lock); > > - return X86EMUL_OKAY; > -} > - > -static const struct hvm_mmio_ops vpci_msix_table_ops = { > - .check = msix_accept, > - .read = msix_read, > - .write = msix_write, > -}; > - > -int vpci_make_msix_hole(const struct pci_dev *pdev) > -{ > - struct domain *d = pdev->domain; > - unsigned int i; > - > - if ( !pdev->vpci->msix ) > - return 0; > - > - /* Make sure there's a hole for the MSIX table/PBA in the p2m. */ > - for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ ) > - { > - unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i)); > - unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) + > - vmsix_table_size(pdev->vpci, i) - 1); > - > - for ( ; start <= end; start++ ) > - { > - p2m_type_t t; > - mfn_t mfn = get_gfn_query(d, start, &t); > - > - switch ( t ) > - { > - case p2m_mmio_dm: > - case p2m_invalid: > - break; > - case p2m_mmio_direct: > - if ( mfn_x(mfn) == start ) > - { > - clear_identity_p2m_entry(d, start); > - break; > - } > - /* fallthrough. */ > - default: > - put_gfn(d, start); > - gprintk(XENLOG_WARNING, > - "%pp: existing mapping (mfn: %" PRI_mfn > - "type: %d) at %#lx clobbers MSIX MMIO area\n", > - &pdev->sbdf, mfn_x(mfn), t, start); > - return -EEXIST; > - } > - put_gfn(d, start); > - } > - } > - > - return 0; > + return VPCI_EMUL_OKAY; > } > > static int init_msix(struct pci_dev *pdev) > @@ -472,11 +401,10 @@ static int init_msix(struct pci_dev *pdev) > vpci_msix_arch_init_entry(&msix->entries[i]); > } > > - if ( list_empty(&d->arch.hvm.msix_tables) ) > - register_mmio_handler(d, &vpci_msix_table_ops); > + register_msix_mmio_handler(d); > + vpci_msix_add_to_msix_table(msix, d); > > pdev->vpci->msix = msix; > - list_add(&msix->next, &d->arch.hvm.msix_tables); You could likely do the registering of the handler and the addition of the table in the same handler: vpci_msix_arch_register or similar. Thanks, Roger.
