[PATCH 0/2] hw: Delay timer_new() from init to realize to avoid memleaks

[PATCH 0/2] hw: Delay timer_new() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

After reviewing various patches from Pan Nengyuan fixing errors
reported Huawei's Euler Robot, I wrote this tiny coccinelle script
to find all occurences of this pattern:

    @ match @
    identifier instance_init;
    typedef Object;
    identifier obj;
    expression val, scale;
    identifier clock_type, callback, opaque;
    position pos;
    @@
    static void instance_init(Object *obj)
    {
      <...
    (
      val = timer_new@pos(clock_type, scale, callback, opaque);
    |
      val = timer_new_ns@pos(clock_type, callback, opaque);
    |
      val = timer_new_us@pos(clock_type, callback, opaque);
    |
      val = timer_new_ms@pos(clock_type, callback, opaque);
    )
      ...>
    }

    @ script:python @
    f << match.instance_init;
    p << match.pos;
    @@
    print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)

The script produces:

  $ docker run --rm -v $PWD:$PWD -w $PWD philmd/coccinelle \
     --macro-file scripts/cocci-macro-file.h \
     --sp-file scripts/coccinelle/init_timer_new.cocci
  init_defs_builtins: /usr/lib/coccinelle/standard.h
  init_defs: scripts/cocci-macro-file.h
  check hw/ipmi/ipmi_bmc_extern.c:505:24 in ipmi_bmc_extern_init()
  check hw/misc/mos6522.c:489:25 in mos6522_init()
  check hw/rtc/pl031.c:194:15 in pl031_init()
  check hw/arm/pxa2xx.c:1137:19 in pxa2xx_rtc_init()
  check target/s390x/cpu.c:283:8 in s390_cpu_initfn()
  check hw/sd/sd.c:2061:26 in sd_instance_init()
  check hw/arm/spitz.c:527:18 in spitz_keyboard_init()
  check hw/arm/strongarm.c:402:19 in strongarm_rtc_init()
  check hw/arm/strongarm.c:1244:26 in strongarm_uart_init()

Pan fixed most of the occurences. This series fixes the last two.

Philippe Mathieu-Daudé (2):
  hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid
    memleaks
  hw/sd/sd: Delay timer_new_ns() from init to realize to avoid memleaks

 hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
 hw/sd/sd.c                | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.21.1

[PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

In commit f3a508eb4e the Euler Robot reported calling timer_new()
in instance_init() can leak heap memory. The easier fix is to
delay the timer creation at instance realize(). Similarly move
timer_del() into a new instance unrealize() method.

This case was found with the following coccinelle script:

    @ match @
    identifier instance_init;
    typedef Object;
    identifier obj;
    expression val, scale;
    identifier clock_type, callback, opaque;
    position pos;
    @@
    static void instance_init(Object *obj)
    {
      <...
    (
      val = timer_new@pos(clock_type, scale, callback, opaque);
    |
      val = timer_new_ns@pos(clock_type, callback, opaque);
    |
      val = timer_new_us@pos(clock_type, callback, opaque);
    |
      val = timer_new_ms@pos(clock_type, callback, opaque);
    )
      ...>
    }

    @ script:python @
    f << match.instance_init;
    p << match.pos;
    @@
    print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Pan Nengyuan <pannengyuan@huawei.com>
---
 hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index f9a13e0a44..9144ac6c38 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
 
     qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
                              chr_event, NULL, ibe, NULL, true);
+
+    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
+}
+
+static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp)
+{
+    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
+
+    timer_del(ibe->extern_timer);
 }
 
 static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
@@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj)
 {
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
 
-    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
     vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
 }
 
@@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj)
 {
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
 
-    timer_del(ibe->extern_timer);
     timer_free(ibe->extern_timer);
 }
 
@@ -528,6 +535,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, void *data)
     bk->handle_reset = ipmi_bmc_extern_handle_reset;
     dc->hotpluggable = false;
     dc->realize = ipmi_bmc_extern_realize;
+    dc->unrealize = ipmi_bmc_extern_unrealize;
     device_class_set_props(dc, ipmi_bmc_extern_properties);
 }
 
-- 
2.21.1

[PATCH 2/2] hw/sd/sd: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

In commit f3a508eb4e the Euler Robot reported calling timer_new()
in instance_init() can leak heap memory. The easier fix is to
delay the timer creation at instance realize(). Similarly move
timer_del() into a new instance unrealize() method.

This case was found with the following coccinelle script:

    @ match @
    identifier instance_init;
    typedef Object;
    identifier obj;
    expression val, scale;
    identifier clock_type, callback, opaque;
    position pos;
    @@
    static void instance_init(Object *obj)
    {
      <...
    (
      val = timer_new@pos(clock_type, scale, callback, opaque);
    |
      val = timer_new_ns@pos(clock_type, callback, opaque);
    |
      val = timer_new_us@pos(clock_type, callback, opaque);
    |
      val = timer_new_ms@pos(clock_type, callback, opaque);
    )
      ...>
    }

    @ script:python @
    f << match.instance_init;
    p << match.pos;
    @@
    print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Pan Nengyuan <pannengyuan@huawei.com>
---
 hw/sd/sd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 71a9af09ab..d72cf3de2a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2058,14 +2058,12 @@ static void sd_instance_init(Object *obj)
     SDState *sd = SD_CARD(obj);
 
     sd->enable = true;
-    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
 }
 
 static void sd_instance_finalize(Object *obj)
 {
     SDState *sd = SD_CARD(obj);
 
-    timer_del(sd->ocr_power_timer);
     timer_free(sd->ocr_power_timer);
 }
 
@@ -2098,6 +2096,15 @@ static void sd_realize(DeviceState *dev, Error **errp)
         }
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
+
+    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
+}
+
+static void sd_unrealize(DeviceState *dev, Error **errp)
+{
+    SDState *sd = SD_CARD(dev);
+
+    timer_del(sd->ocr_power_timer);
 }
 
 static Property sd_properties[] = {
@@ -2118,6 +2125,7 @@ static void sd_class_init(ObjectClass *klass, void *data)
     SDCardClass *sc = SD_CARD_CLASS(klass);
 
     dc->realize = sd_realize;
+    dc->unrealize = sd_unrealize;
     device_class_set_props(dc, sd_properties);
     dc->vmsd = &sd_vmstate;
     dc->reset = sd_reset;
-- 
2.21.1

Re: [PATCH 0/2] hw: Delay timer_new() from init to realize to avoid memleaks

[Richard Henderson]

On 2/15/20 7:47 AM, Philippe Mathieu-Daudé wrote:
> Philippe Mathieu-Daudé (2):
>   hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid
>     memleaks
>   hw/sd/sd: Delay timer_new_ns() from init to realize to avoid memleaks

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Corey Minyard]

On Sat, Feb 15, 2020 at 04:47:05PM +0100, Philippe Mathieu-Daudé wrote:
> In commit f3a508eb4e the Euler Robot reported calling timer_new()
> in instance_init() can leak heap memory. The easier fix is to
> delay the timer creation at instance realize(). Similarly move
> timer_del() into a new instance unrealize() method.
> 
> This case was found with the following coccinelle script:
> 
>     @ match @
>     identifier instance_init;
>     typedef Object;
>     identifier obj;
>     expression val, scale;
>     identifier clock_type, callback, opaque;
>     position pos;
>     @@
>     static void instance_init(Object *obj)
>     {
>       <...
>     (
>       val = timer_new@pos(clock_type, scale, callback, opaque);
>     |
>       val = timer_new_ns@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_us@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_ms@pos(clock_type, callback, opaque);
>     )
>       ...>
>     }
> 
>     @ script:python @
>     f << match.instance_init;
>     p << match.pos;
>     @@
>     print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

This looks ok to me:

Acked-by: Corey Minyard <cminyard@mvista.com>

I can take it into my tree, if you like.

-corey

> ---
> Cc: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index f9a13e0a44..9144ac6c38 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
>  
>      qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
>                               chr_event, NULL, ibe, NULL, true);
> +
> +    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
> +}
> +
> +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
> +
> +    timer_del(ibe->extern_timer);
>  }
>  
>  static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
> @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>  
> -    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
>      vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
>  }
>  
> @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>  
> -    timer_del(ibe->extern_timer);
>      timer_free(ibe->extern_timer);
>  }
>  
> @@ -528,6 +535,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, void *data)
>      bk->handle_reset = ipmi_bmc_extern_handle_reset;
>      dc->hotpluggable = false;
>      dc->realize = ipmi_bmc_extern_realize;
> +    dc->unrealize = ipmi_bmc_extern_unrealize;
>      device_class_set_props(dc, ipmi_bmc_extern_properties);
>  }
>  
> -- 
> 2.21.1
> 

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> In commit f3a508eb4e the Euler Robot reported calling timer_new()
> in instance_init() can leak heap memory. The easier fix is to
> delay the timer creation at instance realize(). Similarly move
> timer_del() into a new instance unrealize() method.
>
> This case was found with the following coccinelle script:
>
>     @ match @
>     identifier instance_init;
>     typedef Object;
>     identifier obj;
>     expression val, scale;
>     identifier clock_type, callback, opaque;
>     position pos;
>     @@
>     static void instance_init(Object *obj)
>     {
>       <...
>     (
>       val = timer_new@pos(clock_type, scale, callback, opaque);
>     |
>       val = timer_new_ns@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_us@pos(clock_type, callback, opaque);
>     |
>       val = timer_new_ms@pos(clock_type, callback, opaque);
>     )
>       ...>
>     }
>
>     @ script:python @
>     f << match.instance_init;
>     p << match.pos;
>     @@
>     print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index f9a13e0a44..9144ac6c38 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
>
>      qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
>                               chr_event, NULL, ibe, NULL, true);
> +
> +    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
> +}
> +
> +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp)
> +{
> +    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
> +
> +    timer_del(ibe->extern_timer);
>  }
>
>  static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
> @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>
> -    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
>      vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
>  }
>
> @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>
> -    timer_del(ibe->extern_timer);
>      timer_free(ibe->extern_timer);
>  }

So we now call timer_new in realize, and timer_del in unrealize,
but timer_free in finalize. This seems unbalanced -- why not
call timer_free in unrealize too, if we're moving things?

Also, this is a case of code that's actually doing things right:
we free the memory that we allocated in init in finalize. So
we're not fixing any leak here, we're just moving code around
(which is reasonable if we're trying to standardize on "call
timer_new in realize, not init", but should be noted in the
commit message).

thanks
-- PMM

Re: [PATCH 2/2] hw/sd/sd: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> In commit f3a508eb4e the Euler Robot reported calling timer_new()
> in instance_init() can leak heap memory. The easier fix is to
> delay the timer creation at instance realize(). Similarly move
> timer_del() into a new instance unrealize() method.

> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 71a9af09ab..d72cf3de2a 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2058,14 +2058,12 @@ static void sd_instance_init(Object *obj)
>      SDState *sd = SD_CARD(obj);
>
>      sd->enable = true;
> -    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
>  }
>
>  static void sd_instance_finalize(Object *obj)
>  {
>      SDState *sd = SD_CARD(obj);
>
> -    timer_del(sd->ocr_power_timer);
>      timer_free(sd->ocr_power_timer);
>  }
>
> @@ -2098,6 +2096,15 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          }
>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>      }
> +
> +    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
> +}
> +
> +static void sd_unrealize(DeviceState *dev, Error **errp)
> +{
> +    SDState *sd = SD_CARD(dev);
> +
> +    timer_del(sd->ocr_power_timer);
>  }

Here too the old code was doing things correctly in that
it does a timer_del/timer_free on the timer it allocates
in instance_init, and the new code has weirdly split the
freeing between unrealize and finalize.

