Re: [PATCH 2/4] reset: Allow multiple stages of system resets

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 17/1/24 18:46, Peter Maydell wrote:
> On Wed, 17 Jan 2024 at 09:16, <peterx@redhat.com> wrote:
>>
>> From: Peter Xu <peterx@redhat.com>
>>
>> QEMU resets do not have a way to order reset hooks.  Add one coarse grained
>> reset stage so that some devices can be reset later than some others.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>   include/sysemu/reset.h |  5 ++++
>>   hw/core/reset.c        | 60 +++++++++++++++++++++++++++++++-----------
>>   2 files changed, 49 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
>> index 609e4d50c2..0de697ce9f 100644
>> --- a/include/sysemu/reset.h
>> +++ b/include/sysemu/reset.h
>> @@ -5,9 +5,14 @@
>>
>>   typedef void QEMUResetHandler(void *opaque);
>>
>> +#define  QEMU_RESET_STAGES_N  2
>> +
> 
> Our reset handling APIs are already pretty complicated, and
> raw qemu_register_reset() is kind of a legacy API that I would
> prefer that we try to move away from, not add extra complexity to.
> 
> Our device reset design already has a multiple-phase system
> (see docs/devel/reset.rst), part of the point of which is to
> try to give us a way to deal with reset-ordering problems.
> I feel like the right way to handle the issue you're trying to
> address is to ensure that the thing that needs to happen last is
> done in the 'exit' phase rather than the 'hold' phase (which is
> where legacy reset methods get called).

I concur. Devices reset is hard, but bus reset is even harder.
Having a quick look, the issues tracked by Alex & Peter might
come from the PCI bridges using the legacy DeviceReset. In
particular functions like:

- hw/pci-bridge/pcie_root_port.c

  46 static void rp_reset_hold(Object *obj)
  47 {
  48     PCIDevice *d = PCI_DEVICE(obj);
  49     DeviceState *qdev = DEVICE(obj);
  50
  51     rp_aer_vector_update(d);
  52     pcie_cap_root_reset(d);
  53     pcie_cap_deverr_reset(d);
  54     pcie_cap_slot_reset(d);
  55     pcie_cap_arifwd_reset(d);
  56     pcie_acs_reset(d);
  57     pcie_aer_root_reset(d);
  58     pci_bridge_reset(qdev);
  59     pci_bridge_disable_base_limit(d);
  60 }

- hw/pci-bridge/pcie_pci_bridge.c

107 static void pcie_pci_bridge_reset(DeviceState *qdev)
108 {
109     PCIDevice *d = PCI_DEVICE(qdev);
110     pci_bridge_reset(qdev);
111     if (msi_present(d)) {
112         msi_reset(d);
113     }
114     shpc_reset(d);
115 }

- hw/pci-bridge/xio3130_downstream.c

  56 static void xio3130_downstream_reset(DeviceState *qdev)
  57 {
  58     PCIDevice *d = PCI_DEVICE(qdev);
  59
  60     pcie_cap_deverr_reset(d);
  61     pcie_cap_slot_reset(d);
  62     pcie_cap_arifwd_reset(d);
  63     pci_bridge_reset(qdev);
  64 }

should really be split and converted as ResettablePhases.

pci_bridge_reset() is likely a ResettableExitPhase one.

> There are some annoying wrinkles here, notably that legacy
> qemu_register_reset() doesn't support 3-phase reset and so
> the phasing only happens for devices reset via the device/bus
> tree hierarchy. But I think the way to go is to try to move
> forward with that design (i.e. expand 3-phase reset to
> qemu_register_reset() and/or move things using qemu_register_reset()
> to device reset where that makes sense), not to ignore it and
> put a completely different reset-ordering API in a different place.

Unfortunately despite DeviceReset being deprecated since 4 years
in commit c11256aa6f ("hw/core: add Resettable support to BusClass
and DeviceClass"), we keep adding code using this legacy API; for
example in the last 4 months:

- e029bb00a7 ("hw/pci-host: Add Astro system bus adapter found on 
PA-RISC machines")
- 2880e676c0 ("Add virtio-sound device stub")
- 4a58330343 ("hw/cxl: Add a switch mailbox CCI function")
- 95e1019a4a ("vhost-user-scsi: free the inflight area when reset")
- 6f9c3aaa34 ("fsl-imx: add simple RTC emulation for i.MX6 and i.MX7 
boards")

Regards,

Phil.