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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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