[Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Cédric Le Goater]

Also use an 'Object *' under the sPAPR machine to hold the RTC
object. Overall, these changes remove an unnecessary and implicit
dependency on SysBus.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c         |   12 +++++++-----
 hw/ppc/spapr_rtc.c     |   10 +++++-----
 include/hw/ppc/spapr.h |    6 +++---
 include/hw/ppc/xics.h  |    2 +-
 4 files changed, 16 insertions(+), 14 deletions(-)

Index: qemu-dgibson-for-2.9.git/include/hw/ppc/xics.h
===================================================================
--- qemu-dgibson-for-2.9.git.orig/include/hw/ppc/xics.h
+++ qemu-dgibson-for-2.9.git/include/hw/ppc/xics.h
@@ -28,7 +28,7 @@
 #ifndef XICS_H
 #define XICS_H
 
-#include "hw/sysbus.h"
+#include "hw/qdev.h"
 
 #define XICS_IPI        0x2
 #define XICS_BUID       0x1
Index: qemu-dgibson-for-2.9.git/hw/ppc/spapr_rtc.c
===================================================================
--- qemu-dgibson-for-2.9.git.orig/hw/ppc/spapr_rtc.c
+++ qemu-dgibson-for-2.9.git/hw/ppc/spapr_rtc.c
@@ -39,11 +39,11 @@
 typedef struct sPAPRRTCState sPAPRRTCState;
 struct sPAPRRTCState {
     /*< private >*/
-    SysBusDevice parent_obj;
+    DeviceState parent_obj;
     int64_t ns_offset;
 };
 
-void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
+void spapr_rtc_read(Object *dev, struct tm *tm, uint32_t *ns)
 {
     sPAPRRTCState *rtc = SPAPR_RTC(dev);
     int64_t host_ns = qemu_clock_get_ns(rtc_clock);
@@ -63,7 +63,7 @@ void spapr_rtc_read(DeviceState *dev, st
     }
 }
 