thanks
-- PMM

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/17/20 2:25 PM, Peter Maydell wrote:
> On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> In commit f3a508eb4e the Euler Robot reported calling timer_new()
>> in instance_init() can leak heap memory. The easier fix is to
>> delay the timer creation at instance realize(). Similarly move
>> timer_del() into a new instance unrealize() method.
>>
>> This case was found with the following coccinelle script:
>>
>>      @ match @
>>      identifier instance_init;
>>      typedef Object;
>>      identifier obj;
>>      expression val, scale;
>>      identifier clock_type, callback, opaque;
>>      position pos;
>>      @@
>>      static void instance_init(Object *obj)
>>      {
>>        <...
>>      (
>>        val = timer_new@pos(clock_type, scale, callback, opaque);
>>      |
>>        val = timer_new_ns@pos(clock_type, callback, opaque);
>>      |
>>        val = timer_new_us@pos(clock_type, callback, opaque);
>>      |
>>        val = timer_new_ms@pos(clock_type, callback, opaque);
>>      )
>>        ...>
>>      }
>>
>>      @ script:python @
>>      f << match.instance_init;
>>      p << match.pos;
>>      @@
>>      print "check %s:%s:%s in %s()" % (p[0].file, p[0].line, p[0].column, f)
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>>   hw/ipmi/ipmi_bmc_extern.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
>> index f9a13e0a44..9144ac6c38 100644
>> --- a/hw/ipmi/ipmi_bmc_extern.c
>> +++ b/hw/ipmi/ipmi_bmc_extern.c
>> @@ -463,6 +463,15 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
>>
>>       qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
>>                                chr_event, NULL, ibe, NULL, true);
>> +
>> +    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
>> +}
>> +
>> +static void ipmi_bmc_extern_unrealize(DeviceState *dev, Error **errp)
>> +{
>> +    IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
>> +
>> +    timer_del(ibe->extern_timer);
>>   }
>>
>>   static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
>> @@ -502,7 +511,6 @@ static void ipmi_bmc_extern_init(Object *obj)
>>   {
>>       IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>>
>> -    ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
>>       vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
>>   }
>>
>> @@ -510,7 +518,6 @@ static void ipmi_bmc_extern_finalize(Object *obj)
>>   {
>>       IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>>
>> -    timer_del(ibe->extern_timer);
>>       timer_free(ibe->extern_timer);
>>   }
> 
> So we now call timer_new in realize, and timer_del in unrealize,
> but timer_free in finalize. This seems unbalanced -- why not
> call timer_free in unrealize too, if we're moving things?
> 
> Also, this is a case of code that's actually doing things right:
> we free the memory that we allocated in init in finalize. So
> we're not fixing any leak here, we're just moving code around
> (which is reasonable if we're trying to standardize on "call
> timer_new in realize, not init", but should be noted in the
> commit message).

While I understand your point, I am confused because the documentation 
on unrealize() and finalize() is rather scarce (and not obvious for 
non-native english speaker). I think I'm not understanding QOM instance 
lifetime well (in particular hotplug devices) so I will let this series go.

Similar confusions:

* https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg03079.html

Eduardo Habkost wrote:
 > Philippe Mathieu-Daudé wrote:
 >> IIUC when we use both init() and realize(), realize() should
 >> only contains on code that consumes the object properties...
 >> But maybe the design is not clear. Then why not move all the
 >> init() code to realize()?
 >
 > Normally I would recommend the opposite: delay as much as
 > possible to realize(), to avoid unwanted side effects when
 > (e.g.) running qom-list-properties.

* https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04143.html

David Hildenbrand wrote:
 >> Philippe Mathieu-Daudé wrote:
 >> Similarly to your other cleanups, shouldn't we move the
 >> timer_new_ns() into a realize() function, then do the
 >> timer_del() in unrealize()?
 >
 > include/hw/qdev-core.h
 >
 > "Trivial field initializations should go into
 > #TypeInfo.instance_init. Operations depending on @props
 > static properties should go into @realize."

> 
> thanks
> -- PMM
> 

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 2/17/20 2:25 PM, Peter Maydell wrote:

> > So we now call timer_new in realize, and timer_del in unrealize,
> > but timer_free in finalize. This seems unbalanced -- why not
> > call timer_free in unrealize too, if we're moving things?
> >
> > Also, this is a case of code that's actually doing things right:
> > we free the memory that we allocated in init in finalize. So
> > we're not fixing any leak here, we're just moving code around
> > (which is reasonable if we're trying to standardize on "call
> > timer_new in realize, not init", but should be noted in the
> > commit message).
>
> While I understand your point, I am confused because the documentation
> on unrealize() and finalize() is rather scarce (and not obvious for
> non-native english speaker). I think I'm not understanding QOM instance
> lifetime well (in particular hotplug devices) so I will let this series go.

Yes, the documentation is really not good at all. The
basic structure as I understand it is that we have two-part
creation and two-part destruction:
 * instance_init is creation part 1
 * realize is creation part 2
 * unrealize is destruction part 1 and is the opposite of realize
 * instance_finalize is destruction part 2 and is the
   opposite of instance_init

(Base QOM objects only have instance_init/instance_finalize;
realize/unrealize exists only for DeviceState objects
and their children.)

ASCII-art state diagram:

  [start] --instance_init-> [inited] --realize-> [realized]
     ^                       |   ^                     |
     \---instance_finalize---/   \-----unrealize-------/

In practice the only sequences we really care about are:
 instance_init; realize; unrealize; instance_finalize
   (a full object creation-use-destruction cycle;
    even if realize fails, unrealize will be called)
 instance_init; realize
   (a subset of the above: for non-hot-pluggable devices
    we will never try to unrealize them, so this is
    as far as it goes for most devices unless they
    returned an error from their realize function)
 instance_init; instance_finalize
   (the monitor does this for introspection of an object
    without actually wanting to create and use it; it's
    also the basic lifecycle for non-DeviceState objects)

