[PATCH 0/3] s390x/pci: fix ISM reset

[PATCH 0/3] s390x/pci: fix ISM reset

[Matthew Rosato]

Commit ef1535901a0 (re-)introduced an issue where passthrough ISM devices
on s390x would enter an error state after reboot.  This was previously fixed
by 03451953c79e, using device reset callbacks, however the change in
ef1535901a0 effectively triggers a cold reset of the pci bus before the
device reset callbacks are triggered.

To resolve this, this series proposes to remove the use of the reset callback
for ISM cleanup and instead trigger ISM reset from subsystem_reset before 
triggering bus resets.  This has to happen before the bus resets because the
reset of s390-pcihost will trigger reset of the PCI bus followed by the
s390-pci bus, and the former will trigger vfio-pci reset / the aperture-wide
unmap that ISM gets upset about.
 
  /s390-pcihost (s390-pcihost)
    /pci.0 (PCI)
    /s390-pcibus.0 (s390-pcibus)
    
While fixing this, it was also noted that kernel warnings could be seen that
indicate a guest ISC reference count error.  That's because in some reset
cases we were not bothering to disable AIF, but would again re-enable it after
the reset (causing the reference count to grow erroneously).  This was a base
issue that went unnoticed because the kernel previously did not detect and
issue a warning for this scenario. 

Matthew Rosato (3):
  s390x/pci: avoid double enable/disable of aif
  s390x/pci: refresh fh before disabling aif
  s390x/pci: drive ISM reset from subsystem reset

 hw/s390x/s390-pci-bus.c         | 26 ++++++++++++++++---------
 hw/s390x/s390-pci-kvm.c         | 34 +++++++++++++++++++++++++++++++--
 hw/s390x/s390-virtio-ccw.c      |  2 ++
 include/hw/s390x/s390-pci-bus.h |  2 ++
 4 files changed, 53 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/3] s390x/pci: avoid double enable/disable of aif

[Matthew Rosato]

Use a flag to keep track of whether AIF is currently enabled.  This can be
used to avoid enabling/disabling AIF multiple times as well as to determine
whether or not it should be disabled during reset processing.

Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for interpreted devices")
Reported-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-kvm.c         | 25 +++++++++++++++++++++++--
 include/hw/s390x/s390-pci-bus.h |  1 +
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
index ff41e4106d..f7e10cfa72 100644
--- a/hw/s390x/s390-pci-kvm.c
+++ b/hw/s390x/s390-pci-kvm.c
@@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void)
 
 int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist)
 {
+    int rc;
     struct kvm_s390_zpci_op args = {
         .fh = pbdev->fh,
         .op = KVM_S390_ZPCIOP_REG_AEN,
@@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist)
         .u.reg_aen.flags = (assist) ? 0 : KVM_S390_ZPCIOP_REGAEN_HOST
     };
 
-    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
+    if (pbdev->aif) {
+        return -EINVAL;
+    }
+
+    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
+    if (rc == 0) {
+        pbdev->aif = true;
+    }
+
+    return rc;
 }
 
 int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
 {
+    int rc;
+
     struct kvm_s390_zpci_op args = {
         .fh = pbdev->fh,
         .op = KVM_S390_ZPCIOP_DEREG_AEN
     };
 
-    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
+    if (!pbdev->aif) {
+        return -EINVAL;
+    }
+
+    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
+    if (rc == 0) {
+        pbev->aif = false;
+    }
+
+    return rc;
 }
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index b1bdbeaeb5..435e788867 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -361,6 +361,7 @@ struct S390PCIBusDevice {
     bool unplug_requested;
     bool interp;
     bool forwarding_assist;
+    bool aif;
     QTAILQ_ENTRY(S390PCIBusDevice) link;
 };
 
-- 
2.43.0

[PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Matthew Rosato]

Typically we refresh the host fh during CLP enable, however it's possible
that the device goes through multiple reset events before the guest
performs another CLP enable.  Let's handle this for now by refreshing the
host handle from vfio before disabling aif.

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-kvm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
index f7e10cfa72..9eef4fc3ec 100644
--- a/hw/s390x/s390-pci-kvm.c
+++ b/hw/s390x/s390-pci-kvm.c
@@ -18,6 +18,7 @@
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/s390-pci-kvm.h"
 #include "hw/s390x/s390-pci-inst.h"
+#include "hw/s390x/s390-pci-vfio.h"
 #include "cpu_models.h"
 
 bool s390_pci_kvm_interp_allowed(void)