-int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset)
+int spapr_rtc_import_offset(Object *dev, int64_t legacy_offset)
 {
     sPAPRRTCState *rtc;
 
@@ -155,7 +155,7 @@ static void rtas_set_time_of_day(PowerPC
 
 static void spapr_rtc_qom_date(Object *obj, struct tm *current_tm, Error **errp)
 {
-    spapr_rtc_read(DEVICE(obj), current_tm, NULL);
+    spapr_rtc_read(obj, current_tm, NULL);
 }
 
 static void spapr_rtc_realize(DeviceState *dev, Error **errp)
@@ -200,7 +200,7 @@ static void spapr_rtc_class_init(ObjectC
 
 static const TypeInfo spapr_rtc_info = {
     .name          = TYPE_SPAPR_RTC,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(sPAPRRTCState),
     .class_init    = spapr_rtc_class_init,
 };
Index: qemu-dgibson-for-2.9.git/hw/ppc/spapr.c
===================================================================
--- qemu-dgibson-for-2.9.git.orig/hw/ppc/spapr.c
+++ qemu-dgibson-for-2.9.git/hw/ppc/spapr.c
@@ -1333,13 +1333,15 @@ static void spapr_create_nvram(sPAPRMach
 
 static void spapr_rtc_create(sPAPRMachineState *spapr)
 {
-    DeviceState *dev = qdev_create(NULL, TYPE_SPAPR_RTC);
+    Object *obj = object_new(TYPE_SPAPR_RTC);
 
-    qdev_init_nofail(dev);
-    spapr->rtc = dev;
+    object_property_add_child(OBJECT(spapr), "rtc", obj, &error_fatal);
+    object_property_set_bool(obj, true, "realized", &error_fatal);
 
-    object_property_add_alias(qdev_get_machine(), "rtc-time",
-                              OBJECT(spapr->rtc), "date", NULL);
+    spapr->rtc = obj;
+
+    object_property_add_alias(OBJECT(spapr), "rtc-time", spapr->rtc, "date",
+                               &error_fatal);
 }
 
 /* Returns whether we want to use VGA or not */
Index: qemu-dgibson-for-2.9.git/include/hw/ppc/spapr.h
===================================================================
--- qemu-dgibson-for-2.9.git.orig/include/hw/ppc/spapr.h
+++ qemu-dgibson-for-2.9.git/include/hw/ppc/spapr.h
@@ -59,7 +59,7 @@ struct sPAPRMachineState {
     QLIST_HEAD(, sPAPRPHBState) phbs;
     struct sPAPRNVRAM *nvram;
     ICSState *ics;
-    DeviceState *rtc;
+    Object *rtc;
 
     void *htab;
     uint32_t htab_shift;
@@ -633,8 +633,8 @@ void spapr_ccs_reset_hook(void *opaque);
 #define TYPE_SPAPR_RTC "spapr-rtc"
 #define TYPE_SPAPR_RNG "spapr-rng"
 
-void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
-int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
+void spapr_rtc_read(Object *obj, struct tm *tm, uint32_t *ns);
+int spapr_rtc_import_offset(Object *obj, int64_t legacy_offset);
 
 int spapr_rng_populate_dt(void *fdt);
 

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Thomas Huth]

On 03.03.2017 14:37, Cédric Le Goater wrote:
> Also use an 'Object *' under the sPAPR machine to hold the RTC
> object.

The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
idea! But what's the advantage of using Object* instead of DeviceState*
in sPAPRMachineState ?

 Thomas

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Cédric Le Goater]

On 03/03/2017 03:13 PM, Thomas Huth wrote:
> On 03.03.2017 14:37, Cédric Le Goater wrote:
>> Also use an 'Object *' under the sPAPR machine to hold the RTC
>> object.
> 
> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
> idea! But what's the advantage of using Object* instead of DeviceState*
> in sPAPRMachineState ?

it makes spapr_rtc_create() a little simpler.  

We could go even further and use a sPAPRRTCState under sPAPRMachineState 
that we  would initialize with object_initialize(). 

C.

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Thomas Huth]

On 03.03.2017 15:49, Cédric Le Goater wrote:
> On 03/03/2017 03:13 PM, Thomas Huth wrote:
>> On 03.03.2017 14:37, Cédric Le Goater wrote:
>>> Also use an 'Object *' under the sPAPR machine to hold the RTC
>>> object.
>>
>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
>> idea! But what's the advantage of using Object* instead of DeviceState*
>> in sPAPRMachineState ?
> 
> it makes spapr_rtc_create() a little simpler.  
> 
> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
> that we  would initialize with object_initialize(). 

I think a sPAPRRTCState* would make more sense here - if you just see an
Object* and are not familiar with the code, you wonder what this pointer
is all about (and you then have to cast it to something different if you
want to do anything with it) ... so IMHO either a DeviceState* or
sPAPRRTCState* is the better choice here.

 Thomas

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Cédric Le Goater]

On 03/03/2017 05:54 PM, Thomas Huth wrote:
> On 03.03.2017 15:49, Cédric Le Goater wrote:
>> On 03/03/2017 03:13 PM, Thomas Huth wrote:
>>> On 03.03.2017 14:37, Cédric Le Goater wrote:
>>>> Also use an 'Object *' under the sPAPR machine to hold the RTC
>>>> object.
>>>
>>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
>>> idea! But what's the advantage of using Object* instead of DeviceState*
>>> in sPAPRMachineState ?
>>
>> it makes spapr_rtc_create() a little simpler.  
>>
>> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
>> that we  would initialize with object_initialize(). 
> 
> I think a sPAPRRTCState* would make more sense here - if you just see an
> Object* and are not familiar with the code, you wonder what this pointer
> is all about (and you then have to cast it to something different if you
> want to do anything with it) ... so IMHO either a DeviceState* or
> sPAPRRTCState* is the better choice here.

I think having a non-pointer is a better for the object lifecycle 
but I don't have a strong opinion on that. We will see what Dave 
prefers.

Thanks,

C. 

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Fri, Mar 03, 2017 at 06:23:15PM +0100, Cédric Le Goater wrote:
> On 03/03/2017 05:54 PM, Thomas Huth wrote:
> > On 03.03.2017 15:49, Cédric Le Goater wrote:
> >> On 03/03/2017 03:13 PM, Thomas Huth wrote:
> >>> On 03.03.2017 14:37, Cédric Le Goater wrote:
> >>>> Also use an 'Object *' under the sPAPR machine to hold the RTC
> >>>> object.
> >>>
> >>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
> >>> idea! But what's the advantage of using Object* instead of DeviceState*
> >>> in sPAPRMachineState ?
> >>
> >> it makes spapr_rtc_create() a little simpler.  
> >>
> >> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
> >> that we  would initialize with object_initialize(). 
> > 
> > I think a sPAPRRTCState* would make more sense here - if you just see an
> > Object* and are not familiar with the code, you wonder what this pointer
> > is all about (and you then have to cast it to something different if you
> > want to do anything with it) ... so IMHO either a DeviceState* or
> > sPAPRRTCState* is the better choice here.
> 
> I think having a non-pointer is a better for the object lifecycle 
> but I don't have a strong opinion on that. We will see what Dave 
> prefers.

You seem to be talking at cross purposes.  AFAICT Thomas is not
talking about the difference between a pointer and a non-pointer, he's
talking about the difference between a generic Object (pointer or
otherwise) and a concrete type.

I agree with Thomas that a specific type is preferable.  I'm not
particularly bothered by the difference between pointer and
non-pointer.

In any case, this has missed the cutoff for qemu-2.9, I'm afraid.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[Cédric Le Goater]

On 03/06/2017 12:23 AM, David Gibson wrote:
> On Fri, Mar 03, 2017 at 06:23:15PM +0100, Cédric Le Goater wrote:
>> On 03/03/2017 05:54 PM, Thomas Huth wrote:
>>> On 03.03.2017 15:49, Cédric Le Goater wrote:
>>>> On 03/03/2017 03:13 PM, Thomas Huth wrote:
>>>>> On 03.03.2017 14:37, Cédric Le Goater wrote:
>>>>>> Also use an 'Object *' under the sPAPR machine to hold the RTC
>>>>>> object.
>>>>>
>>>>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
>>>>> idea! But what's the advantage of using Object* instead of DeviceState*
>>>>> in sPAPRMachineState ?
>>>>
>>>> it makes spapr_rtc_create() a little simpler.  
>>>>
>>>> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
>>>> that we  would initialize with object_initialize(). 
>>>
>>> I think a sPAPRRTCState* would make more sense here - if you just see an
>>> Object* and are not familiar with the code, you wonder what this pointer
>>> is all about (and you then have to cast it to something different if you
>>> want to do anything with it) ... so IMHO either a DeviceState* or
>>> sPAPRRTCState* is the better choice here.
>>
>> I think having a non-pointer is a better for the object lifecycle 
>> but I don't have a strong opinion on that. We will see what Dave 
>> prefers.
> 
> You seem to be talking at cross purposes.  AFAICT Thomas is not
> talking about the difference between a pointer and a non-pointer, he's
> talking about the difference between a generic Object (pointer or
> otherwise) and a concrete type.
> 
> I agree with Thomas that a specific type is preferable.  I'm not
> particularly bothered by the difference between pointer and
> non-pointer.

Using a non-pointer sPAPRRTCState introduces more changes. See below.
This is going a further than just removing the dependency on SysBus. 
To remove only the dependency on SysBus, we can keep a 'DeviceState *' 
under the machine and fix the code accordingly.  

Just tell me what you prefer. I don't mind. Although I think that 
QOM'ifying sPAPR is good thing. After RTC, there is only the NVRAM
object left to handle but I am not sure we can do much about it as 
this is a drive. 
 
> In any case, this has missed the cutoff for qemu-2.9, I'm afraid.

Well, yes. it's not fixing a bug.

Thanks,

C. 


>From b061c1616cd07d04e795cb817768b3d9833d9036 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Fri, 3 Mar 2017 14:29:40 +0100
Subject: [PATCH] ppc/spapr: QOM'ify sPAPRRTCState
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also use an 'sPAPRRTCState' attribute under the sPAPR machine to hold
the RTC object. Overall, these changes remove an unnecessary and
implicit dependency on SysBus.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr.c         | 16 ++++++++--------
 hw/ppc/spapr_events.c  |  2 +-
 hw/ppc/spapr_rtc.c     | 41 +++++++----------------------------------
 include/hw/ppc/spapr.h | 21 ++++++++++++++++-----
 include/hw/ppc/xics.h  |  2 +-
 5 files changed, 33 insertions(+), 49 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c3bb99160545..3a7868ed80da 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1333,13 +1333,13 @@ static void spapr_create_nvram(sPAPRMachineState *spapr)
 
 static void spapr_rtc_create(sPAPRMachineState *spapr)
 {
-    DeviceState *dev = qdev_create(NULL, TYPE_SPAPR_RTC);
-
-    qdev_init_nofail(dev);
-    spapr->rtc = dev;
-
-    object_property_add_alias(qdev_get_machine(), "rtc-time",
-                              OBJECT(spapr->rtc), "date", NULL);
+    object_initialize(&spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC);
+    object_property_add_child(OBJECT(spapr), "rtc", OBJECT(&spapr->rtc),
+                              &error_fatal);
+    object_property_set_bool(OBJECT(&spapr->rtc), true, "realized",
+                              &error_fatal);
+    object_property_add_alias(OBJECT(spapr), "rtc-time", OBJECT(&spapr->rtc),
+                              "date", &error_fatal);
 }
 
 /* Returns whether we want to use VGA or not */
@@ -1377,7 +1377,7 @@ static int spapr_post_load(void *opaque, int version_id)
      * So when migrating from those versions, poke the incoming offset
      * value into the RTC device */
     if (version_id < 3) {
-        err = spapr_rtc_import_offset(spapr->rtc, spapr->rtc_offset);
+        err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
     }
 
     return err;
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 24a5758e6243..f0b28d811242 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -422,7 +422,7 @@ static void spapr_init_maina(struct rtas_event_log_v6_maina *maina,
     maina->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINA);
     maina->hdr.section_length = cpu_to_be16(sizeof(*maina));
     /* FIXME: section version, subtype and creator id? */
-    spapr_rtc_read(spapr->rtc, &tm, NULL);
+    spapr_rtc_read(&spapr->rtc, &tm, NULL);
     year = tm.tm_year + 1900;
     maina->creation_date = cpu_to_be32((to_bcd(year / 100) << 24)
                                        | (to_bcd(year % 100) << 16)
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 3a17ac42e44b..00a4e4c717d5 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -33,19 +33,8 @@
 #include "qapi-event.h"
 #include "qemu/cutils.h"
 
-#define SPAPR_RTC(obj) \
-    OBJECT_CHECK(sPAPRRTCState, (obj), TYPE_SPAPR_RTC)
-
-typedef struct sPAPRRTCState sPAPRRTCState;
-struct sPAPRRTCState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    int64_t ns_offset;
-};
-
-void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
+void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns)
 {
-    sPAPRRTCState *rtc = SPAPR_RTC(dev);
     int64_t host_ns = qemu_clock_get_ns(rtc_clock);
     int64_t guest_ns;
     time_t guest_s;
@@ -63,16 +52,12 @@ void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
     }
 }
 
