[Qemu-devel] [PATCHv2 0/8] pseries: Fix and extend PAPR RTC implementation

[Qemu-devel] [PATCHv2 0/8] pseries: Fix and extend PAPR RTC implementation

[David Gibson]

At the moment, the PAPR RTC implementation (actually a paravirt
firmware interface, rather than a normal device) works directly off
host time, and so doesn't respect the options such as clock=vm which
can be specified in the -rtc command line option.

Extending the PAPR RTC in this way makes it clearer it should probably
be its own separate qdev, so this series also implements that.

Finally it moves the code from mc146818rtc for publishing the RTC time
via QOM into generic QOM helpers, and extends the PAPR RTC driver to
export in a similar way.

I've done some basic sanity testing on this, but not much.  In
particular I haven't tested the migration code much yet, so that might
need revision still.

Changes since v1:
  * Actually create a qdev device for the PAPR RTC
  * Instead of duplicating the QOM publishing code from mc146818, move
    it into a generic helper

[Qemu-devel] [PATCHv2 1/8] pseries: Move sPAPR RTC code into its own file

[David Gibson]

At the moment the RTAS (firmware/hypervisor) time of day functions are
implemented in spapr_rtas.c along with a bunch of other things.  Since
we're going to be expanding these a bit, move the RTAS RTC related code
out into new file spapr_rtc.c.  Also add its own initialization function,
spapr_rtc_init() called from the main machine init routine.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/Makefile.objs   |  2 +-
 hw/ppc/spapr.c         |  3 ++
 hw/ppc/spapr_rtas.c    | 49 -----------------------------
 hw/ppc/spapr_rtc.c     | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 5 files changed, 88 insertions(+), 50 deletions(-)
 create mode 100644 hw/ppc/spapr_rtc.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 19d9920..437955d 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -3,7 +3,7 @@ obj-y += ppc.o ppc_booke.o
 # IBM pSeries (sPAPR)
 obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
-obj-$(CONFIG_PSERIES) += spapr_pci.o
+obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
 obj-y += spapr_pci_vfio.o
 endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 30de25d..16377a3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1446,6 +1446,9 @@ static void ppc_spapr_init(MachineState *machine)
     /* Set up EPOW events infrastructure */
     spapr_events_init(spapr);
 
+    /* Set up the RTC RTAS interfaces */
+    spapr_rtc_init();
+
     /* Set up VIO bus */
     spapr->vio_bus = spapr_vio_bus_init();
 
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2ec2a8e..0f1ae55 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -52,51 +52,6 @@ static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
 }
 
-static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
-                                 uint32_t token, uint32_t nargs,
-                                 target_ulong args,
-                                 uint32_t nret, target_ulong rets)
-{
-    struct tm tm;
-
-    if (nret != 8) {
-        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
-        return;
-    }
-
-    qemu_get_timedate(&tm, spapr->rtc_offset);
-
-    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
-    rtas_st(rets, 1, tm.tm_year + 1900);
-    rtas_st(rets, 2, tm.tm_mon + 1);
-    rtas_st(rets, 3, tm.tm_mday);
-    rtas_st(rets, 4, tm.tm_hour);
-    rtas_st(rets, 5, tm.tm_min);
-    rtas_st(rets, 6, tm.tm_sec);
-    rtas_st(rets, 7, 0); /* we don't do nanoseconds */
-}
-
-static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
-                                 uint32_t token, uint32_t nargs,
-                                 target_ulong args,
-                                 uint32_t nret, target_ulong rets)
-{
-    struct tm tm;
-
-    tm.tm_year = rtas_ld(args, 0) - 1900;
-    tm.tm_mon = rtas_ld(args, 1) - 1;
-    tm.tm_mday = rtas_ld(args, 2);
-    tm.tm_hour = rtas_ld(args, 3);
-    tm.tm_min = rtas_ld(args, 4);
-    tm.tm_sec = rtas_ld(args, 5);
-
-    /* Just generate a monitor event for the change */
-    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
-    spapr->rtc_offset = qemu_timedate_diff(&tm);
-
-    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
-}
-
 static void rtas_power_off(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                            uint32_t token, uint32_t nargs, target_ulong args,
                            uint32_t nret, target_ulong rets)
