Re: [PATCH for-4.20 v3 4/5] x86/pci: disable MSI(-X) on all devices at shutdown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.02.2025 12:02, Roger Pau Monne wrote:
> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -175,6 +175,13 @@ static void nmi_shootdown_cpus(void)
>           */
>          x2apic_enabled = (current_local_apic_mode() == APIC_MODE_X2APIC);
>  
> +        if ( !pcidevs_locked() )
> +            /*
> +             * Assume the PCI device list to be in a consistent state if the
> +             * lock is not held when the crash happened.
> +             */
> +            pci_disable_msi_all();

Hmm, I really meant try-lock to be used here. For recursive locks
rspin_is_locked() tells you only whether the local CPU owns the lock,
whereas here you want to know whether anyone owns it.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1802,6 +1802,45 @@ int iommu_do_pci_domctl(
>      return ret;
>  }
>  
> +struct segment_iter {
> +    int (*handler)(struct pci_dev *pdev, void *arg);
> +    void *arg;
> +    int rc;
> +};
> +
> +static int cf_check iterate_all(struct pci_seg *pseg, void *arg)
> +{
> +    struct segment_iter *iter = arg;
> +    struct pci_dev *pdev;
> +
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        int rc = iter->handler(pdev, iter->arg);
> +
> +        if ( !iter->rc )
> +            iter->rc = rc;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Iterate without locking or preemption over all PCI devices known by Xen.
> + * Expected to be called with interrupts disabled.
> + */
> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
> +                        void *arg)
> +{
> +    struct segment_iter iter = {
> +        .handler = handler,
> +        .arg = arg,
> +    };
> +
> +    ASSERT(!local_irq_is_enabled());

I'm not sure we want to go this far. Maybe my earlier comment was ambiguous
though. What I meant is that the function needs to be documented to be
prepared to be called with IRQs off. I didn't mean that to be a requirement
to call the function (as there's no dependency on that, afaics).

Jan