[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Rahul Singh <Rahul.Singh@xxxxxxx>
  • Date: Thu, 29 Apr 2021 11:59:04 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=s2khhl9pdasRffxjkbVK8Pm3ROh/+0CR90sZoRfSWaI=; b=fABBi3vLAYjaV34FbPa4YEdLE/PK+rlhu2B/b4rvjJq6eXeYK3XseyiKyiRw1IUkgQNlO6gy0AuCzNd5clRn7DzGb6eEWquZgz6eetWUJHGONtPGnE6GOV8769TlKV/s9nFZG0ohT3SvPDMjK0oJR1id3WUqUbWlDSBQqJxMRzdR2m8bl8SBW8ksJwGLmtDlejnliBzpFvf2TrOQPNDUJ89L0hiUbqsfUq9E5eEw4H5W8KUShAGg1GGgIHirTdvGnPZEnHzV9Tv3ErUH2SPraBqjxmal8pM3usoZafY8Ss/X3K4qGbwtR/PwYQ0epdBVRCN6yTBJSIBMDTVCLO5x5Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=odgsXFju7T/hhNGSUvC7DJf/Jpmu4KizK7j9u1/ieYMxiymJvNg7Vt8+2at7yQZe7A9caftsQbOGR9a2Ex3dP0QQ6a2qjAEqHwtvdGubn8TILMyKVRYGLsU7BG11gl7hbfiypUu3cd1i+kfw9VSHBAusLMO+x2Wv+kqu89gvmQdS7DzmaxV8AlJzPQ+6WqD3siFsmhRvhpeza3VYuU1g5n9W9xi9SM/MHWXwIZDEMdSiqaHEVbBgJzrTdXrMlvta5KQoqUKT74Y1vpmzMEMdCfNOelxERSfc0QwzWjECr3pSxmc1FonIGbPxgBNmU/b0cG/HROFr9SFv78rmOBYOeQ==
  • Authentication-results-original: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=arm.com;
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Thu, 29 Apr 2021 11:59:44 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHXOrh4cv5EbJqd3U+gSr1g4kEjParJx4aAgAGhIYA=
  • Thread-topic: [PATCH v3 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

Hi Roger,

> On 28 Apr 2021, at 12:06 pm, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> 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>
> 

Thanks.

> 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.

Ok. I will change it to xfree in next version.

> 
>> +        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?

Yes. That’s why I move all the fields related to MSI to one place to avoid the 
extra CONFIG_PCI_MSI_INTERCEPT instance.

Regards,
Rahul
> 
> Thanks, Roger.


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.