Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> MSI related code in the "passthrough/pci.c” file is not useful for ARM
> when MSI interrupts are injected via GICv3 ITS.
> 
> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
> MSI code for ARM in common code. Also move the arch-specific code to an
> arch-specific directory and implement the stub version for the unused
> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
> Changes since v1:
>  - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>  - Implement stub version of the unused function for ARM.
>  - Move unneeded function to x86 file.     
> ---
>  xen/arch/x86/msi.c            | 64 +++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c | 51 +++++++---------------------
>  xen/drivers/pci/Kconfig       |  4 +++
>  xen/drivers/vpci/Makefile     |  3 +-
>  xen/drivers/vpci/header.c     | 19 +++--------
>  xen/drivers/vpci/msix.c       | 25 ++++++++++++++
>  xen/drivers/vpci/vpci.c       |  3 +-
>  xen/include/asm-arm/msi.h     | 44 ++++++++++++++++++++++++
>  xen/include/asm-x86/msi.h     |  4 +++
>  xen/include/xen/pci.h         | 11 +++---
>  xen/include/xen/vpci.h        | 24 ++++++++++++-
>  xen/xsm/flask/hooks.c         |  8 ++---
>  12 files changed, 195 insertions(+), 65 deletions(-)
>  create mode 100644 xen/include/asm-arm/msi.h
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 5febc0ea4b..a6356f4a63 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>          return;
>  }
>  
> +int alloc_pci_msi(struct pci_dev *pdev)

I would rather name it pci_msi_init...

> +{
> +    unsigned int pos;
> +
> +    INIT_LIST_HEAD(&pdev->msi_list);
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
> +    if ( pos )
> +    {
> +        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> +
> +        pdev->msi_maxvec = multi_msi_capable(ctrl);
> +    }
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> +    if ( pos )
> +    {
> +        struct arch_msix *msix = xzalloc(struct arch_msix);
> +        uint16_t ctrl;
> +
> +        if ( !msix )
> +            return -ENOMEM;
> +
> +        spin_lock_init(&msix->table_lock);
> +
> +        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> +        msix->nr_entries = msix_table_size(ctrl);
> +
> +        pdev->msix = msix;
> +    }
> +
> +    return 0;
> +}
> +
> +void free_pci_msi(struct pci_dev *pdev)

...and pci_msi_free.

> +{
> +    xfree(pdev->msix);
> +}
> +
> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)

This kind of a confusing name - what about pci_msix_assign?

> +{
> +    int rc;
> +
> +    if ( pdev->msix )
> +    {
> +        rc = pci_reset_msix_state(pdev);
> +        if ( rc )
> +            return rc;
> +        msixtbl_init(d);
> +    }
> +
> +    return 0;
> +}
> +
> +void dump_pci_msi(struct pci_dev *pdev)
> +{
> +    struct msi_desc *msi;
> +
> +    list_for_each_entry ( msi, &pdev->msi_list, list )
> +        printk("%d ", msi->irq);
> +}

I wonder, those those function really want to be in a x86 specific
file? There's nothing x86 specific about them AFAICT.

Would it make sense to create a separate msi-intercept.c file with
those that gets included when CONFIG_PCI_MSI_INTERCEPT?

