| Message ID | 20240806160756.182524-4-jmarcin@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-mem: Implement support for suspend+wake-up with plugged memory | expand |
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>
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? 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.
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 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 >
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 {
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(-)