On Mon, 22 Jan 2024 10:06:38 -0500 Matthew Rosato <mjros...@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 <mjros...@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