The difference between hot-pluggable and not is just
whether it's valid to try to unrealize the device.

We should definitely be clearer about what belongs in
instance_init vs what belongs in realize. But where we
have both a "do thing" and a "clean up that thing" task,
we should put the cleanup code in the operation that is
the pair of the operation we put the "do thing" code in
(i.e. do thing in instance_init, clean it up in finalize;
or do thing in realize, clean it up in unrealize).

thanks
-- PMM

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/17/20 3:06 PM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 2/17/20 2:25 PM, Peter Maydell wrote:
> 
>>> So we now call timer_new in realize, and timer_del in unrealize,
>>> but timer_free in finalize. This seems unbalanced -- why not
>>> call timer_free in unrealize too, if we're moving things?
>>>
>>> Also, this is a case of code that's actually doing things right:
>>> we free the memory that we allocated in init in finalize. So
>>> we're not fixing any leak here, we're just moving code around
>>> (which is reasonable if we're trying to standardize on "call
>>> timer_new in realize, not init", but should be noted in the
>>> commit message).
>>
>> While I understand your point, I am confused because the documentation
>> on unrealize() and finalize() is rather scarce (and not obvious for
>> non-native english speaker). I think I'm not understanding QOM instance
>> lifetime well (in particular hotplug devices) so I will let this series go.
> 
> Yes, the documentation is really not good at all. The
> basic structure as I understand it is that we have two-part
> creation and two-part destruction:
>   * instance_init is creation part 1
>   * realize is creation part 2
>   * unrealize is destruction part 1 and is the opposite of realize
>   * instance_finalize is destruction part 2 and is the
>     opposite of instance_init
> 
> (Base QOM objects only have instance_init/instance_finalize;
> realize/unrealize exists only for DeviceState objects
> and their children.)
> 
> ASCII-art state diagram:
> 
>    [start] --instance_init-> [inited] --realize-> [realized]
>       ^                       |   ^                     |
>       \---instance_finalize---/   \-----unrealize-------/
> 
> In practice the only sequences we really care about are:
>   instance_init; realize; unrealize; instance_finalize
>     (a full object creation-use-destruction cycle;
>      even if realize fails, unrealize will be called)
>   instance_init; realize
>     (a subset of the above: for non-hot-pluggable devices
>      we will never try to unrealize them, so this is
>      as far as it goes for most devices unless they
>      returned an error from their realize function)

Per this comment in qdev.c, unrealize() is the expected default:

     /* by default all devices were considered as hotpluggable,
      * so with intent to check it in generic qdev_unplug() /
      * device_set_realized() functions make every device
      * hotpluggable. Devices that shouldn't be hotpluggable,
      * should override it in their class_init()
      */
     dc->hotpluggable = true;

>   instance_init; instance_finalize
>     (the monitor does this for introspection of an object
>      without actually wanting to create and use it; it's
>      also the basic lifecycle for non-DeviceState objects)
> 
> The difference between hot-pluggable and not is just
> whether it's valid to try to unrealize the device.
> 
> We should definitely be clearer about what belongs in
> instance_init vs what belongs in realize. But where we
> have both a "do thing" and a "clean up that thing" task,
> we should put the cleanup code in the operation that is
> the pair of the operation we put the "do thing" code in
> (i.e. do thing in instance_init, clean it up in finalize;
> or do thing in realize, clean it up in unrealize).

With the following code snippet:

-- >8 --
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3937d1eb1a..00d1e5c0e5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -859,6 +859,16 @@ static void device_set_realized(Object *obj, bool 
value, Error **errp)
      bool unattached_parent = false;
      static int unattached_count;

+    if (!dc->hotpluggable && dc->unrealize) {
+        fprintf(stderr,
+                "type '%s' is not hotpluggable and implements 
unrealize()\n",
+                object_get_typename(obj));
+    }
+    if (dc->hotpluggable && !dc->unrealize) {
+        fprintf(stderr, "type '%s' is hotpluggable and misses 
unrealize()\n",
+                object_get_typename(obj));
+    }
+
      if (dev->hotplugged && !dc->hotpluggable) {
          error_setg(errp, QERR_DEVICE_NO_HOTPLUG, 
object_get_typename(obj));
          return;
diff --git a/qom/object.c b/qom/object.c
index 555c8b9d07..2d8d166cba 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -347,6 +347,16 @@ static void type_initialize(TypeImpl *ti)

              type_initialize_interface(ti, t, t);
          }
+
+        if (!type_is_ancestor(ti, type_get_by_name(TYPE_DEVICE))
+                && !type_is_ancestor(ti, type_get_by_name(TYPE_INTERFACE))
+                && !ti->instance_finalize
+                && !parent->instance_finalize
+                && !parent->abstract) {
+            fprintf(stderr,
+                    "type '%s' misses instance_finalize() [parent '%s']\n",
+                    ti->name, parent->name);
+        }
      } else {
          ti->class->properties = g_hash_table_new_full(
              g_str_hash, g_str_equal, NULL, object_property_free);

---

I get for qemu-system-aarch64:


- qdev objects missing instance_finalize():

     bcm2835-sdhost-bus
     PCIE
     pxa2xx-mmci-bus
     qtest-accel
     sdhci-bus
     tcg-accel

- non-hotpluggable devices implementing unrealize():

     VGA

- hotpluggable devices missing unrealize()

     a15mpcore_priv
     a9mpcore_priv
     a9-scu
     acpi-ged
     ads7846
     aer915
     allwinner-a10
     allwinner-a10-pic
     allwinner-A10-timer
     allwinner-ahci
     allwinner-emac
     arm11mpcore_priv
     arm11-scu
     ARM,bitband-memory
     arm.cortex-a9-global-timer
     arm_gic
     arm-gicv2m
     arm-gicv3
     arm_mptimer
     arm-smmuv3
     armsse-cpuid
     armsse-mhu
     armv7m
     armv7m_nvic
     armv7m_systick
     aspeed.fmc-ast2400
     aspeed.fmc-ast2500
     aspeed.fmc-ast2600
     aspeed.gpio-ast2400
     aspeed.gpio-ast2500
     aspeed.gpio-ast2600-1_8v
     aspeed.gpio-ast2600
     aspeed.i2c-ast2400
     aspeed.i2c-ast2500
     aspeed.i2c-ast2600
     aspeed-mmi
     aspeed.rtc
     aspeed.scu-ast2400
     aspeed.scu-ast2500
     aspeed.scu-ast2600
     aspeed.sdhci
     aspeed.sdmc-ast2400
     aspeed.sdmc-ast2500
     aspeed.sdmc-ast2600
     aspeed.spi1-ast2400
     aspeed.spi1-ast2500
     aspeed.spi1-ast2600
     aspeed.spi2-ast2500
     aspeed.spi2-ast2600
     aspeed.timer-ast2400
     aspeed.timer-ast2500
     aspeed.timer-ast2600
     aspeed.vic
     aspeed.wdt-ast2400
     aspeed.wdt-ast2500
     aspeed.wdt-ast2600
     aspeed.xdma
     ast2400-a1
     ast2500-a1
     ast2600-a0
     bcm2835-aux
     bcm2835-dma
     bcm2835-fb
     bcm2835_gpio
     bcm2835-ic
     bcm2835-mbox
     bcm2835-peripherals
     bcm2835-property
     bcm2835-rng
     bcm2835-sdhost
     bcm2835-sys-timer
     bcm2835-thermal
     bcm2836-control
     bcm2836
     bcm2837
     cadence_gem
     cadence_ttc
     cadence_uart
     cfi.pflash01
     cmsdk-apb-dualtimer
     cmsdk-apb-timer
     cmsdk-apb-uart
     cmsdk-apb-watchdog
     corgi-ssp
     cpu-cluster
     designware-pcie-host
     digic
     digic-timer
     digic-uart
     dpcd
     ds1338
     exynos4210.clk
     exynos4210.combiner
     exynos4210-ehci-usb
     exynos4210.fimd
     exynos4210.gic
     exynos4210.i2c
     exynos4210.irq_gate
     exynos4210
     exynos4210.mct
     exynos4210.pmu
     exynos4210.pwm
     exynos4210.rng
     exynos4210.rtc
     exynos4210.uart
     fsl,imx25
     fsl,imx31
     fsl,imx6
     fsl,imx6ul
     fsl,imx7
     ftgmac100
     fw_cfg_mem
     gpex-pcihost
     gpio_i2c
     gpio-key
     highbank-regs
     i2c-ddc
     icp-ctrl-regs
     imx25.ccm
     imx25.gpt
     imx2.wdt
     imx31.ccm
     imx31.gpt
     imx6.ccm
     imx6.gpt
     imx6.src
     imx6ul.ccm
     imx7.analog
     imx7.ccm
     imx7.gpr
     imx7.gpt
     imx7.snvs
     imx.avic
     imx.enet
     imx.epit
     imx.fec
     imx-gpcv2
     imx.gpio
     imx.i2c
     imx.rngc
     imx.serial
     imx.spi
     integrator_core
     integrator_debug
     integrator_pic
     integrator_pit
     iotkit
     iotkit-secctl
     iotkit-sysctl
     iotkit-sysinfo
     l2x0
     lan9118
     lm8323
     luminary-watchdog
     mainstone-fpga
     max1111
     max7310
     microbit.i2c
     mps2-fpgaio
     mps2-scc
     msf2-soc
     msf2-sysreg
     mss-spi
     mss-timer
     musicpal_gpio
     musicpal_key
     musicpal_lcd
     musicpal-misc
     mv88w8618_audio
     mv88w8618_eth
     mv88w8618_flashcfg
     mv88w8618_pic
     mv88w8618_pit
     mv88w8618_wlan
     mx25l25635e
     mx66l1g45g
     mx66u51235f
     n25q128
     n25q256a
     n25q512a11
     nand
     nrf51_soc.gpio
     nrf51-soc
     nrf51_soc.nvm
     nrf51_soc.rng
     nrf51_soc.timer
     nrf51_soc.uart
     omap2-gpio
     omap2-intc
     omap-gpio
     omap_i2c
     omap-intc
     onenand
     or-irq
     pca9552
     pl011
     pl011_luminary
     pl022
     pl031
     pl041
     pl050_keyboard
     pl050_mouse
     pl061
     pl061_luminary
     pl080
     pl081
     pl110
     pl110_versatile
     pl111
     pl181
     pl190
     pl330
     platform-bus-device
     platform-ehci-usb
     pxa25x-timer
     pxa27x-timer
     pxa2xx-dma
     pxa2xx-gpio
     pxa2xx_i2c
     pxa2xx-i2c-slave
     pxa2xx-pcmcia
     pxa2xx_pic
     pxa2xx_rtc
     pxa2xx-ssp
     realview_gic
     realview_mpcore
     realview_pci
     realview_sysctl
     s25sl12801
     scoop
     sd-card
     serial-mm
     sii9022
     sl-nand
     smbus-eeprom
     smc91c111
     sp804
     spitz-keyboard
     spitz-lcdtg
     split-irq
     ssd0303
     ssd0323
     sse-200
     ssi-sd
     sst25vf016b
     sst25wf080
     stellaris-adc
     stellaris_enet
     stellaris-gptm
     stellaris-i2c
     stm32f205-soc
     stm32f2xx-adc
     stm32f2xx-spi
     stm32f2xx-syscfg
     stm32f2xx-timer
     stm32f2xx-usart
     stm32f405-soc
     stm32f4xx-exti
     stm32f4xx-syscfg
     strongarm-gpio
     strongarm_pic
     strongarm-ppc
     strongarm-rtc
     strongarm-ssp
     strongarm-uart
     sysbus-ahci
     sysbus-ohci
     tmp105
     tmp423
     tosa_dac
     tosa-ssp
     twl92230
     tz-mpc
     tz-msc
     tz-ppc
     unimplemented-device
     usb-chipidea
     versatile_i2c
     versatilepb_sic
     versatile_pci
     virtio-mmio
     w25q256
     w25q512jv
     wm8750
     xgmac
     xilinx,zynq_slcr
     xlnx.dpdma
     xlnx.ps7-dev-cfg
     xlnx.ps7-qspi
     xlnx.ps7-spi
     xlnx,ps7-usb
     xlnx.usmp-gqspi
     xlnx.v-dp
     xlnx-versal
     xlnx.zdma
     xlnx-zynmp.rtc
     xlnx.zynqmp_ipi
     xlnx,zynqmp
     xlnx,zynq-xadc
     zipit-lcd

Most of these are sysbus devices. The list is big, I probably have 
something wrong.

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Per this comment in qdev.c, unrealize() is the expected default:
>
>      /* by default all devices were considered as hotpluggable,
>       * so with intent to check it in generic qdev_unplug() /
>       * device_set_realized() functions make every device
>       * hotpluggable. Devices that shouldn't be hotpluggable,
>       * should override it in their class_init()
>       */
>      dc->hotpluggable = true;

This comment sounds like it's documenting what was the
previous default ("were considered") and making a minimal
change to avoid breaking existing code where a device
does want to be hotpluggable but isn't explicitly saying so.
I forget how exactly it works (the mechanism has changed
several times) but in practice a sysbus device is generally
not hotpluggable, and that's what most devices are.

> I get for qemu-system-aarch64:
>
>
> - qdev objects missing instance_finalize():
>
>      bcm2835-sdhost-bus
>      PCIE
>      pxa2xx-mmci-bus
>      qtest-accel
>      sdhci-bus
>      tcg-accel

Note that you don't need an instance_finalize() if it
would have nothing to do, which may be the case here.

> - non-hotpluggable devices implementing unrealize():
>
>      VGA

Not sure which device this is, I couldn't find a TYPE_
define with this name. Is it an abstract or common
device type used by the hotpluggable pci VGA card?

> - hotpluggable devices missing unrealize()
>
>      a15mpcore_priv
>      a9mpcore_priv

Most of these are not really hotpluggable. What is
confusing your test code is that sysbus devices get
the default 'hotpluggable = true' setting, but other
conditions usually prevent hotplugging. (The reason
hotpluggable is true is the default is because of
handling of 'dynamic sysbus' devices which live on
the 'platform bus'.) In particular, I think that
anything with !dc->user_creatable can't be hotplugged
unless board code specifically tries it, which would
be a bug for most of these devices.

Also, if there isn't anything for a device's unrealize
method to do, it doesn't need to provide one.

thanks
-- PMM

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/17/20 5:32 PM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Per this comment in qdev.c, unrealize() is the expected default:
>>
>>       /* by default all devices were considered as hotpluggable,
>>        * so with intent to check it in generic qdev_unplug() /
>>        * device_set_realized() functions make every device
>>        * hotpluggable. Devices that shouldn't be hotpluggable,
>>        * should override it in their class_init()
>>        */
>>       dc->hotpluggable = true;
> 
> This comment sounds like it's documenting what was the
> previous default ("were considered") and making a minimal
> change to avoid breaking existing code where a device
> does want to be hotpluggable but isn't explicitly saying so.
> I forget how exactly it works (the mechanism has changed
> several times) but in practice a sysbus device is generally
> not hotpluggable, and that's what most devices are.
> 
>> I get for qemu-system-aarch64:
>>
>>
>> - qdev objects missing instance_finalize():
>>
>>       bcm2835-sdhost-bus
>>       PCIE
>>       pxa2xx-mmci-bus
>>       qtest-accel
>>       sdhci-bus
>>       tcg-accel
> 
> Note that you don't need an instance_finalize() if it
> would have nothing to do, which may be the case here.
> 
>> - non-hotpluggable devices implementing unrealize():
>>
>>       VGA
> 
> Not sure which device this is, I couldn't find a TYPE_
> define with this name. Is it an abstract or common
> device type used by the hotpluggable pci VGA card?

This is TYPE_PCI_VGA (hw/display/vga-pci.c).

> 
>> - hotpluggable devices missing unrealize()
>>
>>       a15mpcore_priv
>>       a9mpcore_priv
> 
> Most of these are not really hotpluggable. What is
> confusing your test code is that sysbus devices get
> the default 'hotpluggable = true' setting, but other
> conditions usually prevent hotplugging. (The reason
> hotpluggable is true is the default is because of
> handling of 'dynamic sysbus' devices which live on
> the 'platform bus'.) In particular, I think that
> anything with !dc->user_creatable can't be hotplugged
> unless board code specifically tries it, which would
> be a bug for most of these devices.

OK, checking '!dc->user_creatable' removes:

     ads7846
     aer915
     corgi-ssp
     dpcd
     ds1338
     i2c-ddc
     lm8323
     max1111
     max7310
     mx25l25635e
     mx66l1g45g
     mx66u51235f
     n25q128
     n25q256a
     n25q512a11
     nand
     pca9552
     pxa2xx-i2c-slave
     s25sl12801
     sd-card
     sii9022
     spitz-lcdtg
     ssd0303
     ssd0323
     sst25vf016b
     sst25wf080
     tmp105
     tmp423
     tosa_dac
     tosa-ssp
     twl92230
     w25q256
     w25q512jv
     wm8750
     zipit-lcd

Previous list only reduced from 300 to 265.

I noticed this function, maybe I need to check parent_bus too:

static bool device_get_hotpluggable(Object *obj, Error **errp)
{
     DeviceClass *dc = DEVICE_GET_CLASS(obj);
     DeviceState *dev = DEVICE(obj);

     return dc->hotpluggable && (dev->parent_bus == NULL ||
                                 qbus_is_hotpluggable(dev->parent_bus));
}

> 
> Also, if there isn't anything for a device's unrealize
> method to do, it doesn't need to provide one.

This point is hard to check with simply with qtest.

Thanks for your comments, it helped sorting few things out.

> 
> thanks
> -- PMM
> 

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 17:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> OK, checking '!dc->user_creatable' removes:
[...]
> Previous list only reduced from 300 to 265.

You could look at what condition we check for that causes this:

$ ./build/x86/arm-softmmu/qemu-system-arm -M virt -device a15mpcore_priv
qemu-system-arm: -device a15mpcore_priv: Parameter 'driver' expects
pluggable device type

which will probably let you rule out some more devices.

thanks
-- PMM

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/17/20 6:12 PM, Philippe Mathieu-Daudé wrote:
> On 2/17/20 5:32 PM, Peter Maydell wrote:
>> On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé 
[...]
>>> - hotpluggable devices missing unrealize()
>>>
>>>       a15mpcore_priv
>>>       a9mpcore_priv
>>
>> Most of these are not really hotpluggable. What is
>> confusing your test code is that sysbus devices get
>> the default 'hotpluggable = true' setting, but other
>> conditions usually prevent hotplugging. (The reason
>> hotpluggable is true is the default is because of
>> handling of 'dynamic sysbus' devices which live on
>> the 'platform bus'.) In particular, I think that
>> anything with !dc->user_creatable can't be hotplugged
>> unless board code specifically tries it, which would
>> be a bug for most of these devices.
[...]
>> Also, if there isn't anything for a device's unrealize
>> method to do, it doesn't need to provide one.
> 
> This point is hard to check with simply with qtest.
> 
> Thanks for your comments, it helped sorting few things out.

Quick check with TYPE_BITBAND which is a SysBus device, we have:

static void bitband_realize(DeviceState *dev, Error **errp)
{
     BitBandState *s = BITBAND(dev);

     if (!s->source_memory) {
         error_setg(errp, "source-memory property not set");
         return;
     }

     address_space_init(&s->source_as, s->source_memory, "bitband-source");
}

Do we need the equivalent:

static void bitband_unrealize(DeviceState *dev, Error **errp)
{
     BitBandState *s = BITBAND(dev);

     address_space_destroy(&s->source_as);
}

Or instead mark the device user_creatable=false because of the link to a 
TYPE_MEMORY_REGION?

> 
>>
>> thanks
>> -- PMM
>>

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 17:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Quick check with TYPE_BITBAND which is a SysBus device, we have:
>
> static void bitband_realize(DeviceState *dev, Error **errp)
> {
>      BitBandState *s = BITBAND(dev);
>
>      if (!s->source_memory) {
>          error_setg(errp, "source-memory property not set");
>          return;
>      }
>
>      address_space_init(&s->source_as, s->source_memory, "bitband-source");
> }
>
> Do we need the equivalent:
>
> static void bitband_unrealize(DeviceState *dev, Error **errp)
> {
>      BitBandState *s = BITBAND(dev);
>
>      address_space_destroy(&s->source_as);
> }
>
> Or instead mark the device user_creatable=false because of the link to a
> TYPE_MEMORY_REGION?

I don't believe that this device is user-creatable. The
base class sysbus_device_class_init() sets user_creatable
to false by default for all sysbus devices, and a sysbus
device which wants to opt into being user-created has to
set it to true.

Also the device's type name string is "ARM,bitband-memory"
and the -device option at least does not like the comma
in the middle of the name, so I don't know how you'd
create it on the command line even if it wasn't marked
not user-creatable.

thanks
-- PMM

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 17 Feb 2020 at 13:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 2/17/20 2:25 PM, Peter Maydell wrote:
>
>> > So we now call timer_new in realize, and timer_del in unrealize,
>> > but timer_free in finalize. This seems unbalanced -- why not
>> > call timer_free in unrealize too, if we're moving things?
>> >
>> > Also, this is a case of code that's actually doing things right:
>> > we free the memory that we allocated in init in finalize. So
>> > we're not fixing any leak here, we're just moving code around
>> > (which is reasonable if we're trying to standardize on "call
>> > timer_new in realize, not init", but should be noted in the
>> > commit message).
>>
>> While I understand your point, I am confused because the documentation
>> on unrealize() and finalize() is rather scarce (and not obvious for
>> non-native english speaker). I think I'm not understanding QOM instance
>> lifetime well (in particular hotplug devices) so I will let this series go.
>
> Yes, the documentation is really not good at all. The
> basic structure as I understand it is that we have two-part
> creation and two-part destruction:
>  * instance_init is creation part 1
>  * realize is creation part 2
>  * unrealize is destruction part 1 and is the opposite of realize
>  * instance_finalize is destruction part 2 and is the
>    opposite of instance_init
>
> (Base QOM objects only have instance_init/instance_finalize;
> realize/unrealize exists only for DeviceState objects
> and their children.)

The split exists so you can set property values between instance_init()
and realize().  It's how qdev has always worked.  It permits setting
properties one by one even when this results in intermediate states
where invariants involving multiple property values are violated: delay
checking them until realize(), rely on them only while the device is
realized.

Note that both realize() and unrealize() can fail.  instance_init() and
instance_finalize() can't.

> ASCII-art state diagram:
>
>   [start] --instance_init-> [inited] --realize-> [realized]
>      ^                       |   ^                     |
>      \---instance_finalize---/   \-----unrealize-------/
>
> In practice the only sequences we really care about are:
>  instance_init; realize; unrealize; instance_finalize
>    (a full object creation-use-destruction cycle;
>     even if realize fails, unrealize will be called)
>  instance_init; realize
>    (a subset of the above: for non-hot-pluggable devices
>     we will never try to unrealize them, so this is
>     as far as it goes for most devices unless they
>     returned an error from their realize function)
>  instance_init; instance_finalize
>    (the monitor does this for introspection of an object
>     without actually wanting to create and use it; it's
>     also the basic lifecycle for non-DeviceState objects)

In theory, you can realize + unrealize multiple times.  It might even
work in practice sometimes.

> The difference between hot-pluggable and not is just
> whether it's valid to try to unrealize the device.
>
> We should definitely be clearer about what belongs in
> instance_init vs what belongs in realize. But where we
> have both a "do thing" and a "clean up that thing" task,
> we should put the cleanup code in the operation that is
> the pair of the operation we put the "do thing" code in
> (i.e. do thing in instance_init, clean it up in finalize;
> or do thing in realize, clean it up in unrealize).

Not doing so risks introspection leaks or double-frees.

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 17 Feb 2020 at 16:15, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Per this comment in qdev.c, unrealize() is the expected default:
>>
>>      /* by default all devices were considered as hotpluggable,
>>       * so with intent to check it in generic qdev_unplug() /
>>       * device_set_realized() functions make every device
>>       * hotpluggable. Devices that shouldn't be hotpluggable,
>>       * should override it in their class_init()
>>       */
>>      dc->hotpluggable = true;

Please note that "hot-pluggable" is *not* required for "need unrealize()
to work", at least in theory.  Cold plug exists, and cold unplug could
exist.

> This comment sounds like it's documenting what was the
> previous default ("were considered") and making a minimal
> change to avoid breaking existing code where a device
> does want to be hotpluggable but isn't explicitly saying so.

Commit 1a37eca107, six years ago:

    qdev: add "hotpluggable" property to Device
    
    Currently it's possible to make PCIDevice not hotpluggable
    by using no_hotplug field of PCIDeviceClass. However it
    limits this only to PCI devices and prevents from
    generalizing hotplug code.
    
    So add similar field to DeviceClass so it could be reused
    with other Devices and would allow to replace PCI specific
    hotplug callbacks with generic implementation. Following
    patches will replace PCIDeviceClass.no_hotplug with this
    new property.
    
    In addition expose field as "hotpluggable" readonly property,
    to make it possible to read its value via QOM interface.
    
    Make DeviceClass hotpluggable by default as it was assumed
    before.

> I forget how exactly it works (the mechanism has changed
> several times) but in practice a sysbus device is generally
> not hotpluggable, and that's what most devices are.

A device's "hot-pluggability" comes into play only when both bus and
machine support hot-plug.  And before we even get there, the device
needs to be "pluggable", i.e. dc->user_creatable.

Bus types supporting hot plug include PCI, SCSI, USB, virtio-serial-bus.
Grep for qbus_set_hotplug_handler().

Re: [PATCH 1/2] hw/ipmi/bmc: Delay timer_new_ns() from init to realize to avoid memleaks

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 17 Feb 2020 at 17:20, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Quick check with TYPE_BITBAND which is a SysBus device, we have:
>>
>> static void bitband_realize(DeviceState *dev, Error **errp)
>> {
>>      BitBandState *s = BITBAND(dev);
>>
>>      if (!s->source_memory) {
>>          error_setg(errp, "source-memory property not set");
>>          return;
>>      }
>>
>>      address_space_init(&s->source_as, s->source_memory, "bitband-source");
>> }
>>
>> Do we need the equivalent:
>>
>> static void bitband_unrealize(DeviceState *dev, Error **errp)
>> {
>>      BitBandState *s = BITBAND(dev);
>>
>>      address_space_destroy(&s->source_as);
>> }
>>
>> Or instead mark the device user_creatable=false because of the link to a
>> TYPE_MEMORY_REGION?
>
> I don't believe that this device is user-creatable. The
> base class sysbus_device_class_init() sets user_creatable
> to false by default for all sysbus devices, and a sysbus
> device which wants to opt into being user-created has to
> set it to true.

As far as I can tell, you additionally have to
machine_class_allow_dynamic_sysbus_dev().  Sysbus is special.

> Also the device's type name string is "ARM,bitband-memory"
> and the -device option at least does not like the comma
> in the middle of the name, so I don't know how you'd
> create it on the command line even if it wasn't marked
> not user-creatable.

Double the comma.

If I remember correctly, the use of comma in type comes from IEEE-1275.
It's quite inappropriate for QEMU.

Re: [PATCH 2/2] hw/sd/sd: Delay timer_new_ns() from init to realize to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/17/20 2:26 PM, Peter Maydell wrote:
> On Sat, 15 Feb 2020 at 15:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> In commit f3a508eb4e the Euler Robot reported calling timer_new()
>> in instance_init() can leak heap memory. The easier fix is to
>> delay the timer creation at instance realize(). Similarly move
>> timer_del() into a new instance unrealize() method.
> 
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 71a9af09ab..d72cf3de2a 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -2058,14 +2058,12 @@ static void sd_instance_init(Object *obj)
>>      SDState *sd = SD_CARD(obj);
>>
>>      sd->enable = true;
>> -    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
>>  }
>>
>>  static void sd_instance_finalize(Object *obj)
>>  {
>>      SDState *sd = SD_CARD(obj);
>>
>> -    timer_del(sd->ocr_power_timer);
>>      timer_free(sd->ocr_power_timer);
>>  }
>>
>> @@ -2098,6 +2096,15 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>          }
>>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>>      }
>> +
>> +    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
>> +}
>> +
>> +static void sd_unrealize(DeviceState *dev, Error **errp)
>> +{
>> +    SDState *sd = SD_CARD(dev);
>> +
>> +    timer_del(sd->ocr_power_timer);
>>  }
> 
> Here too the old code was doing things correctly in that
> it does a timer_del/timer_free on the timer it allocates
> in instance_init, and the new code has weirdly split the
> freeing between unrealize and finalize.

Indeed I now see it, thanks.

> 
> thanks
> -- PMM
>