@@ -400,10 +355,6 @@ static void core_rtas_register_types(void)
 {
     spapr_rtas_register(RTAS_DISPLAY_CHARACTER, "display-character",
                         rtas_display_character);
-    spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day",
-                        rtas_get_time_of_day);
-    spapr_rtas_register(RTAS_SET_TIME_OF_DAY, "set-time-of-day",
-                        rtas_set_time_of_day);
     spapr_rtas_register(RTAS_POWER_OFF, "power-off", rtas_power_off);
     spapr_rtas_register(RTAS_SYSTEM_REBOOT, "system-reboot",
                         rtas_system_reboot);
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
new file mode 100644
index 0000000..e290ac0
--- /dev/null
+++ b/hw/ppc/spapr_rtc.c
@@ -0,0 +1,83 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * RTAS Real Time Clock
+ *
+ * Copyright (c) 2010-2011 David Gibson, IBM Corporation.
+ * Copyright 2014 David Gibson, Red Hat.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+#include "cpu.h"
+#include "hw/ppc/spapr.h"
+#include "qapi-event.h"
+
+static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                                 uint32_t token, uint32_t nargs,
+                                 target_ulong args,
+                                 uint32_t nret, target_ulong rets)
+{
+    struct tm tm;
+
+    if (nret != 8) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    qemu_get_timedate(&tm, spapr->rtc_offset);
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 1, tm.tm_year + 1900);
+    rtas_st(rets, 2, tm.tm_mon + 1);
+    rtas_st(rets, 3, tm.tm_mday);
+    rtas_st(rets, 4, tm.tm_hour);
+    rtas_st(rets, 5, tm.tm_min);
+    rtas_st(rets, 6, tm.tm_sec);
+    rtas_st(rets, 7, 0); /* we don't do nanoseconds */
+}
+
+static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                                 uint32_t token, uint32_t nargs,
+                                 target_ulong args,
+                                 uint32_t nret, target_ulong rets)
+{
+    struct tm tm;
+
+    tm.tm_year = rtas_ld(args, 0) - 1900;
+    tm.tm_mon = rtas_ld(args, 1) - 1;
+    tm.tm_mday = rtas_ld(args, 2);
+    tm.tm_hour = rtas_ld(args, 3);
+    tm.tm_min = rtas_ld(args, 4);
+    tm.tm_sec = rtas_ld(args, 5);
+
+    /* Just generate a monitor event for the change */
+    qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
+    spapr->rtc_offset = qemu_timedate_diff(&tm);
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+void spapr_rtc_init(void)
+{
+    spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day",
+                        rtas_get_time_of_day);
+    spapr_rtas_register(RTAS_SET_TIME_OF_DAY, "set-time-of-day",
+                        rtas_set_time_of_day);
+}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 749daf4..9a468bc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -479,5 +479,6 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
+void spapr_rtc_init(void);
 
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCHv2 2/8] pseries: Add more parameter validation in RTAS time of day functions

[David Gibson]

Currently, the RTAS time of day functions only partially validate the
number of parameters they receive and return.  Because of how the
parameters are used, this is unlikely to lead to a crash, but it's messy.

This patch adds the missing checks.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index e290ac0..13eeab8 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -36,7 +36,7 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 {
     struct tm tm;
 
-    if (nret != 8) {
+    if ((nargs != 0) || (nret != 8)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
@@ -60,6 +60,11 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 {
     struct tm tm;
 
+    if ((nargs != 7) || (nret != 1)) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_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);
-- 
2.1.0

[Qemu-devel] [PATCHv2 3/8] pseries: Add spapr_rtc_read() helper function

[David Gibson]

The virtual RTC time is used in two places in the pseries machine.  First
is in the RTAS get-time-of-day function which returns the RTC time to the
guest.  Second is in the spapr events code which is used to timestamp
event messages from the hypervisor to the guest.

Currently both call qemu_get_timedate() directly, but we want to change
that so we can properly handle the various -rtc options.  In preparation,
create a helper function to return the virtual RTC time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_events.c  |  2 +-
 hw/ppc/spapr_rtc.c     | 13 +++++++++++--
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 1b6157d..80c0266 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -246,7 +246,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     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? */
-    qemu_get_timedate(&tm, spapr->rtc_offset);
+    spapr_rtc_read(spapr, &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 13eeab8..793368f 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -29,19 +29,28 @@
 #include "hw/ppc/spapr.h"
 #include "qapi-event.h"
 
+void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
+{
+    qemu_get_timedate(tm, spapr->rtc_offset);
+    if (ns) {
+        *ns = 0; /* we don't do nanoseconds, yet */
+    }
+}
+
 static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
 {
     struct tm tm;
+    uint32_t ns;
 
     if ((nargs != 0) || (nret != 8)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
 
-    qemu_get_timedate(&tm, spapr->rtc_offset);
+    spapr_rtc_read(spapr, &tm, &ns);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     rtas_st(rets, 1, tm.tm_year + 1900);
@@ -50,7 +59,7 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     rtas_st(rets, 4, tm.tm_hour);
     rtas_st(rets, 5, tm.tm_min);
     rtas_st(rets, 6, tm.tm_sec);
-    rtas_st(rets, 7, 0); /* we don't do nanoseconds */
+    rtas_st(rets, 7, ns);
 }
 
 static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 9a468bc..80fafa9 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -480,5 +480,6 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 void spapr_rtc_init(void);
+void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns);
 
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCHv2 4/8] Generalize QOM publishing of date and time from mc146818rtc.c

[David Gibson]

The mc146818rtc driver exposes the current RTC date and time via the "date"
property in QOM (which is also aliased to the machine's "rtc-time"
property).  Currently it uses a custom visitor function rtc_get_date to
do this.

This patch introduces new helpers to the QOM core to expose struct tm
valued properties via a getter function, so that this functionality can be
more easily duplicated in other RTC implementations.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/timer/mc146818rtc.c | 44 ++--------------------------
 include/qom/object.h   | 14 +++++++++
 qom/object.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 41 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index f18d128..ecd576d 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -831,49 +831,12 @@ static const MemoryRegionOps cmos_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
-                         const char *name, Error **errp)
+static void rtc_get_date(Object *obj, struct tm *current_tm, Error **errp)
 {
-    Error *err = NULL;
     RTCState *s = MC146818_RTC(obj);
-    struct tm current_tm;
 
     rtc_update_time(s);
-    rtc_get_time(s, &current_tm);
-    visit_start_struct(v, NULL, "struct tm", name, 0, &err);
-    if (err) {
-        goto out;
-    }
-    visit_type_int32(v, &current_tm.tm_year, "tm_year", &err);
-    if (err) {
-        goto out_end;
-    }
-    visit_type_int32(v, &current_tm.tm_mon, "tm_mon", &err);
-    if (err) {
-        goto out_end;
-    }
-    visit_type_int32(v, &current_tm.tm_mday, "tm_mday", &err);
-    if (err) {
-        goto out_end;
-    }
-    visit_type_int32(v, &current_tm.tm_hour, "tm_hour", &err);
-    if (err) {
-        goto out_end;
-    }
-    visit_type_int32(v, &current_tm.tm_min, "tm_min", &err);
-    if (err) {
-        goto out_end;
-    }
-    visit_type_int32(v, &current_tm.tm_sec, "tm_sec", &err);
-    if (err) {
-        goto out_end;
-    }
-out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, errp);
-out:
-    error_propagate(errp, err);
+    rtc_get_time(s, current_tm);
 }
 
 static void rtc_realizefn(DeviceState *dev, Error **errp)
@@ -932,8 +895,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
     qdev_set_legacy_instance_id(dev, base, 3);
     qemu_register_reset(rtc_reset, s);
 
-    object_property_add(OBJECT(s), "date", "struct tm",
-                        rtc_get_date, NULL, NULL, s, NULL);
+    object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);
 
     object_property_add_alias(qdev_get_machine(), "rtc-time",
                               OBJECT(s), "date", NULL);
