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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> +               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 *));


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