[PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific

[PATCH 0/2] xen/pci: Make PCI passthrough code non-x86 specific

[Rahul Singh]

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

[PATCH 1/2] xen/pci: Move PCI ATS code to common directory

[Rahul Singh]

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

[PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Rahul Singh]

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

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Roger Pau Monné]

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.

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Julien Grall]

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

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Roger Pau Monné]

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.

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Julien Grall]

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

Re: [PATCH 1/2] xen/pci: Move PCI ATS code to common directory

[Jan Beulich]

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>

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Jan Beulich]

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

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Roger Pau Monné]

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.

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Rahul Singh]

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.

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Julien Grall]

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

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Jan Beulich]

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

Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

[Rahul Singh]

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