diff --git a/include/qom/object.h b/include/qom/object.h
index 89c3092..ba17146 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1204,6 +1204,20 @@ void object_property_add_bool(Object *obj, const char *name,
                               Error **errp);
 
 /**
+ * object_property_add_tm:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @get: the getter or NULL if the property is write-only.
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add a read-only struct tm valued property using a getter function.
+ * This function will add a property of type 'struct tm'.
+ */
+void object_property_add_tm(Object *obj, const char *name,
+                            void (*get)(Object *, struct tm *, Error **),
+                            Error **errp);
+
+/**
  * object_property_add_uint8_ptr:
  * @obj: the object to add a property to
  * @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index 1812c73..d167038 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1543,6 +1543,85 @@ void object_property_add_bool(Object *obj, const char *name,
     }
 }
 
+typedef struct TMProperty {
+    void (*get)(Object *, struct tm *, Error **);
+} TMProperty;
+
+static void property_get_tm(Object *obj, Visitor *v, void *opaque,
+                            const char *name, Error **errp)
+{
+    TMProperty *prop = opaque;
+    Error *err = NULL;
+    struct tm value;
+
+    prop->get(obj, &value, &err);
+    if (err) {
+        goto out;
+    }
+
+    visit_start_struct(v, NULL, "struct tm", name, 0, &err);
+    if (err) {
+        goto out;
+    }
+    visit_type_int32(v, &value.tm_year, "tm_year", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_int32(v, &value.tm_mon, "tm_mon", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_int32(v, &value.tm_mday, "tm_mday", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_int32(v, &value.tm_hour, "tm_hour", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_int32(v, &value.tm_min, "tm_min", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_int32(v, &value.tm_sec, "tm_sec", &err);
+    if (err) {
+        goto out_end;
+    }
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, errp);
+out:
+    error_propagate(errp, err);
+
+}
+
+static void property_release_tm(Object *obj, const char *name,
+                                void *opaque)
+{
+    TMProperty *prop = opaque;
+    g_free(prop);
+}
+
+void object_property_add_tm(Object *obj, const char *name,
+                            void (*get)(Object *, struct tm *, Error **),
+                            Error **errp)
+{
+    Error *local_err = NULL;
+    TMProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->get = get;
+
+    object_property_add(obj, name, "struct tm",
+                        get ? property_get_tm : NULL, NULL,
+                        property_release_tm,
+                        prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 static char *qdev_get_type(Object *obj, Error **errp)
 {
     return g_strdup(object_get_typename(obj));
-- 
2.1.0

[Qemu-devel] [PATCHv2 5/8] pseries: Make RTAS time of day functions respect -rtc options

[David Gibson]

In the 'pseries' machine the real time clock is provided by a
paravirtualized firmware interface rather than a device per se; the RTAS
get-time-of-day and set-time-of-day calls.

Out current implementations of those work directly off host time (with
an offset), not respecting options such as clock=vm which can be
specified in the -rtc command line option.

This patch reworks the RTAS RTC code to respect those options, primarily
by basing them on the qemu_clock_get_ns(rtc_clock) function instead of
directly on qemu_get_timedate() (which essentially handles host time, not
virtual rtc time).

As a bonus, this means our get-time-of-day function now also returns
nanoseconds.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtc.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 793368f..d6c7a22 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -26,14 +26,24 @@
  *
  */
 #include "cpu.h"
+#include "sysemu/sysemu.h"
 #include "hw/ppc/spapr.h"
 #include "qapi-event.h"
 
+#define NSEC_PER_SEC    1000000000LL
+
 void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
 {
-    qemu_get_timedate(tm, spapr->rtc_offset);
+    int64_t host_ns = qemu_clock_get_ns(rtc_clock);
+    time_t guest_s;
+
+    guest_s = host_ns / NSEC_PER_SEC + spapr->rtc_offset;
+
+    if (tm) {
+        gmtime_r(&guest_s, tm);
+    }
     if (ns) {
-        *ns = 0; /* we don't do nanoseconds, yet */
+        *ns = host_ns % NSEC_PER_SEC;
     }
 }
 