-int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset)
+int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset)
 {
-    sPAPRRTCState *rtc;
-
-    if (!dev) {
+    if (!rtc) {
         return -ENODEV;
     }
 
-    rtc = SPAPR_RTC(dev);
-
     rtc->ns_offset = legacy_offset * NANOSECONDS_PER_SECOND;
 
     return 0;
@@ -91,12 +76,7 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    if (!spapr->rtc) {
-        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
-        return;
-    }
-
-    spapr_rtc_read(spapr->rtc, &tm, &ns);
+    spapr_rtc_read(&spapr->rtc, &tm, &ns);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     rtas_st(rets, 1, tm.tm_year + 1900);
@@ -113,7 +93,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
 {
-    sPAPRRTCState *rtc;
+    sPAPRRTCState *rtc = &spapr->rtc;
     struct tm tm;
     time_t new_s;
     int64_t host_ns;
@@ -123,11 +103,6 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         return;
     }
 
-    if (!spapr->rtc) {
-        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
-        return;
-    }
-
     tm.tm_year = rtas_ld(args, 0) - 1900;
     tm.tm_mon = rtas_ld(args, 1) - 1;
     tm.tm_mday = rtas_ld(args, 2);
@@ -144,8 +119,6 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     /* Generate a monitor event for the change */
     qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
 
-    rtc = SPAPR_RTC(spapr->rtc);
-
     host_ns = qemu_clock_get_ns(rtc_clock);
 
     rtc->ns_offset = (new_s * NANOSECONDS_PER_SECOND) - host_ns;
@@ -155,7 +128,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
 static void spapr_rtc_qom_date(Object *obj, struct tm *current_tm, Error **errp)
 {
-    spapr_rtc_read(DEVICE(obj), current_tm, NULL);
+    spapr_rtc_read(SPAPR_RTC(obj), current_tm, NULL);
 }
 
 static void spapr_rtc_realize(DeviceState *dev, Error **errp)
@@ -200,7 +173,7 @@ static void spapr_rtc_class_init(ObjectClass *oc, void *data)
 
 static const TypeInfo spapr_rtc_info = {
     .name          = TYPE_SPAPR_RTC,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(sPAPRRTCState),
     .class_init    = spapr_rtc_class_init,
 };
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 808aac870359..ba9e689ee230 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -20,6 +20,18 @@ typedef struct sPAPREventSource sPAPREventSource;
 
 #define SPAPR_TIMEBASE_FREQ     512000000ULL
 
+#define TYPE_SPAPR_RTC "spapr-rtc"
+
+#define SPAPR_RTC(obj)                                  \
+    OBJECT_CHECK(sPAPRRTCState, (obj), TYPE_SPAPR_RTC)
+
+typedef struct sPAPRRTCState sPAPRRTCState;
+struct sPAPRRTCState {
+    /*< private >*/
+    DeviceState parent_obj;
+    int64_t ns_offset;
+};
+
 typedef struct sPAPRMachineClass sPAPRMachineClass;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"
@@ -58,7 +70,7 @@ struct sPAPRMachineState {
     QLIST_HEAD(, sPAPRPHBState) phbs;
     struct sPAPRNVRAM *nvram;
     ICSState *ics;
-    DeviceState *rtc;
+    sPAPRRTCState rtc;
 
     void *htab;
     uint32_t htab_shift;
@@ -629,11 +641,10 @@ struct sPAPRConfigureConnectorState {
 
 void spapr_ccs_reset_hook(void *opaque);
 
-#define TYPE_SPAPR_RTC "spapr-rtc"
-#define TYPE_SPAPR_RNG "spapr-rng"
+void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
+int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
 
-void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
-int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
+#define TYPE_SPAPR_RNG "spapr-rng"
 
 int spapr_rng_populate_dt(void *fdt);
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 9a5e715fe553..ce230183a106 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -28,7 +28,7 @@
 #ifndef XICS_H
 #define XICS_H
 
-#include "hw/sysbus.h"
+#include "hw/qdev.h"
 
 #define XICS_IPI        0x2
 #define XICS_BUID       0x1
-- 
2.7.4

Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 11563 bytes --]

On Mon, Mar 06, 2017 at 08:56:07AM +0100, Cédric Le Goater wrote:
> On 03/06/2017 12:23 AM, David Gibson wrote:
> > On Fri, Mar 03, 2017 at 06:23:15PM +0100, Cédric Le Goater wrote:
> >> On 03/03/2017 05:54 PM, Thomas Huth wrote:
> >>> On 03.03.2017 15:49, Cédric Le Goater wrote:
> >>>> On 03/03/2017 03:13 PM, Thomas Huth wrote:
> >>>>> On 03.03.2017 14:37, Cédric Le Goater wrote:
> >>>>>> Also use an 'Object *' under the sPAPR machine to hold the RTC
> >>>>>> object.
> >>>>>
> >>>>> The change from TYPE_SYS_BUS_DEVICE to TYPE_DEVICE is certainly a good
> >>>>> idea! But what's the advantage of using Object* instead of DeviceState*
> >>>>> in sPAPRMachineState ?
> >>>>
> >>>> it makes spapr_rtc_create() a little simpler.  
> >>>>
> >>>> We could go even further and use a sPAPRRTCState under sPAPRMachineState 
> >>>> that we  would initialize with object_initialize(). 
> >>>
> >>> I think a sPAPRRTCState* would make more sense here - if you just see an
> >>> Object* and are not familiar with the code, you wonder what this pointer
> >>> is all about (and you then have to cast it to something different if you
> >>> want to do anything with it) ... so IMHO either a DeviceState* or
> >>> sPAPRRTCState* is the better choice here.
> >>
> >> I think having a non-pointer is a better for the object lifecycle 
> >> but I don't have a strong opinion on that. We will see what Dave 
> >> prefers.
> > 
> > You seem to be talking at cross purposes.  AFAICT Thomas is not
> > talking about the difference between a pointer and a non-pointer, he's
> > talking about the difference between a generic Object (pointer or
> > otherwise) and a concrete type.
> > 
> > I agree with Thomas that a specific type is preferable.  I'm not
> > particularly bothered by the difference between pointer and
> > non-pointer.
> 
> Using a non-pointer sPAPRRTCState introduces more changes. See below.
> This is going a further than just removing the dependency on SysBus. 
> To remove only the dependency on SysBus, we can keep a 'DeviceState *' 
> under the machine and fix the code accordingly.  
> 
> Just tell me what you prefer. I don't mind. Although I think that 

Ok.  I think non-pointer is probably better.  I don't mind if we
convert directly to that, or go via some pointery intermediate step -
just do whichever is easier.

> QOM'ifying sPAPR is good thing. After RTC, there is only the NVRAM
> object left to handle but I am not sure we can do much about it as 
> this is a drive.



>  
> > In any case, this has missed the cutoff for qemu-2.9, I'm afraid.
> 
> Well, yes. it's not fixing a bug.
> 
> Thanks,
> 
> C. 
> 
> 
> >From b061c1616cd07d04e795cb817768b3d9833d9036 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
> Date: Fri, 3 Mar 2017 14:29:40 +0100
> Subject: [PATCH] ppc/spapr: QOM'ify sPAPRRTCState
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Also use an 'sPAPRRTCState' attribute under the sPAPR machine to hold
> the RTC object. Overall, these changes remove an unnecessary and
> implicit dependency on SysBus.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/spapr.c         | 16 ++++++++--------
>  hw/ppc/spapr_events.c  |  2 +-
>  hw/ppc/spapr_rtc.c     | 41 +++++++----------------------------------
>  include/hw/ppc/spapr.h | 21 ++++++++++++++++-----
>  include/hw/ppc/xics.h  |  2 +-
>  5 files changed, 33 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c3bb99160545..3a7868ed80da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1333,13 +1333,13 @@ static void spapr_create_nvram(sPAPRMachineState *spapr)
>  
>  static void spapr_rtc_create(sPAPRMachineState *spapr)
>  {
> -    DeviceState *dev = qdev_create(NULL, TYPE_SPAPR_RTC);
> -
> -    qdev_init_nofail(dev);
> -    spapr->rtc = dev;
> -
> -    object_property_add_alias(qdev_get_machine(), "rtc-time",
> -                              OBJECT(spapr->rtc), "date", NULL);
> +    object_initialize(&spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC);
> +    object_property_add_child(OBJECT(spapr), "rtc", OBJECT(&spapr->rtc),
> +                              &error_fatal);
> +    object_property_set_bool(OBJECT(&spapr->rtc), true, "realized",
> +                              &error_fatal);
> +    object_property_add_alias(OBJECT(spapr), "rtc-time", OBJECT(&spapr->rtc),
> +                              "date", &error_fatal);
>  }
>  
>  /* Returns whether we want to use VGA or not */
> @@ -1377,7 +1377,7 @@ static int spapr_post_load(void *opaque, int version_id)
>       * So when migrating from those versions, poke the incoming offset
>       * value into the RTC device */
>      if (version_id < 3) {
> -        err = spapr_rtc_import_offset(spapr->rtc, spapr->rtc_offset);
> +        err = spapr_rtc_import_offset(&spapr->rtc, spapr->rtc_offset);
>      }
>  
>      return err;
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 24a5758e6243..f0b28d811242 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -422,7 +422,7 @@ static void spapr_init_maina(struct rtas_event_log_v6_maina *maina,
>      maina->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINA);
>      maina->hdr.section_length = cpu_to_be16(sizeof(*maina));
>      /* FIXME: section version, subtype and creator id? */
> -    spapr_rtc_read(spapr->rtc, &tm, NULL);
> +    spapr_rtc_read(&spapr->rtc, &tm, NULL);
>      year = tm.tm_year + 1900;
>      maina->creation_date = cpu_to_be32((to_bcd(year / 100) << 24)
>                                         | (to_bcd(year % 100) << 16)
> diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
> index 3a17ac42e44b..00a4e4c717d5 100644
> --- a/hw/ppc/spapr_rtc.c
> +++ b/hw/ppc/spapr_rtc.c
> @@ -33,19 +33,8 @@
>  #include "qapi-event.h"
>  #include "qemu/cutils.h"
>  
> -#define SPAPR_RTC(obj) \
> -    OBJECT_CHECK(sPAPRRTCState, (obj), TYPE_SPAPR_RTC)
> -
> -typedef struct sPAPRRTCState sPAPRRTCState;
> -struct sPAPRRTCState {
> -    /*< private >*/
> -    SysBusDevice parent_obj;
> -    int64_t ns_offset;
> -};
> -
> -void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
> +void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns)
>  {
> -    sPAPRRTCState *rtc = SPAPR_RTC(dev);
>      int64_t host_ns = qemu_clock_get_ns(rtc_clock);
>      int64_t guest_ns;
>      time_t guest_s;
> @@ -63,16 +52,12 @@ void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
>      }
>  }
>  
> -int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset)
> +int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset)
>  {
> -    sPAPRRTCState *rtc;
> -
> -    if (!dev) {
> +    if (!rtc) {
>          return -ENODEV;
>      }
>  
> -    rtc = SPAPR_RTC(dev);
> -
>      rtc->ns_offset = legacy_offset * NANOSECONDS_PER_SECOND;
>  
>      return 0;
> @@ -91,12 +76,7 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    if (!spapr->rtc) {
> -        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> -        return;
> -    }
> -
> -    spapr_rtc_read(spapr->rtc, &tm, &ns);
> +    spapr_rtc_read(&spapr->rtc, &tm, &ns);
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>      rtas_st(rets, 1, tm.tm_year + 1900);
> @@ -113,7 +93,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                                   target_ulong args,
>                                   uint32_t nret, target_ulong rets)
>  {
> -    sPAPRRTCState *rtc;
> +    sPAPRRTCState *rtc = &spapr->rtc;
>      struct tm tm;
>      time_t new_s;
>      int64_t host_ns;
> @@ -123,11 +103,6 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> -    if (!spapr->rtc) {
> -        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> -        return;
> -    }
> -
>      tm.tm_year = rtas_ld(args, 0) - 1900;
>      tm.tm_mon = rtas_ld(args, 1) - 1;
>      tm.tm_mday = rtas_ld(args, 2);
> @@ -144,8 +119,6 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      /* Generate a monitor event for the change */
>      qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
>  
> -    rtc = SPAPR_RTC(spapr->rtc);
> -
>      host_ns = qemu_clock_get_ns(rtc_clock);
>  
>      rtc->ns_offset = (new_s * NANOSECONDS_PER_SECOND) - host_ns;
> @@ -155,7 +128,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  
>  static void spapr_rtc_qom_date(Object *obj, struct tm *current_tm, Error **errp)
>  {
> -    spapr_rtc_read(DEVICE(obj), current_tm, NULL);
> +    spapr_rtc_read(SPAPR_RTC(obj), current_tm, NULL);
>  }
>  
>  static void spapr_rtc_realize(DeviceState *dev, Error **errp)
> @@ -200,7 +173,7 @@ static void spapr_rtc_class_init(ObjectClass *oc, void *data)
>  
>  static const TypeInfo spapr_rtc_info = {
>      .name          = TYPE_SPAPR_RTC,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_DEVICE,
>      .instance_size = sizeof(sPAPRRTCState),
>      .class_init    = spapr_rtc_class_init,
>  };
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 808aac870359..ba9e689ee230 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -20,6 +20,18 @@ typedef struct sPAPREventSource sPAPREventSource;
>  
>  #define SPAPR_TIMEBASE_FREQ     512000000ULL
>  
> +#define TYPE_SPAPR_RTC "spapr-rtc"
> +
> +#define SPAPR_RTC(obj)                                  \
> +    OBJECT_CHECK(sPAPRRTCState, (obj), TYPE_SPAPR_RTC)
> +
> +typedef struct sPAPRRTCState sPAPRRTCState;
> +struct sPAPRRTCState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    int64_t ns_offset;
> +};
> +
>  typedef struct sPAPRMachineClass sPAPRMachineClass;
>  
>  #define TYPE_SPAPR_MACHINE      "spapr-machine"
> @@ -58,7 +70,7 @@ struct sPAPRMachineState {
>      QLIST_HEAD(, sPAPRPHBState) phbs;
>      struct sPAPRNVRAM *nvram;
>      ICSState *ics;
> -    DeviceState *rtc;
> +    sPAPRRTCState rtc;
>  
>      void *htab;
>      uint32_t htab_shift;
> @@ -629,11 +641,10 @@ struct sPAPRConfigureConnectorState {
>  
>  void spapr_ccs_reset_hook(void *opaque);
>  
> -#define TYPE_SPAPR_RTC "spapr-rtc"
> -#define TYPE_SPAPR_RNG "spapr-rng"
> +void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> +int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>  
> -void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
> -int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
> +#define TYPE_SPAPR_RNG "spapr-rng"
>  
>  int spapr_rng_populate_dt(void *fdt);
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 9a5e715fe553..ce230183a106 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -28,7 +28,7 @@
>  #ifndef XICS_H
>  #define XICS_H
>  
> -#include "hw/sysbus.h"
> +#include "hw/qdev.h"
>  
>  #define XICS_IPI        0x2
>  #define XICS_BUID       0x1

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]