[Qemu-devel] [PATCH 0/5] pseries: Fix and extend PAPR RTC implementation

[Qemu-devel] [PATCH 0/5] 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.

This patch addresses this also making some other cleanups and small
fixes to the PAPR RTC.  I'm confident patches 1..4 are ready to apply,
I'm little less sure about 5.  I think it's an improvement over the
current code, but it may still have problems in certain edge cases,
and I've minimally tested it with migration.

[Qemu-devel] [PATCH 1/5] 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] [PATCH 2/5] 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     | 12 ++++++++++--
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 12 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 e290ac0..9ccefbc 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -29,19 +29,27 @@
 #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 (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 +58,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] [PATCH 3/5] 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 9ccefbc..fac0017 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -44,7 +44,7 @@ static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     struct tm tm;
     uint32_t ns;
 
-    if (nret != 8) {
+    if ((nargs != 0) || (nret != 8)) {
         rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
@@ -68,6 +68,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] [PATCH 4/5] pseries: Export /machine "rtc-time" property

[David Gibson]

On x86, the guest's RTC can be read with QMP via the "rtc-time" property
on the /machine object.  This is due to an explicit QOM alias being set up
by the mc146818rtc driver, and doesn't work on other targets.

This extends the pseries RTAS RTC to populate that property, for improved
management compatibility with x86 guests.

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

diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index fac0017..13e74f1 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -28,6 +28,7 @@
 #include "cpu.h"
 #include "hw/ppc/spapr.h"
 #include "qapi-event.h"
+#include "qapi/visitor.h"
 
 void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
 {
@@ -87,10 +88,58 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
+static void spapr_qom_rtc_time(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    Error *err = NULL;
+    sPAPREnvironment *spapr = opaque;
+    struct tm current_tm;
+
+    spapr_rtc_read(spapr, &current_tm, NULL);
+
+    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);
+}
+
 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);
+
+    object_property_add(qdev_get_machine(), "rtc-time", "struct tm",
+                        spapr_qom_rtc_time, NULL, NULL, spapr, NULL);
 }
-- 
2.1.0

[Qemu-devel] [PATCH 5/5] 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 | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index 13e74f1..ba133fe 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -26,15 +26,24 @@
  *
  */
 #include "cpu.h"
+#include "sysemu/sysemu.h"
 #include "hw/ppc/spapr.h"
 #include "qapi-event.h"
 #include "qapi/visitor.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;
 }
 
 static void rtas_get_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
@@ -68,6 +77,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,9 +92,18 @@ 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);
 }
@@ -135,6 +155,17 @@ out:
 
 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

Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[Alexander Graf]

On 16.12.14 01:43, David Gibson wrote:
> 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();

Do you think we could make it a device instead?


Alex

Re: [Qemu-devel] [PATCH 2/5] pseries: Add spapr_rtc_read() helper function

[Alexander Graf]

On 16.12.14 01:43, David Gibson wrote:
> 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     | 12 ++++++++++--
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 12 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 e290ac0..9ccefbc 100644
> --- a/hw/ppc/spapr_rtc.c
> +++ b/hw/ppc/spapr_rtc.c
> @@ -29,19 +29,27 @@
>  #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)

checkpatch? ;)


Alex

Re: [Qemu-devel] [PATCH 4/5] pseries: Export /machine "rtc-time" property

[Alexander Graf]

On 16.12.14 01:43, David Gibson wrote:
> On x86, the guest's RTC can be read with QMP via the "rtc-time" property
> on the /machine object.  This is due to an explicit QOM alias being set up
> by the mc146818rtc driver, and doesn't work on other targets.
> 
> This extends the pseries RTAS RTC to populate that property, for improved
> management compatibility with x86 guests.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Yeah, this really should happen inside an rtc device.

> ---
>  hw/ppc/spapr_rtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
> index fac0017..13e74f1 100644
> --- a/hw/ppc/spapr_rtc.c
> +++ b/hw/ppc/spapr_rtc.c
> @@ -28,6 +28,7 @@
>  #include "cpu.h"
>  #include "hw/ppc/spapr.h"
>  #include "qapi-event.h"
> +#include "qapi/visitor.h"
>  
>  void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
>  {
> @@ -87,10 +88,58 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> +static void spapr_qom_rtc_time(Object *obj, Visitor *v, void *opaque,
> +                               const char *name, Error **errp)

Also this function is mostly copy&paste from the mc14foo driver. Do you
think you can share this somehow?

I also think the alias should get created by the machine, not the device
itself.


Alex

Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[David Gibson]

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

On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf wrote:
> 
> 
> On 16.12.14 01:43, David Gibson wrote:
> > 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();
> 
> Do you think we could make it a device instead?

Um.. I guess.  Is there a standard place to put such pseudo-devices in
the bus heirarchy?

-- 
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] [PATCH 2/5] pseries: Add spapr_rtc_read() helper function

[David Gibson]

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

On Tue, Dec 16, 2014 at 02:01:29AM +0100, Alexander Graf wrote:
> 
> 
> On 16.12.14 01:43, David Gibson wrote:
> > 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     | 12 ++++++++++--
> >  include/hw/ppc/spapr.h |  1 +
> >  3 files changed, 12 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 e290ac0..9ccefbc 100644
> > --- a/hw/ppc/spapr_rtc.c
> > +++ b/hw/ppc/spapr_rtc.c
> > @@ -29,19 +29,27 @@
> >  #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)
> 
> checkpatch? ;)

Sod.  It's been too long since I was doing much upstream work :).

-- 
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] [PATCH 4/5] pseries: Export /machine "rtc-time" property

[David Gibson]

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

On Tue, Dec 16, 2014 at 02:04:49AM +0100, Alexander Graf wrote:
> 
> 
> On 16.12.14 01:43, David Gibson wrote:
> > On x86, the guest's RTC can be read with QMP via the "rtc-time" property
> > on the /machine object.  This is due to an explicit QOM alias being set up
> > by the mc146818rtc driver, and doesn't work on other targets.
> > 
> > This extends the pseries RTAS RTC to populate that property, for improved
> > management compatibility with x86 guests.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Yeah, this really should happen inside an rtc device.
> 
> > ---
> >  hw/ppc/spapr_rtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
> > index fac0017..13e74f1 100644
> > --- a/hw/ppc/spapr_rtc.c
> > +++ b/hw/ppc/spapr_rtc.c
> > @@ -28,6 +28,7 @@
> >  #include "cpu.h"
> >  #include "hw/ppc/spapr.h"
> >  #include "qapi-event.h"
> > +#include "qapi/visitor.h"
> >  
> >  void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
> >  {
> > @@ -87,10 +88,58 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >  }
> >  
> > +static void spapr_qom_rtc_time(Object *obj, Visitor *v, void *opaque,
> > +                               const char *name, Error **errp)
> 
> Also this function is mostly copy&paste from the mc14foo driver. Do you
> think you can share this somehow?

I guess I can make a helper function.

> I also think the alias should get created by the machine, not the device
> itself.

Again, I'm copying what mc146818rtc does.

-- 
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] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[Alexander Graf]

> Am 16.12.2014 um 02:24 schrieb David Gibson <david@gibson.dropbear.id.au>:
> 
>> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf wrote:
>> 
>> 
>>> On 16.12.14 01:43, David Gibson wrote:
>>> 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();
>> 
>> Do you think we could make it a device instead?
> 
> Um.. I guess.  Is there a standard place to put such pseudo-devices in
> the bus heirarchy?

How about we combine this with some cleanup work and create a new spapr bus that (in the long term) exposes all the details we carry around in the spapr struct to its children via the bus?

Alex

> 
> -- 
> 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

Re: [Qemu-devel] [PATCH 4/5] pseries: Export /machine "rtc-time" property

[Alexander Graf]

> Am 16.12.2014 um 03:18 schrieb David Gibson <david@gibson.dropbear.id.au>:
> 
>> On Tue, Dec 16, 2014 at 02:04:49AM +0100, Alexander Graf wrote:
>> 
>> 
>>> On 16.12.14 01:43, David Gibson wrote:
>>> On x86, the guest's RTC can be read with QMP via the "rtc-time" property
>>> on the /machine object.  This is due to an explicit QOM alias being set up
>>> by the mc146818rtc driver, and doesn't work on other targets.
>>> 
>>> This extends the pseries RTAS RTC to populate that property, for improved
>>> management compatibility with x86 guests.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> 
>> Yeah, this really should happen inside an rtc device.
>> 
>>> ---
>>> hw/ppc/spapr_rtc.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 49 insertions(+)
>>> 
>>> diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
>>> index fac0017..13e74f1 100644
>>> --- a/hw/ppc/spapr_rtc.c
>>> +++ b/hw/ppc/spapr_rtc.c
>>> @@ -28,6 +28,7 @@
>>> #include "cpu.h"
>>> #include "hw/ppc/spapr.h"
>>> #include "qapi-event.h"
>>> +#include "qapi/visitor.h"
>>> 
>>> void spapr_rtc_read(sPAPREnvironment *spapr, struct tm *tm, uint32_t *ns)
>>> {
>>> @@ -87,10 +88,58 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>> }
>>> 
>>> +static void spapr_qom_rtc_time(Object *obj, Visitor *v, void *opaque,
>>> +                               const char *name, Error **errp)
>> 
>> Also this function is mostly copy&paste from the mc14foo driver. Do you
>> think you can share this somehow?
> 
> I guess I can make a helper function.
> 
>> I also think the alias should get created by the machine, not the device
>> itself.
> 
> Again, I'm copying what mc146818rtc does.

Yup, but unfortunately layering violations that made it upstream are still violating our layering ;)

Alex

> 
> -- 
> 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

Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[David Gibson]

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

On Tue, Dec 16, 2014 at 10:41:16AM +0100, Alexander Graf wrote:
> 
> 
> 
> > Am 16.12.2014 um 02:24 schrieb David Gibson <david@gibson.dropbear.id.au>:
> > 
> >> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf wrote:
> >> 
> >> 
> >>> On 16.12.14 01:43, David Gibson wrote:
> >>> 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();
> >> 
> >> Do you think we could make it a device instead?
> > 
> > Um.. I guess.  Is there a standard place to put such pseudo-devices in
> > the bus heirarchy?
> 
> How about we combine this with some cleanup work and create a new
> spapr bus that (in the long term) exposes all the details we carry
> around in the spapr struct to its children via the bus?

I've thought about this a bit more.  It really doesn't make sense to
put the rtc device anywhere except the root bus (which optionally
could become an spapr sub-type of the default root bus type).

But, while splitting this into its own device would be cleaner, I
don't think we should delay real behavioural fixes for that cleanup.
Working out how to split out the device without breaking old machine
types, and keeping migration working is making my head hurt.

-- 
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] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[Alexander Graf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 18.12.14 06:43, David Gibson wrote:
> On Tue, Dec 16, 2014 at 10:41:16AM +0100, Alexander Graf wrote:
>> 
>> 
>> 
>>> Am 16.12.2014 um 02:24 schrieb David Gibson
>>> <david@gibson.dropbear.id.au>:
>>> 
>>>> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf
>>>> wrote:
>>>> 
>>>> 
>>>>> On 16.12.14 01:43, David Gibson wrote: 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();
>>>> 
>>>> Do you think we could make it a device instead?
>>> 
>>> Um.. I guess.  Is there a standard place to put such
>>> pseudo-devices in the bus heirarchy?
>> 
>> How about we combine this with some cleanup work and create a
>> new spapr bus that (in the long term) exposes all the details we
>> carry around in the spapr struct to its children via the bus?
> 
> I've thought about this a bit more.  It really doesn't make sense
> to put the rtc device anywhere except the root bus (which
> optionally could become an spapr sub-type of the default root bus
> type).

Well, we could always just leave sysbus completely unused and instead
create our own global spapr root bus. In fact, that's probably what we
should do ;).

If that blows your mind for this patch set, we can make the RTC a
sysbus device too though.

> But, while splitting this into its own device would be cleaner, I 
> don't think we should delay real behavioural fixes for that
> cleanup. Working out how to split out the device without breaking
> old machine types, and keeping migration working is making my head
> hurt.

I don't think it's that hard. Just create a compat version for version
2 of "spapr" vmstates and have a post_load hook in there that shoves
the rtc offset into the new rtc device via a qom property. You should
even be able to just find the RTC device by path.

Keep in mind that we don't support new -> old migration.


Alex
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
Comment: GPGTools - http://gpgtools.org

iQIcBAEBAgAGBQJUk2TrAAoJECszeR4D/txglhEP/jey0NvjfAan3LKNxHZhseF+
1gS9/5A648w+tsuR4NzAeTTWVbSsCiDvlS8AmISkUHmMb+Zx1J6NMzuAso8xtEeL
ahMQUrtcskzz+o5b7U4ZDRMRyejRaT4fq5ryYENkxrUXZLjMf82FxbRVSFgYb4s5
89Ct8Ts9ad0OSWQYJXuHYbFRBQCyMKBpFbmW0c5FO275oH+dspsjHpyKjtU9fvaV
XE/4gnF93rXLnZNBhPKNR74jiSZYv364wb1q8Pb8OiuMacKLlBxCby0PKLfcGwEy
o13S7T7v1yrbeB/6/zGT092DpjONQRkkQ5LWZXH9UfwWPcMThg4ByoYRT9ULocUj
PRrwIm8JeXgTyJC6YcZJwxGZhFJAipLJ5SxW56YFqNvAvi2Kdp2SllZFBHy6G1zQ
tmm3CMKkHkQ4DOao48NfRlncZB49UC8MUz80eLq/fUFs2F+SEPJl3s8HN79fo2/r
N8OBnOqSaPRJ28XhZ1Lk6ytkzeE+ymHQWuDJCeiFegJ75G0CeQDG7CrI1N6w4M9V
H/7C0/vooGYDxcnprLSZpeUYdHOR7EqkGZ+ppZ6kCeRWuH7hZS8I5rqMNPeCDZpJ
v1kgmcwhFcbPo3vJ1TXNhkFQe3iU+qh/DMnWogw6NwXXLBrb1XFeslqTFi+lI2Ln
3dC64uWiOQnhllmpHz6Z
=X0rP
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[David Gibson]

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

On Fri, Dec 19, 2014 at 12:36:11AM +0100, Alexander Graf wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 18.12.14 06:43, David Gibson wrote:
> > On Tue, Dec 16, 2014 at 10:41:16AM +0100, Alexander Graf wrote:
> >> 
> >> 
> >> 
> >>> Am 16.12.2014 um 02:24 schrieb David Gibson
> >>> <david@gibson.dropbear.id.au>:
> >>> 
> >>>> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf
> >>>> wrote:
> >>>> 
> >>>> 
> >>>>> On 16.12.14 01:43, David Gibson wrote: 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();
> >>>> 
> >>>> Do you think we could make it a device instead?
> >>> 
> >>> Um.. I guess.  Is there a standard place to put such
> >>> pseudo-devices in the bus heirarchy?
> >> 
> >> How about we combine this with some cleanup work and create a
> >> new spapr bus that (in the long term) exposes all the details we
> >> carry around in the spapr struct to its children via the bus?
> > 
> > I've thought about this a bit more.  It really doesn't make sense
> > to put the rtc device anywhere except the root bus (which
> > optionally could become an spapr sub-type of the default root bus
> > type).
> 
> Well, we could always just leave sysbus completely unused and instead
> create our own global spapr root bus. In fact, that's probably what we
> should do ;).

We could, but I absolutely disagree that it's what we should do.

What is the sysbus for, if not to represent devices on the system that
don't fall under some other bus bridge.  Putting an spapr bus under it
is indirection for no purpose.

> If that blows your mind for this patch set, we can make the RTC a
> sysbus device too though.
> 
> > But, while splitting this into its own device would be cleaner, I 
> > don't think we should delay real behavioural fixes for that
> > cleanup. Working out how to split out the device without breaking
> > old machine types, and keeping migration working is making my head
> > hurt.
> 
> I don't think it's that hard. Just create a compat version for version
> 2 of "spapr" vmstates and have a post_load hook in there that shoves
> the rtc offset into the new rtc device via a qom property. You should
> even be able to just find the RTC device by path.

Yeah, I kind of figured that out myself in the meantime.  It's bit
ugly, because we need to retain the otherwise useless rtc_offset field
in sPAPREnvironment just to be loaded by the vmstatdescription then
pushed into the rtc, but I guess it works.

> Keep in mind that we don't support new -> old migration.

