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

[PATCH v2 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.


Changes for v2:
- Fold a typo fix from patch 2 into patch 1 where it belongs
- Add block comment re: timing of ISM reset
- Add review tags


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      |  8 ++++++++
 include/hw/s390x/s390-pci-bus.h |  2 ++
 4 files changed, 59 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v2 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>
Reviewed-by: Eric Farman <farman@linux.ibm.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..1ee510436c 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) {
+        pbdev->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 v2 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>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-kvm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/s390x/s390-pci-kvm.c b/hw/s390x/s390-pci-kvm.c
index 1ee510436c..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,6 +65,14 @@ 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) {
         pbdev->aif = false;
-- 
2.43.0

[PATCH v2 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      |  8 ++++++++
 include/hw/s390x/s390-pci-bus.h |  1 +
 3 files changed, 26 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 eaf61d3640..c99682b07d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -118,6 +118,14 @@ static void subsystem_reset(void)
     DeviceState *dev;
     int i;
 
+    /*
+     * ISM firmware is sensitive to unexpected changes to the IOMMU, which can
+     * occur during reset of the vfio-pci device (unmap of entire aperture).
+     * Ensure any passthrough ISM devices are reset now, while CPUs are paused
+     * but before vfio-pci cleanup occurs.
+     */
+    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 v2 3/3] s390x/pci: drive ISM reset from subsystem reset

[Eric Farman]

On Thu, 2024-01-18 at 13:51 -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      |  8 ++++++++
>  include/hw/s390x/s390-pci-bus.h |  1 +
>  3 files changed, 26 insertions(+), 9 deletions(-)

Thanks for the reminder on ISM/interpretation in v1.

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

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

[Cédric Le Goater]

On 1/18/24 19:51, Matthew Rosato wrote:
> 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.
> 
> 
> Changes for v2:
> - Fold a typo fix from patch 2 into patch 1 where it belongs
> - Add block comment re: timing of ISM reset
> - Add review tags


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

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

[Halil Pasic]

On Thu, 18 Jan 2024 13:51:51 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index eaf61d3640..c99682b07d 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -118,6 +118,14 @@ static void subsystem_reset(void)
>      DeviceState *dev;
>      int i;
>  
> +    /*
> +     * ISM firmware is sensitive to unexpected changes to the IOMMU, which can
> +     * occur during reset of the vfio-pci device (unmap of entire aperture).
> +     * Ensure any passthrough ISM devices are reset now, while CPUs are paused
> +     * but before vfio-pci cleanup occurs.
> +     */
> +    s390_pci_ism_reset();

Hm I'm not sure about special casing ISM in here. In my opinion the loop
below shall take care of all the reset.

For TYPE_AP_BRIDGE and TYPE_VIRTUAL_CSS_BRIDGE AFAIU a
device_cold_reset() on all objects of those types results in the resets
of objects that hang below these buses.

I guess this also happens for the S390PCIBusDevices, but not for the
actual PCI devices.

My understanding is that the entire PCI subsystem is to be reset when
a subsystem reset is performed.

I'm not familiar enough with our PCI emulation to know if a reset
to the TYPE_S390_PCI_HOST_BRIDGE is supposed to be sufficient to
accomplish that.

I have the feeling, I am missing something... Can you help me understand
this please?


> +
>      for (i = 0; i < ARRAY_SIZE(reset_dev_types); i++) {
>          dev = DEVICE(object_resolve_path_type("", reset_dev_types[i], NULL));
>          if (dev) 

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

[Michael Tokarev]

18.01.2024 21:51, 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

Is it this a material for -stable, or there's no need to bother?
(changes 1 and 2 applies to 7.2 (while 2 fixes later change),
all 3 applies to 8.1 (while 3 fixes later change), and all 3 can be
picked up for 8.2, I guess).

Thanks,

/mjt

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

[Michael Tokarev]

22.01.2024 13:18, Michael Tokarev :
..
> Is it this a material for -stable, or there's no need to bother?

Actually it's been Cc'd to qemu-stable@ already, I haven't noticed.
Still there's a question which branches should get which patches.

> (changes 1 and 2 applies to 7.2 (while 2 fixes later change),
> all 3 applies to 8.1 (while 3 fixes later change), and all 3 can be
> picked up for 8.2, I guess).

07b2c8e034d80f s390x/pci: avoid double enable/disable of aif
  Fixes: v7.1.0-416-gd0bc7091c2 s390x/pci: enable adapter event notification for interpreted devices

30e35258e25c75  s390x/pci: refresh fh before disabling aif
  Fixes: v7.2.0-51-g03451953c7 s390x/pci: reset ISM passthrough devices on shutdown and system reset

68c691ca99a253 s390x/pci: drive ISM reset from subsystem reset
  Fixes: v8.1.0-654-gef1535901a s390x: do a subsystem reset before the unprotect on reboot
  Fixes: v7.2.0-51-g03451953c7 s390x/pci: reset ISM passthrough devices on shutdown and system reset

So all 3 are okay for 8.2.

What about 8.1 and 7.2 which are the current still-maintained stable branches?
(I think this 8.1 release will be the last in series).

Thanks,

/mjt

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

[Thomas Huth]

On 22/01/2024 11.31, Michael Tokarev wrote:
> 22.01.2024 13:18, Michael Tokarev :
> ..
>> Is it this a material for -stable, or there's no need to bother?
> 
> Actually it's been Cc'd to qemu-stable@ already, I haven't noticed.
> Still there's a question which branches should get which patches.
...
> So all 3 are okay for 8.2.
> 
> What about 8.1 and 7.2 which are the current still-maintained stable branches?
> (I think this 8.1 release will be the last in series).

IIUC the main issue that this series fixes is the bug that has been 
uncovered by commit ef1535901a07f2e49fa25c8bcee7f0b73801d824 ("s390x: do a 
subsystem reset before the unprotect on reboot") - and that one is in 8.2 
only. So I think it should be OK to just backport this to 8.2 and skip 8.1 
and 7.2.

  Thomas

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

[Matthew Rosato]

On 1/19/24 4:07 PM, Halil Pasic wrote:
> On Thu, 18 Jan 2024 13:51:51 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index eaf61d3640..c99682b07d 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -118,6 +118,14 @@ static void subsystem_reset(void)
>>      DeviceState *dev;
>>      int i;
>>  
>> +    /*
>> +     * ISM firmware is sensitive to unexpected changes to the IOMMU, which can
>> +     * occur during reset of the vfio-pci device (unmap of entire aperture).
>> +     * Ensure any passthrough ISM devices are reset now, while CPUs are paused
>> +     * but before vfio-pci cleanup occurs.
>> +     */
>> +    s390_pci_ism_reset();
> 
> Hm I'm not sure about special casing ISM in here. In my opinion the loop
> below shall take care of all the reset.
> 
> For TYPE_AP_BRIDGE and TYPE_VIRTUAL_CSS_BRIDGE AFAIU a
> device_cold_reset() on all objects of those types results in the resets
> of objects that hang below these buses.
> 
> I guess this also happens for the S390PCIBusDevices, but not for the
> actual PCI devices.

PCI is a bit different because we have both the PCI root bus and the s390 pci bus --  When we reset the s390-pcihost in the device_cold_reset() loop, the root pci bus will also receive a reset and in practice this causes the vfio-pci devices to get cleaned up (this includes an unmap of the entire iommu aperture) and this happens before we get to the reset of S390PCIBusDevices.  This order is OK for other device types who are not sensitive to the IOMMU being wiped out in this manner, but ISM is effectively treating some portion of the IOMMU as state data and is not expecting this UNMAP.  Triggering the reset as we do here causes the host device to throw out the existing state data, so we want to do that at a point in time after CPU pause and before vfio-pci cleanup; this is basically working around a quirk of ISM devices.

FWIW, this series of fixes was already pulled.  I think for a fix, this location in code was the safe bet -- But if we can figure out a way to ensure the reset targeted S390PCIBusDevices first before the root PCI bus then I could see a follow-on cleanup patch that moves this logic back into s390 pci bus code (e.g. allowing the loop to take care of all the reset once again). 

Thanks,
Matt

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

[Matthew Rosato]

On 1/22/24 5:49 AM, Thomas Huth wrote:
> On 22/01/2024 11.31, Michael Tokarev wrote:
>> 22.01.2024 13:18, Michael Tokarev :
>> ..
>>> Is it this a material for -stable, or there's no need to bother?
>>
>> Actually it's been Cc'd to qemu-stable@ already, I haven't noticed.
>> Still there's a question which branches should get which patches.
> ...
>> So all 3 are okay for 8.2.
>>
>> What about 8.1 and 7.2 which are the current still-maintained stable branches?
>> (I think this 8.1 release will be the last in series).
> 
> IIUC the main issue that this series fixes is the bug that has been uncovered by commit ef1535901a07f2e49fa25c8bcee7f0b73801d824 ("s390x: do a subsystem reset before the unprotect on reboot") - and that one is in 8.2 only. So I think it should be OK to just backport this to 8.2 and skip 8.1 and 7.2.
> 

Agreed.

Thanks,
Matt

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

[Halil Pasic]

On Mon, 22 Jan 2024 10:06:38 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 1/19/24 4:07 PM, Halil Pasic wrote:
> > On Thu, 18 Jan 2024 13:51:51 -0500
> > Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> >   
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index eaf61d3640..c99682b07d 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -118,6 +118,14 @@ static void subsystem_reset(void)
> >>      DeviceState *dev;
> >>      int i;
> >>  
> >> +    /*
> >> +     * ISM firmware is sensitive to unexpected changes to the IOMMU, which can
> >> +     * occur during reset of the vfio-pci device (unmap of entire aperture).
> >> +     * Ensure any passthrough ISM devices are reset now, while CPUs are paused
> >> +     * but before vfio-pci cleanup occurs.
> >> +     */
> >> +    s390_pci_ism_reset();  
> > 
> > Hm I'm not sure about special casing ISM in here. In my opinion the loop
> > below shall take care of all the reset.
> > 
> > For TYPE_AP_BRIDGE and TYPE_VIRTUAL_CSS_BRIDGE AFAIU a
> > device_cold_reset() on all objects of those types results in the resets
> > of objects that hang below these buses.
> > 
> > I guess this also happens for the S390PCIBusDevices, but not for the
> > actual PCI devices.  
> 
> PCI is a bit different because we have both the PCI root bus and the s390 pci bus --  When we reset the s390-pcihost in the device_cold_reset() loop, the root pci bus will also receive a reset and in practice this causes the vfio-pci devices to get cleaned up (this includes an unmap of the entire iommu aperture) and this happens before we get to the reset of S390PCIBusDevices.  This order is OK for other device types who are not sensitive to the IOMMU being wiped out in this manner, but ISM is effectively treating some portion of the IOMMU as state data and is not expecting this UNMAP.  Triggering the reset as we do here causes the host device to throw out the existing state data, so we want to do that at a point in time after CPU pause and before vfio-pci cleanup; this is basically working around a quirk of ISM devices.
>

I am still a bit confused. Are you saying that when subsystem_reset() is
called, the resets happen in an order that leads to problems with ISM
but when qemu_devices_reset() is called the resets happen in an order
favorable to ISM?

Anyway the important thing is that we are functionally covered. My
concern is just the how.

 
> FWIW, this series of fixes was already pulled.  I think for a fix, this location in code was the safe bet -- But if we can figure out a way to ensure the reset targeted S390PCIBusDevices first before the root PCI bus then I could see a follow-on cleanup patch that moves this logic back into s390 pci bus code (e.g. allowing the loop to take care of all the reset once again). 
> 

Yes that makes sense. Should I find the time, I can come back to this
too.

Regards,
Halil