| Message ID | 20240119163512.3810301-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | buses: switch to 3-phase-reset | expand |
On Fri, Jan 19, 2024 at 04:35:07PM +0000, Peter Maydell wrote: > This patchset switches the handful of bus types that implement a > reset method over to using the 3-phase-reset APIs, and then removes > the transitional infrastructure from the core bus class that was > supporting the ability to have bus types that use old-style reset. > > I wrote this ages ago and recently picked it back up because of a > recent PCI related reset ordering problem noted by Peter Xu. I'm not > sure if this patchset is necessary as a part of fixing that ordering > problem (it might even be possible now to have the intel_iommu device > use 3-phase reset and put the relevant parts of its reset into the > 'exit' phase), but either way we really ought to do this cleanup > to reduce the amount of legacy/transitional handling we have. > > In theory this patchset should be fine and shouldn't be changing > behaviour. On the other hand the reason I never sent it out when I > originally wrote it was that I ran into a test failure in the > BootLinuxConsole.test_sh4_r2d avocado test. Rerunning all the > avocado tests I don't see that failing now, so maybe I was just > confused by a flaky test back then. > > In any case, this could probably use a thorough soak testing with > workloads that do resets of the PCI bus; I've only done 'make check' > and 'make check-avocado' on it. But I wanted to get it out onto the > list anyway. > > thanks > -- PMM From a quick look, we need this cleanup Acked-by: Michael S. Tsirkin <mst@redhat.com> I'll try some tests too, and report. > Peter Maydell (5): > pci: Switch bus reset to 3-phase-reset > vmbus: Switch bus reset to 3-phase-reset > adb: Switch bus reset to 3-phase-reset > hw/s390x/css-bridge: switch virtual-css bus to 3-phase-reset > hw/core: Remove transitional infrastructure from BusClass > > include/hw/qdev-core.h | 2 -- > hw/core/bus.c | 67 ------------------------------------------ > hw/hyperv/vmbus.c | 7 +++-- > hw/input/adb.c | 7 +++-- > hw/pci/pci.c | 10 ++++--- > hw/s390x/css-bridge.c | 5 ++-- > 6 files changed, 17 insertions(+), 81 deletions(-) > > -- > 2.34.1
Hi, Peter, On Fri, Jan 19, 2024 at 04:35:07PM +0000, Peter Maydell wrote: > I wrote this ages ago and recently picked it back up because of a > recent PCI related reset ordering problem noted by Peter Xu. I'm not > sure if this patchset is necessary as a part of fixing that ordering > problem (it might even be possible now to have the intel_iommu device > use 3-phase reset and put the relevant parts of its reset into the > 'exit' phase), but either way we really ought to do this cleanup > to reduce the amount of legacy/transitional handling we have. The VFIO issue I was working on may not directly benefit from this series iiuc, as it's more of an special ordering on both (1) VFIO special case reset path using qemu_register_reset(), and (2) VT-d device is not put at the right place in the QOM hierachy [1]. Said that, thanks a lot for posting the patches; they all look reasonable and good cleanups to the reset infrastructure, afaict. [1] https://lore.kernel.org/r/ZapYii9nr5Tj9ClE@x1n
Hello, On 1/22/24 03:06, Peter Xu wrote: > Hi, Peter, > > On Fri, Jan 19, 2024 at 04:35:07PM +0000, Peter Maydell wrote: >> I wrote this ages ago and recently picked it back up because of a >> recent PCI related reset ordering problem noted by Peter Xu. I'm not >> sure if this patchset is necessary as a part of fixing that ordering >> problem (it might even be possible now to have the intel_iommu device >> use 3-phase reset and put the relevant parts of its reset into the >> 'exit' phase), but either way we really ought to do this cleanup >> to reduce the amount of legacy/transitional handling we have. > > The VFIO issue I was working on may not directly benefit from this series > iiuc, as it's more of an special ordering on both (1) VFIO special case > reset path using qemu_register_reset(), and (2) VT-d device is not put at > the right place in the QOM hierachy [1]. > > Said that, thanks a lot for posting the patches; they all look reasonable > and good cleanups to the reset infrastructure, afaict. Yes. I took the series in my vfio testing environment (x86_64 and s390x) and didn't see any issue. I will keep it for further testing. Thanks, C.
On 1/22/24 15:19, Cédric Le Goater wrote: > Hello, > > On 1/22/24 03:06, Peter Xu wrote: >> Hi, Peter, >> >> On Fri, Jan 19, 2024 at 04:35:07PM +0000, Peter Maydell wrote: >>> I wrote this ages ago and recently picked it back up because of a >>> recent PCI related reset ordering problem noted by Peter Xu. I'm not >>> sure if this patchset is necessary as a part of fixing that ordering >>> problem (it might even be possible now to have the intel_iommu device >>> use 3-phase reset and put the relevant parts of its reset into the >>> 'exit' phase), but either way we really ought to do this cleanup >>> to reduce the amount of legacy/transitional handling we have. >> >> The VFIO issue I was working on may not directly benefit from this series >> iiuc, as it's more of an special ordering on both (1) VFIO special case >> reset path using qemu_register_reset(), and (2) VT-d device is not put at >> the right place in the QOM hierachy [1]. >> >> Said that, thanks a lot for posting the patches; they all look reasonable >> and good cleanups to the reset infrastructure, afaict. > > > Yes. I took the series in my vfio testing environment (x86_64 and s390x) and > didn't see any issue. I will keep it for further testing. Acked-by: Cédric Le Goater <clg@redhat.com> Tested-by: Cédric Le Goater <clg@redhat.com> Thanks, C.
On Fri, 19 Jan 2024 at 16:35, Peter Maydell <peter.maydell@linaro.org> wrote: > This patchset switches the handful of bus types that implement a > reset method over to using the 3-phase-reset APIs, and then removes > the transitional infrastructure from the core bus class that was > supporting the ability to have bus types that use old-style reset. Thanks all for the review and testing. It seems like there weren't any negative reports, so I'm going to take this via the target-arm.next queue. We're still early in the 9.0 release cycle so we have plenty of time for debugging or (if necessary) reverting if any unexpected consequences turn up... -- PMM