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