| Message ID | 20190716101657.23327-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu groups + cleanup | expand |
On 16/07/2019 11:16, Paul Durrant wrote: > ...and use it for setup_hwdom_pci_devices() and dump_pci_devices(). > > The unlock/process-pending-softirqs/lock sequence that was in > _setup_hwdom_pci_devices() is now done in the generic iterator function, > which does mean it is also done (unnecessarily) in the case of > dump_pci_devices(), since run_all_nonirq_keyhandlers() will call > process_pending_softirqs() before invoking each key handler anyway, but > this is not performance critical code. > > The "==== segment XXXX ====" headline that was in _dump_pci_devices() has > been dropped because it is non-trivial to deal with it when using a > generic all-device iterator and, since the segment number is included > in every log line anyway, it didn't add much value anyway. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com> > --- > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wl@xen.org> > > v3: > - Addressed review comments from Roger. > > v2: > - New in v2. > --- > xen/drivers/passthrough/pci.c | 121 ++++++++++++++++++++++++------------------ > xen/include/xen/pci.h | 1 + > 2 files changed, 69 insertions(+), 53 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index e88689425d..4bb9996049 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1134,64 +1134,87 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt, > ctxt->d->domain_id, err); > } > > -static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg) > +static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg) > { > struct setup_hwdom *ctxt = arg; > - int bus, devfn; > + struct domain *d = ctxt->d; > > - for ( bus = 0; bus < 256; bus++ ) > + if ( !pdev->domain ) > + { > + pdev->domain = d; > + list_add(&pdev->domain_list, &d->pdev_list); > + setup_one_hwdom_device(ctxt, pdev); > + } > + else if ( pdev->domain == dom_xen ) > { > - for ( devfn = 0; devfn < 256; devfn++ ) > + pdev->domain = d; > + setup_one_hwdom_device(ctxt, pdev); > + pdev->domain = dom_xen; > + } > + else if ( pdev->domain != d ) > + printk(XENLOG_WARNING "Dom%pd owning %04x:%02x:%02x.%u?\n", Just %pd. It takes care of rendering d%u or d[$name] for system domains. Can be fixed on commit. > + pdev->domain, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn)); > + > + return 0; > +} > + > +struct psdi_ctxt { > + int (*cb)(struct pci_dev *, void *); > + void *arg; > +}; > + > +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg) > +{ > + struct psdi_ctxt *ctxt = arg; > + unsigned int bus, devfn; > + int rc = 0; > + > + /* > + * We don't iterate by walking pseg->alldevs_list here because that > + * would make the pcidevs_unlock()/lock() sequence below unsafe. > + */ > + for ( bus = 0; !rc && bus < 256; bus++ ) > + for ( devfn = 0; !rc && devfn < 256; devfn++ ) > { > struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn); > > if ( !pdev ) > continue; > > - if ( !pdev->domain ) > - { > - pdev->domain = ctxt->d; > - list_add(&pdev->domain_list, &ctxt->d->pdev_list); > - setup_one_hwdom_device(ctxt, pdev); > - } > - else if ( pdev->domain == dom_xen ) > - { > - pdev->domain = ctxt->d; > - setup_one_hwdom_device(ctxt, pdev); > - pdev->domain = dom_xen; > - } > - else if ( pdev->domain != ctxt->d ) > - printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", > - pdev->domain->domain_id, pseg->nr, bus, > - PCI_SLOT(devfn), PCI_FUNC(devfn)); > - > - if ( iommu_verbose ) > - { > - pcidevs_unlock(); > - process_pending_softirqs(); > - pcidevs_lock(); > - } > - } > + rc = ctxt->cb(pdev, ctxt->arg); > > - if ( !iommu_verbose ) > - { > + /* > + * Err on the safe side and assume the callback has taken > + * a significant amount of time. > + */ > pcidevs_unlock(); > process_pending_softirqs(); > pcidevs_lock(); > } > - } > > - return 0; > + return rc; > +} > + > +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg) > +{ > + struct psdi_ctxt ctxt = { .cb = cb, .arg = arg }; > + int rc; > + > + pcidevs_lock(); > + rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt); > + pcidevs_unlock(); > + > + return rc; > } > > void __hwdom_init setup_hwdom_pci_devices( > struct domain *d, int (*handler)(u8 devfn, struct pci_dev *)) > { > struct setup_hwdom ctxt = { .d = d, .handler = handler }; > + int rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); > > - pcidevs_lock(); > - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); > - pcidevs_unlock(); > + ASSERT(!rc); > } > > #ifdef CONFIG_ACPI > @@ -1294,24 +1317,18 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev) > } > #endif > > -static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > +static int dump_pci_device(struct pci_dev *pdev, 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("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > - pseg->nr, pdev->bus, > - PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > - pdev->domain ? pdev->domain->domain_id : -1, > - (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > - list_for_each_entry ( msi, &pdev->msi_list, list ) > - printk("%d ", msi->irq); > - printk(">\n"); > - } > + printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn), > + pdev->domain ? pdev->domain->domain_id : -1, > + (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > + list_for_each_entry ( msi, &pdev->msi_list, list ) > + printk("%d ", msi->irq); I know you're just copying the existing code, but this would be better to strip the space after the <, and use " %d" here, which will drop the final space before the > in the eventual output. Again, can be fixed on commit. ~Andrew > + printk(">\n"); > > return 0; > } > @@ -1319,9 +1336,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > static void dump_pci_devices(unsigned char ch) > { > printk("==== PCI devices ====\n"); > - pcidevs_lock(); > - pci_segments_iterate(_dump_pci_devices, NULL); > - pcidevs_unlock(); > + pci_pdevs_iterate(dump_pci_device, NULL); > } > > static int __init setup_dump_pcidevs(void) > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 04a9f46cc3..79eb25417b 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -154,6 +154,7 @@ int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus); > struct pci_dev *pci_lock_pdev(int seg, int bus, int devfn); > struct pci_dev *pci_lock_domain_pdev( > struct domain *, int seg, int bus, int devfn); > +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg); > > void setup_hwdom_pci_devices(struct domain *, > int (*)(u8 devfn, struct pci_dev *));
> -----Original Message----- > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > Sent: 16 July 2019 11:25 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Jan Beulich <jbeulich@suse.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Julien Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>; > Wei Liu <wl@xen.org> > Subject: Re: [PATCH v3 2/4] pci: add all-device iterator function... > > On 16/07/2019 11:16, Paul Durrant wrote: > > ...and use it for setup_hwdom_pci_devices() and dump_pci_devices(). > > > > The unlock/process-pending-softirqs/lock sequence that was in > > _setup_hwdom_pci_devices() is now done in the generic iterator function, > > which does mean it is also done (unnecessarily) in the case of > > dump_pci_devices(), since run_all_nonirq_keyhandlers() will call > > process_pending_softirqs() before invoking each key handler anyway, but > > this is not performance critical code. > > > > The "==== segment XXXX ====" headline that was in _dump_pci_devices() has > > been dropped because it is non-trivial to deal with it when using a > > generic all-device iterator and, since the segment number is included > > in every log line anyway, it didn't add much value anyway. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com> > > --- > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Tim Deegan <tim@xen.org> > > Cc: Wei Liu <wl@xen.org> > > > > v3: > > - Addressed review comments from Roger. > > > > v2: > > - New in v2. > > --- > > xen/drivers/passthrough/pci.c | 121 ++++++++++++++++++++++++------------------ > > xen/include/xen/pci.h | 1 + > > 2 files changed, 69 insertions(+), 53 deletions(-) > > > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > > index e88689425d..4bb9996049 100644 > > --- a/xen/drivers/passthrough/pci.c > > +++ b/xen/drivers/passthrough/pci.c > > @@ -1134,64 +1134,87 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom > *ctxt, > > ctxt->d->domain_id, err); > > } > > > > -static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg) > > +static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg) > > { > > struct setup_hwdom *ctxt = arg; > > - int bus, devfn; > > + struct domain *d = ctxt->d; > > > > - for ( bus = 0; bus < 256; bus++ ) > > + if ( !pdev->domain ) > > + { > > + pdev->domain = d; > > + list_add(&pdev->domain_list, &d->pdev_list); > > + setup_one_hwdom_device(ctxt, pdev); > > + } > > + else if ( pdev->domain == dom_xen ) > > { > > - for ( devfn = 0; devfn < 256; devfn++ ) > > + pdev->domain = d; > > + setup_one_hwdom_device(ctxt, pdev); > > + pdev->domain = dom_xen; > > + } > > + else if ( pdev->domain != d ) > > + printk(XENLOG_WARNING "Dom%pd owning %04x:%02x:%02x.%u?\n", > > Just %pd. It takes care of rendering d%u or d[$name] for system domains. > > Can be fixed on commit. > Ok. > > + pdev->domain, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > > + PCI_FUNC(pdev->devfn)); > > + > > + return 0; > > +} > > + > > +struct psdi_ctxt { > > + int (*cb)(struct pci_dev *, void *); > > + void *arg; > > +}; > > + > > +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg) > > +{ > > + struct psdi_ctxt *ctxt = arg; > > + unsigned int bus, devfn; > > + int rc = 0; > > + > > + /* > > + * We don't iterate by walking pseg->alldevs_list here because that > > + * would make the pcidevs_unlock()/lock() sequence below unsafe. > > + */ > > + for ( bus = 0; !rc && bus < 256; bus++ ) > > + for ( devfn = 0; !rc && devfn < 256; devfn++ ) > > { > > struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn); > > > > if ( !pdev ) > > continue; > > > > - if ( !pdev->domain ) > > - { > > - pdev->domain = ctxt->d; > > - list_add(&pdev->domain_list, &ctxt->d->pdev_list); > > - setup_one_hwdom_device(ctxt, pdev); > > - } > > - else if ( pdev->domain == dom_xen ) > > - { > > - pdev->domain = ctxt->d; > > - setup_one_hwdom_device(ctxt, pdev); > > - pdev->domain = dom_xen; > > - } > > - else if ( pdev->domain != ctxt->d ) > > - printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", > > - pdev->domain->domain_id, pseg->nr, bus, > > - PCI_SLOT(devfn), PCI_FUNC(devfn)); > > - > > - if ( iommu_verbose ) > > - { > > - pcidevs_unlock(); > > - process_pending_softirqs(); > > - pcidevs_lock(); > > - } > > - } > > + rc = ctxt->cb(pdev, ctxt->arg); > > > > - if ( !iommu_verbose ) > > - { > > + /* > > + * Err on the safe side and assume the callback has taken > > + * a significant amount of time. > > + */ > > pcidevs_unlock(); > > process_pending_softirqs(); > > pcidevs_lock(); > > } > > - } > > > > - return 0; > > + return rc; > > +} > > + > > +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg) > > +{ > > + struct psdi_ctxt ctxt = { .cb = cb, .arg = arg }; > > + int rc; > > + > > + pcidevs_lock(); > > + rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt); > > + pcidevs_unlock(); > > + > > + return rc; > > } > > > > void __hwdom_init setup_hwdom_pci_devices( > > struct domain *d, int (*handler)(u8 devfn, struct pci_dev *)) > > { > > struct setup_hwdom ctxt = { .d = d, .handler = handler }; > > + int rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); > > > > - pcidevs_lock(); > > - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); > > - pcidevs_unlock(); > > + ASSERT(!rc); > > } > > > > #ifdef CONFIG_ACPI > > @@ -1294,24 +1317,18 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev) > > } > > #endif > > > > -static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > > +static int dump_pci_device(struct pci_dev *pdev, 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("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > > - pseg->nr, pdev->bus, > > - PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > > - pdev->domain ? pdev->domain->domain_id : -1, > > - (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > > - list_for_each_entry ( msi, &pdev->msi_list, list ) > > - printk("%d ", msi->irq); > > - printk(">\n"); > > - } > > + printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > > + PCI_FUNC(pdev->devfn), > > + pdev->domain ? pdev->domain->domain_id : -1, > > + (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > > + list_for_each_entry ( msi, &pdev->msi_list, list ) > > + printk("%d ", msi->irq); > > I know you're just copying the existing code, but this would be better > to strip the space after the <, and use " %d" here, which will drop the > final space before the > in the eventual output. > > Again, can be fixed on commit. If you prefer that then that's fine. Paul > > ~Andrew > > > + printk(">\n"); > > > > return 0; > > } > > @@ -1319,9 +1336,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > > static void dump_pci_devices(unsigned char ch) > > { > > printk("==== PCI devices ====\n"); > > - pcidevs_lock(); > > - pci_segments_iterate(_dump_pci_devices, NULL); > > - pcidevs_unlock(); > > + pci_pdevs_iterate(dump_pci_device, NULL); > > } > > > > static int __init setup_dump_pcidevs(void) > > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > > index 04a9f46cc3..79eb25417b 100644 > > --- a/xen/include/xen/pci.h > > +++ b/xen/include/xen/pci.h > > @@ -154,6 +154,7 @@ int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus); > > struct pci_dev *pci_lock_pdev(int seg, int bus, int devfn); > > struct pci_dev *pci_lock_domain_pdev( > > struct domain *, int seg, int bus, int devfn); > > +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg); > > > > void setup_hwdom_pci_devices(struct domain *, > > int (*)(u8 devfn, struct pci_dev *));
On 16.07.2019 12:16, Paul Durrant wrote: > ...and use it for setup_hwdom_pci_devices() and dump_pci_devices(). > > The unlock/process-pending-softirqs/lock sequence that was in > _setup_hwdom_pci_devices() is now done in the generic iterator function, > which does mean it is also done (unnecessarily) in the case of > dump_pci_devices(), since run_all_nonirq_keyhandlers() will call > process_pending_softirqs() before invoking each key handler anyway, but > this is not performance critical code. > > The "==== segment XXXX ====" headline that was in _dump_pci_devices() has > been dropped because it is non-trivial to deal with it when using a > generic all-device iterator and, since the segment number is included > in every log line anyway, it didn't add much value anyway. For overall output volume it would perhaps be better to have the headline and drop the redundancy on every line. But that's just a side note, not something I expect you to change. > +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg) > +{ > + struct psdi_ctxt *ctxt = arg; > + unsigned int bus, devfn; > + int rc = 0; > + > + /* > + * We don't iterate by walking pseg->alldevs_list here because that > + * would make the pcidevs_unlock()/lock() sequence below unsafe. > + */ > + for ( bus = 0; !rc && bus < 256; bus++ ) > + for ( devfn = 0; !rc && devfn < 256; devfn++ ) > { > struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn); > > if ( !pdev ) > continue; > > - if ( !pdev->domain ) > - { > - pdev->domain = ctxt->d; > - list_add(&pdev->domain_list, &ctxt->d->pdev_list); > - setup_one_hwdom_device(ctxt, pdev); > - } > - else if ( pdev->domain == dom_xen ) > - { > - pdev->domain = ctxt->d; > - setup_one_hwdom_device(ctxt, pdev); > - pdev->domain = dom_xen; > - } > - else if ( pdev->domain != ctxt->d ) > - printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", > - pdev->domain->domain_id, pseg->nr, bus, > - PCI_SLOT(devfn), PCI_FUNC(devfn)); > - > - if ( iommu_verbose ) > - { > - pcidevs_unlock(); > - process_pending_softirqs(); > - pcidevs_lock(); > - } > - } > + rc = ctxt->cb(pdev, ctxt->arg); > > - if ( !iommu_verbose ) > - { > + /* > + * Err on the safe side and assume the callback has taken > + * a significant amount of time. > + */ > pcidevs_unlock(); > process_pending_softirqs(); > pcidevs_lock(); This behavior is not generally acceptable to an arbitrary caller. Therefore I think a prominent notice is needed at the top of the function. Even worse, the latest post-boot and outside of debug-key handling this isn't safe at all: You'd have to re-check that pseg hasn't gone away (this can't happen right now, but isn't impossible in principle), and more generally that the pseg tree hasn't changed. Since this would be a little difficult to arrange, I think doing so would better be left to the callback, or be controlled by an extra argument passed to pci_pdevs_iterate() (in both cases eliminating the need for a prominent warning at the top of the function). > @@ -1294,24 +1317,18 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev) > } > #endif > > -static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > +static int dump_pci_device(struct pci_dev *pdev, 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("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > - pseg->nr, pdev->bus, > - PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), > - pdev->domain ? pdev->domain->domain_id : -1, > - (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > - list_for_each_entry ( msi, &pdev->msi_list, list ) > - printk("%d ", msi->irq); > - printk(">\n"); > - } > + printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > + PCI_FUNC(pdev->devfn), > + pdev->domain ? pdev->domain->domain_id : -1, > + (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); > + list_for_each_entry ( msi, &pdev->msi_list, list ) > + printk("%d ", msi->irq); > + printk(">\n"); > > return 0; > } Seeing this code I don't think it would be difficult to arrange for the head line to be logged whenever the segment changes. You don't use "arg" right now, after all, which could point to a local variable ... > @@ -1319,9 +1336,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg) > static void dump_pci_devices(unsigned char ch) > { > printk("==== PCI devices ====\n"); > - pcidevs_lock(); > - pci_segments_iterate(_dump_pci_devices, NULL); > - pcidevs_unlock(); > + pci_pdevs_iterate(dump_pci_device, NULL); ... here, initialized to e.g. UINT_MAX. Jan
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index e88689425d..4bb9996049 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1134,64 +1134,87 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt, ctxt->d->domain_id, err); } -static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg) +static int __hwdom_init setup_hwdom_pci_device(struct pci_dev *pdev, void *arg) { struct setup_hwdom *ctxt = arg; - int bus, devfn; + struct domain *d = ctxt->d; - for ( bus = 0; bus < 256; bus++ ) + if ( !pdev->domain ) + { + pdev->domain = d; + list_add(&pdev->domain_list, &d->pdev_list); + setup_one_hwdom_device(ctxt, pdev); + } + else if ( pdev->domain == dom_xen ) { - for ( devfn = 0; devfn < 256; devfn++ ) + pdev->domain = d; + setup_one_hwdom_device(ctxt, pdev); + pdev->domain = dom_xen; + } + else if ( pdev->domain != d ) + printk(XENLOG_WARNING "Dom%pd owning %04x:%02x:%02x.%u?\n", + pdev->domain, pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn)); + + return 0; +} + +struct psdi_ctxt { + int (*cb)(struct pci_dev *, void *); + void *arg; +}; + +static int pci_segment_devices_iterate(struct pci_seg *pseg, void *arg) +{ + struct psdi_ctxt *ctxt = arg; + unsigned int bus, devfn; + int rc = 0; + + /* + * We don't iterate by walking pseg->alldevs_list here because that + * would make the pcidevs_unlock()/lock() sequence below unsafe. + */ + for ( bus = 0; !rc && bus < 256; bus++ ) + for ( devfn = 0; !rc && devfn < 256; devfn++ ) { struct pci_dev *pdev = pci_get_pdev(pseg->nr, bus, devfn); if ( !pdev ) continue; - if ( !pdev->domain ) - { - pdev->domain = ctxt->d; - list_add(&pdev->domain_list, &ctxt->d->pdev_list); - setup_one_hwdom_device(ctxt, pdev); - } - else if ( pdev->domain == dom_xen ) - { - pdev->domain = ctxt->d; - setup_one_hwdom_device(ctxt, pdev); - pdev->domain = dom_xen; - } - else if ( pdev->domain != ctxt->d ) - printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", - pdev->domain->domain_id, pseg->nr, bus, - PCI_SLOT(devfn), PCI_FUNC(devfn)); - - if ( iommu_verbose ) - { - pcidevs_unlock(); - process_pending_softirqs(); - pcidevs_lock(); - } - } + rc = ctxt->cb(pdev, ctxt->arg); - if ( !iommu_verbose ) - { + /* + * Err on the safe side and assume the callback has taken + * a significant amount of time. + */ pcidevs_unlock(); process_pending_softirqs(); pcidevs_lock(); } - } - return 0; + return rc; +} + +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg) +{ + struct psdi_ctxt ctxt = { .cb = cb, .arg = arg }; + int rc; + + pcidevs_lock(); + rc = pci_segments_iterate(pci_segment_devices_iterate, &ctxt); + pcidevs_unlock(); + + return rc; } void __hwdom_init setup_hwdom_pci_devices( struct domain *d, int (*handler)(u8 devfn, struct pci_dev *)) { struct setup_hwdom ctxt = { .d = d, .handler = handler }; + int rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); - pcidevs_lock(); - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); - pcidevs_unlock(); + ASSERT(!rc); } #ifdef CONFIG_ACPI @@ -1294,24 +1317,18 @@ bool_t pcie_aer_get_firmware_first(const struct pci_dev *pdev) } #endif -static int _dump_pci_devices(struct pci_seg *pseg, void *arg) +static int dump_pci_device(struct pci_dev *pdev, 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("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", - pseg->nr, pdev->bus, - PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), - pdev->domain ? pdev->domain->domain_id : -1, - (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); - list_for_each_entry ( msi, &pdev->msi_list, list ) - printk("%d ", msi->irq); - printk(">\n"); - } + printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ", + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), + PCI_FUNC(pdev->devfn), + pdev->domain ? pdev->domain->domain_id : -1, + (pdev->node != NUMA_NO_NODE) ? pdev->node : -1); + list_for_each_entry ( msi, &pdev->msi_list, list ) + printk("%d ", msi->irq); + printk(">\n"); return 0; } @@ -1319,9 +1336,7 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg) static void dump_pci_devices(unsigned char ch) { printk("==== PCI devices ====\n"); - pcidevs_lock(); - pci_segments_iterate(_dump_pci_devices, NULL); - pcidevs_unlock(); + pci_pdevs_iterate(dump_pci_device, NULL); } static int __init setup_dump_pcidevs(void) diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 04a9f46cc3..79eb25417b 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -154,6 +154,7 @@ int find_upstream_bridge(u16 seg, u8 *bus, u8 *devfn, u8 *secbus); struct pci_dev *pci_lock_pdev(int seg, int bus, int devfn); struct pci_dev *pci_lock_domain_pdev( struct domain *, int seg, int bus, int devfn); +int pci_pdevs_iterate(int (*cb)(struct pci_dev *, void *), void *arg); void setup_hwdom_pci_devices(struct domain *, int (*)(u8 devfn, struct pci_dev *));