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

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

On Tue, Feb 11, 2025 at 12:34:41PM +0100, Jan Beulich wrote:
> 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.

Indeed, I always forget about this quirk of recursive locks.  I will
need to introduce a new pcidevs_trylock() helper then.

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

Well, I mostly did this because the function is traversing the list of
PCI devices list without any locking, and wanted to make it clear
interrupts might not be safe in case they perform modifications to the
list of PCI devices (I don't think we have such usage).

Since I need to do a v4 of this one I don't mind dropping the assert.

Thanks, Roger.