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().
Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
hw/virtio/virtio-mem.c | 39 ++++++++++++++++++++++------------
include/hw/virtio/virtio-mem.h | 4 ++++
2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ef64bf1b4a..4f2fd7dc2e 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -895,18 +895,6 @@ static int virtio_mem_validate_features(VirtIODevice *vdev)
return 0;
}
-static void virtio_mem_system_reset(void *opaque)
-{
- VirtIOMEM *vmem = VIRTIO_MEM(opaque);
-
- /*
- * During usual resets, we will unplug all memory and shrink the usable
- * region size. This is, however, not possible in all scenarios. Then,
- * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
- */
- virtio_mem_unplug_all(vmem);
-}
-
static void virtio_mem_prepare_mr(VirtIOMEM *vmem)
{
const uint64_t region_size = memory_region_size(&vmem->memdev->mr);
@@ -1123,7 +1111,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
vmstate_register_any(VMSTATE_IF(vmem),
&vmstate_virtio_mem_device_early, vmem);
}
- qemu_register_reset(virtio_mem_system_reset, vmem);
+ qemu_register_resettable(OBJECT(vmem));
/*
* Set ourselves as RamDiscardManager before the plug handler maps the
@@ -1143,7 +1131,7 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
* found via an address space anymore. Unset ourselves.
*/
memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
- qemu_unregister_reset(virtio_mem_system_reset, vmem);
+ qemu_unregister_resettable(OBJECT(vmem));
if (vmem->early_migration) {
vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
vmem);
@@ -1843,12 +1831,31 @@ static void virtio_mem_unplug_request_check(VirtIOMEM *vmem, Error **errp)
}
}
+static ResettableState *virtio_mem_get_reset_state(Object *obj)
+{
+ VirtIOMEM *vmem = VIRTIO_MEM(obj);
+ return &vmem->reset_state;
+}
+
+static void virtio_mem_system_reset_hold(Object *obj, ResetType type)
+{
+ VirtIOMEM *vmem = VIRTIO_MEM(obj);
+
+ /*
+ * During usual resets, we will unplug all memory and shrink the usable
+ * region size. This is, however, not possible in all scenarios. Then,
+ * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
+ */
+ virtio_mem_unplug_all(vmem);
+}
+
static void virtio_mem_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
VirtIOMEMClass *vmc = VIRTIO_MEM_CLASS(klass);
RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(klass);
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
device_class_set_props(dc, virtio_mem_properties);
dc->vmsd = &vmstate_virtio_mem;
@@ -1875,6 +1882,9 @@ static void virtio_mem_class_init(ObjectClass *klass, void *data)
rdmc->replay_discarded = virtio_mem_rdm_replay_discarded;
rdmc->register_listener = virtio_mem_rdm_register_listener;
rdmc->unregister_listener = virtio_mem_rdm_unregister_listener;
+
+ rc->get_state = virtio_mem_get_reset_state;
+ rc->phases.hold = virtio_mem_system_reset_hold;
}
static const TypeInfo virtio_mem_info = {
@@ -1887,6 +1897,7 @@ static const TypeInfo virtio_mem_info = {
.class_size = sizeof(VirtIOMEMClass),
.interfaces = (InterfaceInfo[]) {
{ TYPE_RAM_DISCARD_MANAGER },
+ { TYPE_RESETTABLE_INTERFACE },
{ }
},
};
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index 5f5b02b8f9..79f197216b 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -13,6 +13,7 @@
#ifndef HW_VIRTIO_MEM_H
#define HW_VIRTIO_MEM_H
+#include "hw/resettable.h"
#include "standard-headers/linux/virtio_mem.h"
#include "hw/virtio/virtio.h"
#include "qapi/qapi-types-misc.h"
@@ -115,6 +116,9 @@ struct VirtIOMEM {
/* listeners to notify on plug/unplug activity. */
QLIST_HEAD(, RamDiscardListener) rdl_list;
+
+ /* State of the resettable container */
+ ResettableState reset_state;
};
struct VirtIOMEMClass {
--
2.45.2On 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?
thanks
-- PMM
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".
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.
--
Cheers,
David / dhildenb
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
On 06.08.24 18:07, Juraj Marcin 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(). > > Signed-off-by: Juraj Marcin <jmarcin@redhat.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com> -- Cheers, David / dhildenb
© 2016 - 2024 Red Hat, Inc.