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.

Fixes: 654a36d857ff949e0d1989904b76f53fded9dc83
Signed-off-by: Thomas Huth <thuth@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)
-- 
1.8.3.1

Thomas Huth <thuth@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 <thuth@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 <armbru@redhat.com>