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

[Juraj Marcin]

On Thu, Aug 8, 2024 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.

I have gone through the code once more and I also think that. I think
that removing the interface from VirtIOMEM type info (as it already is
in the parent) and then overriding the Resettable methods in
virtio_mem_class_init() should be enough. This should also include
setting rc->get_transitional_function to NULL, so the
device_get_transitional_reset() does not interfere with the 3 phase
reset.

>
> thanks
> -- PMM
>

-- 

Juraj Marcin