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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 16.07.2019 12: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.

For overall output volume it would perhaps be better to have the
headline and drop the redundancy on every line. But that's just a
side note, not something I expect you to change.

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

This behavior is not generally acceptable to an arbitrary caller.
Therefore I think a prominent notice is needed at the top of the
function.

Even worse, the latest post-boot and outside of debug-key handling
this isn't safe at all: You'd have to re-check that pseg hasn't
gone away (this can't happen right now, but isn't impossible in
principle), and more generally that the pseg tree hasn't changed.
Since this would be a little difficult to arrange, I think doing
so would better be left to the callback, or be controlled by an
extra argument passed to pci_pdevs_iterate() (in both cases
eliminating the need for a prominent warning at the top of the
function).

> @@ -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);
> +    printk(">\n");
>  
>      return 0;
>  }

Seeing this code I don't think it would be difficult to arrange for
the head line to be logged whenever the segment changes. You don't
use "arg" right now, after all, which could point to a local
variable ...

> @@ -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);

... here, initialized to e.g. UINT_MAX.

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