| Message ID | 20190715123710.1780-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu groups + cleanup | expand |
On Mon, Jul 15, 2019 at 01:37:08PM +0100, 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> Just some trivial comments. Thanks. > --- > 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> > > v2: > - New in v2. > --- > xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++------------------- > xen/include/xen/pci.h | 1 + > 2 files changed, 68 insertions(+), 53 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index e88689425d..179cb7e17e 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1134,54 +1134,78 @@ 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 ) > { > - for ( devfn = 0; devfn < 256; devfn++ ) > + pdev->domain = d; > + list_add(&pdev->domain_list, &d->pdev_list); > + setup_one_hwdom_device(ctxt, pdev); > + } > + else if ( pdev->domain == dom_xen ) > + { > + pdev->domain = d; > + setup_one_hwdom_device(ctxt, pdev); > + pdev->domain = dom_xen; > + } > + else if ( pdev->domain != d ) > + printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", > + pdev->domain->domain_id, pdev->seg, pdev->bus, You can use %pd here to print the domain. > + 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; > + int bus, devfn; unsigned for both the above. > + 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)); > + rc = ctxt->cb(pdev, ctxt->arg); > > - if ( iommu_verbose ) > - { > - pcidevs_unlock(); > - process_pending_softirqs(); > - pcidevs_lock(); > - } > - } > - > - 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( > @@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices( > { > struct setup_hwdom ctxt = { .d = d, .handler = handler }; > > - pcidevs_lock(); > - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); > - pcidevs_unlock(); > + pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); Since this now returns an error code it wold be good to handle it, even if it's just: rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); if ( rc ) ASSERT_UNREACHABLE(); setup_hwdom_pci_device will always return 0, hence a failure here is not possible AFAICT. Same with the other usage of pci_pdevs_iterate below. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 15 July 2019 16:21 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew > Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; > Julien Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com> > Subject: Re: [Xen-devel] [PATCH v2 2/4] pci: add all-device iterator function... > > On Mon, Jul 15, 2019 at 01:37:08PM +0100, 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> > Thanks. > Just some trivial comments. > > Thanks. > > > --- > > 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> > > > > v2: > > - New in v2. > > --- > > xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++------------------- > > xen/include/xen/pci.h | 1 + > > 2 files changed, 68 insertions(+), 53 deletions(-) > > > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > > index e88689425d..179cb7e17e 100644 > > --- a/xen/drivers/passthrough/pci.c > > +++ b/xen/drivers/passthrough/pci.c > > @@ -1134,54 +1134,78 @@ 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 ) > > { > > - for ( devfn = 0; devfn < 256; devfn++ ) > > + pdev->domain = d; > > + list_add(&pdev->domain_list, &d->pdev_list); > > + setup_one_hwdom_device(ctxt, pdev); > > + } > > + else if ( pdev->domain == dom_xen ) > > + { > > + pdev->domain = d; > > + setup_one_hwdom_device(ctxt, pdev); > > + pdev->domain = dom_xen; > > + } > > + else if ( pdev->domain != d ) > > + printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", > > + pdev->domain->domain_id, pdev->seg, pdev->bus, > > You can use %pd here to print the domain. > Oh, I didn't know about that one. > > + 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; > > + int bus, devfn; > > unsigned for both the above. > Ok. > > + 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)); > > + rc = ctxt->cb(pdev, ctxt->arg); > > > > - if ( iommu_verbose ) > > - { > > - pcidevs_unlock(); > > - process_pending_softirqs(); > > - pcidevs_lock(); > > - } > > - } > > - > > - 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( > > @@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices( > > { > > struct setup_hwdom ctxt = { .d = d, .handler = handler }; > > > > - pcidevs_lock(); > > - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); > > - pcidevs_unlock(); > > + pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); > > Since this now returns an error code it wold be good to handle it, > even if it's just: > > rc = pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); > if ( rc ) > ASSERT_UNREACHABLE(); > > setup_hwdom_pci_device will always return 0, hence a failure here is > not possible AFAICT. Same with the other usage of pci_pdevs_iterate > below. An ASSERT in setup_hwdom_pci_devices() sounds like a good idea. I think I'll leave one out of dump_pci_devices() though, since that's a debug-key handler. Paul > > Thanks, Roger.
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index e88689425d..179cb7e17e 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1134,54 +1134,78 @@ 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 ) { - for ( devfn = 0; devfn < 256; devfn++ ) + pdev->domain = d; + list_add(&pdev->domain_list, &d->pdev_list); + setup_one_hwdom_device(ctxt, pdev); + } + else if ( pdev->domain == dom_xen ) + { + pdev->domain = d; + setup_one_hwdom_device(ctxt, pdev); + pdev->domain = dom_xen; + } + else if ( pdev->domain != d ) + printk(XENLOG_WARNING "Dom%d owning %04x:%02x:%02x.%u?\n", + pdev->domain->domain_id, 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; + 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)); + rc = ctxt->cb(pdev, ctxt->arg); - if ( iommu_verbose ) - { - pcidevs_unlock(); - process_pending_softirqs(); - pcidevs_lock(); - } - } - - 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( @@ -1189,9 +1213,7 @@ void __hwdom_init setup_hwdom_pci_devices( { struct setup_hwdom ctxt = { .d = d, .handler = handler }; - pcidevs_lock(); - pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt); - pcidevs_unlock(); + pci_pdevs_iterate(setup_hwdom_pci_device, &ctxt); } #ifdef CONFIG_ACPI @@ -1294,24 +1316,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 +1335,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 *));
...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> --- 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> v2: - New in v2. --- xen/drivers/passthrough/pci.c | 120 +++++++++++++++++++++++------------------- xen/include/xen/pci.h | 1 + 2 files changed, 68 insertions(+), 53 deletions(-)