Re: [PATCH 3/4] virtio-mem: Implement Resettable interface instead of using LegacyReset

[Peter Maydell]

On Thu, 8 Aug 2024 at 13:37, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.08.24 14:25, Peter Maydell wrote:
> > On Tue, 6 Aug 2024 at 17:08, Juraj Marcin <jmarcin@redhat.com> wrote:
> >>
> >> LegacyReset does not pass ResetType to the reset callback method, which
> >> the new Resettable interface uses. Due to this, virtio-mem cannot use
> >> the new RESET_TYPE_WAKEUP to skip reset during wake-up from a suspended
> >> state.
> >>
> >> This patch adds the Resettable interface to the VirtioMemClass interface
> >> list, implements the necessary methods and replaces
> >> qemu_[un]register_reset() calls with qemu_[un]register_resettable().
> >
> >> @@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
> >>       .class_size = sizeof(VirtIOMEMClass),
> >>       .interfaces = (InterfaceInfo[]) {
> >>           { TYPE_RAM_DISCARD_MANAGER },
> >> +        { TYPE_RESETTABLE_INTERFACE },
> >>           { }
> >>       },
> >>   };
> >
> > TYPE_VIRTIO_MEM is-a TYPE_VIRTIO_DEVICE, which is-a TYPE_DEVICE,
> > which implements the TYPE_RESETTABLE_INTERFACE. In other words,
> > as a device this is already Resettable. Re-implementing the
> > interface doesn't seem like the right thing here (it probably
> > breaks the general reset implementation in the base class).
> > Maybe what you want to do here is implement the Resettable
> > methods that you already have?
>
> TYPE_DEVICE indeed is TYPE_RESETTABLE_INTERFACE.
>
> And there, we implement a single "dc->reset", within which we
> unconditionally use "RESET_TYPE_COLD".

That's the glue that implements compatibility with the legacy
DeviceClass::reset method.

There's two kinds of glue here:

(1) When a device is reset via a method that is three-phase-reset
aware (including full system reset), if the device (i.e. some
subclass of TYPE_DEVICE) implements the Resettable methods, then
they get used. If the device doesn't implement those methods,
then the base class logic will arrange to call the legacy
DeviceClass::reset method of the subclass. This is what
device_transitional_reset() is doing.

(2) When a three-phase-reset device is reset via a method that is not
three-phase aware, the glue in the other direction is the
default DeviceState::reset method which is device_phases_reset(),
which does a RESET_TYPE_COLD reset for each phase in turn.
Here we have to pick a RESET_TYPE because the old legacy
reset API had no concept of reset types at all.
The set of cases where this can happen is now very restricted
because I've been gradually trying to convert places that can
trigger a reset to be three-phase aware. I think the only
remaining case is "parent class is 3-phase but it has a subclass
that is not 3-phase aware and tries to chain to the parent
class reset using device_class_set_parent_reset()", and the
only remaining cases of that are s390 CPU and s390 virtio-ccw.

For TYPE_VIRTIO_MEM neither of these should matter.

Other places where RESET_TYPE_COLD gets used:
 * device_cold_reset() is a function to say "cold reset this
   device", and so it always uses RESET_TYPE_COLD. The assumption
   is that the caller knows they wanted a cold reset; they can
   use resettable_reset(OBJECT(x), RESET_TYPE_FOO) if they want to
   trigger some other kind of reset on a specific device
 * similarly bus_cold_reset() for bus resets
 * when a hot-plug device is first realized, it gets a
   RESET_TYPE_COLD (makes sense to me, this is like "power on")

I think these should all not be relevant to the WAKEUP
usecase here.

> Looks like more plumbing might be required to get the actual reset type
> to the device that way, unless I am missing the easy way out.

I think the plumbing should all be in place already.

thanks
-- PMM