[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/vpci: msix: move x86 specific code to x86 file
Hi Roger, Thanks for reviewing the code. > On 14 Dec 2021, at 12:37 pm, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote: > > 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 <rahul.singh@xxxxxxx> >> --- >> 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. Ok. >> 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. Ok. > >> #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). Ok. > >> >> 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. Ok. > >> >> 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. Ack. > >> + 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. I need the wrapper because {read,write}{l,q} function argument is different for ARM and x86. ARM {read,wrie}(l,q} function argument is pointer to the address whereas X86 {read,wrie}(l,q} function argument is address itself. > >> 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. Ok. Regards, Rahul > > Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |