[Qemu-devel] [PATCH 0/3] fw_cfg: misc fixes

[Qemu-devel] [PATCH 0/3] fw_cfg: misc fixes

[Michael S. Tsirkin]

Here are some misc fixes to the fw cfg object
handling.
I've queued them on my pci branch temporarily
as it's useful to cleanup some pci things.
I'm using this with Laszlo's cleanup patch
that got rid of void * in pc.c too -
that's queued there as well.

Please review and comment.

Michael S. Tsirkin (3):
  pvpanic: use FWCfgState explicitly
  fw_cfg: add API to find FW cfg object
  fw_cfg: fw_cfg is a singleton

 hw/misc/pvpanic.c         |  4 ++--
 hw/nvram/fw_cfg.c         | 18 +++++++++++++-----
 include/hw/nvram/fw_cfg.h |  2 ++
 3 files changed, 17 insertions(+), 7 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Michael S. Tsirkin]

Use the type-safe FWCfgState structure instead
of the unsafe void *.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/misc/pvpanic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 31e1b1d..1483f27 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
 {
     PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
     static bool port_configured;
-    void *fw_cfg;
+    FWCfgState *fw_cfg;
 
     memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
     isa_register_ioport(dev, &s->io, s->ioport);
-- 
MST

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Laszlo Ersek]

On 05/30/13 15:27, Michael S. Tsirkin wrote:
> Use the type-safe FWCfgState structure instead
> of the unsafe void *.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/misc/pvpanic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 31e1b1d..1483f27 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>  {
>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>      static bool port_configured;
> -    void *fw_cfg;
> +    FWCfgState *fw_cfg;
>  
>      memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
>      isa_register_ioport(dev, &s->io, s->ioport);
> 

Doesn't this break your build? Lower down in the function there's

        fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);

and object_resolve_path() returns a pointer-to-Object, not
pointer-to-FWCfgState.

But for starters I'm quite confused about how the unpatched function
works. What it does amounts to:

  fw_cfg_add_file(object_resolve_path(...), ...);

But, again object_resolve_path() returns pointer-to-Object. I'm checking
"struct Object" in "include/qom/object.h", and it suggests that derived
structs should embed Object as first member. However FWCfgState is *not*
such a derived member. What's going on here?

http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450

Laszlo

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Paolo Bonzini]

Il 30/05/2013 17:05, Laszlo Ersek ha scritto:
> But, again object_resolve_path() returns pointer-to-Object. I'm checking
> "struct Object" in "include/qom/object.h", and it suggests that derived
> structs should embed Object as first member. However FWCfgState is *not*
> such a derived member. What's going on here?
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
> http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450

FWCfgState embeds Object via SysBusDevice and DeviceState.

Paolo

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Laszlo Ersek]

On 05/30/13 17:41, Paolo Bonzini wrote:
> Il 30/05/2013 17:05, Laszlo Ersek ha scritto:
>> But, again object_resolve_path() returns pointer-to-Object. I'm checking
>> "struct Object" in "include/qom/object.h", and it suggests that derived
>> structs should embed Object as first member. However FWCfgState is *not*
>> such a derived member. What's going on here?
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
>> http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450
> 
> FWCfgState embeds Object via SysBusDevice and DeviceState.

Clearly an evil trick by a devious mind.

Thanks :)
Laszlo

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Laszlo Ersek]

On 05/30/13 17:05, Laszlo Ersek wrote:
> On 05/30/13 15:27, Michael S. Tsirkin wrote:
>> Use the type-safe FWCfgState structure instead
>> of the unsafe void *.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/misc/pvpanic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>> index 31e1b1d..1483f27 100644
>> --- a/hw/misc/pvpanic.c
>> +++ b/hw/misc/pvpanic.c
>> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>>  {
>>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>>      static bool port_configured;
>> -    void *fw_cfg;
>> +    FWCfgState *fw_cfg;
>>  
>>      memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
>>      isa_register_ioport(dev, &s->io, s->ioport);
>>
> 
> Doesn't this break your build? Lower down in the function there's
> 
>         fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
> 
> and object_resolve_path() returns a pointer-to-Object, not
> pointer-to-FWCfgState.

Paolo explained the guts, but don't we still need a downcast here? (No
idea how to do that nicely in the object model du jour -- maybe
OBJECT_CHECK() or similar?)

Laszlo

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Paolo Bonzini]

Il 30/05/2013 18:03, Laszlo Ersek ha scritto:
> On 05/30/13 17:05, Laszlo Ersek wrote:
>> On 05/30/13 15:27, Michael S. Tsirkin wrote:
>>> Use the type-safe FWCfgState structure instead
>>> of the unsafe void *.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/misc/pvpanic.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
>>> index 31e1b1d..1483f27 100644
>>> --- a/hw/misc/pvpanic.c
>>> +++ b/hw/misc/pvpanic.c
>>> @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>>>  {
>>>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
>>>      static bool port_configured;
>>> -    void *fw_cfg;
>>> +    FWCfgState *fw_cfg;
>>>  
>>>      memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
>>>      isa_register_ioport(dev, &s->io, s->ioport);
>>>
>>
>> Doesn't this break your build? Lower down in the function there's
>>
>>         fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
>>
>> and object_resolve_path() returns a pointer-to-Object, not
>> pointer-to-FWCfgState.
> 
> Paolo explained the guts, but don't we still need a downcast here? (No
> idea how to do that nicely in the object model du jour -- maybe
> OBJECT_CHECK() or similar?)

Patch 2 addresses that.

Paolo

Re: [Qemu-devel] [PATCH 1/3] pvpanic: use FWCfgState explicitly

[Michael S. Tsirkin]

On Thu, May 30, 2013 at 05:05:51PM +0200, Laszlo Ersek wrote:
> On 05/30/13 15:27, Michael S. Tsirkin wrote:
> > Use the type-safe FWCfgState structure instead
> > of the unsafe void *.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/misc/pvpanic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> > index 31e1b1d..1483f27 100644
> > --- a/hw/misc/pvpanic.c
> > +++ b/hw/misc/pvpanic.c
> > @@ -90,7 +90,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
> >  {
> >      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> >      static bool port_configured;
> > -    void *fw_cfg;
> > +    FWCfgState *fw_cfg;
> >  
> >      memory_region_init_io(&s->io, &pvpanic_ops, s, "pvpanic", 1);
> >      isa_register_ioport(dev, &s->io, s->ioport);
> > 
> 
> Doesn't this break your build? Lower down in the function there's
> 
>         fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
> 
> and object_resolve_path() returns a pointer-to-Object, not
> pointer-to-FWCfgState.

I see, I think I should reorder the patches.

> But for starters I'm quite confused about how the unpatched function
> works. What it does amounts to:
> 
>   fw_cfg_add_file(object_resolve_path(...), ...);
> 
> But, again object_resolve_path() returns pointer-to-Object. I'm checking
> "struct Object" in "include/qom/object.h", and it suggests that derived
> structs should embed Object as first member. However FWCfgState is *not*
> such a derived member. What's going on here?
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/201544/focus=201564
> http://thread.gmane.org/gmane.comp.emulators.qemu/204452/focus=204450
> 
> Laszlo

[Qemu-devel] [PATCH 2/3] fw_cfg: add API to find FW cfg object

[Michael S. Tsirkin]

Remove some code duplication by adding a
function to look up the fw cfg file.
This way, we don't need to duplicate same strings everywhere.
Use by both fw cfg and pvpanic device.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/misc/pvpanic.c         |  2 +-
 hw/nvram/fw_cfg.c         | 15 ++++++++++++---
 include/hw/nvram/fw_cfg.h |  2 ++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 1483f27..910e44f 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -96,7 +96,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
     isa_register_ioport(dev, &s->io, s->ioport);
 
     if (!port_configured) {
-        fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
+        fw_cfg = fw_cfg_find();
         if (fw_cfg) {
             fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
                             g_memdup(&s->ioport, sizeof(s->ioport)),
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 479113b..df3f089 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -32,6 +32,9 @@
 
 #define FW_CFG_SIZE 2
 #define FW_CFG_DATA_SIZE 1
+#define TYPE_FW_CFG "fw_cfg"
+#define FW_CFG_NAME "fw_cfg"
+#define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
 typedef struct FWCfgEntry {
     uint32_t len;
@@ -493,8 +496,8 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
 
     s = DO_UPCAST(FWCfgState, busdev.qdev, dev);
 
-    if (!object_resolve_path("/machine/fw_cfg", NULL)) {
-        object_property_add_child(qdev_get_machine(), "fw_cfg", OBJECT(s),
+    if (!object_resolve_path(FW_CFG_PATH, NULL)) {
+        object_property_add_child(qdev_get_machine(), FW_CFG_NAME, OBJECT(s),
                                   NULL);
     }
 
@@ -553,6 +556,12 @@ static Property fw_cfg_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+FWCfgState *fw_cfg_find(void)
+{
+    return OBJECT_CHECK(FWCfgState, object_resolve_path(FW_CFG_PATH, NULL),
+                        TYPE_FW_CFG);
+}
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -566,7 +575,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo fw_cfg_info = {
-    .name          = "fw_cfg",
+    .name          = TYPE_FW_CFG,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(FWCfgState),
     .class_init    = fw_cfg_class_init,
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f37714e..f60dd67 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -73,6 +73,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
+FWCfgState *fw_cfg_find(void);
+
 #endif /* NO_QEMU_PROTOS */
 
 #endif
-- 
MST

Re: [Qemu-devel] [PATCH 2/3] fw_cfg: add API to find FW cfg object

[Laszlo Ersek]

On 05/30/13 15:28, Michael S. Tsirkin wrote:

> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 1483f27..910e44f 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -96,7 +96,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>      isa_register_ioport(dev, &s->io, s->ioport);
>  
>      if (!port_configured) {
> -        fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
> +        fw_cfg = fw_cfg_find();
>          if (fw_cfg) {
>              fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
>                              g_memdup(&s->ioport, sizeof(s->ioport)),

Explains 1/3 too, OK.
Laszlo

Re: [Qemu-devel] [PATCH 2/3] fw_cfg: add API to find FW cfg object

[Hu Tao]

On Thu, May 30, 2013 at 04:28:01PM +0300, Michael S. Tsirkin wrote:
> Remove some code duplication by adding a
> function to look up the fw cfg file.
> This way, we don't need to duplicate same strings everywhere.
> Use by both fw cfg and pvpanic device.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/misc/pvpanic.c         |  2 +-
>  hw/nvram/fw_cfg.c         | 15 ++++++++++++---
>  include/hw/nvram/fw_cfg.h |  2 ++
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 1483f27..910e44f 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -96,7 +96,7 @@ static int pvpanic_isa_initfn(ISADevice *dev)
>      isa_register_ioport(dev, &s->io, s->ioport);
>  
>      if (!port_configured) {
> -        fw_cfg = object_resolve_path("/machine/fw_cfg", NULL);
> +        fw_cfg = fw_cfg_find();
>          if (fw_cfg) {
>              fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
>                              g_memdup(&s->ioport, sizeof(s->ioport)),
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 479113b..df3f089 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -32,6 +32,9 @@
>  
>  #define FW_CFG_SIZE 2
>  #define FW_CFG_DATA_SIZE 1
> +#define TYPE_FW_CFG "fw_cfg"
> +#define FW_CFG_NAME "fw_cfg"
> +#define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
>  typedef struct FWCfgEntry {
>      uint32_t len;
> @@ -493,8 +496,8 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>  
>      s = DO_UPCAST(FWCfgState, busdev.qdev, dev);
>  
> -    if (!object_resolve_path("/machine/fw_cfg", NULL)) {
> -        object_property_add_child(qdev_get_machine(), "fw_cfg", OBJECT(s),
> +    if (!object_resolve_path(FW_CFG_PATH, NULL)) {
> +        object_property_add_child(qdev_get_machine(), FW_CFG_NAME, OBJECT(s),
>                                    NULL);
>      }
>  
> @@ -553,6 +556,12 @@ static Property fw_cfg_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +FWCfgState *fw_cfg_find(void)
> +{
> +    return OBJECT_CHECK(FWCfgState, object_resolve_path(FW_CFG_PATH, NULL),
> +                        TYPE_FW_CFG);
> +}
> +
>  static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -566,7 +575,7 @@ static void fw_cfg_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo fw_cfg_info = {
> -    .name          = "fw_cfg",
> +    .name          = TYPE_FW_CFG,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(FWCfgState),
>      .class_init    = fw_cfg_class_init,
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f37714e..f60dd67 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -73,6 +73,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>  FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>                          hwaddr crl_addr, hwaddr data_addr);
>  
> +FWCfgState *fw_cfg_find(void);

How about fw_cfg_get()? It makes me more comfortable than fw_cfg_find().

> +
>  #endif /* NO_QEMU_PROTOS */
>  
>  #endif
> -- 
> MST
> 

[Qemu-devel] [PATCH 3/3] fw_cfg: fw_cfg is a singleton

[Michael S. Tsirkin]

Make sure we only have a single instance ever:
because if it isn't we can't find it so it's
useless anyway.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/nvram/fw_cfg.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index df3f089..3c255ce 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -496,10 +496,9 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
 
     s = DO_UPCAST(FWCfgState, busdev.qdev, dev);
 
-    if (!object_resolve_path(FW_CFG_PATH, NULL)) {
-        object_property_add_child(qdev_get_machine(), FW_CFG_NAME, OBJECT(s),
-                                  NULL);
-    }
+    assert(!object_resolve_path(FW_CFG_PATH, NULL));
+
+    object_property_add_child(qdev_get_machine(), FW_CFG_NAME, OBJECT(s), NULL);
 
     qdev_init_nofail(dev);
 
-- 
MST

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: misc fixes

[Laszlo Ersek]

On 05/30/13 15:27, Michael S. Tsirkin wrote:
> Here are some misc fixes to the fw cfg object
> handling.
> I've queued them on my pci branch temporarily
> as it's useful to cleanup some pci things.
> I'm using this with Laszlo's cleanup patch
> that got rid of void * in pc.c too -
> that's queued there as well.
> 
> Please review and comment.
> 
> Michael S. Tsirkin (3):
>   pvpanic: use FWCfgState explicitly
>   fw_cfg: add API to find FW cfg object
>   fw_cfg: fw_cfg is a singleton
> 
>  hw/misc/pvpanic.c         |  4 ++--
>  hw/nvram/fw_cfg.c         | 18 +++++++++++++-----
>  include/hw/nvram/fw_cfg.h |  2 ++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: misc fixes

[Hu Tao]

On Thu, May 30, 2013 at 04:27:56PM +0300, Michael S. Tsirkin wrote:
> Here are some misc fixes to the fw cfg object
> handling.
> I've queued them on my pci branch temporarily
> as it's useful to cleanup some pci things.
> I'm using this with Laszlo's cleanup patch
> that got rid of void * in pc.c too -
> that's queued there as well.
> 
> Please review and comment.
> 
> Michael S. Tsirkin (3):
>   pvpanic: use FWCfgState explicitly
>   fw_cfg: add API to find FW cfg object
>   fw_cfg: fw_cfg is a singleton
> 
>  hw/misc/pvpanic.c         |  4 ++--
>  hw/nvram/fw_cfg.c         | 18 +++++++++++++-----
>  include/hw/nvram/fw_cfg.h |  2 ++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> -- 
> MST
> 

Series looks good to me, except a reorder.