>  static void dump_msi(unsigned char key)
>  {
>      unsigned int irq;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 705137f8be..1b473502a1 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>  {
>      struct pci_dev *pdev;
>      unsigned int pos;
> +    int rc;
>  
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>          if ( pdev->bus == bus && pdev->devfn == devfn )
> @@ -327,35 +328,12 @@ 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;
> -    INIT_LIST_HEAD(&pdev->msi_list);
> -
> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn),
> -                              PCI_CAP_ID_MSI);
> -    if ( pos )
> -    {
> -        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> -
> -        pdev->msi_maxvec = multi_msi_capable(ctrl);
> -    }
>  
> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn),
> -                              PCI_CAP_ID_MSIX);
> -    if ( pos )
> +    rc = alloc_pci_msi(pdev);
> +    if ( rc )
>      {
> -        struct arch_msix *msix = xzalloc(struct arch_msix);
> -        uint16_t ctrl;
> -
> -        if ( !msix )
> -        {
> -            xfree(pdev);
> -            return NULL;
> -        }
> -        spin_lock_init(&msix->table_lock);
> -
> -        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> -        msix->nr_entries = msix_table_size(ctrl);
> -
> -        pdev->msix = msix;
> +        xfree(pdev);
> +        return NULL;
>      }
>  
>      list_add(&pdev->alldevs_list, &pseg->alldevs_list);
> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct 
> pci_dev *pdev)
>      }
>  
>      list_del(&pdev->alldevs_list);
> -    xfree(pdev->msix);
> +    free_pci_msi(pdev);
>      xfree(pdev);
>  }
>  
> @@ -1271,7 +1249,6 @@ 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;
> -    struct msi_desc *msi;
>  
>      printk("==== segment %04x ====\n", pseg->nr);
>  
> @@ -1280,8 +1257,7 @@ 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);
> -        list_for_each_entry ( msi, &pdev->msi_list, list )
> -               printk("%d ", msi->irq);
> +        dump_pci_msi(pdev);
>          printk(">\n");
>      }
>  
> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>  }
>  __initcall(setup_dump_pcidevs);
>  
> +
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>  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

This is not exactly related to MSI intercepts, the IOMMU interrupt
remapping table will also be used for interrupt entries for devices
being used by Xen directly, where no intercept is required.

And then you also want to gate the hook from iommu_ops itself with
CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.

>  
>  static int iommu_add_device(struct pci_dev *pdev)
>  {
> @@ -1429,13 +1408,9 @@ 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));
>  
> -    if ( pdev->msix )
> -    {
> -        rc = pci_reset_msix_state(pdev);
> -        if ( rc )
> -            goto done;
> -        msixtbl_init(d);
> -    }
> +    rc = pci_assign_msix_init(d, pdev);
> +    if ( rc )
> +        goto done;
>  
>      pdev->fault.count = 0;
>  
> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
> index 7da03fa13b..7ab92bde6e 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,7 @@
>  
>  config HAS_PCI
>       bool
> +
> +config PCI_MSI_INTERCEPT
> +     def_bool y
> +     depends on X86 && HAS_PCI
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 55d1bdfda0..f91fa71a40 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_PCI_MSI_INTERCEPT) += msi.o msix.o
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba9a036202..a7695a0e2a 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -206,7 +206,6 @@ 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;
> -    const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i;
>      int rc;
>  
> @@ -244,21 +243,11 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>      }
>  
>      /* Remove any MSIX regions if present. */
> -    for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> +    rc = remove_msix_regions(pdev,mem);

Coding style: missing space between parameters.

