Re: [PATCH v3 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

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

On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
> MSI code that implements MSI functionality to support MSI within XEN is
> not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
> gate the code for ARM.
> 
> Currently, we have no idea how MSI functionality will be supported for
> other architecture therefore we have decided to move the code under
> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
> code but to avoid an extra flag we decided to use this.
> 
> No functional change intended.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>

I think this is fine, as we don't really want to add another Kconfig
option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
code.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Some nits below...

> ---
> Changes since v2:
>  - This patch is added in this version.
> ---
>  xen/drivers/passthrough/msi-intercept.c | 41 +++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c           | 34 ++++----------------
>  xen/include/xen/msi-intercept.h         |  7 +++++
>  xen/include/xen/pci.h                   | 11 ++++---
>  4 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/msi-intercept.c 
> b/xen/drivers/passthrough/msi-intercept.c
> index 1edae6d4e8..33ab71514d 100644
> --- a/xen/drivers/passthrough/msi-intercept.c
> +++ b/xen/drivers/passthrough/msi-intercept.c
> @@ -19,6 +19,47 @@
>  #include <asm/msi.h>
>  #include <asm/hvm/io.h>
>  
> +int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    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 pdev_msi_deinit(struct pci_dev *pdev)
> +{
> +    XFREE(pdev->msix);
> +}
> +
>  int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>  {
>      int rc;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 298be21b5b..b1e3c711ad 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 = pdev_msi_init(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);

There's no need to use XFREE here, plain xfree is fine since pdev is a
local variable so makes no sense to assign to NULL before returning.

> +        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);
> +    pdev_msi_deinit(pdev);
>      xfree(pdev);
>  }
>  
> diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
> index 77c105e286..38ff7a09e1 100644
> --- a/xen/include/xen/msi-intercept.h
> +++ b/xen/include/xen/msi-intercept.h
> @@ -21,16 +21,23 @@
>  
>  #include <asm/msi.h>
>  
> +int pdev_msi_init(struct pci_dev *pdev);
> +void pdev_msi_deinit(struct pci_dev *pdev);
>  int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
>  void pdev_dump_msi(const struct pci_dev *pdev);
>  
>  #else /* !CONFIG_PCI_MSI_INTERCEPT */
> +static inline int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
>  
>  static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>  {
>      return 0;
>  }
>  
> +static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
>  static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
>  static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>  
> 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

This seems to introduce some padding between the sbdf and the msi_list
field. I guess that's better than having two different
CONFIG_PCI_MSI_INTERCEPT guarded regions?

Thanks, Roger.