Re: [PATCH v3 2/3] xen/pci: Refactor PCI MSI intercept related code

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Roger,

> On 28 Apr 2021, at 11:42 am, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Mon, Apr 26, 2021 at 05:21:26PM +0100, Rahul Singh wrote:
>> MSI intercept related code 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 and also implemented the stub version
>> for the unused code for ARM to avoid compilation error when HAS_PCI
>> is enabled for ARM.
>> 
>> No functional change intended.
>> 
>> 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.
>> 
>> Changes since v2:
>> - Rename function name as per the comments.
>> - Created a separate non-arch specific file msi-intercept.c.
>> ---
>> xen/arch/x86/Kconfig                    |  1 +
>> xen/drivers/passthrough/Makefile        |  1 +
>> xen/drivers/passthrough/msi-intercept.c | 52 +++++++++++++++++++++++++
>> xen/drivers/passthrough/pci.c           | 16 +++-----
>> 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/xen/msi-intercept.h         | 48 +++++++++++++++++++++++
>> xen/include/xen/vpci.h                  | 23 +++++++++++
>> xen/xsm/flask/hooks.c                   |  8 ++--
>> 12 files changed, 170 insertions(+), 33 deletions(-)
>> create mode 100644 xen/drivers/passthrough/msi-intercept.c
>> create mode 100644 xen/include/xen/msi-intercept.h
>> 
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 32b9f23a20..4d72798f00 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 PCI_MSI_INTERCEPT
>>      select HAS_PDX
>>      select HAS_SCHED_GRANULARITY
>>      select HAS_UBSAN
> 
> This list is sorted alphabetically AFAICT, and new additions should
> respect that.

Ok. As per the Jan request, I will modify the Kconfig name 
HAS_PCI_MSI_INTERCEPT. 
> 
>> diff --git a/xen/drivers/passthrough/Makefile 
>> b/xen/drivers/passthrough/Makefile
>> index 445690e3e5..3c707706b0 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_PCI_MSI_INTERCEPT) += msi-intercept.o
>> diff --git a/xen/drivers/passthrough/msi-intercept.c 
>> b/xen/drivers/passthrough/msi-intercept.c
>> new file mode 100644
>> index 0000000000..1edae6d4e8
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/msi-intercept.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
> 
> Since this is code movement, I think it should keep the copyright of
> the file they are moved from.
> 

Ok. 

>> + *
>> + * 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/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <asm/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;
>> +}
>> +
>> +void pdev_dump_msi(const struct pci_dev *pdev)
>> +{
>> +    const struct msi_desc *msi;
>> +
>> +    list_for_each_entry ( msi, &pdev->msi_list, list )
>> +        printk("%d ", msi->irq);
>> +}
>> +/*
> 
> Missing newline.

Ack.
> 
>> + * 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 199ce08612..298be21b5b 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-intercept.h>
>> #include <xsm/xsm.h>
>> -#include <asm/msi.h>
>> #include "ats.h"
>> 
>> struct pci_seg {
>> @@ -1271,7 +1271,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 +1279,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);
>> +        pdev_dump_msi(pdev);
>>         printk(">\n");
> 
> Maybe the whole '- MSIs < ... >' should be removed, instead of
> printing an empty list of MSIs when MSI interception is not supported?
> 
> Or else you give the impression that no MSIs are in use, when instead
> Xen is not tracking them.
> 
Ok I will remove - MSIs < … > when  MSI interception is not supported.
>>     }
>> 
>> @@ -1422,13 +1420,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..e6bf0b7441 100644
>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>> 
>> config HAS_PCI
>>      bool
>> +
>> +config PCI_MSI_INTERCEPT
>> +    bool
>> +    depends on 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..ec735c5b4b 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 = vpci_remove_msix_regions(pdev, mem);
>> +    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..3efbaebc92 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 vpci_remove_msix_regions(const struct pci_dev *pdev, struct rangeset 
>> *mem)
> 
> You could pass struct vpci here, since there's no need to pass the
> pdev, but I don't really have a strong opinion.
Ack.
> 
>> +{
>> +    const struct vpci_msix *msix = pdev->vpci->msix;
>> +    unsigned int i;
>> +    int rc;
> 
> Nit: you can define rc inside the loop, like it's done for start and
> end.

Ack.
> 
>> +
>> +    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);
> 
> You can now also move vmsix_table_{base,addr,size} to be local static
> inlines to msix.c instead of being defined in the header AFAICT.

Ok.
> 
>> +
>> +        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..85084dd924 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);
>> +    vpci_msi_free(pdev->vpci);
>>     xfree(pdev->vpci);
>>     pdev->vpci = NULL;
>> }
>> diff --git a/xen/include/xen/msi-intercept.h 
>> b/xen/include/xen/msi-intercept.h
>> new file mode 100644
>> index 0000000000..77c105e286
>> --- /dev/null
>> +++ b/xen/include/xen/msi-intercept.h
>> @@ -0,0 +1,48 @@
>> +/*
>> + * 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 __XEN_MSI_INTERCEPT_H_
>> +#define __XEN_MSI_INTERCEPT_H_
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> +
>> +#include <asm/msi.h>
>> +
>> +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_msix_assign(struct domain *d, struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> +
>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>> +
>> +#endif /* __XEN_MSI_INTERCEPT_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/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 9f5b5d52e1..2cd3647305 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 */
>> #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,6 +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 vpci_remove_msix_regions(const struct pci_dev *pdev, struct rangeset 
>> *mem);
>> 
>> /*
>>  * Helper functions to fetch MSIX related data. They are used by both the
>> @@ -208,6 +212,25 @@ static inline unsigned int vmsix_entry_nr(const struct 
>> vpci_msix *msix,
>> {
>>     return entry - msix->entries;
>> }
>> +
>> +static inline void vpci_msi_free(struct vpci *vpci)
>> +{
>> +    XFREE(vpci->msix);
>> +    XFREE(vpci->msi);
>> +}
>> +#else /* !CONFIG_PCI_MSI_INTERCEPT */
>> +static inline int vpci_make_msix_hole(const struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline int vpci_remove_msix_regions(const struct pci_dev *pdev,
>> +                                      struct rangeset *mem)
> 
> Nit: line seems to not be properly aligned.

Ack.

Regards,
Rahul
> 
> Thanks, Roger.