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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2025 16:06, Roger Pau Monne wrote:
> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -177,6 +177,7 @@ static void nmi_shootdown_cpus(void)
>  
>          disable_IO_APIC();
>          hpet_disable();
> +        pci_disable_msi_all();
>      }

Apart from my concern below regarding use of the function in this context,
for both uses I wonder in how far the order of the three calls above may
matter. I can't really give a precise reason, but to me it feels like the
PCI device processing may better be done first.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1803,6 +1803,39 @@ 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;
> +}
> +
> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
> +                        void *arg)
> +{
> +    struct segment_iter iter = {
> +        .handler = handler,
> +        .arg = arg,
> +    };
> +
> +    return pci_segments_iterate(iterate_all, &iter) ?: iter.rc;
> +}

My earlier concern remains as far as e.g. list traversal goes, especially
when we're called from nmi_shootdown_cpus() context. The lists themselves
may be screwed, after all. Whereas disable_IO_APIC() and hpet_disable()
don't involve any list traversal, and even if they did those lists would
be stable post-boot.

We may want to talk about the up- and down-sides of this on the x86 call
later in the day.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -226,6 +226,10 @@ struct pci_dev *pci_get_pdev(const struct domain *d, 
> pci_sbdf_t sbdf);
>  struct pci_dev *pci_get_real_pdev(pci_sbdf_t sbdf);
>  void pci_check_disable_device(u16 seg, u8 bus, u8 devfn);
>  
> +/* Iterate without locking or preemption over all PCI devices known by Xen. 
> */
> +int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
> +                        void *arg);

Oh, I see you added the comment here that I did ask for. As it's pretty
important for people to notice, may I ask that it be replicated in (or
ahead of) the function definition? And then there perhaps also mentioning
that one needs to be aware of the function being expected to run with IRQs
off (to make clear that it's not a simple matter of adding preemption
checks, for example).

Jan