Re: [Xen-devel] [PATCH v3 2/4] pci: add all-device iterator function...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Sent: 16 July 2019 11:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> 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@xxxxxxxxxx>
> > Reviewed-by: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> >
> > 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 *));

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel