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