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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

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.

> +               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.

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