@@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
         return -EINVAL;
     }
 
+    /*
+     * The device may have already been reset but we still want to relinquish
+     * the guest ISC, so always be sure to use an up-to-date host fh.
+     */
+    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
+        return -EPERM;
+    }
+
     rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
     if (rc == 0) {
-        pbev->aif = false;
+        pbdev->aif = false;
     }
 
     return rc;
-- 
2.43.0

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

[Matthew Rosato]

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);
+        }
+    }
+}
+
 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
-- 
2.43.0

Re: [PATCH 1/3] s390x/pci: avoid double enable/disable of aif

[Eric Farman]

On Tue, 2024-01-16 at 17:31 -0500, Matthew Rosato wrote:
> Use a flag to keep track of whether AIF is currently enabled.  This
> can be
> used to avoid enabling/disabling AIF multiple times as well as to
> determine
> whether or not it should be disabled during reset processing.
> 
> Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for
> interpreted devices")
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  hw/s390x/s390-pci-kvm.c         | 25 +++++++++++++++++++++++--
>  include/hw/s390x/s390-pci-bus.h |  1 +
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index ff41e4106d..f7e10cfa72 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void)
>  
>  int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib,
> bool assist)
>  {
> +    int rc;
>      struct kvm_s390_zpci_op args = {
>          .fh = pbdev->fh,
>          .op = KVM_S390_ZPCIOP_REG_AEN,
> @@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice
> *pbdev, ZpciFib *fib, bool assist)
>          .u.reg_aen.flags = (assist) ? 0 :
> KVM_S390_ZPCIOP_REGAEN_HOST
>      };
>  
> -    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (pbdev->aif) {
> +        return -EINVAL;
> +    }
> +
> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (rc == 0) {
> +        pbdev->aif = true;
> +    }
> +
> +    return rc;
>  }
>  
>  int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>  {
> +    int rc;
> +
>      struct kvm_s390_zpci_op args = {
>          .fh = pbdev->fh,
>          .op = KVM_S390_ZPCIOP_DEREG_AEN
>      };
>  
> -    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (!pbdev->aif) {
> +        return -EINVAL;
> +    }
> +
> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (rc == 0) {
> +        pbev->aif = false;

s/pbev/pbdev/

You fix this in patch 2. :)

With that fixed:

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> +    }
> +
> +    return rc;
>  }
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-
> pci-bus.h
> index b1bdbeaeb5..435e788867 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -361,6 +361,7 @@ struct S390PCIBusDevice {
>      bool unplug_requested;
>      bool interp;
>      bool forwarding_assist;
> +    bool aif;
>      QTAILQ_ENTRY(S390PCIBusDevice) link;
>  };
>  

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Eric Farman]

On Tue, 2024-01-16 at 17:31 -0500, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's
> possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing
> the
> host handle from vfio before disabling aif.
> 
> 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-kvm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

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

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Cédric Le Goater]

Hello Matthew,

On 1/16/24 23:31, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing the
> host handle from vfio before disabling aif.
> 
> 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-kvm.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index f7e10cfa72..9eef4fc3ec 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -18,6 +18,7 @@
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "hw/s390x/s390-pci-kvm.h"
>   #include "hw/s390x/s390-pci-inst.h"
> +#include "hw/s390x/s390-pci-vfio.h"
>   #include "cpu_models.h"
>   
>   bool s390_pci_kvm_interp_allowed(void)
> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>           return -EINVAL;
>       }
>   
> +    /*
> +     * The device may have already been reset but we still want to relinquish
> +     * the guest ISC, so always be sure to use an up-to-date host fh.
> +     */
> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
> +        return -EPERM;
> +    }
> +
>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>       if (rc == 0) {
> -        pbev->aif = false;
> +        pbdev->aif = false;
>       }

This belongs to patch 1.


Thanks,

C.


>   
>       return rc;

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Cédric Le Goater]

On 1/16/24 23:31, Matthew Rosato wrote:
> Typically we refresh the host fh during CLP enable, however it's possible
> that the device goes through multiple reset events before the guest
> performs another CLP enable.  Let's handle this for now by refreshing the
> host handle from vfio before disabling aif.
> 
> 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-kvm.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index f7e10cfa72..9eef4fc3ec 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -18,6 +18,7 @@
>   #include "hw/s390x/s390-pci-bus.h"
>   #include "hw/s390x/s390-pci-kvm.h"
>   #include "hw/s390x/s390-pci-inst.h"
> +#include "hw/s390x/s390-pci-vfio.h"
>   #include "cpu_models.h"
>   
>   bool s390_pci_kvm_interp_allowed(void)
> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>           return -EINVAL;
>       }
>   
> +    /*
> +     * The device may have already been reset but we still want to relinquish
> +     * the guest ISC, so always be sure to use an up-to-date host fh.
> +     */
> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
> +        return -EPERM;
> +    }

