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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Rahul,

> On 28 Sep 2021, at 19:18, Rahul Singh <rahul.singh@xxxxxxx> wrote:
> 
> On Arm, the initial plan is to only support GICv3 ITS which doesn't
> require us to manage the MSIs because the HW will protect against
> spoofing. Move the code under CONFIG_HAS_PCI_MSI flag to gate the code
> for ARM.
> 
> No functional change intended.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> Reviewed-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand

> ---
> Change in v3: none 
> Change in v2: Fixed minor comments
> ---
> xen/arch/x86/Kconfig             |  1 +
> xen/drivers/passthrough/Makefile |  1 +
> xen/drivers/passthrough/msi.c    | 83 ++++++++++++++++++++++++++++++++
> xen/drivers/passthrough/pci.c    | 54 +++++----------------
> xen/drivers/pci/Kconfig          |  4 ++
> xen/include/xen/msi.h            | 43 +++++++++++++++++
> xen/xsm/flask/hooks.c            |  8 +--
> 7 files changed, 149 insertions(+), 45 deletions(-)
> create mode 100644 xen/drivers/passthrough/msi.c
> create mode 100644 xen/include/xen/msi.h
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 1f83518ee0..b4abfca46f 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -20,6 +20,7 @@ config X86
>       select HAS_NS16550
>       select HAS_PASSTHROUGH
>       select HAS_PCI
> +     select HAS_PCI_MSI
>       select HAS_PDX
>       select HAS_SCHED_GRANULARITY
>       select HAS_UBSAN
> diff --git a/xen/drivers/passthrough/Makefile 
> b/xen/drivers/passthrough/Makefile
> index 445690e3e5..a5efa22714 100644
> --- a/xen/drivers/passthrough/Makefile
> +++ b/xen/drivers/passthrough/Makefile
> @@ -7,3 +7,4 @@ obj-y += iommu.o
> obj-$(CONFIG_HAS_PCI) += pci.o
> obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> obj-$(CONFIG_HAS_PCI) += ats.o
> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o
> diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c
> new file mode 100644
> index 0000000000..ce1a450f6f
> --- /dev/null
> +++ b/xen/drivers/passthrough/msi.c
> @@ -0,0 +1,83 @@
> +#include <xen/init.h>
> +#include <xen/pci.h>
> +#include <xen/msi.h>
> +#include <asm/hvm/io.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    if ( pdev->msix )
> +    {
> +        rc = pci_reset_msix_state(pdev);
> +        if ( rc )
> +            return rc;
> +        msixtbl_init(d);
> +    }
> +
> +    return 0;
> +}
> +
> +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);
> +}
> +
> +void pdev_dump_msi(const struct pci_dev *pdev)
> +{
> +    const struct msi_desc *msi;
> +
> +    if ( list_empty(&pdev->msi_list) )
> +        return;
> +
> +    printk(" - MSIs < ");
> +    list_for_each_entry ( msi, &pdev->msi_list, list )
> +        printk("%d ", msi->irq);
> +    printk(">");
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index fc4fa2e5c3..8996403161 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -32,8 +32,8 @@
> #include <xen/softirq.h>
> #include <xen/tasklet.h>
> #include <xen/vpci.h>
> +#include <xen/msi.h>
> #include <xsm/xsm.h>
> -#include <asm/msi.h>
> #include "ats.h"
> 
> struct pci_seg {
> @@ -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);
> +        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);
> }
> 
> @@ -1271,18 +1249,16 @@ 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);
> 
>     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>     {
> -        printk("%pp - %pd - node %-3d - MSIs < ",
> +        printk("%pp - %pd - node %-3d",
>                &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);
> -        printk(">\n");
> +        pdev_dump_msi(pdev);
> +        printk("\n");
>     }
> 
>     return 0;
> @@ -1422,13 +1398,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 = pdev_msix_assign(d, pdev);
> +    if ( rc )
> +        goto done;
> 
>     pdev->fault.count = 0;
> 
> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
> index 7da03fa13b..c6a7bc8007 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,7 @@
> 
> config HAS_PCI
>       bool
> +
> +config HAS_PCI_MSI
> +     bool
> +     depends on HAS_PCI
> diff --git a/xen/include/xen/msi.h b/xen/include/xen/msi.h
> new file mode 100644
> index 0000000000..c903d0050c
> --- /dev/null
> +++ b/xen/include/xen/msi.h
> @@ -0,0 +1,43 @@
> +#ifndef __XEN_MSI_H_
> +#define __XEN_MSI_H_
> +
> +#include <xen/pci.h>
> +
> +#ifdef CONFIG_HAS_PCI_MSI
> +
> +#include <asm/msi.h>
> +
> +int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
> +int pdev_msi_init(struct pci_dev *pdev);
> +void pdev_msi_deinit(struct pci_dev *pdev);
> +void pdev_dump_msi(const struct pci_dev *pdev);
> +
> +#else /* !CONFIG_HAS_PCI_MSI */
> +
> +static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
> +
> +static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
> +
> +#endif /* CONFIG_HAS_PCI_MSI */
> +
> +#endif /* __XEN_MSI_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 25e87180b4..ea9a12bd71 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 ) {
> @@ -874,7 +874,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;
> 
> @@ -940,7 +940,7 @@ static int flask_unmap_domain_pirq (struct domain *d)
> static int flask_unmap_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 pci_dev *pdev = data;
>     u32 machine_bdf = (pdev->seg << 16) | (pdev->bus << 8) | pdev->devfn;
> 
> -- 
> 2.17.1
>