* [PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific @ 2021-04-06 11:39 Rahul Singh 2021-04-06 11:39 ` [PATCH 1/2] xen/pci: Move PCI ATS code to common directory Rahul Singh 2021-04-06 11:39 ` [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI Rahul Singh 0 siblings, 2 replies; 14+ messages in thread From: Rahul Singh @ 2021-04-06 11:39 UTC (permalink / raw) To: xen-devel Cc: bertrand.marquis, rahul.singh, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné, Daniel De Graaf This patch series is preparatory work to implement the PCI passthrough support for the ARM architecture. Rahul Singh (2): xen/pci: Move PCI ATS code to common directory xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI xen/drivers/passthrough/Makefile | 1 + xen/drivers/passthrough/{x86 => }/ats.c | 2 +- xen/drivers/passthrough/pci.c | 17 +++++++++++++++++ xen/drivers/passthrough/x86/Makefile | 1 - xen/drivers/pci/Kconfig | 4 ++++ xen/drivers/vpci/Makefile | 3 ++- xen/drivers/vpci/header.c | 6 ++++++ xen/drivers/vpci/vpci.c | 2 ++ xen/include/xen/vpci.h | 4 ++++ xen/xsm/flask/hooks.c | 6 +++--- 10 files changed, 40 insertions(+), 6 deletions(-) rename xen/drivers/passthrough/{x86 => }/ats.c (99%) -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] xen/pci: Move PCI ATS code to common directory 2021-04-06 11:39 [PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific Rahul Singh @ 2021-04-06 11:39 ` Rahul Singh 2021-04-06 15:16 ` Jan Beulich 2021-04-06 11:39 ` [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI Rahul Singh 1 sibling, 1 reply; 14+ messages in thread From: Rahul Singh @ 2021-04-06 11:39 UTC (permalink / raw) To: xen-devel; +Cc: bertrand.marquis, rahul.singh, Jan Beulich, Paul Durrant PCI ATS code is common for all architecture, move code to common directory to be usable for other architectures. No functional change intended. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/Makefile | 1 + xen/drivers/passthrough/{x86 => }/ats.c | 2 +- xen/drivers/passthrough/x86/Makefile | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) rename xen/drivers/passthrough/{x86 => }/ats.c (99%) diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile index cc646612c7..445690e3e5 100644 --- a/xen/drivers/passthrough/Makefile +++ b/xen/drivers/passthrough/Makefile @@ -6,3 +6,4 @@ obj-$(CONFIG_ARM) += arm/ obj-y += iommu.o obj-$(CONFIG_HAS_PCI) += pci.o obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o +obj-$(CONFIG_HAS_PCI) += ats.o diff --git a/xen/drivers/passthrough/x86/ats.c b/xen/drivers/passthrough/ats.c similarity index 99% rename from xen/drivers/passthrough/x86/ats.c rename to xen/drivers/passthrough/ats.c index 4628ffde45..7f7b16dc49 100644 --- a/xen/drivers/passthrough/x86/ats.c +++ b/xen/drivers/passthrough/ats.c @@ -16,7 +16,7 @@ #include <xen/sched.h> #include <xen/pci.h> #include <xen/pci_regs.h> -#include "../ats.h" +#include "ats.h" bool_t __read_mostly ats_enabled = 0; boolean_param("ats", ats_enabled); diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile index 69284a5d19..75b2885336 100644 --- a/xen/drivers/passthrough/x86/Makefile +++ b/xen/drivers/passthrough/x86/Makefile @@ -1,3 +1,2 @@ -obj-y += ats.o obj-y += iommu.o obj-$(CONFIG_HVM) += hvm.o -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] xen/pci: Move PCI ATS code to common directory 2021-04-06 11:39 ` [PATCH 1/2] xen/pci: Move PCI ATS code to common directory Rahul Singh @ 2021-04-06 15:16 ` Jan Beulich 0 siblings, 0 replies; 14+ messages in thread From: Jan Beulich @ 2021-04-06 15:16 UTC (permalink / raw) To: Rahul Singh; +Cc: bertrand.marquis, Paul Durrant, xen-devel On 06.04.2021 13:39, Rahul Singh wrote: > PCI ATS code is common for all architecture, move code to common > directory to be usable for other architectures. > > No functional change intended. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> Acked-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 11:39 [PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific Rahul Singh 2021-04-06 11:39 ` [PATCH 1/2] xen/pci: Move PCI ATS code to common directory Rahul Singh @ 2021-04-06 11:39 ` Rahul Singh 2021-04-06 14:13 ` Roger Pau Monné 1 sibling, 1 reply; 14+ messages in thread From: Rahul Singh @ 2021-04-06 11:39 UTC (permalink / raw) To: xen-devel Cc: bertrand.marquis, rahul.singh, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné, Daniel De Graaf MSI support is not implemented for ARM architecture but it is enabled for x86 architecture and referenced in common passthrough/pci.c code. Therefore introducing the new flag to gate the MSI code for ARM in common code to avoid compilation error when HAS_PCI is enabled for ARM. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/pci.c | 17 +++++++++++++++++ xen/drivers/pci/Kconfig | 4 ++++ xen/drivers/vpci/Makefile | 3 ++- xen/drivers/vpci/header.c | 6 ++++++ xen/drivers/vpci/vpci.c | 2 ++ xen/include/xen/vpci.h | 4 ++++ xen/xsm/flask/hooks.c | 6 +++--- 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 705137f8be..390b960d83 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -33,7 +33,9 @@ #include <xen/tasklet.h> #include <xen/vpci.h> #include <xsm/xsm.h> +#ifdef CONFIG_HAS_PCI_MSI #include <asm/msi.h> +#endif #include "ats.h" struct pci_seg { @@ -327,6 +329,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) *((u8*) &pdev->bus) = bus; *((u8*) &pdev->devfn) = devfn; pdev->domain = NULL; + +#ifdef CONFIG_HAS_PCI_MSI INIT_LIST_HEAD(&pdev->msi_list); pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), @@ -357,6 +361,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) pdev->msix = msix; } +#endif list_add(&pdev->alldevs_list, &pseg->alldevs_list); @@ -449,7 +454,9 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev) } list_del(&pdev->alldevs_list); +#ifdef CONFIG_HAS_PCI_MSI xfree(pdev->msix); +#endif xfree(pdev); } @@ -827,7 +834,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn) list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list ) if ( pdev->bus == bus && pdev->devfn == devfn ) { +#ifdef CONFIG_HAS_PCI_MSI pci_cleanup_msi(pdev); +#endif ret = iommu_remove_device(pdev); if ( pdev->domain ) list_del(&pdev->domain_list); @@ -1271,7 +1280,9 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev) static int _dump_pci_devices(struct pci_seg *pseg, void *arg) { struct pci_dev *pdev; +#ifdef CONFIG_HAS_PCI_MSI struct msi_desc *msi; +#endif printk("==== segment %04x ====\n", pseg->nr); @@ -1280,8 +1291,10 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg) printk("%pp - %pd - node %-3d - MSIs < ", &pdev->sbdf, pdev->domain, (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); +#ifdef CONFIG_HAS_PCI_MSI list_for_each_entry ( msi, &pdev->msi_list, list ) printk("%d ", msi->irq); +#endif printk(">\n"); } @@ -1303,12 +1316,14 @@ static int __init setup_dump_pcidevs(void) } __initcall(setup_dump_pcidevs); +#ifdef CONFIG_HAS_PCI_MSI int iommu_update_ire_from_msi( struct msi_desc *msi_desc, struct msi_msg *msg) { return iommu_intremap ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0; } +#endif static int iommu_add_device(struct pci_dev *pdev) { @@ -1429,6 +1444,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) ASSERT(pdev && (pdev->domain == hardware_domain || pdev->domain == dom_io)); +#ifdef CONFIG_HAS_PCI_MSI if ( pdev->msix ) { rc = pci_reset_msix_state(pdev); @@ -1436,6 +1452,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) goto done; msixtbl_init(d); } +#endif pdev->fault.count = 0; diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig index 7da03fa13b..695781ce3a 100644 --- a/xen/drivers/pci/Kconfig +++ b/xen/drivers/pci/Kconfig @@ -1,3 +1,7 @@ config HAS_PCI bool + +config HAS_PCI_MSI + def_bool y + depends on X86 && HAS_PCI diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile index 55d1bdfda0..1a1413b93e 100644 --- a/xen/drivers/vpci/Makefile +++ b/xen/drivers/vpci/Makefile @@ -1 +1,2 @@ -obj-y += vpci.o header.o msi.o msix.o +obj-y += vpci.o header.o +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ba9a036202..b90c345612 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, * FIXME: punching holes after the p2m has been set up might be racy for * DomU usage, needs to be revisited. */ +#ifdef CONFIG_HAS_PCI_MSI if ( map && !rom_only && vpci_make_msix_hole(pdev) ) return; +#endif for ( i = 0; i < ARRAY_SIZE(header->bars); i++ ) { @@ -206,7 +208,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) struct vpci_header *header = &pdev->vpci->header; struct rangeset *mem = rangeset_new(NULL, NULL, 0); struct pci_dev *tmp, *dev = NULL; +#ifdef CONFIG_HAS_PCI_MSI const struct vpci_msix *msix = pdev->vpci->msix; +#endif unsigned int i; int rc; @@ -243,6 +247,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) } } +#ifdef CONFIG_HAS_PCI_MSI /* Remove any MSIX regions if present. */ for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ ) { @@ -260,6 +265,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) return rc; } } +#endif /* CONFIG_HAS_PCI_MSI */ /* * Check for overlaps with other BARs. Note that only BARs that are diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index cbd1bac7fc..474eb2f0ac 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -48,8 +48,10 @@ void vpci_remove_device(struct pci_dev *pdev) xfree(r); } spin_unlock(&pdev->vpci->lock); +#ifdef CONFIG_HAS_PCI_MSI xfree(pdev->vpci->msix); xfree(pdev->vpci->msi); +#endif xfree(pdev->vpci); pdev->vpci = NULL; } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 9f5b5d52e1..d81588ba64 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -91,6 +91,7 @@ struct vpci { /* FIXME: currently there's no support for SR-IOV. */ } header; +#ifdef CONFIG_HAS_PCI_MSI /* MSI data. */ struct vpci_msi { /* Address. */ @@ -136,6 +137,7 @@ struct vpci { struct vpci_arch_msix_entry arch; } entries[]; } *msix; +#endif /* CONFIG_HAS_PCI_MSI */ #endif }; @@ -148,6 +150,7 @@ struct vpci_vcpu { }; #ifdef __XEN__ +#ifdef CONFIG_HAS_PCI_MSI void vpci_dump_msi(void); /* Make sure there's a hole in the p2m for the MSIX mmio areas. */ @@ -208,6 +211,7 @@ static inline unsigned int vmsix_entry_nr(const struct vpci_msix *msix, { return entry - msix->entries; } +#endif /* CONFIG_HAS_PCI_MSI */ #endif /* __XEN__ */ #else /* !CONFIG_HAS_VPCI */ diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 3b7313b949..df594c80a9 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -21,7 +21,7 @@ #include <xen/guest_access.h> #include <xen/xenoprof.h> #include <xen/iommu.h> -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_PCI_MSI #include <asm/msi.h> #endif #include <public/xen.h> @@ -114,7 +114,7 @@ static int get_irq_sid(int irq, u32 *sid, struct avc_audit_data *ad) } return security_irq_sid(irq, sid); } -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_PCI_MSI { struct irq_desc *desc = irq_to_desc(irq); if ( desc->msi_desc && desc->msi_desc->dev ) { @@ -868,7 +868,7 @@ static int flask_map_domain_pirq (struct domain *d) static int flask_map_domain_msi (struct domain *d, int irq, const void *data, u32 *sid, struct avc_audit_data *ad) { -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_PCI_MSI const struct msi_info *msi = data; u32 machine_bdf = (msi->seg << 16) | (msi->bus << 8) | msi->devfn; -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 11:39 ` [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI Rahul Singh @ 2021-04-06 14:13 ` Roger Pau Monné 2021-04-06 14:30 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Roger Pau Monné @ 2021-04-06 14:13 UTC (permalink / raw) To: Rahul Singh Cc: xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Daniel De Graaf On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: > MSI support is not implemented for ARM architecture but it is enabled > for x86 architecture and referenced in common passthrough/pci.c code. > > Therefore introducing the new flag to gate the MSI code for ARM in > common code to avoid compilation error when HAS_PCI is enabled for ARM. Is such option really interesting long term? IIRC PCI Express mandates MSI support, at which point I don't see much value in being able to compile out the MSI support. So while maybe helpful for Arm PCI efforts ATM, I'm not sure it warrants a Kconfig option, I would rather see Arm introduce dummy helpers for the missing functionality, even if unimplemented at the moment. Thanks, Roger. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 14:13 ` Roger Pau Monné @ 2021-04-06 14:30 ` Julien Grall 2021-04-06 14:59 ` Roger Pau Monné 2021-04-06 15:25 ` Jan Beulich 0 siblings, 2 replies; 14+ messages in thread From: Julien Grall @ 2021-04-06 14:30 UTC (permalink / raw) To: Roger Pau Monné, Rahul Singh Cc: xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf Hi Roger, On 06/04/2021 15:13, Roger Pau Monné wrote: > On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >> MSI support is not implemented for ARM architecture but it is enabled >> for x86 architecture and referenced in common passthrough/pci.c code. >> >> Therefore introducing the new flag to gate the MSI code for ARM in >> common code to avoid compilation error when HAS_PCI is enabled for ARM. > > Is such option really interesting long term? > > IIRC PCI Express mandates MSI support, at which point I don't see much > value in being able to compile out the MSI support. I am pretty sure there are board out with PCI support but no MSI support. Anyway, even if the spec may mandate it... > > So while maybe helpful for Arm PCI efforts ATM, I'm not sure it > warrants a Kconfig option, I would rather see Arm introduce dummy > helpers for the missing functionality, even if unimplemented at the > moment. ... from my understanding, most of (if not all) the MSI code is not very useful on Arm when using the GICv3 ITS. The GICv3 ITS will do the isolation for you and therefore we should not need to keep track of the state at the vPCI level. So I think we want to be able to compile out the code if not used. That said, I think providing stub would be better to avoid multiple #ifdef in the same function. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 14:30 ` Julien Grall @ 2021-04-06 14:59 ` Roger Pau Monné 2021-04-06 15:09 ` Julien Grall 2021-04-06 15:25 ` Jan Beulich 1 sibling, 1 reply; 14+ messages in thread From: Roger Pau Monné @ 2021-04-06 14:59 UTC (permalink / raw) To: Julien Grall Cc: Rahul Singh, xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf On Tue, Apr 06, 2021 at 03:30:01PM +0100, Julien Grall wrote: > Hi Roger, > > On 06/04/2021 15:13, Roger Pau Monné wrote: > > On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: > > > MSI support is not implemented for ARM architecture but it is enabled > > > for x86 architecture and referenced in common passthrough/pci.c code. > > > > > > Therefore introducing the new flag to gate the MSI code for ARM in > > > common code to avoid compilation error when HAS_PCI is enabled for ARM. > > > > Is such option really interesting long term? > > > > IIRC PCI Express mandates MSI support, at which point I don't see much > > value in being able to compile out the MSI support. > > I am pretty sure there are board out with PCI support but no MSI support. > Anyway, even if the spec may mandate it... > > > > > So while maybe helpful for Arm PCI efforts ATM, I'm not sure it > > warrants a Kconfig option, I would rather see Arm introduce dummy > > helpers for the missing functionality, even if unimplemented at the > > moment. > > ... from my understanding, most of (if not all) the MSI code is not very > useful on Arm when using the GICv3 ITS. > > The GICv3 ITS will do the isolation for you and therefore we should not need > to keep track of the state at the vPCI level. Right, but MSI has nothing to do with isolation, is just the capability to setup interrupts from PCI devices. What about systems without GICv3 ITS, is there an aim to support those also? (as from my reading of your reply those would require more auditing of the MSI accesses by the guests) > So I think we want to be able to compile out the code if not used. That > said, I think providing stub would be better to avoid multiple #ifdef in the > same function. I think providing stubs is the way to go, that should allow to remove the unneeded code without having to explicitly drop MSI support. As said before, I think it's fine to provide those unimplemented for Arm ATM, can be filled later if there's more pressing PCI work to do first. Thanks, Roger. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 14:59 ` Roger Pau Monné @ 2021-04-06 15:09 ` Julien Grall 2021-04-06 15:58 ` Roger Pau Monné 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2021-04-06 15:09 UTC (permalink / raw) To: Roger Pau Monné Cc: Rahul Singh, xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf On 06/04/2021 15:59, Roger Pau Monné wrote: > On Tue, Apr 06, 2021 at 03:30:01PM +0100, Julien Grall wrote: >> Hi Roger, >> >> On 06/04/2021 15:13, Roger Pau Monné wrote: >>> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >>>> MSI support is not implemented for ARM architecture but it is enabled >>>> for x86 architecture and referenced in common passthrough/pci.c code. >>>> >>>> Therefore introducing the new flag to gate the MSI code for ARM in >>>> common code to avoid compilation error when HAS_PCI is enabled for ARM. >>> >>> Is such option really interesting long term? >>> >>> IIRC PCI Express mandates MSI support, at which point I don't see much >>> value in being able to compile out the MSI support. >> >> I am pretty sure there are board out with PCI support but no MSI support. >> Anyway, even if the spec may mandate it... >> >>> >>> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it >>> warrants a Kconfig option, I would rather see Arm introduce dummy >>> helpers for the missing functionality, even if unimplemented at the >>> moment. >> >> ... from my understanding, most of (if not all) the MSI code is not very >> useful on Arm when using the GICv3 ITS. >> >> The GICv3 ITS will do the isolation for you and therefore we should not need >> to keep track of the state at the vPCI level. > > Right, but MSI has nothing to do with isolation, is just the > capability to setup interrupts from PCI devices. What about systems > without GICv3 ITS, is there an aim to support those also? (as from my > reading of your reply those would require more auditing of the MSI > accesses by the guests) I am not aware of any plan for them so far. > >> So I think we want to be able to compile out the code if not used. That >> said, I think providing stub would be better to avoid multiple #ifdef in the >> same function. > > I think providing stubs is the way to go, that should allow to remove > the unneeded code without having to explicitly drop MSI support. > As > said before, I think it's fine to provide those unimplemented for Arm > ATM, can be filled later if there's more pressing PCI work to do > first. We should remove unneeded and *avoid* allocation. Providing stub for existing functions will only address the first problem. For the allocation (see alloc_pdev()) , we will need to move it in separate function and gate them to prevent the allocation. It would be wrong to gate the code with #ifdef CONFIG_X86. So I think Rahul's idea to provide the new #ifdef is correct. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 15:09 ` Julien Grall @ 2021-04-06 15:58 ` Roger Pau Monné 2021-04-07 17:52 ` Rahul Singh 0 siblings, 1 reply; 14+ messages in thread From: Roger Pau Monné @ 2021-04-06 15:58 UTC (permalink / raw) To: Julien Grall Cc: Rahul Singh, xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf On Tue, Apr 06, 2021 at 04:09:34PM +0100, Julien Grall wrote: > > > On 06/04/2021 15:59, Roger Pau Monné wrote: > > On Tue, Apr 06, 2021 at 03:30:01PM +0100, Julien Grall wrote: > > > So I think we want to be able to compile out the code if not used. That > > > said, I think providing stub would be better to avoid multiple #ifdef in the > > > same function. > > > > I think providing stubs is the way to go, that should allow to remove > > the unneeded code without having to explicitly drop MSI support. As > > said before, I think it's fine to provide those unimplemented for Arm > > ATM, can be filled later if there's more pressing PCI work to do > > first. > > We should remove unneeded and *avoid* allocation. Providing stub for > existing functions will only address the first problem. > > For the allocation (see alloc_pdev()) , we will need to move it in separate > function and gate them to prevent the allocation. > > It would be wrong to gate the code with #ifdef CONFIG_X86. So I think > Rahul's idea to provide the new #ifdef is correct. I think all this needs to be in the commit message then, because from my reading of the current message it seems like MSI code is only removed because MSI support is not implemented on Arm, rather than Arm not requiring such strict tracking of MSI accesses and MSI interrupt setup. Likely the naming of the option needs to be adjusted also together with the reduction of it's scope to stuff that explicitly needs to be removed in the preprocessor opposed to adding arch specific stubs. Thanks, Roger. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 15:58 ` Roger Pau Monné @ 2021-04-07 17:52 ` Rahul Singh 0 siblings, 0 replies; 14+ messages in thread From: Rahul Singh @ 2021-04-07 17:52 UTC (permalink / raw) To: Roger Pau Monné Cc: Julien Grall, xen-devel, Bertrand Marquis, Jan Beulich, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf Hi, > On 6 Apr 2021, at 4:58 pm, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Tue, Apr 06, 2021 at 04:09:34PM +0100, Julien Grall wrote: >> >> >> On 06/04/2021 15:59, Roger Pau Monné wrote: >>> On Tue, Apr 06, 2021 at 03:30:01PM +0100, Julien Grall wrote: >>>> So I think we want to be able to compile out the code if not used. That >>>> said, I think providing stub would be better to avoid multiple #ifdef in the >>>> same function. >>> >>> I think providing stubs is the way to go, that should allow to remove >>> the unneeded code without having to explicitly drop MSI support. As >>> said before, I think it's fine to provide those unimplemented for Arm >>> ATM, can be filled later if there's more pressing PCI work to do >>> first. >> >> We should remove unneeded and *avoid* allocation. Providing stub for >> existing functions will only address the first problem. >> >> For the allocation (see alloc_pdev()) , we will need to move it in separate >> function and gate them to prevent the allocation. >> >> It would be wrong to gate the code with #ifdef CONFIG_X86. So I think >> Rahul's idea to provide the new #ifdef is correct. > > I think all this needs to be in the commit message then, because from > my reading of the current message it seems like MSI code is only > removed because MSI support is not implemented on Arm, rather than Arm > not requiring such strict tracking of MSI accesses and MSI interrupt > setup. Likely the naming of the option needs to be adjusted also > together with the reduction of it's scope to stuff that explicitly > needs to be removed in the preprocessor opposed to adding arch > specific stubs. > Thanks everyone for reviewing the code. MSI related code in the "passthrough/pci.c” file is not useful for ARM when MSI interrupts are injected via GICv3 ITS and at the same time there is no plan to support MSI interrupts for ARM without GICv3 ITS that's why I thought it is ok to compile out the code with the new kconfig option. I didn’t realize that HAS_PCI_MSI option will not work for x68 if someone disables the HAS_PCI_MSI option for X86. I was under the impression that MSI functionality always be enabled for x86. I also agree that having too many #ifdef in the code is not good as it makes code harder to read. I will try to modify the code to add a stub version of the unneeded code for Arm and will send the next version of the patch. I will also modify the commit message accordingly. Regards, Rahul > > Thanks, Roger. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 14:30 ` Julien Grall 2021-04-06 14:59 ` Roger Pau Monné @ 2021-04-06 15:25 ` Jan Beulich 2021-04-07 18:06 ` Julien Grall 1 sibling, 1 reply; 14+ messages in thread From: Jan Beulich @ 2021-04-06 15:25 UTC (permalink / raw) To: Julien Grall, Rahul Singh Cc: xen-devel, bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf, Roger Pau Monné On 06.04.2021 16:30, Julien Grall wrote: > Hi Roger, > > On 06/04/2021 15:13, Roger Pau Monné wrote: >> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >>> MSI support is not implemented for ARM architecture but it is enabled >>> for x86 architecture and referenced in common passthrough/pci.c code. >>> >>> Therefore introducing the new flag to gate the MSI code for ARM in >>> common code to avoid compilation error when HAS_PCI is enabled for ARM. >> >> Is such option really interesting long term? >> >> IIRC PCI Express mandates MSI support, at which point I don't see much >> value in being able to compile out the MSI support. > > I am pretty sure there are board out with PCI support but no MSI > support. Anyway, even if the spec may mandate it... > >> >> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it >> warrants a Kconfig option, I would rather see Arm introduce dummy >> helpers for the missing functionality, even if unimplemented at the >> moment. > > ... from my understanding, most of (if not all) the MSI code is not very > useful on Arm when using the GICv3 ITS. > > The GICv3 ITS will do the isolation for you and therefore we should not > need to keep track of the state at the vPCI level. But that's then not "has PCI MSI" but "need to intercept PCI MSI accesses", i.e. I don't think the Kconfig option is correctly named. If a device with MSI support is used, you can't make that MSI support go away, after all. And of course I agree with the desire to have less #ifdef-ary here. Jan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-06 15:25 ` Jan Beulich @ 2021-04-07 18:06 ` Julien Grall 2021-04-08 6:00 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2021-04-07 18:06 UTC (permalink / raw) To: Jan Beulich, Rahul Singh Cc: xen-devel, bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf, Roger Pau Monné Hi Jan, On 06/04/2021 16:25, Jan Beulich wrote: > On 06.04.2021 16:30, Julien Grall wrote: >> Hi Roger, >> >> On 06/04/2021 15:13, Roger Pau Monné wrote: >>> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >>>> MSI support is not implemented for ARM architecture but it is enabled >>>> for x86 architecture and referenced in common passthrough/pci.c code. >>>> >>>> Therefore introducing the new flag to gate the MSI code for ARM in >>>> common code to avoid compilation error when HAS_PCI is enabled for ARM. >>> >>> Is such option really interesting long term? >>> >>> IIRC PCI Express mandates MSI support, at which point I don't see much >>> value in being able to compile out the MSI support. >> >> I am pretty sure there are board out with PCI support but no MSI >> support. Anyway, even if the spec may mandate it... >> >>> >>> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it >>> warrants a Kconfig option, I would rather see Arm introduce dummy >>> helpers for the missing functionality, even if unimplemented at the >>> moment. >> >> ... from my understanding, most of (if not all) the MSI code is not very >> useful on Arm when using the GICv3 ITS. >> >> The GICv3 ITS will do the isolation for you and therefore we should not >> need to keep track of the state at the vPCI level. > > But that's then not "has PCI MSI" but "need to intercept PCI MSI > accesses", i.e. I don't think the Kconfig option is correctly > named. If a device with MSI support is used, you can't make that > MSI support go away, after all. That's actually a good point. Rahul, do you think the config can be renamed to something like CONFIG_PCI_MSI_NEED_INTERCEPT? Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-07 18:06 ` Julien Grall @ 2021-04-08 6:00 ` Jan Beulich 2021-04-08 8:45 ` Rahul Singh 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2021-04-08 6:00 UTC (permalink / raw) To: Julien Grall, Rahul Singh Cc: xen-devel, bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf, Roger Pau Monné On 07.04.2021 20:06, Julien Grall wrote: > Hi Jan, > > On 06/04/2021 16:25, Jan Beulich wrote: >> On 06.04.2021 16:30, Julien Grall wrote: >>> Hi Roger, >>> >>> On 06/04/2021 15:13, Roger Pau Monné wrote: >>>> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >>>>> MSI support is not implemented for ARM architecture but it is enabled >>>>> for x86 architecture and referenced in common passthrough/pci.c code. >>>>> >>>>> Therefore introducing the new flag to gate the MSI code for ARM in >>>>> common code to avoid compilation error when HAS_PCI is enabled for ARM. >>>> >>>> Is such option really interesting long term? >>>> >>>> IIRC PCI Express mandates MSI support, at which point I don't see much >>>> value in being able to compile out the MSI support. >>> >>> I am pretty sure there are board out with PCI support but no MSI >>> support. Anyway, even if the spec may mandate it... >>> >>>> >>>> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it >>>> warrants a Kconfig option, I would rather see Arm introduce dummy >>>> helpers for the missing functionality, even if unimplemented at the >>>> moment. >>> >>> ... from my understanding, most of (if not all) the MSI code is not very >>> useful on Arm when using the GICv3 ITS. >>> >>> The GICv3 ITS will do the isolation for you and therefore we should not >>> need to keep track of the state at the vPCI level. >> >> But that's then not "has PCI MSI" but "need to intercept PCI MSI >> accesses", i.e. I don't think the Kconfig option is correctly >> named. If a device with MSI support is used, you can't make that >> MSI support go away, after all. > > That's actually a good point. Rahul, do you think the config can be > renamed to something like CONFIG_PCI_MSI_NEED_INTERCEPT? Minor remark: In this name I'd be inclined to suggest to omit NEED. Jan ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI 2021-04-08 6:00 ` Jan Beulich @ 2021-04-08 8:45 ` Rahul Singh 0 siblings, 0 replies; 14+ messages in thread From: Rahul Singh @ 2021-04-08 8:45 UTC (permalink / raw) To: Jan Beulich Cc: Julien Grall, xen-devel, Bertrand Marquis, Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Daniel De Graaf, Roger Pau Monné Hi, > On 8 Apr 2021, at 7:00 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 07.04.2021 20:06, Julien Grall wrote: >> Hi Jan, >> >> On 06/04/2021 16:25, Jan Beulich wrote: >>> On 06.04.2021 16:30, Julien Grall wrote: >>>> Hi Roger, >>>> >>>> On 06/04/2021 15:13, Roger Pau Monné wrote: >>>>> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote: >>>>>> MSI support is not implemented for ARM architecture but it is enabled >>>>>> for x86 architecture and referenced in common passthrough/pci.c code. >>>>>> >>>>>> Therefore introducing the new flag to gate the MSI code for ARM in >>>>>> common code to avoid compilation error when HAS_PCI is enabled for ARM. >>>>> >>>>> Is such option really interesting long term? >>>>> >>>>> IIRC PCI Express mandates MSI support, at which point I don't see much >>>>> value in being able to compile out the MSI support. >>>> >>>> I am pretty sure there are board out with PCI support but no MSI >>>> support. Anyway, even if the spec may mandate it... >>>> >>>>> >>>>> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it >>>>> warrants a Kconfig option, I would rather see Arm introduce dummy >>>>> helpers for the missing functionality, even if unimplemented at the >>>>> moment. >>>> >>>> ... from my understanding, most of (if not all) the MSI code is not very >>>> useful on Arm when using the GICv3 ITS. >>>> >>>> The GICv3 ITS will do the isolation for you and therefore we should not >>>> need to keep track of the state at the vPCI level. >>> >>> But that's then not "has PCI MSI" but "need to intercept PCI MSI >>> accesses", i.e. I don't think the Kconfig option is correctly >>> named. If a device with MSI support is used, you can't make that >>> MSI support go away, after all. >> >> That's actually a good point. Rahul, do you think the config can be >> renamed to something like CONFIG_PCI_MSI_NEED_INTERCEPT? > > Minor remark: In this name I'd be inclined to suggest to omit NEED. > OK . I will use the name CONFIG_PCI_MSI_INTERCEPT and will send next version of the patch. Regards, Rahul > Jan ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-04-08 8:46 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-06 11:39 [PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific Rahul Singh 2021-04-06 11:39 ` [PATCH 1/2] xen/pci: Move PCI ATS code to common directory Rahul Singh 2021-04-06 15:16 ` Jan Beulich 2021-04-06 11:39 ` [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI Rahul Singh 2021-04-06 14:13 ` Roger Pau Monné 2021-04-06 14:30 ` Julien Grall 2021-04-06 14:59 ` Roger Pau Monné 2021-04-06 15:09 ` Julien Grall 2021-04-06 15:58 ` Roger Pau Monné 2021-04-07 17:52 ` Rahul Singh 2021-04-06 15:25 ` Jan Beulich 2021-04-07 18:06 ` Julien Grall 2021-04-08 6:00 ` Jan Beulich 2021-04-08 8:45 ` Rahul Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).