> +    if ( rc )
>      {
> -        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);
> -
> -        rc = rangeset_remove_range(mem, start, end);
> -        if ( rc )
> -        {
> -            printk(XENLOG_G_WARNING
> -                   "Failed to remove MSIX table [%lx, %lx]: %d\n",
> -                   start, end, rc);
> -            rangeset_destroy(mem);
> -            return rc;
> -        }
> +        rangeset_destroy(mem);
> +        return rc;
>      }
>  
>      /*
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 846f1b8d70..ca0a9b5665 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -428,6 +428,31 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem)

This needs to start with a vpci_ prefix, since it's a global function.
What about vpci_msix_remove_regions.

> +{
> +    const struct vpci_msix *msix = pdev->vpci->msix;
> +    unsigned int i;
> +    int rc;
> +
> +    for ( i = 0; msix && i < ARRAY_SIZE(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);
> +
> +        rc = rangeset_remove_range(mem, start, end);
> +        if ( rc )
> +        {
> +            printk(XENLOG_G_WARNING
> +                    "Failed to remove MSIX table [%lx, %lx]: %d\n",
> +                    start, end, rc);
> +            return rc;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int init_msix(struct pci_dev *pdev)
>  {
>      struct domain *d = pdev->domain;
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index cbd1bac7fc..ebe40b0538 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>          xfree(r);
>      }
>      spin_unlock(&pdev->vpci->lock);
> -    xfree(pdev->vpci->msix);
> -    xfree(pdev->vpci->msi);
> +    free_vpci_msi(pdev);
>      xfree(pdev->vpci);
>      pdev->vpci = NULL;
>  }
> diff --git a/xen/include/asm-arm/msi.h b/xen/include/asm-arm/msi.h
> new file mode 100644
> index 0000000000..c54193310e
> --- /dev/null
> +++ b/xen/include/asm-arm/msi.h
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ASM_MSI_H_
> +#define __ASM_MSI_H_
> +
> +static inline int alloc_pci_msi(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev 
> *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
> +static inline void free_pci_msi(struct pci_dev *pdev) {}
> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
> +
> +#endif /* __ASM_MSI_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Should you instead have a non-arch specific file with those dummy
helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?

> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index e228b0f3f3..7ef0161b67 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>  void guest_mask_msi_irq(struct irq_desc *, bool mask);
>  void ack_nonmaskable_msi_irq(struct irq_desc *);
>  void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> +int alloc_pci_msi(struct pci_dev *pdev);
> +void free_pci_msi(struct pci_dev *pdev);
> +void dump_pci_msi(struct pci_dev *pdev);
> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev);
>  
>  #endif /* __ASM_MSI_H */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 8e3d4d9454..f5b57270be 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -79,10 +79,6 @@ struct pci_dev {
>      struct list_head alldevs_list;
>      struct list_head domain_list;
>  
> -    struct list_head msi_list;
> -
> -    struct arch_msix *msix;
> -
>      struct domain *domain;
>  
>      const union {
> @@ -94,7 +90,14 @@ struct pci_dev {
>          pci_sbdf_t sbdf;
>      };
>  
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> +    struct list_head msi_list;
> +
> +    struct arch_msix *msix;
> +
>      uint8_t msi_maxvec;
> +#endif
> +
>      uint8_t phantom_stride;
>  
>      nodeid_t node; /* NUMA node */
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 9f5b5d52e1..a6b12717b1 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_PCI_MSI_INTERCEPT
>      /* MSI data. */
>      struct vpci_msi {
>        /* Address. */
> @@ -136,6 +137,7 @@ struct vpci {
>              struct vpci_arch_msix_entry arch;
>          } entries[];
>      } *msix;
> +#endif /* CONFIG_PCI_MSI_INTERCEPT */

Note that here you just remove two pointers from the struct, not that
I'm opposed to it, but it's not that much space that's saved anyway.
Ie: it might also be fine to just leave them as NULL unconditionally
on Arm.

>  #endif
>  };
>  
> @@ -148,6 +150,7 @@ struct vpci_vcpu {
>  };
>  
>  #ifdef __XEN__
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>  void vpci_dump_msi(void);
>  
>  /* Make sure there's a hole in the p2m for the MSIX mmio areas. */
> @@ -174,7 +177,7 @@ int __must_check vpci_msix_arch_disable_entry(struct 
> vpci_msix_entry *entry,
>                                                const struct pci_dev *pdev);
>  void vpci_msix_arch_init_entry(struct vpci_msix_entry *entry);
>  int vpci_msix_arch_print(const struct vpci_msix *msix);
> -
> +int remove_msix_regions(const struct pci_dev *pdev, struct rangeset *mem);
>  /*
>   * Helper functions to fetch MSIX related data. They are used by both the
>   * emulated MSIX code and the BAR handlers.
> @@ -208,6 +211,25 @@ static inline unsigned int vmsix_entry_nr(const struct 
> vpci_msix *msix,
>  {
>      return entry - msix->entries;
>  }
> +
> +static inline void free_vpci_msi(const struct pci_dev *pdev)

vpci_msi_free please.

Thanks, Roger.