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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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

Thanks.

> Just some trivial comments.
> 
> Thanks.
> 
> > ---
> > 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>
> >
> > 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.

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