From patchwork Mon Mar 6 07:56:07 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?C=C3=A9dric_Le_Goater?= X-Patchwork-Id: 735607 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vcBvg16Xhz9s78 for ; Mon, 6 Mar 2017 18:57:03 +1100 (AEDT) Received: from localhost ([::1]:42260 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cknVs-0007l4-JC for incoming@patchwork.ozlabs.org; Mon, 06 Mar 2017 02:57:00 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cknVL-0007jJ-2v for qemu-devel@nongnu.org; Mon, 06 Mar 2017 02:56:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cknVH-0003BK-TB for qemu-devel@nongnu.org; Mon, 06 Mar 2017 02:56:27 -0500 Received: from 7.mo68.mail-out.ovh.net ([46.105.63.230]:47351) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cknVH-000335-Jg for qemu-devel@nongnu.org; Mon, 06 Mar 2017 02:56:23 -0500 Received: from player750.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo68.mail-out.ovh.net (Postfix) with ESMTP id 2D93744328 for ; Mon, 6 Mar 2017 08:56:13 +0100 (CET) Received: from zorba.kaod.org (LFbn-1-10647-27.w90-89.abo.wanadoo.fr [90.89.233.27]) (Authenticated sender: postmaster@kaod.org) by player750.ha.ovh.net (Postfix) with ESMTPSA id 1413A180074; Mon, 6 Mar 2017 08:56:08 +0100 (CET) To: David Gibson References: <1488548254-456-1-git-send-email-clg@kaod.org> <39ad7cac-b29e-b01e-723c-8cbdb1cf3ca9@redhat.com> <20170305232328.GB12030@umbus.fritz.box> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <4743c70a-c8e4-a8a6-f685-9e7119d575b9@kaod.org> Date: Mon, 6 Mar 2017 08:56:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170305232328.GB12030@umbus.fritz.box> X-Ovh-Tracer-Id: 1593429847042001728 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeelhedrfeelgdejjecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 46.105.63.230 Subject: Re: [Qemu-devel] [PATCH] ppc/spapr: QOM'ify sPAPRRTCState X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Huth , qemu-ppc@nongnu.org, qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "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?= 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 --- 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