@@ -68,6 +78,8 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t nret, target_ulong rets)
 {
     struct tm tm;
+    time_t new_s;
+    int64_t host_ns;
 
     if ((nargs != 7) || (nret != 1)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
@@ -81,15 +93,35 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     tm.tm_min = rtas_ld(args, 4);
     tm.tm_sec = rtas_ld(args, 5);
 
-    /* Just generate a monitor event for the change */
+    new_s = mktimegm(&tm);
+    if (new_s == -1) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    /* Generate a monitor event for the change */
     qapi_event_send_rtc_change(qemu_timedate_diff(&tm), &error_abort);
-    spapr->rtc_offset = qemu_timedate_diff(&tm);
+
+    host_ns = qemu_clock_get_ns(rtc_clock);
+
+    spapr->rtc_offset = new_s - host_ns / NSEC_PER_SEC;
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
 void spapr_rtc_init(void)
 {
+    struct tm tm;
+    time_t host_s;
+    int64_t rtc_ns;
+
+    /* Initialize the RTAS RTC from host time */
+
+    qemu_get_timedate(&tm, 0);
+    host_s = mktimegm(&tm);
+    rtc_ns = qemu_clock_get_ns(rtc_clock);
+    spapr->rtc_offset = host_s - rtc_ns / NSEC_PER_SEC;
+
     spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day",
                         rtas_get_time_of_day);
     spapr_rtas_register(RTAS_SET_TIME_OF_DAY, "set-time-of-day",
-- 
2.1.0

[Qemu-devel] [PATCHv2 6/8] pseries: Make the PAPR RTC a qdev device

[David Gibson]

At present the PAPR RTC isn't a "device" as such - it's accessed only via
firmware/hypervisor calls, and is handled in the sPAPR core code.  This
becomes inconvenient as we extend it in various ways.

This patch makes the PAPR RTC a separate device in the qemu device model.

For now, the only piece of device state - the rtc_offset - is still kept in
the global sPAPREnvironment structure.  That's clearly wrong, but leaving
it to be fixed in a following patch makes for a clearer separation between
the internal re-organization of the device, and the behavioural changes
(because the migration stream format needs to change slightly when the
offset is moved into the device's own state).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 10 +++++++++-
 hw/ppc/spapr_events.c  |  2 +-
 hw/ppc/spapr_rtc.c     | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/ppc/spapr.h |  7 +++++--
 4 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 16377a3..9722b42 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -955,6 +955,14 @@ static void spapr_create_nvram(sPAPREnvironment *spapr)
     spapr->nvram = (struct sPAPRNVRAM *)dev;
 }
 
+static void spapr_rtc_create(sPAPREnvironment *spapr)
+{
+    DeviceState *dev = qdev_create(NULL, TYPE_SPAPR_RTC);
+
+    qdev_init_nofail(dev);
+    spapr->rtc = dev;
+}
+
 /* Returns whether we want to use VGA or not */
 static int spapr_vga_init(PCIBus *pci_bus)
 {
@@ -1447,7 +1455,7 @@ static void ppc_spapr_init(MachineState *machine)
     spapr_events_init(spapr);
 
     /* Set up the RTC RTAS interfaces */
-    spapr_rtc_init();
+    spapr_rtc_create(spapr);
 
     /* Set up VIO bus */
     spapr->vio_bus = spapr_vio_bus_init();
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 80c0266..283e96b 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -246,7 +246,7 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     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, &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 d6c7a22..b9f4704 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -30,13 +30,25 @@
 #include "hw/ppc/spapr.h"
 #include "qapi-event.h"
 
+#define SPAPR_RTC(obj) \
+    OBJECT_CHECK(sPAPRRTCState, (obj), TYPE_SPAPR_RTC)
+
+typedef struct sPAPRRTCState sPAPRRTCState;
+struct sPAPRRTCState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+};
+
 #define NSEC_PER_SEC    1000000000LL
 
-void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
+void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns)
 {
+    sPAPRRTCState *rtc = SPAPR_RTC(dev);
     int64_t host_ns = qemu_clock_get_ns(rtc_clock);
     time_t guest_s;
 
+    assert(rtc);
+
     guest_s = host_ns / NSEC_PER_SEC + spapr->rtc_offset;
 
     if (tm) {
@@ -60,7 +72,12 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         return;
     }
 
-    spapr_rtc_read(spapr, &tm, &ns);
+    if (!spapr->rtc) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    spapr_rtc_read(spapr->rtc, &tm, &ns);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     rtas_st(rets, 1, tm.tm_year + 1900);
@@ -86,6 +103,11 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *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);
@@ -109,7 +131,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
-void spapr_rtc_init(void)
+static void spapr_rtc_realize(DeviceState *dev, Error **errp)
 {
     struct tm tm;
     time_t host_s;
@@ -121,9 +143,30 @@ void spapr_rtc_init(void)
     host_s = mktimegm(&tm);
     rtc_ns = qemu_clock_get_ns(rtc_clock);
     spapr->rtc_offset = host_s - rtc_ns / NSEC_PER_SEC;
+}
+
+static void spapr_rtc_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = spapr_rtc_realize;
 
     spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day",
                         rtas_get_time_of_day);
     spapr_rtas_register(RTAS_SET_TIME_OF_DAY, "set-time-of-day",
                         rtas_set_time_of_day);
 }
+
+static const TypeInfo spapr_rtc_info = {
+    .name          = TYPE_SPAPR_RTC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(sPAPRRTCState),
+    .class_size = sizeof(XICSStateClass),
+    .class_init    = spapr_rtc_class_init,
+};
+
+static void spapr_rtc_register_types(void)
+{
+    type_register_static(&spapr_rtc_info);
+}
+type_init(spapr_rtc_register_types)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 80fafa9..9b7cc9d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -15,6 +15,7 @@ typedef struct sPAPREnvironment {
     QLIST_HEAD(, sPAPRPHBState) phbs;
     struct sPAPRNVRAM *nvram;
     XICSState *icp;
+    DeviceState *rtc;
 
     hwaddr ram_limit;
     void *htab;
@@ -479,7 +480,9 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
-void spapr_rtc_init(void);
-void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns);
+
+#define TYPE_SPAPR_RTC "spapr-rtc"
+
+void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
 
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[David Gibson]

The initial creation of the PAPR RTC qdev class left a wart - the rtc's
offset was left in the sPAPREnvironment structure, accessed via a global.

This patch moves it into the RTC device's own state structure, were it
belongs.  This requires a small change to the migration stream format.  In
order to handle incoming streams from older versions, we also need to
retain the rtc_offset field in the sPAPREnvironment structure, so that it
can be loaded into via the vmsd, then pushed into the RTC device.

Since we're changing the migration format, this also takes the opportunity
to change the rtc offset from a value in seconds to a value in nanoseconds,
allowing nanosecond offsets between host and guest rtc time, if desired.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 19 ++++++++++++++++++-
 hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9722b42..3070be0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
     }
 }
 
+static int spapr_post_load(void *opaque, int version_id)
+{
+    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
+    int err = 0;
+
+    /* In earlier versions, there was no seperate qdev for the PAPR
+     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
+     * 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);
+    }
+
+    return err;
+}
+
 static const VMStateDescription vmstate_spapr = {
     .name = "spapr",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 1,
+    .post_load = spapr_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UNUSED(4), /* used to be @next_irq */
 
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index b9f4704..5ad0823 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -37,6 +37,7 @@ typedef struct sPAPRRTCState sPAPRRTCState;
 struct sPAPRRTCState {
     /*< private >*/
     SysBusDevice parent_obj;
+    int64_t ns_offset;
 };
 
 #define NSEC_PER_SEC    1000000000LL
@@ -45,20 +46,37 @@ void spapr_rtc_read(DeviceState *dev, 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;
 
     assert(rtc);
 
-    guest_s = host_ns / NSEC_PER_SEC + spapr->rtc_offset;
+    guest_ns = host_ns + rtc->ns_offset;
+    guest_s = guest_ns / NSEC_PER_SEC;
 
     if (tm) {
         gmtime_r(&guest_s, tm);
     }
     if (ns) {
-        *ns = host_ns % NSEC_PER_SEC;
+        *ns = guest_ns;
     }
 }
 
+int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset)
+{
+    sPAPRRTCState *rtc;
+
+    if (!dev) {
+        return -ENODEV;
+    }
+
+    rtc = SPAPR_RTC(dev);
+
+    rtc->ns_offset = legacy_offset * NSEC_PER_SEC;
+
+    return 0;
+}
+
 static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args,
@@ -94,6 +112,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  target_ulong args,
                                  uint32_t nret, target_ulong rets)
 {
+    sPAPRRTCState *rtc;
     struct tm tm;
     time_t new_s;
     int64_t host_ns;
@@ -124,15 +143,18 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *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);
 
-    spapr->rtc_offset = new_s - host_ns / NSEC_PER_SEC;
+    rtc->ns_offset = (new_s * NSEC_PER_SEC) - host_ns;
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
 static void spapr_rtc_realize(DeviceState *dev, Error **errp)
 {
+    sPAPRRTCState *rtc = SPAPR_RTC(dev);
     struct tm tm;
     time_t host_s;
     int64_t rtc_ns;
@@ -142,14 +164,25 @@ static void spapr_rtc_realize(DeviceState *dev, Error **errp)
     qemu_get_timedate(&tm, 0);
     host_s = mktimegm(&tm);
     rtc_ns = qemu_clock_get_ns(rtc_clock);
-    spapr->rtc_offset = host_s - rtc_ns / NSEC_PER_SEC;
+    rtc->ns_offset = host_s * NSEC_PER_SEC - rtc_ns;
 }
 
