Re: [PATCH 3/3] s390x/pci: drive ISM reset from subsystem reset

[Cédric Le Goater]

On 1/17/24 22:11, Matthew Rosato wrote:
> On 1/17/24 10:19 AM, Matthew Rosato wrote:
> > On 1/17/24 6:01 AM, Cédric Le Goater wrote:
> > > Adding Alex,
> > >
> > > On 1/16/24 23:31, Matthew Rosato wrote:
> > > > ISM devices are sensitive to manipulation of the IOMMU, so the ISM device
> > > > needs to be reset before the vfio-pci device is reset (triggering a full
> > > > UNMAP).  In order to ensure this occurs, trigger ISM device resets from
> > > > subsystem_reset before triggering the PCI bus reset (which will also
> > > > trigger vfio-pci reset).  This only needs to be done for ISM devices
> > > > which were enabled for use by the guest.
> > > > Further, ensure that AIF is disabled as part of the reset event.
> > > >
> > > > Fixes: ef1535901a ("s390x: do a subsystem reset before the unprotect on reboot")
> > > > Fixes: 03451953c7 ("s390x/pci: reset ISM passthrough devices on shutdown and system 
> > > > reset")
> > > > Reported-by: Cédric Le Goater <clg@redhat.com>
> > > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > > > ---
> > > >    hw/s390x/s390-pci-bus.c         | 26 +++++++++++++++++---------
> > > >    hw/s390x/s390-virtio-ccw.c      |  2 ++
> > > >    include/hw/s390x/s390-pci-bus.h |  1 +
> > > >    3 files changed, 20 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > > index 347580ebac..3e57d5faca 100644
> > > > --- a/hw/s390x/s390-pci-bus.c
> > > > +++ b/hw/s390x/s390-pci-bus.c
> > > > @@ -151,20 +151,12 @@ static void s390_pci_shutdown_notifier(Notifier *n, void 
> > > > *opaque)
> > > >        pci_device_reset(pbdev->pdev);
> > > >    }
> > > >    -static void s390_pci_reset_cb(void *opaque)
> > > > -{
> > > > -    S390PCIBusDevice *pbdev = opaque;
> > > > -
> > > > -    pci_device_reset(pbdev->pdev);
> > > > -}
> > > > -
> > > >    static void s390_pci_perform_unplug(S390PCIBusDevice *pbdev)
> > > >    {
> > > >        HotplugHandler *hotplug_ctrl;
> > > >          if (pbdev->pft == ZPCI_PFT_ISM) {
> > > >            notifier_remove(&pbdev->shutdown_notifier);
> > > > -        qemu_unregister_reset(s390_pci_reset_cb, pbdev);
> > > >        }
> > > >          /* Unplug the PCI device */
> > > > @@ -1132,7 +1124,6 @@ static void s390_pcihost_plug(HotplugHandler 
> > > > *hotplug_dev, DeviceState *dev,
> > > >                if (pbdev->pft == ZPCI_PFT_ISM) {
> > > >                    pbdev->shutdown_notifier.notify = s390_pci_shutdown_notifier;
> > > >                    qemu_register_shutdown_notifier(&pbdev->shutdown_notifier);
> > > > -                qemu_register_reset(s390_pci_reset_cb, pbdev);
> > > >                }
> > > >            } else {
> > > >                pbdev->fh |= FH_SHM_EMUL;
> > > > @@ -1279,6 +1270,23 @@ static void s390_pci_enumerate_bridge(PCIBus *bus, 
> > > > PCIDevice *pdev,
> > > >        pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, s->bus_no, 1);
> > > >    }
> > > >    +void s390_pci_ism_reset(void)
> > > > +{
> > > > +    S390pciState *s = s390_get_phb();
> > > > +
> > > > +    S390PCIBusDevice *pbdev, *next;
> > > > +
> > > > +    /* Trigger reset event for each passthrough ISM device currently in-use */
> > > > +    QTAILQ_FOREACH_SAFE(pbdev, &s->zpci_devs, link, next) {
> > > > +        if (pbdev->interp && pbdev->pft == ZPCI_PFT_ISM &&
> > > > +            pbdev->fh & FH_MASK_ENABLE) {
> > > > +            s390_pci_kvm_aif_disable(pbdev);
> > > > +
> > > > +            pci_device_reset(pbdev->pdev);
> > > > +        }
> > > > +    }
> > > > +}
> > >
> > >
> > > Could we instead define a VFIOPCIDevice::resetfn handler for these
> > > ISM devices (1014:04ed) ? This would be cleaner if possible.
> > >
> > > If so, as a prerequisite, we would need to introduce in a little VFIO
> > > helper to define custom reset handlers.
> > >
> > > Thanks,
> > >
> > > C.
> >
> > Oh interesting, I had not noticed that.  This may well work -- resetfn is 
> > currently setup via vfio_setup_resetfn_quirk but it would probably be easier to 
> > have a helper that takes the vdev and a function pointer so that we can provide 
> > a platform-specific reset handler (rather than having hw/vfio/pci-quirks.c 
> > worry about CONFIG_S390 etc).  I'll have to play around with this.
> >   
>
> Hmm, it was a good idea but I don't think this will work.  I tried to hack 
> something together today but I'm definitely seeing paths where the 
> vfio_listener_region_del happens before the call to vfio_pci_reset (which would 
> ultimately trigger the new custom resetfn).

OK.
 
> Perhaps we should stick with the call from subsystem_reset -- it will ensure 
> that the ISM cleanup happens after guest CPUs are stopped but before vfio does 
> its cleanup.

Let's keep the subsystem_reset() method then. Please add a comment on the reset 
ordering.

Thanks,

C.



>