* [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState @ 2017-03-03 13:37 Cédric Le Goater 2017-03-03 14:13 ` Thomas Huth 0 siblings, 1 reply; 8+ messages in thread From: Cédric Le Goater @ 2017-03-03 13:37 UTC (permalink / raw) To: David Gibson; +Cc: Thomas Huth, qemu-ppc, qemu-devel, 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); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-03 13:37 [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState Cédric Le Goater @ 2017-03-03 14:13 ` Thomas Huth 2017-03-03 14:49 ` Cédric Le Goater 0 siblings, 1 reply; 8+ messages in thread From: Thomas Huth @ 2017-03-03 14:13 UTC (permalink / raw) To: Cédric Le Goater, David Gibson; +Cc: qemu-ppc, qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-03 14:13 ` Thomas Huth @ 2017-03-03 14:49 ` Cédric Le Goater 2017-03-03 16:54 ` Thomas Huth 0 siblings, 1 reply; 8+ messages in thread From: Cédric Le Goater @ 2017-03-03 14:49 UTC (permalink / raw) To: Thomas Huth, David Gibson; +Cc: qemu-ppc, qemu-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-03 14:49 ` Cédric Le Goater @ 2017-03-03 16:54 ` Thomas Huth 2017-03-03 17:23 ` Cédric Le Goater 0 siblings, 1 reply; 8+ messages in thread From: Thomas Huth @ 2017-03-03 16:54 UTC (permalink / raw) To: Cédric Le Goater, David Gibson; +Cc: qemu-ppc, qemu-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-03 16:54 ` Thomas Huth @ 2017-03-03 17:23 ` Cédric Le Goater 2017-03-05 23:23 ` David Gibson 0 siblings, 1 reply; 8+ messages in thread From: Cédric Le Goater @ 2017-03-03 17:23 UTC (permalink / raw) To: Thomas Huth, David Gibson; +Cc: qemu-ppc, qemu-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-03 17:23 ` Cédric Le Goater @ 2017-03-05 23:23 ` David Gibson 2017-03-06 7:56 ` Cédric Le Goater 0 siblings, 1 reply; 8+ messages in thread From: David Gibson @ 2017-03-05 23:23 UTC (permalink / raw) To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-05 23:23 ` David Gibson @ 2017-03-06 7:56 ` Cédric Le Goater 2017-03-06 9:01 ` David Gibson 0 siblings, 1 reply; 8+ messages in thread From: Cédric Le Goater @ 2017-03-06 7:56 UTC (permalink / raw) To: David Gibson; +Cc: Thomas Huth, qemu-ppc, qemu-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState 2017-03-06 7:56 ` Cédric Le Goater @ 2017-03-06 9:01 ` David Gibson 0 siblings, 0 replies; 8+ messages in thread From: David Gibson @ 2017-03-06 9:01 UTC (permalink / raw) To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-06 9:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-03 13:37 [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState Cédric Le Goater 2017-03-03 14:13 ` Thomas Huth 2017-03-03 14:49 ` Cédric Le Goater 2017-03-03 16:54 ` Thomas Huth 2017-03-03 17:23 ` Cédric Le Goater 2017-03-05 23:23 ` David Gibson 2017-03-06 7:56 ` Cédric Le Goater 2017-03-06 9:01 ` David Gibson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.