+static const VMStateDescription vmstate_spapr_rtc = {
+    .name = "spapr/rtc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(ns_offset, sPAPRRTCState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void spapr_rtc_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = spapr_rtc_realize;
+    dc->vmsd = &vmstate_spapr_rtc;
 
     spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day",
                         rtas_get_time_of_day);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 9b7cc9d..e07c3a2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -27,7 +27,7 @@ typedef struct sPAPREnvironment {
     void *rtas_blob;
     void *fdt_skel;
     target_ulong entry_point;
-    uint64_t rtc_offset;
+    uint64_t rtc_offset; /* Now used only during incoming migration */
     struct PPCTimebase tb;
     bool has_graphics;
 
@@ -484,5 +484,6 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
 #define TYPE_SPAPR_RTC "spapr-rtc"
 
 void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
+int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
 
 #endif /* !defined (__HW_SPAPR_H__) */
-- 
2.1.0

[Qemu-devel] [PATCHv2 8/8] pseries: Export RTC time via QOM

[David Gibson]

On x86, the guest's RTC can be read with QMP, either from the RTC device's
"date" property or via the "rtc-time" property on the machine (which is an
alias to the former).  This is set up in the mc146818rtc driver, and
doesn't work on other targets.

This patch adds a similar "date" property to the pseries machine's RTAS RTC
and adds a compatible alias to the machine.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c     | 3 +++
 hw/ppc/spapr_rtc.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3070be0..88fabc8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -961,6 +961,9 @@ static void spapr_rtc_create(sPAPREnvironment *spapr)
 
     qdev_init_nofail(dev);
     spapr->rtc = dev;
+
+    object_property_add_alias(qdev_get_machine(), "rtc-time",
+                              OBJECT(spapr->rtc), "date", NULL);
 }
 
 /* Returns whether we want to use VGA or not */
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 5ad0823..83eb7c1 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -152,6 +152,11 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
+static void spapr_rtc_qom_date(Object *obj, struct tm *current_tm, Error **errp)
+{
+    spapr_rtc_read(DEVICE(obj), current_tm, NULL);
+}
+
 static void spapr_rtc_realize(DeviceState *dev, Error **errp)
 {
     sPAPRRTCState *rtc = SPAPR_RTC(dev);
@@ -165,6 +170,8 @@ static void spapr_rtc_realize(DeviceState *dev, Error **errp)
     host_s = mktimegm(&tm);
     rtc_ns = qemu_clock_get_ns(rtc_clock);
     rtc->ns_offset = host_s * NSEC_PER_SEC - rtc_ns;
+
+    object_property_add_tm(OBJECT(rtc), "date", spapr_rtc_qom_date, NULL);
 }
 
 static const VMStateDescription vmstate_spapr_rtc = {
-- 
2.1.0

Re: [Qemu-devel] [PATCHv2 8/8] pseries: Export RTC time via QOM

[Alexander Graf]

On 23.12.14 01:17, David Gibson wrote:
> On x86, the guest's RTC can be read with QMP, either from the RTC device's
> "date" property or via the "rtc-time" property on the machine (which is an
> alias to the former).  This is set up in the mc146818rtc driver, and
> doesn't work on other targets.
> 
> This patch adds a similar "date" property to the pseries machine's RTAS RTC
> and adds a compatible alias to the machine.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Very nice, can we somehow get rid of an exported spapr_rtc_read() with
this as well?


Alex

Re: [Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[Alexander Graf]

On 23.12.14 01:17, David Gibson wrote:
> The initial creation of the PAPR RTC qdev class left a wart - the rtc's
> offset was left in the sPAPREnvironment structure, accessed via a global.
> 
> This patch moves it into the RTC device's own state structure, were it
> belongs.  This requires a small change to the migration stream format.  In
> order to handle incoming streams from older versions, we also need to
> retain the rtc_offset field in the sPAPREnvironment structure, so that it
> can be loaded into via the vmsd, then pushed into the RTC device.
> 
> Since we're changing the migration format, this also takes the opportunity
> to change the rtc offset from a value in seconds to a value in nanoseconds,
> allowing nanosecond offsets between host and guest rtc time, if desired.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         | 19 ++++++++++++++++++-
>  hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
>  include/hw/ppc/spapr.h |  3 ++-
>  3 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9722b42..3070be0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
>      }
>  }
>  
> +static int spapr_post_load(void *opaque, int version_id)
> +{
> +    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
> +    int err = 0;
> +
> +    /* In earlier versions, there was no seperate qdev for the PAPR
> +     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
> +     * 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);

Do you think you could do this via a qom property set rather than an
explicit function call into the rtc code base instead?

> +    }
> +
> +    return err;
> +}
> +
>  static const VMStateDescription vmstate_spapr = {
>      .name = "spapr",
> -    .version_id = 2,
> +    .version_id = 3,
>      .minimum_version_id = 1,
> +    .post_load = spapr_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UNUSED(4), /* used to be @next_irq */

Shouldn't we make the rtc_offset field conditional on version < 3 as
well here? In fact, you could do the same for the legacy next_irq. That
way we don't need to move data over the wire that doesn't get used anymore.


Alex

Re: [Qemu-devel] [PATCHv2 0/8] pseries: Fix and extend PAPR RTC implementation

[Alexander Graf]

On 23.12.14 01:16, David Gibson wrote:
> At the moment, the PAPR RTC implementation (actually a paravirt
> firmware interface, rather than a normal device) works directly off
> host time, and so doesn't respect the options such as clock=vm which
> can be specified in the -rtc command line option.
> 
> Extending the PAPR RTC in this way makes it clearer it should probably
> be its own separate qdev, so this series also implements that.
> 
> Finally it moves the code from mc146818rtc for publishing the RTC time
> via QOM into generic QOM helpers, and extends the PAPR RTC driver to
> export in a similar way.
> 
> I've done some basic sanity testing on this, but not much.  In
> particular I haven't tested the migration code much yet, so that might
> need revision still.

I think we're on the right track. After this patch set, I would like to
see no exported functions related to RTC in include/hw/ppc/spapr.h left
though :). I'm fairly sure you can get all of them handled via qom
properties instead.


Alex

Re: [Qemu-devel] [PATCHv2 8/8] pseries: Export RTC time via QOM

[David Gibson]

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

On Tue, Dec 23, 2014 at 01:26:58AM +0100, Alexander Graf wrote:
> 
> 
> On 23.12.14 01:17, David Gibson wrote:
> > On x86, the guest's RTC can be read with QMP, either from the RTC device's
> > "date" property or via the "rtc-time" property on the machine (which is an
> > alias to the former).  This is set up in the mc146818rtc driver, and
> > doesn't work on other targets.
> > 
> > This patch adds a similar "date" property to the pseries machine's RTAS RTC
> > and adds a compatible alias to the machine.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Very nice, can we somehow get rid of an exported spapr_rtc_read() with
> this as well?

Uhhh.. we could, but I really don't like the idea.

It seems perverse to encode the current time into JSON, just so we can
decode it into a struct tm again in the events code.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[David Gibson]

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

On Tue, Dec 23, 2014 at 01:30:11AM +0100, Alexander Graf wrote:
> 
> 
> On 23.12.14 01:17, David Gibson wrote:
> > The initial creation of the PAPR RTC qdev class left a wart - the rtc's
> > offset was left in the sPAPREnvironment structure, accessed via a global.
> > 
> > This patch moves it into the RTC device's own state structure, were it
> > belongs.  This requires a small change to the migration stream format.  In
> > order to handle incoming streams from older versions, we also need to
> > retain the rtc_offset field in the sPAPREnvironment structure, so that it
> > can be loaded into via the vmsd, then pushed into the RTC device.
> > 
> > Since we're changing the migration format, this also takes the opportunity
> > to change the rtc offset from a value in seconds to a value in nanoseconds,
> > allowing nanosecond offsets between host and guest rtc time, if desired.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c         | 19 ++++++++++++++++++-
> >  hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
> >  include/hw/ppc/spapr.h |  3 ++-
> >  3 files changed, 57 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 9722b42..3070be0 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
> >      }
> >  }
> >  
> > +static int spapr_post_load(void *opaque, int version_id)
> > +{
> > +    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
> > +    int err = 0;
> > +
> > +    /* In earlier versions, there was no seperate qdev for the PAPR
> > +     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
> > +     * 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);
> 
> Do you think you could do this via a qom property set rather than an
> explicit function call into the rtc code base instead?

Hrm.  So, I could expose ns_offset as a property, but I'm not sure
it's a good idea.  Apart from this one instance to handle backwards
compat migration, it's a purely internal value - seeting it from the
command line is unlikely to have the desired effect, since it will get
overwritten by spapr_rtc_realize().

> 
> > +    }
> > +
> > +    return err;
> > +}
> > +
> >  static const VMStateDescription vmstate_spapr = {
> >      .name = "spapr",
> > -    .version_id = 2,
> > +    .version_id = 3,
> >      .minimum_version_id = 1,
> > +    .post_load = spapr_post_load,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UNUSED(4), /* used to be @next_irq */
> 
> Shouldn't we make the rtc_offset field conditional on version < 3 as
> well here? In fact, you could do the same for the legacy next_irq. That
> way we don't need to move data over the wire that doesn't get used anymore.

Uh, yeah, I guess we can do that.  It's just a bit awkward, because
the vmstatedescription includes built in handling for "earliest
version" but not "last version", so I'll need to write a custom test
function.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 8/8] pseries: Export RTC time via QOM

[Alexander Graf]

> Am 23.12.2014 um 04:14 schrieb David Gibson <david@gibson.dropbear.id.au>:
> 
>> On Tue, Dec 23, 2014 at 01:26:58AM +0100, Alexander Graf wrote:
>> 
>> 
>>> On 23.12.14 01:17, David Gibson wrote:
>>> On x86, the guest's RTC can be read with QMP, either from the RTC device's
>>> "date" property or via the "rtc-time" property on the machine (which is an
>>> alias to the former).  This is set up in the mc146818rtc driver, and
>>> doesn't work on other targets.
>>> 
>>> This patch adds a similar "date" property to the pseries machine's RTAS RTC
>>> and adds a compatible alias to the machine.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> 
>> Very nice, can we somehow get rid of an exported spapr_rtc_read() with
>> this as well?
> 
> Uhhh.. we could, but I really don't like the idea.
> 
> It seems perverse to encode the current time into JSON, just so we can
> decode it into a struct tm again in the events code.

I don't understand - what does JSON have to do with it?

Alex

