From: Rahul Singh <Rahul.Singh@arm.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
Bertrand Marquis <Bertrand.Marquis@arm.com>,
Jan Beulich <jbeulich@suse.com>, Paul Durrant <paul@xen.org>,
Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <george.dunlap@citrix.com>,
Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v3 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN
Date: Thu, 29 Apr 2021 11:59:04 +0000 [thread overview]
Message-ID: <11500C08-219E-4638-AD4A-9DADD20910ED@arm.com> (raw)
In-Reply-To: <YIlBnQO+iuFFx2XO@Air-de-Roger>
Hi Roger,
> On 28 Apr 2021, at 12:06 pm, Roger Pau Monné <roger.pau@citrix.com> 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@arm.com>
>
> 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@citrix.com>
>
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.
prev parent reply other threads:[~2021-04-29 11:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-26 16:21 [PATCH v3 0/3] xen/pci: Make PCI passthrough code non-x86 specific Rahul Singh
2021-04-26 16:21 ` [PATCH v3 1/3] xen/iommu: Move iommu_update_ire_from_msi(..) to xen/iommu.h Rahul Singh
2021-04-27 15:58 ` Jan Beulich
2021-04-26 16:21 ` [PATCH v3 2/3] xen/pci: Refactor PCI MSI intercept related code Rahul Singh
2021-04-28 10:42 ` Roger Pau Monné
2021-04-28 13:57 ` Jan Beulich
2021-04-29 8:27 ` Rahul Singh
2021-04-29 8:23 ` Rahul Singh
2021-04-28 14:06 ` Jan Beulich
2021-04-29 11:31 ` Rahul Singh
2021-04-29 11:47 ` Jan Beulich
2021-04-26 16:21 ` [PATCH v3 3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN Rahul Singh
2021-04-28 11:06 ` Roger Pau Monné
2021-04-28 14:11 ` Jan Beulich
2021-04-28 14:26 ` Julien Grall
2021-04-29 11:59 ` Rahul Singh [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=11500C08-219E-4638-AD4A-9DADD20910ED@arm.com \
--to=rahul.singh@arm.com \
--cc=Bertrand.Marquis@arm.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=paul@xen.org \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).