-- 
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] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[Alexander Graf]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 19.12.14 06:39, David Gibson wrote:
> On Fri, Dec 19, 2014 at 12:36:11AM +0100, Alexander Graf wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> 
>> 
>> On 18.12.14 06:43, David Gibson wrote:
>>> On Tue, Dec 16, 2014 at 10:41:16AM +0100, Alexander Graf
>>> wrote:
>>>> 
>>>> 
>>>> 
>>>>> Am 16.12.2014 um 02:24 schrieb David Gibson 
>>>>> <david@gibson.dropbear.id.au>:
>>>>> 
>>>>>> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf 
>>>>>> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 16.12.14 01:43, David Gibson wrote: 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();
>>>>>> 
>>>>>> Do you think we could make it a device instead?
>>>>> 
>>>>> Um.. I guess.  Is there a standard place to put such 
>>>>> pseudo-devices in the bus heirarchy?
>>>> 
>>>> How about we combine this with some cleanup work and create
>>>> a new spapr bus that (in the long term) exposes all the
>>>> details we carry around in the spapr struct to its children
>>>> via the bus?
>>> 
>>> I've thought about this a bit more.  It really doesn't make
>>> sense to put the rtc device anywhere except the root bus
>>> (which optionally could become an spapr sub-type of the default
>>> root bus type).
>> 
>> Well, we could always just leave sysbus completely unused and
>> instead create our own global spapr root bus. In fact, that's
>> probably what we should do ;).
> 
> We could, but I absolutely disagree that it's what we should do.
> 
> What is the sysbus for, if not to represent devices on the system
> that don't fall under some other bus bridge.  Putting an spapr bus
> under it is indirection for no purpose.

Sysbus is a big bucket used to represent a number of buses that have
devices that

  * are MMIO or PIO mapped into CPU visible memory regions
  * have hard IRQ wirings

Most of the devices on sPAPR are not mapped into MMIO space at all -
they have their own hcall based access methods.

For IRQs, sPAPR has its own allocation algorithm as well, which is
vastly different from anything we have with SysBus today.

I don't think we can reuse much more of SysBusDevice other than
everything that we already have in DeviceState - and at that point we
can just inherit sPAPRDeviceState from DeviceState and completely
ignore SysBus.

> 
>> If that blows your mind for this patch set, we can make the RTC
>> a sysbus device too though.
>> 
>>> But, while splitting this into its own device would be cleaner,
>>> I don't think we should delay real behavioural fixes for that 
>>> cleanup. Working out how to split out the device without
>>> breaking old machine types, and keeping migration working is
>>> making my head hurt.
>> 
>> I don't think it's that hard. Just create a compat version for
>> version 2 of "spapr" vmstates and have a post_load hook in there
>> that shoves the rtc offset into the new rtc device via a qom
>> property. You should even be able to just find the RTC device by
>> path.
> 
> Yeah, I kind of figured that out myself in the meantime.  It's bit 
> ugly, because we need to retain the otherwise useless rtc_offset
> field in sPAPREnvironment just to be loaded by the
> vmstatdescription then pushed into the rtc, but I guess it works.

Yeah, but i think we can live with the 4 bytes of otherwise unused
allocated memory ;)


Alex
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
Comment: GPGTools - http://gpgtools.org

iQIcBAEBAgAGBQJUk/bmAAoJECszeR4D/txgh1cP+gND5VZIEM8DOi7RIBIJrqCi
1xX28arHqiqcNBpOg7zdqkYwdAaUU3XQ9H4TcDa9Xj3jZbU83Hp3UXwFgKZHbTKK
MOcI7+aCVECsMrPfLnZvFCqPbuIS8zmTUzHNm8IMlDmt4AK16JbXJQSuCoMG+RB0
iKf0k55tprlEnW67ZO/AL7UTZWGCFq34cLhKqZFNb5GmBJpXu1MO3qg4TrekdlLh
/OEPDWMtRVAjsR/kJbokhpe5jPgusDTHu+ZBDtYzTr6wqgTjt3gfs+Jja2VCsaVm
vGGfdzM/Hyh04PfLEN22Iyd2y4sWUgeUCWzU3Q2Bjxrw+fPT3wsX78GqnQentFK8
0WQ2UL48vF7drdcVtsqtJYN379Bnz/CH0pUelIWNowjuhLPWwxbx3vDFpcIVq7dy
uHvNveTblL56dvf/WLn3ubdwUZ1+WIUiRE1sbQHyc/77/nY+umS500y49tVT3Xuy
rP36PyfKEOnciXEeWlOwhsyFWQg/8d3JKhqXAsW2FCuw5pYtKxsK+LVbCgI1GGJP
XFM8QRsjbEVJXB5OR7aIxBCEnNrScGyTaH+SPQkoWnV6tlnbXPj3tyJnUyXvEMfk
n59xvWvKka7LPERYLqY6TM4rD4524GVivuqdVaeMJ0c1NKp+J3jEBInKGQ/IicD2
yd2JOEvtS3PBNYonukOo
=gTRM
-----END PGP SIGNATURE-----