Re: [Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[Alexander Graf]

> Am 23.12.2014 um 04:56 schrieb David Gibson <david@gibson.dropbear.id.au>:
> 
>> On Tue, Dec 23, 2014 at 01:30:11AM +0100, Alexander Graf wrote:
>> 
>> 
>>> On 23.12.14 01:17, David Gibson wrote:
>>> The initial creation of the PAPR RTC qdev class left a wart - the rtc's
>>> offset was left in the sPAPREnvironment structure, accessed via a global.
>>> 
>>> This patch moves it into the RTC device's own state structure, were it
>>> belongs.  This requires a small change to the migration stream format.  In
>>> order to handle incoming streams from older versions, we also need to
>>> retain the rtc_offset field in the sPAPREnvironment structure, so that it
>>> can be loaded into via the vmsd, then pushed into the RTC device.
>>> 
>>> Since we're changing the migration format, this also takes the opportunity
>>> to change the rtc offset from a value in seconds to a value in nanoseconds,
>>> allowing nanosecond offsets between host and guest rtc time, if desired.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> hw/ppc/spapr.c         | 19 ++++++++++++++++++-
>>> hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
>>> include/hw/ppc/spapr.h |  3 ++-
>>> 3 files changed, 57 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 9722b42..3070be0 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
>>>     }
>>> }
>>> 
>>> +static int spapr_post_load(void *opaque, int version_id)
>>> +{
>>> +    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
>>> +    int err = 0;
>>> +
>>> +    /* In earlier versions, there was no seperate qdev for the PAPR
>>> +     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
>>> +     * 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);
>> 
>> Do you think you could do this via a qom property set rather than an
>> explicit function call into the rtc code base instead?
> 
> Hrm.  So, I could expose ns_offset as a property, but I'm not sure
> it's a good idea.  Apart from this one instance to handle backwards
> compat migration, it's a purely internal value - seeting it from the
> command line is unlikely to have the desired effect, since it will get
> overwritten by spapr_rtc_realize().

It's not about setting it on the command line, it's a question on how we separate our interfaces. The way you handle it right now we need to make the RTAS code link with the RTC code. With QOM properties this would happen with a by-name convention both sides use.

If you can show me that the code looks cleaner with native C exported functions, I could be persuaded otherwise though.

> 
>> 
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> +
>>> static const VMStateDescription vmstate_spapr = {
>>>     .name = "spapr",
>>> -    .version_id = 2,
>>> +    .version_id = 3,
>>>     .minimum_version_id = 1,
>>> +    .post_load = spapr_post_load,
>>>     .fields = (VMStateField[]) {
>>>         VMSTATE_UNUSED(4), /* used to be @next_irq */
>> 
>> Shouldn't we make the rtc_offset field conditional on version < 3 as
>> well here? In fact, you could do the same for the legacy next_irq. That
>> way we don't need to move data over the wire that doesn't get used anymore.
> 
> Uh, yeah, I guess we can do that.  It's just a bit awkward, because
> the vmstatedescription includes built in handling for "earliest
> version" but not "last version", so I'll need to write a custom test
> function.

Yeah, though I wouldn't call it awkward ;).

Alex

Re: [Qemu-devel] [PATCHv2 8/8] pseries: Export RTC time via QOM

[David Gibson]

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

On Tue, Dec 23, 2014 at 07:33:30AM +0100, Alexander Graf wrote:
> 
> 
> 
> > Am 23.12.2014 um 04:14 schrieb David Gibson <david@gibson.dropbear.id.au>:
> > 
> >> On Tue, Dec 23, 2014 at 01:26:58AM +0100, Alexander Graf wrote:
> >> 
> >> 
> >>> On 23.12.14 01:17, David Gibson wrote:
> >>> On x86, the guest's RTC can be read with QMP, either from the RTC device's
> >>> "date" property or via the "rtc-time" property on the machine (which is an
> >>> alias to the former).  This is set up in the mc146818rtc driver, and
> >>> doesn't work on other targets.
> >>> 
> >>> This patch adds a similar "date" property to the pseries machine's RTAS RTC
> >>> and adds a compatible alias to the machine.
> >>> 
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> 
> >> Very nice, can we somehow get rid of an exported spapr_rtc_read() with
> >> this as well?
> > 
> > Uhhh.. we could, but I really don't like the idea.
> > 
> > It seems perverse to encode the current time into JSON, just so we can
> > decode it into a struct tm again in the events code.
> 
> I don't understand - what does JSON have to do with it?

My misunderstanding, it doesn't actually deal with JSON here.  But if
I'm following how this would work, it would be a way of getting a
struct tm from point A to point B, by:
  1. building a temporary visitor object (object_property_get_qobject)
  2. finding and calling the general property getter callback
     (object_property_get)
  3. Making multiple callbacks into the visitor with pieces of point
     A's struct tm (property_get_tm)
  4. In those visitor callbacks building an associative array
     (qmp_output_start_struct), then filling it with object wrappers
     around integers (qmp_output_type_int)
  5. Digging through the nested qobject thus constructed to extract
     the various ints and put them into point B's struct tm
     (hypothetical object_property_get_tm)
  6. Cleaning up the various temporaries created during the process


Seriously, w. t. f.!?

Maybe there's a simpler way of doing this via the object property
mechanism, but it's sure as hell not obvious to me.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[David Gibson]

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

On Tue, Dec 23, 2014 at 07:41:22AM +0100, Alexander Graf wrote:
> 
> 
> 
> > Am 23.12.2014 um 04:56 schrieb David Gibson <david@gibson.dropbear.id.au>:
> > 
> >> On Tue, Dec 23, 2014 at 01:30:11AM +0100, Alexander Graf wrote:
> >> 
> >> 
> >>> On 23.12.14 01:17, David Gibson wrote:
> >>> The initial creation of the PAPR RTC qdev class left a wart - the rtc's
> >>> offset was left in the sPAPREnvironment structure, accessed via a global.
> >>> 
> >>> This patch moves it into the RTC device's own state structure, were it
> >>> belongs.  This requires a small change to the migration stream format.  In
> >>> order to handle incoming streams from older versions, we also need to
> >>> retain the rtc_offset field in the sPAPREnvironment structure, so that it
> >>> can be loaded into via the vmsd, then pushed into the RTC device.
> >>> 
> >>> Since we're changing the migration format, this also takes the opportunity
> >>> to change the rtc offset from a value in seconds to a value in nanoseconds,
> >>> allowing nanosecond offsets between host and guest rtc time, if desired.
> >>> 
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>> hw/ppc/spapr.c         | 19 ++++++++++++++++++-
> >>> hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
> >>> include/hw/ppc/spapr.h |  3 ++-
> >>> 3 files changed, 57 insertions(+), 6 deletions(-)
> >>> 
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index 9722b42..3070be0 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
> >>>     }
> >>> }
> >>> 
> >>> +static int spapr_post_load(void *opaque, int version_id)
> >>> +{
> >>> +    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
> >>> +    int err = 0;
> >>> +
> >>> +    /* In earlier versions, there was no seperate qdev for the PAPR
> >>> +     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
> >>> +     * 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);
> >> 
> >> Do you think you could do this via a qom property set rather than an
> >> explicit function call into the rtc code base instead?
> > 
> > Hrm.  So, I could expose ns_offset as a property, but I'm not sure
> > it's a good idea.  Apart from this one instance to handle backwards
> > compat migration, it's a purely internal value - seeting it from the
> > command line is unlikely to have the desired effect, since it will get
> > overwritten by spapr_rtc_realize().
> 
> It's not about setting it on the command line,

I realise that, but as I understand it, putting the qom property there
inevitably means that it *could* be set from the command line, and
that doesn't sound like a good thing to me.

> it's a question on
> how we separate our interfaces. The way you handle it right now we
> need to make the RTAS code link with the RTC code. With QOM
> properties this would happen with a by-name convention both sides
> use.

Well, yes, and my point is that that there *should not* be a general
interface to set the offset, because setting the offset never makes
sense, except for this backwards compatibility shim.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]