Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

["Andreas Färber" <afaerber@suse.de>]

Am 11.06.2014 18:49, schrieb Paolo Bonzini:
> From: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Add a link to rtc under /machine providing a stable
> location for management apps to query "date" field.
> 
> {"execute":"qom-get","arguments":{"path":"/machine/rtc","property":"date"} }
> 
> Suggested by Paolo Bonzini.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: mtosatti@redhat.com <mtosatti@redhat.com>

Do those management apps want a generic location for each machine or
just for PC? I know several ARM boards that have different ones on SoC
and on PMIC (I2C) for instance. In that case it may be safer to
implement an interface and to search for that? Or at least give the
"rtc" node a more generic property type for ABI stability? I.e. you only
want "date" here, not any other properties of the mc146818rtc.

> ---
>  hw/timer/mc146818rtc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..8c706e1 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -893,6 +893,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  
>      object_property_add(OBJECT(s), "date", "struct tm",
>                          rtc_get_date, NULL, NULL, s, NULL);
> +
> +    object_property_add_alias(qdev_get_machine(), "rtc",
> +                              OBJECT(s), NULL, &error_abort);

Irrespective of the error_abort discussion, realize feels too late for
setting up such a property. My suggestion would be to do it in PC
machine init instead - or if not feasible, here in instance_init with
NULL errp.

>  }
>  
>  ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
> @@ -932,11 +935,17 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
>      dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
> +static void rtc_finalize(Object *obj)
> +{
> +    object_property_del(qdev_get_machine(), "rtc", NULL);

In Peter's multiple-RTC scenario this means the first RTC unparented(?)
will remove the property, not necessarily the RTC the property points
to. Not really relevant in practice, just pointing out that it's not
just about adding.

Do we need to clean it up at all? And why not just use an
"old-fashioned" link<> property?

Regards,
Andreas

> +}
> +
>  static const TypeInfo mc146818rtc_info = {
>      .name          = TYPE_MC146818_RTC,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(RTCState),
>      .class_init    = rtc_class_initfn,
> +    .instance_finalize = rtc_finalize,
>  };
>  
>  static void mc146818rtc_register_types(void)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg