Thomas Huth <th...@redhat.com> writes:
> There is currently a funny problem with the "mc146818rtc" device:
> 1) Start QEMU like this:
> qemu-system-ppc64 -M pseries -S
> 2) At the HMP monitor, enter "info qom-tree". Note that there is an
> entry for "/rtc (spapr-rtc)".
> 3) Introspect the mc146818rtc device like this:
> device_add mc146818rtc,help
> 4) Run "info qom-tree" again. The "/rtc" entry is gone now!
>
> The rtc_finalize() function of the mc146818rtc device has two bugs: First,
> it tries to remove a "rtc" property, while the rtc_realizefn() added a
> "rtc-time" property instead. And second, it should have been done in an
> unrealize function, not in a finalize function, to avoid that this causes
> problems during introspection.
>
> But since adding aliases to the global machine state should not be done
> from a device's realize function anyway, let's rather fix this issue
> by moving the creation of the alias to the code that creates the device
> (and thus is run from the machine init functions instead). We can then
> remove the object_property_del() completely. In prep.c, the code for
> setting up the alias is added to the function which deals already with
> the rtc device for the "40p" machine. The "prep" machine is ignored
> here since it is scheduled for removal anyway.
That's okay as long as this patch goes in after the removal. And then
you don't need this sentence.
> Fixes: 654a36d857ff949e0d1989904b76f53fded9dc83
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
> hw/ppc/prep.c | 3 +++
> hw/timer/mc146818rtc.c | 12 +++---------
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570..91a8f42 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -696,6 +696,9 @@ static int prep_set_cmos_checksum(DeviceState *dev, void
> *opaque)
> rtc_set_memory(rtc, 0x3e, checksum & 0xff);
> rtc_set_memory(rtc, 0x2f, checksum >> 8);
> rtc_set_memory(rtc, 0x3f, checksum >> 8);
> +
> + object_property_add_alias(qdev_get_machine(), "rtc-time",
> OBJECT(rtc),
> + "date", NULL);
> }
> return 0;
> }
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 3a14075..a504f03 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -995,9 +995,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>
> object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);
>
> - object_property_add_alias(qdev_get_machine(), "rtc-time",
> - OBJECT(s), "date", NULL);
> -
> qdev_init_gpio_out(dev, &s->irq, 1);
> }
>
> @@ -1019,6 +1016,9 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int
> base_year, qemu_irq intercept_irq)
> }
> QLIST_INSERT_HEAD(&rtc_devices, s, link);
>
> + object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(s),
> + "date", NULL);
> +
> return isadev;
> }
>
> @@ -1052,17 +1052,11 @@ static void rtc_class_initfn(ObjectClass *klass, void
> *data)
> dc->user_creatable = false;
> }
>
> -static void rtc_finalize(Object *obj)
> -{
> - object_property_del(qdev_get_machine(), "rtc", NULL);
> -}
> -
> 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)
Let's see... the device is created in two places, mc146818_rtc_init()
and i82378_realize(). You add the alias in the former, but not the
latter. Instead, you add it where the i82378 device is created: in
ibm_40p_init(). ppc_prep_init() also creates it, but you intentionally
ignore that one. Net effect: you add the alias a little later. Looks
good to me.
Preferably with the superfluous sentence deleted from the commit
message:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
