| Message ID | d0b7b457762d481b19c8da6c2d55ff4acb4d6291.1629366665.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
Hi Rahul, On 19/08/2021 13:02, Rahul Singh wrote: > MSI code that implements MSI functionality to support MSI within XEN is > not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI flag to gate Can you clarify what you mean by not usable? Is it because we lack of support or we have no plan to use it? Cheers, > the code for ARM. > > No functional change intended. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > xen/arch/x86/Kconfig | 1 + > xen/drivers/passthrough/Makefile | 1 + > xen/drivers/passthrough/msi.c | 96 ++++++++++++++++++++++++++++++++ > xen/drivers/passthrough/pci.c | 54 +++++------------- > xen/drivers/pci/Kconfig | 4 ++ > xen/include/xen/msi.h | 56 +++++++++++++++++++ > xen/xsm/flask/hooks.c | 8 +-- > 7 files changed, 175 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 9b164db641..7b46ee98c5 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -19,6 +19,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..15ad0f8160 > --- /dev/null > +++ b/xen/drivers/passthrough/msi.c > @@ -0,0 +1,96 @@ > +/* > + * Copyright (C) 2008, Netronome Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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; > +} > + > +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; > + > + 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..67f5686ab6 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..b2d5bc6f9d > --- /dev/null > +++ b/xen/include/xen/msi.h > @@ -0,0 +1,56 @@ > +/* > + * Copyright (C) 2008, Netronome Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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_H_ > +#define __XEN_MSI_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 f1a1217c98..fdcfeb984c 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; > >
Hi Julien, > On 19 Aug 2021, at 1:18 pm, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 19/08/2021 13:02, Rahul Singh wrote: >> MSI code that implements MSI functionality to support MSI within XEN is >> not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI flag to gate > > Can you clarify what you mean by not usable? Is it because we lack of support or we have no plan to use it? We have no plan to use it. Code moved to CONFIG_HAS_PCI_MSI will only be required for ARM if we decide to support PCI device access (PCI MSI interrupt support) within XEN. As of now, we are planning to add support for PCI device access for DOM0/DOMU guests not for XEN. Regards, Rahul
On 19.08.2021 14:02, Rahul Singh wrote: > --- /dev/null > +++ b/xen/drivers/passthrough/msi.c > @@ -0,0 +1,96 @@ > +/* > + * Copyright (C) 2008, Netronome Systems, Inc. While generally copying copyright statements when splitting source files is probably wanted (or even necessary) I doubt this is suitable here: None of the MSI code that you move was contributed by them afaict. > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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> You surely mean xen/msi.h here: Headers like this one should always be included by the producer, no matter that it builds fine without. Else you risk declarations and definitions to go out of sync. > +#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; > + > + printk("- MSIs < "); > + list_for_each_entry ( msi, &pdev->msi_list, list ) > + printk("%d ", msi->irq); > + printk(">"); While not an exact equivalent of the original code then, could I talk you into adding an early list_empty() check, suppressing any output from this function if that one returned "true"? > @@ -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 ", Together with the request above the trailin blank here also wants to become a leading blank in pdev_dump_msi(). > --- /dev/null > +++ b/xen/include/xen/msi.h > @@ -0,0 +1,56 @@ > +/* > + * Copyright (C) 2008, Netronome Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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_H_ > +#define __XEN_MSI_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) Please be consistent with blank lines you add; here you also want one after the #else. > +{ > + 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) {} Especially for (but perhaps not limited to) this !HAS_PCI_MSI case (where you don't include asm/msi.h and its possible dependents) please forward-declare struct-s you use in prototypes or inline stubs (outside the #ifdef, that is). This will allow including this header without having to care about prereq headers. If you agree with and make all the suggested or requested changes, feel free to add Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 8/19/21 8:02 AM, Rahul Singh wrote: > MSI code that implements MSI functionality to support MSI within XEN is > not usable on ARM. 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@arm.com> > --- > xen/arch/x86/Kconfig | 1 + > xen/drivers/passthrough/Makefile | 1 + > xen/drivers/passthrough/msi.c | 96 ++++++++++++++++++++++++++++++++ > xen/drivers/passthrough/pci.c | 54 +++++------------- > xen/drivers/pci/Kconfig | 4 ++ > xen/include/xen/msi.h | 56 +++++++++++++++++++ > xen/xsm/flask/hooks.c | 8 +-- > 7 files changed, 175 insertions(+), 45 deletions(-) > create mode 100644 xen/drivers/passthrough/msi.c > create mode 100644 xen/include/xen/msi.h > ... > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index f1a1217c98..fdcfeb984c 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; > > Straightforward, so I see no issue with the flask related changes. Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> v/r dps
Hi Jan, > On 24 Aug 2021, at 4:53 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 19.08.2021 14:02, Rahul Singh wrote: >> --- /dev/null >> +++ b/xen/drivers/passthrough/msi.c >> @@ -0,0 +1,96 @@ >> +/* >> + * Copyright (C) 2008, Netronome Systems, Inc. > > While generally copying copyright statements when splitting source > files is probably wanted (or even necessary) I doubt this is > suitable here: None of the MSI code that you move was contributed > by them afaict. Let me remove the "Copyright Copyright (C) 2008, Netronome Systems, Inc.” . Can you please help me what copyright I will add for the next patch ? > >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope 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> > > You surely mean xen/msi.h here: Headers like this one should always > be included by the producer, no matter that it builds fine without. > Else you risk declarations and definitions to go out of sync. Ok . Let me include here “xen/msi.h” and move other required includes to “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; >> + >> + printk("- MSIs < "); >> + list_for_each_entry ( msi, &pdev->msi_list, list ) >> + printk("%d ", msi->irq); >> + printk(">"); > > While not an exact equivalent of the original code then, could I > talk you into adding an early list_empty() check, suppressing any > output from this function if that one returned "true”? Ok. > >> @@ -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 ", > > Together with the request above the trailin blank here also wants to > become a leading blank in pdev_dump_msi() Ok. >> --- /dev/null >> +++ b/xen/include/xen/msi.h >> @@ -0,0 +1,56 @@ >> +/* >> + * Copyright (C) 2008, Netronome Systems, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope 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_H_ >> +#define __XEN_MSI_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) > > Please be consistent with blank lines you add; here you also want one > after the #else. Ok. > >> +{ >> + 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) {} > > Especially for (but perhaps not limited to) this !HAS_PCI_MSI case > (where you don't include asm/msi.h and its possible dependents) > please forward-declare struct-s you use in prototypes or inline > stubs (outside the #ifdef, that is). This will allow including > this header without having to care about prereq headers. Ok. Let me do modification in next version. Regards, Rahul > > If you agree with and make all the suggested or requested changes, > feel free to add > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Jan >
On 31.08.2021 14:31, Rahul Singh wrote: >> On 24 Aug 2021, at 4:53 pm, Jan Beulich <jbeulich@suse.com> wrote: >> On 19.08.2021 14:02, Rahul Singh wrote: >>> --- /dev/null >>> +++ b/xen/drivers/passthrough/msi.c >>> @@ -0,0 +1,96 @@ >>> +/* >>> + * Copyright (C) 2008, Netronome Systems, Inc. >> >> While generally copying copyright statements when splitting source >> files is probably wanted (or even necessary) I doubt this is >> suitable here: None of the MSI code that you move was contributed >> by them afaict. > > Let me remove the "Copyright Copyright (C) 2008, Netronome Systems, Inc.” . > Can you please help me what copyright I will add for the next patch ? None? If you look around, you will find that far from all source files have such a line (or multiple of them). >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms and conditions of the GNU General Public License, >>> + * version 2, as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope 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> >> >> You surely mean xen/msi.h here: Headers like this one should always >> be included by the producer, no matter that it builds fine without. >> Else you risk declarations and definitions to go out of sync. > Ok . Let me include here “xen/msi.h” and move other required includes to “xen/msi.h" Why move stuff? xen/msi.h is fins to include asm/msi.h. It's just that including asm/msi.h here is not enough. Jan
On 19/08/2021 15:16, Rahul Singh wrote: > Hi Julien, Hi Rahul, Sorry for the late reply. >> On 19 Aug 2021, at 1:18 pm, Julien Grall <julien@xen.org> wrote: >> >> Hi Rahul, >> >> On 19/08/2021 13:02, Rahul Singh wrote: >>> MSI code that implements MSI functionality to support MSI within XEN is >>> not usable on ARM. Move the code under CONFIG_HAS_PCI_MSI flag to gate >> >> Can you clarify what you mean by not usable? Is it because we lack of support or we have no plan to use it? > > We have no plan to use it. Code moved to CONFIG_HAS_PCI_MSI will only be required for ARM if we I read "not usable", as the code doesn't yet work on Arm. This is quite possible, but from you wrote the main reason is that the code is not necessary yet (see below) on Arm. > decide to support PCI device access (PCI MSI interrupt support) within XEN. As of now, we are planning > to add support for PCI device access for DOM0/DOMU guests not for XEN. This is probably going to happen sooner rather than later as, AFAICT, there are SMMUs out which signals using MSIs. But, AFAICT, this code would also be used if we need to manage the MSIs in Xen on behalf of the guest (such as if the platform is using GICv2m). That said, I would be fine with gating the MSI code behind a new config. However, I think the commit message wants to be clarified into why we don't need the option. Maybe something like: "On Arm, the inital plan is to only support GICv3 ITS which doesn't require us to manage the MSIs because the HW will protect against spoofing. Introduce a new option XXXX to protect the MSI code. " Cheers,
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 9b164db641..7b46ee98c5 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -19,6 +19,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..15ad0f8160 --- /dev/null +++ b/xen/drivers/passthrough/msi.c @@ -0,0 +1,96 @@ +/* + * Copyright (C) 2008, Netronome Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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; +} + +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; + + 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..67f5686ab6 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..b2d5bc6f9d --- /dev/null +++ b/xen/include/xen/msi.h @@ -0,0 +1,56 @@ +/* + * Copyright (C) 2008, Netronome Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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_H_ +#define __XEN_MSI_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 f1a1217c98..fdcfeb984c 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;
MSI code that implements MSI functionality to support MSI within XEN is not usable on ARM. 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@arm.com> --- xen/arch/x86/Kconfig | 1 + xen/drivers/passthrough/Makefile | 1 + xen/drivers/passthrough/msi.c | 96 ++++++++++++++++++++++++++++++++ xen/drivers/passthrough/pci.c | 54 +++++------------- xen/drivers/pci/Kconfig | 4 ++ xen/include/xen/msi.h | 56 +++++++++++++++++++ xen/xsm/flask/hooks.c | 8 +-- 7 files changed, 175 insertions(+), 45 deletions(-) create mode 100644 xen/drivers/passthrough/msi.c create mode 100644 xen/include/xen/msi.h