The callers of s390_pci_kvm_aif_disable() all test the original host
function with :

    if (pbdev->interp && (pbdev->fh & FH_MASK_ENABLE))

This change possibly fetches a new one. Shouldn't we move the test
also in s390_pci_kvm_aif_disable() ?


Thanks,

C.



> +
>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>       if (rc == 0) {
> -        pbev->aif = false;
> +        pbdev->aif = false;
>       }
>   
>       return rc;

Re: [PATCH 1/3] s390x/pci: avoid double enable/disable of aif

[Cédric Le Goater]

On 1/16/24 23:31, Matthew Rosato wrote:
> Use a flag to keep track of whether AIF is currently enabled.  This can be
> used to avoid enabling/disabling AIF multiple times as well as to determine
> whether or not it should be disabled during reset processing.

Why don't we disable AIF always at reset ? Doesn't KVM handle multiple calls
to KVM_S390_ZPCIOP_DEREG_AEN cleanly ? Just asking, I am no expert there.

Thanks,

C.


> Fixes: d0bc7091c2 ("s390x/pci: enable adapter event notification for interpreted devices")
> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-kvm.c         | 25 +++++++++++++++++++++++--
>   include/hw/s390x/s390-pci-bus.h |  1 +
>   2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
> index ff41e4106d..f7e10cfa72 100644
> --- a/hw/s390x/s390-pci-kvm.c
> +++ b/hw/s390x/s390-pci-kvm.c
> @@ -27,6 +27,7 @@ bool s390_pci_kvm_interp_allowed(void)
>   
>   int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist)
>   {
> +    int rc;
>       struct kvm_s390_zpci_op args = {
>           .fh = pbdev->fh,
>           .op = KVM_S390_ZPCIOP_REG_AEN,
> @@ -38,15 +39,35 @@ int s390_pci_kvm_aif_enable(S390PCIBusDevice *pbdev, ZpciFib *fib, bool assist)
>           .u.reg_aen.flags = (assist) ? 0 : KVM_S390_ZPCIOP_REGAEN_HOST
>       };
>   
> -    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (pbdev->aif) {
> +        return -EINVAL;
> +    }
> +
> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (rc == 0) {
> +        pbdev->aif = true;
> +    }
> +
> +    return rc;
>   }
>   
>   int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>   {
> +    int rc;
> +
>       struct kvm_s390_zpci_op args = {
>           .fh = pbdev->fh,
>           .op = KVM_S390_ZPCIOP_DEREG_AEN
>       };
>   
> -    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (!pbdev->aif) {
> +        return -EINVAL;
> +    }
> +
> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
> +    if (rc == 0) {
> +        pbev->aif = false;
> +    }
> +
> +    return rc;
>   }
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index b1bdbeaeb5..435e788867 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -361,6 +361,7 @@ struct S390PCIBusDevice {
>       bool unplug_requested;
>       bool interp;
>       bool forwarding_assist;
> +    bool aif;
>       QTAILQ_ENTRY(S390PCIBusDevice) link;
>   };
>   

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

[Cédric Le Goater]

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.




> +
>   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

Re: [PATCH 1/3] s390x/pci: avoid double enable/disable of aif

[Matthew Rosato]

>> -    return kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>> +    if (!pbdev->aif) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>> +    if (rc == 0) {
>> +        pbev->aif = false;
> 
> s/pbev/pbdev/
> 
> You fix this in patch 2. :)
> 

Oops, thanks for catching that.

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

[Matthew Rosato]

On 1/16/24 10:01 PM, Eric Farman wrote:
> On Tue, 2024-01-16 at 17:31 -0500, Matthew Rosato wrote:

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

ISM passthrough is currently only allowed when interpretation is enabled.  So the check is redundant today but allows us to re-evaluate the need if we ever add support for ISM without interpretation.

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Matthew Rosato]

On 1/17/24 5:31 AM, Cédric Le Goater wrote:
> Hello Matthew,
> 
> On 1/16/24 23:31, Matthew Rosato wrote:
>> Typically we refresh the host fh during CLP enable, however it's possible
>> that the device goes through multiple reset events before the guest
>> performs another CLP enable.  Let's handle this for now by refreshing the
>> host handle from vfio before disabling aif.
>>
>> 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-kvm.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
>> index f7e10cfa72..9eef4fc3ec 100644
>> --- a/hw/s390x/s390-pci-kvm.c
>> +++ b/hw/s390x/s390-pci-kvm.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/s390x/s390-pci-bus.h"
>>   #include "hw/s390x/s390-pci-kvm.h"
>>   #include "hw/s390x/s390-pci-inst.h"
>> +#include "hw/s390x/s390-pci-vfio.h"
>>   #include "cpu_models.h"
>>     bool s390_pci_kvm_interp_allowed(void)
>> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>>           return -EINVAL;
>>       }
>>   +    /*
>> +     * The device may have already been reset but we still want to relinquish
>> +     * the guest ISC, so always be sure to use an up-to-date host fh.
>> +     */
>> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
>> +        return -EPERM;
>> +    }
>> +
>>       rc = kvm_vm_ioctl(kvm_state, KVM_S390_ZPCI_OP, &args);
>>       if (rc == 0) {
>> -        pbev->aif = false;
>> +        pbdev->aif = false;
>>       }
> 
> This belongs to patch 1.
> 
> 

Thanks for pointing that out, will fix.

Re: [PATCH 1/3] s390x/pci: avoid double enable/disable of aif

[Matthew Rosato]

On 1/17/24 5:54 AM, Cédric Le Goater wrote:
> On 1/16/24 23:31, Matthew Rosato wrote:
>> Use a flag to keep track of whether AIF is currently enabled.  This can be
>> used to avoid enabling/disabling AIF multiple times as well as to determine
>> whether or not it should be disabled during reset processing.
> 
> Why don't we disable AIF always at reset ? Doesn't KVM handle multiple calls
> to KVM_S390_ZPCIOP_DEREG_AEN cleanly ? Just asking, I am no expert there.
>

This may be some amount of defensive programming on my part :) Really, we're more concerned about enabling AIF twice without disabling AIF in between, and if we attempt to do so we should fail immediately rather than try messing with the hostdev.

The kernel warning you were seeing was exactly because we got the guest ISC users count wrong due to a mismatch between the number of enables and disables; in a sense, that warning is the KVM cleanup handling things (by disabling the gisc that AIF was using but also spitting out the warning that we got the gisc usage count wrong).  

I suspect you're right in that there's room to improve the KVM code so that we can catch this earlier rather than continuing to increment/decrement the guest ISC count and waiting to catch it at the end of the VM lifecycle (the GISC count is per-VM, but we should only ever have 1 instance per-device but are allowing >1 per-device).  I'll have a look at that separately from this series but IMHO QEMU should also try and 'behave' and be aware of what it previously enabled/disabled; if QEMU is trying to enable AIF multiple times then QEMU is doing something wrong.

Re: [PATCH 2/3] s390x/pci: refresh fh before disabling aif

[Matthew Rosato]

On 1/17/24 5:40 AM, Cédric Le Goater wrote:
> On 1/16/24 23:31, Matthew Rosato wrote:
>> Typically we refresh the host fh during CLP enable, however it's possible
>> that the device goes through multiple reset events before the guest
>> performs another CLP enable.  Let's handle this for now by refreshing the
>> host handle from vfio before disabling aif.
>>
>> 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-kvm.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
>> index f7e10cfa72..9eef4fc3ec 100644
>> --- a/hw/s390x/s390-pci-kvm.c
>> +++ b/hw/s390x/s390-pci-kvm.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/s390x/s390-pci-bus.h"
>>   #include "hw/s390x/s390-pci-kvm.h"
>>   #include "hw/s390x/s390-pci-inst.h"
>> +#include "hw/s390x/s390-pci-vfio.h"
>>   #include "cpu_models.h"
>>     bool s390_pci_kvm_interp_allowed(void)
>> @@ -64,9 +65,17 @@ int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
>>           return -EINVAL;
>>       }
>>   +    /*
>> +     * The device may have already been reset but we still want to relinquish
>> +     * the guest ISC, so always be sure to use an up-to-date host fh.
>> +     */
>> +    if (!s390_pci_get_host_fh(pbdev, &args.fh)) {
>> +        return -EPERM;
>> +    }
> 
> The callers of s390_pci_kvm_aif_disable() all test the original host
> function with :
> 
>    if (pbdev->interp && (pbdev->fh & FH_MASK_ENABLE))
> 
> This change possibly fetches a new one. Shouldn't we move the test
> also in s390_pci_kvm_aif_disable() ?
> 
> 

Those codepaths are actually testing the enablement bit of the guest-visible function handle, not the host function handle.  Basically asking "was the guest using this device".  These handles (host and guest) are sync'd during guest CLP enable (necessary for interpretation to work) but will become disjoint once we reset the device until the next guest CLP ENABLE.  They can also become disjoint once the guest does a CLP DISABLE.
What this change is doing is basically making sure we disable AIF on the hostdev, but does NOT replace pbdev->fh.  The guest-visible function handle will get sync'd again when the guest does a new CLP ENABLE after reset.

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

[Matthew Rosato]

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.
 

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

[Matthew Rosato]

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

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.

Re: [PATCH 0/3] s390x/pci: fix ISM reset

[Michael Tokarev]

17.01.2024 01:31, Matthew Rosato:
> Commit ef1535901a0 (re-)introduced an issue where passthrough ISM devices
> on s390x would enter an error state after reboot.  This was previously fixed
> by 03451953c79e, using device reset callbacks, however the change in
> ef1535901a0 effectively triggers a cold reset of the pci bus before the
> device reset callbacks are triggered.
> 
> To resolve this, this series proposes to remove the use of the reset callback
> for ISM cleanup and instead trigger ISM reset from subsystem_reset before
> triggering bus resets.  This has to happen before the bus resets because the
> reset of s390-pcihost will trigger reset of the PCI bus followed by the
> s390-pci bus, and the former will trigger vfio-pci reset / the aperture-wide
> unmap that ISM gets upset about.
>   
>    /s390-pcihost (s390-pcihost)
>      /pci.0 (PCI)
>      /s390-pcibus.0 (s390-pcibus)
>      
> While fixing this, it was also noted that kernel warnings could be seen that
> indicate a guest ISC reference count error.  That's because in some reset
> cases we were not bothering to disable AIF, but would again re-enable it after
> the reset (causing the reference count to grow erroneously).  This was a base
> issue that went unnoticed because the kernel previously did not detect and
> issue a warning for this scenario.

Is it a -stable material, or not worth picking up for stable?

Thanks,

/mjt

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.



> 

Re: [PATCH 0/3] s390x/pci: fix ISM reset

[Thomas Huth]

On 18/01/2024 07.03, Michael Tokarev wrote:
> 17.01.2024 01:31, Matthew Rosato:
>> Commit ef1535901a0 (re-)introduced an issue where passthrough ISM devices
>> on s390x would enter an error state after reboot.  This was previously fixed
>> by 03451953c79e, using device reset callbacks, however the change in
>> ef1535901a0 effectively triggers a cold reset of the pci bus before the
>> device reset callbacks are triggered.
>>
>> To resolve this, this series proposes to remove the use of the reset callback
>> for ISM cleanup and instead trigger ISM reset from subsystem_reset before
>> triggering bus resets.  This has to happen before the bus resets because the
>> reset of s390-pcihost will trigger reset of the PCI bus followed by the
>> s390-pci bus, and the former will trigger vfio-pci reset / the aperture-wide
>> unmap that ISM gets upset about.
>>    /s390-pcihost (s390-pcihost)
>>      /pci.0 (PCI)
>>      /s390-pcibus.0 (s390-pcibus)
>> While fixing this, it was also noted that kernel warnings could be seen that
>> indicate a guest ISC reference count error.  That's because in some reset
>> cases we were not bothering to disable AIF, but would again re-enable it 
>> after
>> the reset (causing the reference count to grow erroneously).  This was a base
>> issue that went unnoticed because the kernel previously did not detect and
>> issue a warning for this scenario.
> 
> Is it a -stable material, or not worth picking up for stable?

It's definitely stable material, but IIUC there will be a v2 with some minor 
fixes.

  Thomas

Re: [PATCH 0/3] s390x/pci: fix ISM reset

[Michael Tokarev]

18.01.2024 10:19, Thomas Huth:
>> Is it a -stable material, or not worth picking up for stable?
> 
> It's definitely stable material, but IIUC there will be a v2 with some minor fixes.

Yeah, I figured there will be v2.

Just to remind, - please add Cc: qemu-stable@ when appropriate (or mark it any other way,
or just forward it qemu-stable@, whatever, - just so it wont get lost).  There's no need
to do that for this patchset, as I already noticed this one :)

Thank you for the comments!

/mjt