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

[Eric Farman]

On Tue, 2024-01-16 at 17:31 -0500, 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);

Do we care about the loss of a reset for ISM devices in a
!interpretation case? (I seem to think such a configuration is not
possible today, and so we don't care, but could use a reminder.)

> +        }
> +    }
> +}
> +
>  static void s390_pcihost_reset(DeviceState *dev)
>  {
>      S390pciState *s = S390_PCI_HOST_BRIDGE(dev);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1169e20b94..4de04f7e9f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -118,6 +118,8 @@ static void subsystem_reset(void)
>      DeviceState *dev;
>      int i;
>  
> +    s390_pci_ism_reset();
> +
>      for (i = 0; i < ARRAY_SIZE(reset_dev_types); i++) {
>          dev = DEVICE(object_resolve_path_type("",
> reset_dev_types[i], NULL));
>          if (dev) {
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-
> pci-bus.h
> index 435e788867..2c43ea123f 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -401,5 +401,6 @@ S390PCIBusDevice
> *s390_pci_find_dev_by_target(S390pciState *s,
>                                                const char *target);
>  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
>                                                 S390PCIBusDevice
> *pbdev);
> +void s390_pci_ism_reset(void);
>  
>  #endif