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

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

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

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

-- 
Peter Xu

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, 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