[RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object

[RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object

[Philippe Mathieu-Daudé]

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

[RFC PATCH 01/10] hw/misc/vmcoreinfo: Declare QOM type using DEFINE_TYPES macro

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 01/10] hw/misc/vmcoreinfo: Declare QOM type using DEFINE_TYPES macro

[Daniel P. Berrangé]

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 :|

[RFC PATCH 02/10] hw/misc/vmcoreinfo: Rename opaque pointer as 'opaque'

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 02/10] hw/misc/vmcoreinfo: Rename opaque pointer as 'opaque'

[Daniel P. Berrangé]

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 :|

[RFC PATCH 03/10] hw/misc/vmcoreinfo: Un-inline vmcoreinfo_find()

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 03/10] hw/misc/vmcoreinfo: Un-inline vmcoreinfo_find()

[Daniel P. Berrangé]

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 :|

[RFC PATCH 04/10] hw/misc/vmcoreinfo: Rename VMCOREINFO_DEVICE -> TYPE_VMCOREINFO_DEVICE

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 04/10] hw/misc/vmcoreinfo: Rename VMCOREINFO_DEVICE -> TYPE_VMCOREINFO_DEVICE

[Daniel P. Berrangé]

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 :|

Re: [RFC PATCH 04/10] hw/misc/vmcoreinfo: Rename VMCOREINFO_DEVICE -> TYPE_VMCOREINFO_DEVICE

[Philippe Mathieu-Daudé]

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

[RFC PATCH 05/10] hw/misc/vmcoreinfo: Convert to three-phase reset interface

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 05/10] hw/misc/vmcoreinfo: Convert to three-phase reset interface

[Daniel P. Berrangé]

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 :|

[RFC PATCH 06/10] hw/misc/vmcoreinfo: Move vmstate_vmcoreinfo[] around

[Philippe Mathieu-Daudé]

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

[RFC PATCH 07/10] hw/misc/vmcoreinfo: Factor vmcoreinfo_device_realize() out

[Philippe Mathieu-Daudé]

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

[RFC PATCH 08/10] hw/misc/vmcoreinfo: Implement 'vmcore-info' object

[Philippe Mathieu-Daudé]

'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

Re: [RFC PATCH 08/10] hw/misc/vmcoreinfo: Implement 'vmcore-info' object

[Marc-André Lureau]

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
>

Re: [RFC PATCH 08/10] hw/misc/vmcoreinfo: Implement 'vmcore-info' object

[Philippe Mathieu-Daudé]

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@linaro.org/)

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(-)

[RFC PATCH 09/10] hw/misc/vmcoreinfo: Deprecate '-device vmcoreinfo'

[Philippe Mathieu-Daudé]

'-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

[RFC PATCH-for-10.2 10/10] hw/misc/vmcoreinfo: Remove legacy '-device vmcoreinfo'

[Philippe Mathieu-Daudé]

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

Re: [RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object

[Daniel P. Berrangé]

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 :|

Re: [RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

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 ?


>

[-- Attachment #2: Type: text/html, Size: 1760 bytes --]

Re: [RFC PATCH 00/10] hw/misc/vmcoreinfo: Convert from QDev to plain Object

[Philippe Mathieu-Daudé]

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.