[RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object
No reason for vmcoreinfo to be based on QDev, since it doesn't use any QDev API. Demote to plain Object. Since we can only register one type, introduce a new one for object: 'vmcore-info' (dash separator), keeping 'vmcoreinfo' device during the deprecation period. Philippe Mathieu-Daudé (10): hw/misc/vmcoreinfo: Declare QOM type using DEFINE_TYPES macro hw/misc/vmcoreinfo: Rename opaque pointer as 'opaque' hw/misc/vmcoreinfo: Un-inline vmcoreinfo_find() hw/misc/vmcoreinfo: Rename VMCOREINFO_DEVICE -> TYPE_VMCOREINFO_DEVICE hw/misc/vmcoreinfo: Convert to three-phase reset interface hw/misc/vmcoreinfo: Move vmstate_vmcoreinfo[] around hw/misc/vmcoreinfo: Factor vmcoreinfo_device_realize() out hw/misc/vmcoreinfo: Implement 'vmcore-info' object hw/misc/vmcoreinfo: Deprecate '-device vmcoreinfo' hw/misc/vmcoreinfo: Remove legacy '-device vmcoreinfo' docs/about/removed-features.rst | 5 + include/hw/misc/vmcoreinfo.h | 26 ++--- hw/misc/vmcoreinfo.c | 167 +++++++++++++++++++------------- 3 files changed, 116 insertions(+), 82 deletions(-) -- 2.47.1
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 833773ade52..84b211e9117 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -93,16 +93,13 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_MISC, dc->categories); } -static const TypeInfo vmcoreinfo_device_info = { - .name = VMCOREINFO_DEVICE, - .parent = TYPE_DEVICE, - .instance_size = sizeof(VMCoreInfoState), - .class_init = vmcoreinfo_device_class_init, +static const TypeInfo vmcoreinfo_types[] = { + { + .name = VMCOREINFO_DEVICE, + .parent = TYPE_DEVICE, + .instance_size = sizeof(VMCoreInfoState), + .class_init = vmcoreinfo_device_class_init, + } }; -static void vmcoreinfo_register_types(void) -{ - type_register_static(&vmcoreinfo_device_info); -} - -type_init(vmcoreinfo_register_types) +DEFINE_TYPES(vmcoreinfo_types) -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:48PM +0100, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Both QEMUResetHandler and FWCfgWriteCallback take an opaque pointer argument, no need to cast. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 84b211e9117..ad5a4dec596 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -18,17 +18,17 @@ #include "migration/vmstate.h" #include "hw/misc/vmcoreinfo.h" -static void fw_cfg_vmci_write(void *dev, off_t offset, size_t len) +static void fw_cfg_vmci_write(void *opaque, off_t offset, size_t len) { - VMCoreInfoState *s = VMCOREINFO(dev); + VMCoreInfoState *s = opaque; s->has_vmcoreinfo = offset == 0 && len == sizeof(s->vmcoreinfo) && s->vmcoreinfo.guest_format != FW_CFG_VMCOREINFO_FORMAT_NONE; } -static void vmcoreinfo_reset(void *dev) +static void vmcoreinfo_reset(void *opaque) { - VMCoreInfoState *s = VMCOREINFO(dev); + VMCoreInfoState *s = opaque; s->has_vmcoreinfo = false; memset(&s->vmcoreinfo, 0, sizeof(s->vmcoreinfo)); @@ -65,7 +65,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) * This device requires to register a global reset because it is * not plugged to a bus (which, as its QOM parent, would reset it). */ - qemu_register_reset(vmcoreinfo_reset, dev); + qemu_register_reset(vmcoreinfo_reset, s); vmcoreinfo_state = s; } -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:49PM +0100, Philippe Mathieu-Daudé wrote:
Both QEMUResetHandler and FWCfgWriteCallback take an opaque pointer argument, no need to cast.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 13 ++++++------- hw/misc/vmcoreinfo.c | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index 0b7b55d400a..da1066d540c 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -30,12 +30,11 @@ struct VMCoreInfoState { FWCfgVMCoreInfo vmcoreinfo; }; -/* returns NULL unless there is exactly one device */ -static inline VMCoreInfoState *vmcoreinfo_find(void) -{ - Object *o = object_resolve_path_type("", VMCOREINFO_DEVICE, NULL); - - return o ? VMCOREINFO(o) : NULL; -} +/** + * vmcoreinfo_find: + * + * Returns NULL unless there is exactly one instance. + */ +VMCoreInfoState *vmcoreinfo_find(void); #endif diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index ad5a4dec596..c5bb5c9fa52 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -42,11 +42,12 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; - /* Given that this function is executing, there is at least one VMCOREINFO - * device. Check if there are several. + /* + * Given that this function is executing, there is at least one + * VMCOREINFO instance. Check if there are several. */ if (!vmcoreinfo_find()) { - error_setg(errp, "at most one %s device is permitted", + error_setg(errp, "at most one %s instance is permitted", VMCOREINFO_DEVICE); return; } @@ -103,3 +104,12 @@ static const TypeInfo vmcoreinfo_types[] = { }; DEFINE_TYPES(vmcoreinfo_types) + +VMCoreInfoState *vmcoreinfo_find(void) +{ + Object *obj; + + obj = object_resolve_path_type("", TYPE_VMCOREINFO_DEVICE, NULL); + + return obj ? (VMCoreInfoState *)obj : NULL; +} -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:50PM +0100, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 13 ++++++------- hw/misc/vmcoreinfo.c | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index ad5a4dec596..c5bb5c9fa52 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -42,11 +42,12 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED;
- /* Given that this function is executing, there is at least one VMCOREINFO - * device. Check if there are several. + /* + * Given that this function is executing, there is at least one + * VMCOREINFO instance. Check if there are several. */ if (!vmcoreinfo_find()) { - error_setg(errp, "at most one %s device is permitted", + error_setg(errp, "at most one %s instance is permitted", VMCOREINFO_DEVICE); return; }
This chunk is unrelated to the claimed goal of this commit. It makes sense as a change though, so feel free to add my R-B when splitting into a separate commit.
@@ -103,3 +104,12 @@ static const TypeInfo vmcoreinfo_types[] = { };
DEFINE_TYPES(vmcoreinfo_types) + +VMCoreInfoState *vmcoreinfo_find(void) +{ + Object *obj; + + obj = object_resolve_path_type("", TYPE_VMCOREINFO_DEVICE, NULL); + + return obj ? (VMCoreInfoState *)obj : NULL; +}
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
-- 2.47.1
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Follow the assumed QOM type definition style, prefixing with 'TYPE_'. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 6 +++--- hw/misc/vmcoreinfo.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index da1066d540c..122c69686b0 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -16,10 +16,10 @@ #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h" -#define VMCOREINFO_DEVICE "vmcoreinfo" +#define TYPE_VMCOREINFO_DEVICE "vmcoreinfo" typedef struct VMCoreInfoState VMCoreInfoState; -DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO, - VMCOREINFO_DEVICE) +DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, + TYPE_VMCOREINFO_DEVICE) typedef struct fw_cfg_vmcoreinfo FWCfgVMCoreInfo; diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index c5bb5c9fa52..9822615cfed 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -37,7 +37,7 @@ static void vmcoreinfo_reset(void *opaque) static void vmcoreinfo_realize(DeviceState *dev, Error **errp) { - VMCoreInfoState *s = VMCOREINFO(dev); + VMCoreInfoState *s = VMCOREINFO_DEVICE(dev); FWCfgState *fw_cfg = fw_cfg_find(); /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; @@ -48,13 +48,13 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) */ if (!vmcoreinfo_find()) { error_setg(errp, "at most one %s instance is permitted", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; } if (!fw_cfg || !fw_cfg->dma_enabled) { error_setg(errp, "%s device requires fw_cfg with DMA", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; } @@ -96,7 +96,7 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) static const TypeInfo vmcoreinfo_types[] = { { - .name = VMCOREINFO_DEVICE, + .name = TYPE_VMCOREINFO_DEVICE, .parent = TYPE_DEVICE, .instance_size = sizeof(VMCoreInfoState), .class_init = vmcoreinfo_device_class_init, -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:51PM +0100, Philippe Mathieu-Daudé wrote:
Follow the assumed QOM type definition style, prefixing with 'TYPE_'.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 6 +++--- hw/misc/vmcoreinfo.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index da1066d540c..122c69686b0 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -16,10 +16,10 @@ #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h"
-#define VMCOREINFO_DEVICE "vmcoreinfo" +#define TYPE_VMCOREINFO_DEVICE "vmcoreinfo"
Yes to adding TYPE_, but while there would also drop _DEVICE. IMHO the best practice is for TYPE_<NNNN> where NNNN matches the "nnnn" name. An extra suffix doesn't add value, unless we've got some other symbol clashing which is almost never the case.
typedef struct VMCoreInfoState VMCoreInfoState; -DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO, - VMCOREINFO_DEVICE) +DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, + TYPE_VMCOREINFO_DEVICE)
typedef struct fw_cfg_vmcoreinfo FWCfgVMCoreInfo;
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index c5bb5c9fa52..9822615cfed 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -37,7 +37,7 @@ static void vmcoreinfo_reset(void *opaque)
static void vmcoreinfo_realize(DeviceState *dev, Error **errp) { - VMCoreInfoState *s = VMCOREINFO(dev); + VMCoreInfoState *s = VMCOREINFO_DEVICE(dev); FWCfgState *fw_cfg = fw_cfg_find(); /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; @@ -48,13 +48,13 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) */ if (!vmcoreinfo_find()) { error_setg(errp, "at most one %s instance is permitted", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; }
if (!fw_cfg || !fw_cfg->dma_enabled) { error_setg(errp, "%s device requires fw_cfg with DMA", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; }
@@ -96,7 +96,7 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data)
static const TypeInfo vmcoreinfo_types[] = { { - .name = VMCOREINFO_DEVICE, + .name = TYPE_VMCOREINFO_DEVICE, .parent = TYPE_DEVICE, .instance_size = sizeof(VMCoreInfoState), .class_init = vmcoreinfo_device_class_init, -- 2.47.1
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 19/12/24 17:59, Daniel P. Berrangé wrote:
On Thu, Dec 19, 2024 at 04:38:51PM +0100, Philippe Mathieu-Daudé wrote:
Follow the assumed QOM type definition style, prefixing with 'TYPE_'.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 6 +++--- hw/misc/vmcoreinfo.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index da1066d540c..122c69686b0 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -16,10 +16,10 @@ #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h"
-#define VMCOREINFO_DEVICE "vmcoreinfo" +#define TYPE_VMCOREINFO_DEVICE "vmcoreinfo"
Yes to adding TYPE_, but while there would also drop _DEVICE. IMHO the best practice is for TYPE_<NNNN> where NNNN matches the "nnnn" name. An extra suffix doesn't add value, unless we've got some other symbol clashing which is almost never the case.
Yeah, I added the _DEVICE suffix in preparation of the plain object implementation.
typedef struct VMCoreInfoState VMCoreInfoState; -DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO, - VMCOREINFO_DEVICE) +DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, + TYPE_VMCOREINFO_DEVICE)
typedef struct fw_cfg_vmcoreinfo FWCfgVMCoreInfo;
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index c5bb5c9fa52..9822615cfed 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -37,7 +37,7 @@ static void vmcoreinfo_reset(void *opaque)
static void vmcoreinfo_realize(DeviceState *dev, Error **errp) { - VMCoreInfoState *s = VMCOREINFO(dev); + VMCoreInfoState *s = VMCOREINFO_DEVICE(dev); FWCfgState *fw_cfg = fw_cfg_find(); /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; @@ -48,13 +48,13 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) */ if (!vmcoreinfo_find()) { error_setg(errp, "at most one %s instance is permitted", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; }
if (!fw_cfg || !fw_cfg->dma_enabled) { error_setg(errp, "%s device requires fw_cfg with DMA", - VMCOREINFO_DEVICE); + TYPE_VMCOREINFO_DEVICE); return; }
@@ -96,7 +96,7 @@ static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data)
static const TypeInfo vmcoreinfo_types[] = { { - .name = VMCOREINFO_DEVICE, + .name = TYPE_VMCOREINFO_DEVICE, .parent = TYPE_DEVICE, .instance_size = sizeof(VMCoreInfoState), .class_init = vmcoreinfo_device_class_init, -- 2.47.1
With regards, Daniel
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 9822615cfed..093bede655e 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -26,9 +26,9 @@ static void fw_cfg_vmci_write(void *opaque, off_t offset, size_t len) && s->vmcoreinfo.guest_format != FW_CFG_VMCOREINFO_FORMAT_NONE; } -static void vmcoreinfo_reset(void *opaque) +static void vmcoreinfo_reset_hold(Object *obj, ResetType type) { - VMCoreInfoState *s = opaque; + VMCoreInfoState *s = (VMCoreInfoState *)obj; s->has_vmcoreinfo = false; memset(&s->vmcoreinfo, 0, sizeof(s->vmcoreinfo)); @@ -66,7 +66,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) * This device requires to register a global reset because it is * not plugged to a bus (which, as its QOM parent, would reset it). */ - qemu_register_reset(vmcoreinfo_reset, s); + qemu_register_resettable(OBJECT(s)); vmcoreinfo_state = s; } @@ -87,11 +87,13 @@ static const VMStateDescription vmstate_vmcoreinfo = { static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); + ResettableClass *rc = RESETTABLE_CLASS(klass); dc->vmsd = &vmstate_vmcoreinfo; dc->realize = vmcoreinfo_realize; dc->hotpluggable = false; set_bit(DEVICE_CATEGORY_MISC, dc->categories); + rc->phases.hold = vmcoreinfo_reset_hold; } static const TypeInfo vmcoreinfo_types[] = { -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:52PM +0100, Philippe Mathieu-Daudé wrote:
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
In order to simplify the next commit, move vmstate_vmcoreinfo[] around. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 093bede655e..55f9d437a94 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -18,6 +18,20 @@ #include "migration/vmstate.h" #include "hw/misc/vmcoreinfo.h" +static const VMStateDescription vmstate_vmcoreinfo = { + .name = "vmcoreinfo", + .version_id = 1, + .minimum_version_id = 1, + .fields = (const VMStateField[]) { + VMSTATE_BOOL(has_vmcoreinfo, VMCoreInfoState), + VMSTATE_UINT16(vmcoreinfo.host_format, VMCoreInfoState), + VMSTATE_UINT16(vmcoreinfo.guest_format, VMCoreInfoState), + VMSTATE_UINT32(vmcoreinfo.size, VMCoreInfoState), + VMSTATE_UINT64(vmcoreinfo.paddr, VMCoreInfoState), + VMSTATE_END_OF_LIST() + }, +}; + static void fw_cfg_vmci_write(void *opaque, off_t offset, size_t len) { VMCoreInfoState *s = opaque; @@ -70,20 +84,6 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) vmcoreinfo_state = s; } -static const VMStateDescription vmstate_vmcoreinfo = { - .name = "vmcoreinfo", - .version_id = 1, - .minimum_version_id = 1, - .fields = (const VMStateField[]) { - VMSTATE_BOOL(has_vmcoreinfo, VMCoreInfoState), - VMSTATE_UINT16(vmcoreinfo.host_format, VMCoreInfoState), - VMSTATE_UINT16(vmcoreinfo.guest_format, VMCoreInfoState), - VMSTATE_UINT32(vmcoreinfo.size, VMCoreInfoState), - VMSTATE_UINT64(vmcoreinfo.paddr, VMCoreInfoState), - VMSTATE_END_OF_LIST() - }, -}; - static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); -- 2.47.1
In preparation of implementing a UserCreatable callback in the next commit, factor vmcoreinfo_device_realize() out. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/misc/vmcoreinfo.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index 55f9d437a94..a0511ea0da4 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -49,9 +49,8 @@ static void vmcoreinfo_reset_hold(Object *obj, ResetType type) s->vmcoreinfo.host_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF); } -static void vmcoreinfo_realize(DeviceState *dev, Error **errp) +static void vmcoreinfo_realize(VMCoreInfoState *s, Error **errp) { - VMCoreInfoState *s = VMCOREINFO_DEVICE(dev); FWCfgState *fw_cfg = fw_cfg_find(); /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; @@ -84,13 +83,18 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp) vmcoreinfo_state = s; } +static void vmcoreinfo_device_realize(DeviceState *dev, Error **errp) +{ + vmcoreinfo_realize(VMCOREINFO_DEVICE(dev), errp); +} + static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); ResettableClass *rc = RESETTABLE_CLASS(klass); dc->vmsd = &vmstate_vmcoreinfo; - dc->realize = vmcoreinfo_realize; + dc->realize = vmcoreinfo_device_realize; dc->hotpluggable = false; set_bit(DEVICE_CATEGORY_MISC, dc->categories); rc->phases.hold = vmcoreinfo_reset_hold; -- 2.47.1
'vmcore-info' object allow to transition from '-device' to 'object', following the deprecation process. No need to modify VMCoreInfoState since DeviceState already inherits from Object state. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 4 ++- hw/misc/vmcoreinfo.c | 48 +++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index 122c69686b0..d4cce42cee6 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -16,8 +16,10 @@ #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h" +#define TYPE_VMCOREINFO "vmcore-info" +OBJECT_DECLARE_SIMPLE_TYPE(VMCoreInfoState, VMCOREINFO) + #define TYPE_VMCOREINFO_DEVICE "vmcoreinfo" -typedef struct VMCoreInfoState VMCoreInfoState; DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, TYPE_VMCOREINFO_DEVICE) diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index a0511ea0da4..e2258e08fb1 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -12,11 +12,11 @@ #include "qemu/osdep.h" #include "qapi/error.h" -#include "qemu/module.h" #include "sysemu/reset.h" #include "hw/nvram/fw_cfg.h" #include "migration/vmstate.h" #include "hw/misc/vmcoreinfo.h" +#include "qom/object_interfaces.h" static const VMStateDescription vmstate_vmcoreinfo = { .name = "vmcoreinfo", @@ -32,6 +32,11 @@ static const VMStateDescription vmstate_vmcoreinfo = { }, }; +static char *vmcoreinfo_get_vmstate_id(VMStateIf *vmif) +{ + return g_strdup(TYPE_VMCOREINFO); +} + static void fw_cfg_vmci_write(void *opaque, off_t offset, size_t len) { VMCoreInfoState *s = opaque; @@ -88,6 +93,32 @@ static void vmcoreinfo_device_realize(DeviceState *dev, Error **errp) vmcoreinfo_realize(VMCOREINFO_DEVICE(dev), errp); } +static bool vmcoreinfo_can_be_deleted(UserCreatable *uc) +{ + return false; +} + +static void vmcoreinfo_complete(UserCreatable *uc, Error **errp) +{ + if (vmstate_register_any(VMSTATE_IF(uc), &vmstate_vmcoreinfo, uc) < 0) { + error_setg(errp, "%s: Failed to register vmstate", TYPE_VMCOREINFO); + } + + vmcoreinfo_realize(VMCOREINFO(uc), errp); +} + +static void vmcoreinfo_class_init(ObjectClass *oc, void *data) +{ + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); + VMStateIfClass *vc = VMSTATE_IF_CLASS(oc); + ResettableClass *rc = RESETTABLE_CLASS(oc); + + ucc->complete = vmcoreinfo_complete; + ucc->can_be_deleted = vmcoreinfo_can_be_deleted; + vc->get_id = vmcoreinfo_get_vmstate_id; + rc->phases.hold = vmcoreinfo_reset_hold; +} + static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -106,6 +137,18 @@ static const TypeInfo vmcoreinfo_types[] = { .parent = TYPE_DEVICE, .instance_size = sizeof(VMCoreInfoState), .class_init = vmcoreinfo_device_class_init, + }, + { + .name = TYPE_VMCOREINFO, + .parent = TYPE_OBJECT, + .instance_size = sizeof(VMCoreInfoState), + .class_init = vmcoreinfo_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_RESETTABLE_INTERFACE }, + { TYPE_USER_CREATABLE }, + { TYPE_VMSTATE_IF }, + { } + } } }; @@ -116,6 +159,9 @@ VMCoreInfoState *vmcoreinfo_find(void) Object *obj; obj = object_resolve_path_type("", TYPE_VMCOREINFO_DEVICE, NULL); + if (!obj) { + obj = object_resolve_path_type("", TYPE_VMCOREINFO, NULL); + } return obj ? (VMCoreInfoState *)obj : NULL; } -- 2.47.1
Hi On Thu, Dec 19, 2024 at 7:39 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
'vmcore-info' object allow to transition from '-device' to 'object', following the deprecation process.
Is there a strong motivation behind this? just replacing -device with -object doesn't really give anything, does it? Also I'd rather keep the name "vmcoreinfo" since that's how it used to be, and also the name used by the kernel ELF etc.
No need to modify VMCoreInfoState since DeviceState already inherits from Object state.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 4 ++- hw/misc/vmcoreinfo.c | 48 +++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index 122c69686b0..d4cce42cee6 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -16,8 +16,10 @@ #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h"
+#define TYPE_VMCOREINFO "vmcore-info" +OBJECT_DECLARE_SIMPLE_TYPE(VMCoreInfoState, VMCOREINFO) + #define TYPE_VMCOREINFO_DEVICE "vmcoreinfo" -typedef struct VMCoreInfoState VMCoreInfoState; DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, TYPE_VMCOREINFO_DEVICE)
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index a0511ea0da4..e2258e08fb1 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -12,11 +12,11 @@
#include "qemu/osdep.h" #include "qapi/error.h" -#include "qemu/module.h" #include "sysemu/reset.h" #include "hw/nvram/fw_cfg.h" #include "migration/vmstate.h" #include "hw/misc/vmcoreinfo.h" +#include "qom/object_interfaces.h"
static const VMStateDescription vmstate_vmcoreinfo = { .name = "vmcoreinfo", @@ -32,6 +32,11 @@ static const VMStateDescription vmstate_vmcoreinfo = { }, };
+static char *vmcoreinfo_get_vmstate_id(VMStateIf *vmif) +{ + return g_strdup(TYPE_VMCOREINFO); +} + static void fw_cfg_vmci_write(void *opaque, off_t offset, size_t len) { VMCoreInfoState *s = opaque; @@ -88,6 +93,32 @@ static void vmcoreinfo_device_realize(DeviceState *dev, Error **errp) vmcoreinfo_realize(VMCOREINFO_DEVICE(dev), errp); }
+static bool vmcoreinfo_can_be_deleted(UserCreatable *uc) +{ + return false; +} + +static void vmcoreinfo_complete(UserCreatable *uc, Error **errp) +{ + if (vmstate_register_any(VMSTATE_IF(uc), &vmstate_vmcoreinfo, uc) < 0) { + error_setg(errp, "%s: Failed to register vmstate", TYPE_VMCOREINFO); + } + + vmcoreinfo_realize(VMCOREINFO(uc), errp); +} + +static void vmcoreinfo_class_init(ObjectClass *oc, void *data) +{ + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); + VMStateIfClass *vc = VMSTATE_IF_CLASS(oc); + ResettableClass *rc = RESETTABLE_CLASS(oc); + + ucc->complete = vmcoreinfo_complete; + ucc->can_be_deleted = vmcoreinfo_can_be_deleted; + vc->get_id = vmcoreinfo_get_vmstate_id; + rc->phases.hold = vmcoreinfo_reset_hold; +} + static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -106,6 +137,18 @@ static const TypeInfo vmcoreinfo_types[] = { .parent = TYPE_DEVICE, .instance_size = sizeof(VMCoreInfoState), .class_init = vmcoreinfo_device_class_init, + }, + { + .name = TYPE_VMCOREINFO, + .parent = TYPE_OBJECT, + .instance_size = sizeof(VMCoreInfoState), + .class_init = vmcoreinfo_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_RESETTABLE_INTERFACE }, + { TYPE_USER_CREATABLE }, + { TYPE_VMSTATE_IF }, + { } + } } };
@@ -116,6 +159,9 @@ VMCoreInfoState *vmcoreinfo_find(void) Object *obj;
obj = object_resolve_path_type("", TYPE_VMCOREINFO_DEVICE, NULL); + if (!obj) { + obj = object_resolve_path_type("", TYPE_VMCOREINFO, NULL); + }
return obj ? (VMCoreInfoState *)obj : NULL; } -- 2.47.1
On 20/12/24 09:50, Marc-André Lureau wrote:
Hi
On Thu, Dec 19, 2024 at 7:39 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
'vmcore-info' object allow to transition from '-device' to 'object', following the deprecation process.
Is there a strong motivation behind this? just replacing -device with -object doesn't really give anything, does it?
No. (Daniel mentioned the same on the cover letter: https://lore.kernel.org/qemu-devel/2ae4799b-f78d-4fde-becb-9ee7f767e8f1@lina...) Sorry for wasting review cycles.
Also I'd rather keep the name "vmcoreinfo" since that's how it used to be, and also the name used by the kernel ELF etc.
No need to modify VMCoreInfoState since DeviceState already inherits from Object state.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/hw/misc/vmcoreinfo.h | 4 ++- hw/misc/vmcoreinfo.c | 48 +++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-)
'-device vmcoreinfo' is replaced by '-object vmcore-info'. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- docs/about/deprecated.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index d6809f94ea1..57a3d734081 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -79,6 +79,11 @@ described with -smp are supported by the target machine. Use ``-run-with user=..`` instead. +``-device vmcoreinfo`` (since 10.0) +''''''''''''''''''''''''''''''''''' + +Use ``-object vmcore-info`` instead. + User-mode emulator command line arguments ----------------------------------------- -- 2.47.1
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- docs/about/deprecated.rst | 5 ---- docs/about/removed-features.rst | 5 ++++ include/hw/misc/vmcoreinfo.h | 3 +-- hw/misc/vmcoreinfo.c | 44 ++++++--------------------------- 4 files changed, 13 insertions(+), 44 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 57a3d734081..d6809f94ea1 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -79,11 +79,6 @@ described with -smp are supported by the target machine. Use ``-run-with user=..`` instead. -``-device vmcoreinfo`` (since 10.0) -''''''''''''''''''''''''''''''''''' - -Use ``-object vmcore-info`` instead. - User-mode emulator command line arguments ----------------------------------------- diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index cb1388049a8..6fedf13c133 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -555,6 +555,11 @@ to produce an odd effect (rotating input but not display output). But this was never intended or documented behaviour, so we have dropped the options along with the machine models they were intended for. +``-device vmcoreinfo`` (removed in 10.2) +'''''''''''''''''''''''''''''''''''''''' + +``-device vmcoreinfo`` has been replaced by ``-object vmcore-info``. + User-mode emulator command line arguments ----------------------------------------- diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h index d4cce42cee6..56af12d33a4 100644 --- a/include/hw/misc/vmcoreinfo.h +++ b/include/hw/misc/vmcoreinfo.h @@ -12,7 +12,6 @@ #ifndef VMCOREINFO_H #define VMCOREINFO_H -#include "hw/qdev-core.h" #include "standard-headers/linux/qemu_fw_cfg.h" #include "qom/object.h" @@ -26,7 +25,7 @@ DECLARE_INSTANCE_CHECKER(VMCoreInfoState, VMCOREINFO_DEVICE, typedef struct fw_cfg_vmcoreinfo FWCfgVMCoreInfo; struct VMCoreInfoState { - DeviceState parent_obj; + Object parent_obj; bool has_vmcoreinfo; FWCfgVMCoreInfo vmcoreinfo; diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c index e2258e08fb1..36d1143722e 100644 --- a/hw/misc/vmcoreinfo.c +++ b/hw/misc/vmcoreinfo.c @@ -54,8 +54,9 @@ static void vmcoreinfo_reset_hold(Object *obj, ResetType type) s->vmcoreinfo.host_format = cpu_to_le16(FW_CFG_VMCOREINFO_FORMAT_ELF); } -static void vmcoreinfo_realize(VMCoreInfoState *s, Error **errp) +static void vmcoreinfo_complete(UserCreatable *uc, Error **errp) { + VMCoreInfoState *s = VMCOREINFO(uc); FWCfgState *fw_cfg = fw_cfg_find(); /* for gdb script dump-guest-memory.py */ static VMCoreInfoState * volatile vmcoreinfo_state G_GNUC_UNUSED; @@ -76,6 +77,10 @@ static void vmcoreinfo_realize(VMCoreInfoState *s, Error **errp) return; } + if (vmstate_register_any(VMSTATE_IF(s), &vmstate_vmcoreinfo, s) < 0) { + error_setg(errp, "%s: Failed to register vmstate", TYPE_VMCOREINFO); + } + fw_cfg_add_file_callback(fw_cfg, FW_CFG_VMCOREINFO_FILENAME, NULL, fw_cfg_vmci_write, s, &s->vmcoreinfo, sizeof(s->vmcoreinfo), false); @@ -88,25 +93,11 @@ static void vmcoreinfo_realize(VMCoreInfoState *s, Error **errp) vmcoreinfo_state = s; } -static void vmcoreinfo_device_realize(DeviceState *dev, Error **errp) -{ - vmcoreinfo_realize(VMCOREINFO_DEVICE(dev), errp); -} - static bool vmcoreinfo_can_be_deleted(UserCreatable *uc) { return false; } -static void vmcoreinfo_complete(UserCreatable *uc, Error **errp) -{ - if (vmstate_register_any(VMSTATE_IF(uc), &vmstate_vmcoreinfo, uc) < 0) { - error_setg(errp, "%s: Failed to register vmstate", TYPE_VMCOREINFO); - } - - vmcoreinfo_realize(VMCOREINFO(uc), errp); -} - static void vmcoreinfo_class_init(ObjectClass *oc, void *data) { UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc); @@ -119,25 +110,7 @@ static void vmcoreinfo_class_init(ObjectClass *oc, void *data) rc->phases.hold = vmcoreinfo_reset_hold; } -static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data) -{ - DeviceClass *dc = DEVICE_CLASS(klass); - ResettableClass *rc = RESETTABLE_CLASS(klass); - - dc->vmsd = &vmstate_vmcoreinfo; - dc->realize = vmcoreinfo_device_realize; - dc->hotpluggable = false; - set_bit(DEVICE_CATEGORY_MISC, dc->categories); - rc->phases.hold = vmcoreinfo_reset_hold; -} - static const TypeInfo vmcoreinfo_types[] = { - { - .name = TYPE_VMCOREINFO_DEVICE, - .parent = TYPE_DEVICE, - .instance_size = sizeof(VMCoreInfoState), - .class_init = vmcoreinfo_device_class_init, - }, { .name = TYPE_VMCOREINFO, .parent = TYPE_OBJECT, @@ -158,10 +131,7 @@ VMCoreInfoState *vmcoreinfo_find(void) { Object *obj; - obj = object_resolve_path_type("", TYPE_VMCOREINFO_DEVICE, NULL); - if (!obj) { - obj = object_resolve_path_type("", TYPE_VMCOREINFO, NULL); - } + obj = object_resolve_path_type("", TYPE_VMCOREINFO, NULL); return obj ? (VMCoreInfoState *)obj : NULL; } -- 2.47.1
On Thu, Dec 19, 2024 at 04:38:47PM +0100, Philippe Mathieu-Daudé wrote:
No reason for vmcoreinfo to be based on QDev, since it doesn't use any QDev API. Demote to plain Object.
I'm not especially convinced by that rationale, at least in the short term[1]. As a user of QEMU, I would tend to view -device as being the way to create devices that are visible to the guest, and -object as being for dealing with host backends. vmcoreinfo is quite an unusal device, in that if only exists as a fwcfg field, but that's an internal impl detail from a user's POV, and it is still a guest visible device type. So while it may not leverage QDev API, I still feel it is more of a fit for -device, than -object. Is there any functional benefit to moving it to -object, that outweighs the disruption ? With regards, Daniel [1] I say "short term", because overall I'm of the opinion that -device, -machine, -cpu, -chardev, etc should not exist, and -object ought to be made capable of instantiating any type of object whatever subclass it might be. I doubt that will change any time in the forseeable future though, so it is more of a hypothetical point. -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, 19 Dec, 2024, 9:27 pm Daniel P. Berrangé, <berrange@redhat.com> wrote:
On Thu, Dec 19, 2024 at 04:38:47PM +0100, Philippe Mathieu-Daudé wrote:
No reason for vmcoreinfo to be based on QDev, since it doesn't use any QDev API. Demote to plain Object.
I'm not especially convinced by that rationale, at least in the short term[1].
As a user of QEMU, I would tend to view -device as being the way to create devices that are visible to the guest, and -object as being for dealing with host backends.
vmcoreinfo is quite an unusal device, in that if only exists as a fwcfg field, but that's an internal impl detail from a user's POV, and it is still a guest visible device type.
So while it may not leverage QDev API, I still feel it is more of a fit for -device, than -object.
+1 to this. Is there any functional benefit
to moving it to -object, that outweighs the disruption ?
On 19/12/24 16:57, Daniel P. Berrangé wrote:
On Thu, Dec 19, 2024 at 04:38:47PM +0100, Philippe Mathieu-Daudé wrote:
No reason for vmcoreinfo to be based on QDev, since it doesn't use any QDev API. Demote to plain Object.
I'm not especially convinced by that rationale, at least in the short term[1].
As a user of QEMU, I would tend to view -device as being the way to create devices that are visible to the guest, and -object as being for dealing with host backends.
Hmm.
vmcoreinfo is quite an unusal device, in that if only exists as a fwcfg field, but that's an internal impl detail from a user's POV, and it is still a guest visible device type.
So while it may not leverage QDev API, I still feel it is more of a fit for -device, than -object. Is there any functional benefit to moving it to -object, that outweighs the disruption ?
No. Patches 1-5 are still useful cleanups although.
[1] I say "short term", because overall I'm of the opinion that -device, -machine, -cpu, -chardev, etc should not exist, and -object ought to be made capable of instantiating any type of object whatever subclass it might be. I doubt that will change any time in the forseeable future though, so it is more of a hypothetical point.
Agreed.
participants (4)
-
Ani Sinha -
Daniel P. Berrangé -
Marc-André Lureau -
Philippe Mathieu-Daudé