[Qemu-devel] [PATCH 0/4] power7/8 migration patches

[Qemu-devel] [PATCH 0/4] power7/8 migration patches

[Alexey Kardashevskiy]

The first patch is in Paolo's "kvm/next", others are new. They are independend
but may not apply if to change their order. Please comment. Thanks.


Alexey Kardashevskiy (4):
  kvm: Add set_one_reg/get_one_reg helpers
  spapr: Enable DABRX special register
  KVM: PPC: Support POWER8 registers
  spapr: Add support for time base offset migration

 hw/ppc/ppc.c                | 120 ++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c              |   4 +-
 include/hw/ppc/spapr.h      |   2 +
 include/sysemu/kvm.h        |  21 ++++++++
 kvm-all.c                   |  18 +++++++
 target-ppc/cpu-qom.h        |  16 ++++++
 target-ppc/cpu.h            |  18 +++++++
 target-ppc/kvm.c            |  43 ++++++++++++++++
 target-ppc/machine.c        |  38 +++++++++++++-
 target-ppc/translate_init.c |  28 ++++++++---
 trace-events                |   3 ++
 11 files changed, 301 insertions(+), 10 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH 1/4] kvm: Add set_one_reg/get_one_reg helpers

[Alexey Kardashevskiy]

This adds QEMU wrappers for KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctls.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* changed comments

v2:
* added Doc Comments
* removed error_print
---
 include/sysemu/kvm.h | 21 +++++++++++++++++++++
 kvm-all.c            | 18 ++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 0bee1e8..c3471de 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -353,4 +353,25 @@ void kvm_init_irq_routing(KVMState *s);
  *          > 0: irq chip was created
  */
 int kvm_arch_irqchip_create(KVMState *s);
+
+/**
+ * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
+ * @id: The register ID
+ * @addr: The pointer to a value must point to a variable of the correct
+ * type/size for the register being accessed.
+ *
+ * Returns: 0 on success, or a negative errno on failure.
+ */
+int kvm_set_one_reg(CPUState *cs, uint64_t id, void *addr);
+
+/**
+ * kvm_get_one_reg - get a register value from KVM via KVM_GET_ONE_REG ioctl
+ * @id: The register ID
+ * @addr: The pointer to a value must point to a variable of the correct
+ * type/size for the register being accessed.
+ *
+ * Returns: 0 on success, or a negative errno on failure.
+ */
+int kvm_get_one_reg(CPUState *cs, uint64_t id, void *addr);
+
 #endif
diff --git a/kvm-all.c b/kvm-all.c
index cd4111d..fc30181 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2114,3 +2114,21 @@ int kvm_create_device(KVMState *s, uint64_t type, bool test)
 
     return test ? 0 : create_dev.fd;
 }
+
+int kvm_set_one_reg(CPUState *cs, uint64_t id, void *addr)
+{
+    struct kvm_one_reg reg = {
+        .id = id,
+        .addr = (uintptr_t)addr,
+    };
+    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+}
+
+int kvm_get_one_reg(CPUState *cs, uint64_t id, void *addr)
+{
+    struct kvm_one_reg reg = {
+        .id = id,
+        .addr = (uintptr_t)addr,
+    };
+    return kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+}
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexey Kardashevskiy]

This advertises Data Address Breakpoint Register Extension (DABRX) to
the guest via hyperrtas list and enables it to migrate.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c              | 1 +
 target-ppc/translate_init.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a11e121..451c473 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
     uint32_t start_prop = cpu_to_be32(initrd_base);
     uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
     char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
+        "\0hcall-xdabr"
         "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
     char qemu_hypertas_prop[] = "hcall-memop1";
     uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index d07e186..1627bb0 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
                      SPR_NOACCESS, SPR_NOACCESS,
                      &spr_read_generic, &spr_write_generic,
                      KVM_REG_PPC_PMC6, 0x00000000);
+    spr_register_kvm(env, SPR_DABRX, "DABRX",
+                     SPR_NOACCESS, SPR_NOACCESS,
+                     SPR_NOACCESS, SPR_NOACCESS,
+                     KVM_REG_PPC_DABRX, 0x00000000);
 #endif /* !CONFIG_USER_ONLY */
     gen_spr_amr(env);
     /* XXX : not implemented */
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Alexey Kardashevskiy]

This enabled KVM and migration support for a number of POWER8 registers:
* Program Prioirty Register (PPR)
* Sampled Instruction Address Register (SIAR)
* Sampled Data Address Register (SDAR)
* Vector Registers Save Register (VRSAVE)

This enables save/restore of transactional state if MSR_TS is set
in the MSR.

This adds new POWER8 registers support required for migration.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-ppc/cpu.h            | 18 ++++++++++++++++++
 target-ppc/kvm.c            | 38 ++++++++++++++++++++++++++++++++++++++
 target-ppc/machine.c        | 34 ++++++++++++++++++++++++++++++++++
 target-ppc/translate_init.c | 24 ++++++++++++++++--------
 4 files changed, 106 insertions(+), 8 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2719c08..2b476b4 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -426,6 +426,8 @@ struct ppc_slb_t {
 #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
 #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
 #define MSR_SHV  60 /* hypervisor state                               hflags */
+#define MSR_TS   33 /* Transactional state, 2 bits (Book3s)                  */
+#define MSR_TM   32 /* Transactional Memory Available (Book3s)               */
 #define MSR_CM   31 /* Computation mode for BookE                     hflags */
 #define MSR_ICM  30 /* Interrupt computation mode for BookE                  */
 #define MSR_THV  29 /* hypervisor state for 32 bits PowerPC           hflags */
@@ -1081,6 +1083,20 @@ struct CPUPPCState {
      */
     uint8_t fit_period[4];
     uint8_t wdt_period[4];
+
+    /* Transactional memory state migration */
+    target_ulong tm_gpr[32];
+    ppc_avr_t tm_vsr[64];
+    uint64_t tm_cr;
+    uint64_t tm_lr;
+    uint64_t tm_ctr;
+    uint64_t tm_fpscr;
+    uint64_t tm_amr;
+    uint64_t tm_ppr;
+    uint64_t tm_vrsave;
+    uint32_t tm_vscr;
+    uint64_t tm_dscr;
+    uint64_t tm_tar;
 };
 
 #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
@@ -1479,6 +1495,8 @@ static inline int cpu_mmu_index (CPUPPCState *env)
 #define SPR_MPC_MD_EPN        (0x30B)
 #define SPR_PERFC             (0x30C)
 #define SPR_MPC_MD_TWB        (0x30C)
+#define SPR_BOOK3S_SIAR       (0x30C)
+#define SPR_BOOK3S_SDAR       (0x30D)
 #define SPR_PERFD             (0x30D)
 #define SPR_MPC_MD_TWC        (0x30D)
 #define SPR_PERFE             (0x30E)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9974b10..ead69fa 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
 
 #ifdef TARGET_PPC64
+        if ((cpu->env.msr >> MSR_TS) & 3) {
+            for (i = 0; i < sizeof(env->tm_gpr)/sizeof(env->tm_gpr[0]); i++) {
+                kvm_set_one_reg(cs, KVM_REG_PPC_TM_GPR(i), &env->tm_gpr[i]);
+            }
+            for (i = 0; i < sizeof(env->tm_vsr)/sizeof(env->tm_vsr[0]); i++) {
+                kvm_set_one_reg(cs, KVM_REG_PPC_TM_VSR(i), &env->tm_vsr[i]);
+            }
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_CR, &env->tm_cr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_LR, &env->tm_lr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_CTR, &env->tm_ctr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_FPSCR, &env->tm_fpscr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_AMR, &env->tm_amr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_PPR, &env->tm_ppr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_VRSAVE, &env->tm_vrsave);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_VSCR, &env->tm_vscr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_DSCR, &env->tm_dscr);
+            kvm_set_one_reg(cs, KVM_REG_PPC_TM_TAR, &env->tm_tar);
+        }
+
         if (cap_papr) {
             if (kvm_put_vpa(cs) < 0) {
                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
@@ -1090,6 +1109,25 @@ int kvm_arch_get_registers(CPUState *cs)
         }
 
 #ifdef TARGET_PPC64
+        if ((cpu->env.msr >> MSR_TS) & 3) {
+            for (i = 0; i < sizeof(env->tm_gpr)/sizeof(env->tm_gpr[0]); i++) {
+                kvm_get_one_reg(cs, KVM_REG_PPC_TM_GPR(i), &env->tm_gpr[i]);
+            }
+            for (i = 0; i < sizeof(env->tm_vsr)/sizeof(env->tm_vsr[0]); i++) {
+                kvm_get_one_reg(cs, KVM_REG_PPC_TM_VSR(i), &env->tm_vsr[i]);
+            }
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_CR, &env->tm_cr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_LR, &env->tm_lr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_CTR, &env->tm_ctr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_FPSCR, &env->tm_fpscr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_AMR, &env->tm_amr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_PPR, &env->tm_ppr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_VRSAVE, &env->tm_vrsave);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_VSCR, &env->tm_vscr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_DSCR, &env->tm_dscr);
+            kvm_get_one_reg(cs, KVM_REG_PPC_TM_TAR, &env->tm_tar);
+        }
+
         if (cap_papr) {
             if (kvm_get_vpa(cs) < 0) {
                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 063b379..691071d 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -247,6 +247,37 @@ static const VMStateDescription vmstate_vsx = {
     },
 };
 
+#ifdef TARGET_PPC64
+/* Transactional memory state */
+static bool tm_needed(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+    return (cpu->env.msr >> MSR_TS) & 3;
+}
+
+static const VMStateDescription vmstate_tm = {
+    .name = "cpu/tm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINTTL_ARRAY(env.tm_gpr, PowerPCCPU, 32),
+        VMSTATE_AVR_ARRAY(env.tm_vsr, PowerPCCPU, 64),
+        VMSTATE_UINT64(env.tm_cr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_lr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_ctr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_fpscr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_amr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_ppr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_vrsave, PowerPCCPU),
+        VMSTATE_UINT32(env.tm_vscr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_dscr, PowerPCCPU),
+        VMSTATE_UINT64(env.tm_tar, PowerPCCPU),
+        VMSTATE_END_OF_LIST()
+    },
+};
+#endif
+
 static bool sr_needed(void *opaque)
 {
 #ifdef TARGET_PPC64
@@ -518,6 +549,9 @@ const VMStateDescription vmstate_ppc_cpu = {
             .needed = sr_needed,
         } , {
 #ifdef TARGET_PPC64
+            .vmsd = &vmstate_tm,
+            .needed = tm_needed,
+        } , {
             .vmsd = &vmstate_slb,
             .needed = slb_needed,
         } , {
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 1627bb0..4b20c29 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env)
                  SPR_NOACCESS, SPR_NOACCESS,
                  &spr_read_generic, SPR_NOACCESS,
                  0x80800000);
-    spr_register(env, SPR_VRSAVE, "SPR_VRSAVE",
-                 &spr_read_generic, &spr_write_generic,
-                 &spr_read_generic, &spr_write_generic,
-                 0x00000000);
-    spr_register(env, SPR_PPR, "PPR",
-                 &spr_read_generic, &spr_write_generic,
-                 &spr_read_generic, &spr_write_generic,
-                 0x00000000);
+    spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
+                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic,
+                     KVM_REG_PPC_VRSAVE, 0x00000000);
+    spr_register_kvm(env, SPR_PPR, "PPR",
+                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic,
+                     KVM_REG_PPC_PPR, 0x00000000);
+    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
+                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic,
+                     KVM_REG_PPC_SIAR, 0x00000000);
+    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
+                     &spr_read_generic, &spr_write_generic,
+                     &spr_read_generic, &spr_write_generic,
+                     KVM_REG_PPC_SDAR, 0x00000000);
     /* Logical partitionning */
     spr_register_kvm(env, SPR_LPCR, "LPCR",
                      SPR_NOACCESS, SPR_NOACCESS,
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexey Kardashevskiy]

This allows guests to have a different timebase origin from the host.

This is needed for migration, where a guest can migrate from one host
to another and the two hosts might have a different timebase origin.
However, the timebase seen by the guest must not go backwards, and
should go forwards only by a small amount corresponding to the time
taken for the migration.

This is only supported for recent POWER hardware which has the TBU40
(timebase upper 40 bits) register. That includes POWER6, 7, 8 but not
970.

This adds kvm_access_one_reg() to access a special register which is not
in env->spr.

The feature must be present in the host kernel.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v4:
* made it per machine timebase offser rather than per CPU

v3:
* kvm_access_one_reg moved out to a separate patch
* tb_offset and host_timebase were replaced with guest_timebase as
the destionation does not really care of offset on the source

v2:
* bumped the vmstate_ppc_cpu version
* defined version for the env.tb_env field
---
 hw/ppc/ppc.c           | 120 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c         |   3 +-
 include/hw/ppc/spapr.h |   2 +
 target-ppc/cpu-qom.h   |  16 +++++++
 target-ppc/kvm.c       |   5 +++
 target-ppc/machine.c   |   4 +-
 trace-events           |   3 ++
 7 files changed, 151 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 9c2a132..b51db1b 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -29,9 +29,11 @@
 #include "sysemu/cpus.h"
 #include "hw/timer/m48t59.h"
 #include "qemu/log.h"
+#include "qemu/error-report.h"
 #include "hw/loader.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
+#include "trace.h"
 
 //#define PPC_DEBUG_IRQ
 //#define PPC_DEBUG_TB
@@ -797,6 +799,124 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
     cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
 }
 
+/*
+ * Calculate timebase on the destination side of migration
+ *
+ * We calculate new timebase offset as shown below:
+ * 1) Gtb2 = Gtb1 + max(tod2 - tod1, 0)
+ *    Gtb2 = tb2 + off2
+ * 2) tb2 + off2 = Gtb1 + max(tod2 - tod1, 0)
+ * 3) off2 = Gtb1 - tb2 + max(tod2 - tod1, 0)
+ *
+ * where:
+ * Gtb2 - destination guest timebase
+ * tb2 - destination host timebase
+ * off2 - destination timebase offset
+ * tod2 - destination time of the day
+ * Gtb1 - source guest timebase
+ * tod1 - source time of the day
+ *
+ * The result we want is in @off2
+ *
+ * Two conditions must be met for @off2:
+ * 1) off2 must be multiple of 2^24 ticks as it will be set via TBU40 SPR
+ * 2) Gtb2 >= Gtb1
+ */
+static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
+{
+    uint64_t tb2, tod2;
+    int64_t off2;
+    int ratio = tb->freq / 1000000;
+    struct timeval tv;
+
+    tb2 = cpu_get_real_ticks();
+    gettimeofday(&tv, NULL);
+    tod2 = tv.tv_sec * 1000000 + tv.tv_usec;
+
+    off2 = tb->guest_timebase - tb2;
+    if ((tod2 > tb->time_of_the_day) &&
+        (tod2 - tb->time_of_the_day < 1000000)) {
+        off2 += (tod2 - tb->time_of_the_day) * ratio;
+    }
+    off2 = ROUND_UP(off2, 1 << 24);
+
+    return off2;
+}
+
+static void timebase_pre_save(void *opaque)
+{
+    PPCTimebaseOffset *tb = opaque;
+    struct timeval tv;
+    uint64_t ticks = cpu_get_real_ticks();
+    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
+
+    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
+
+    gettimeofday(&tv, NULL);
+    tb->time_of_the_day = tv.tv_sec * 1000000 + tv.tv_usec;
+    /*
+     * tb_offset is only expected to be changed by migration so
+     * there is no need to update it from KVM here
+     */
+    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
+}
+
+static int timebase_pre_load(void *opaque)
+{
+    PPCTimebaseOffset *tb = opaque;
+    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
+
+    if (!first_ppc_cpu->env.tb_env) {
+        error_report("No timebase object");
+        return -1;
+    }
+
+    /* Initialize @freq so it will be checked during migration */
+    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
+
+    return 0;
+}
+
+static int timebase_post_load(void *opaque, int version_id)
+{
+    PPCTimebaseOffset *tb = opaque;
+    CPUState *cpu;
+    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
+    int64_t tb_offset = first_ppc_cpu->env.tb_env->tb_offset;
+    int64_t tb_offset2 = cpu_ppc_adjust_tb_offset(tb);
+
+    trace_ppc_tb_adjust(tb_offset, tb_offset2,
+                        (int64_t)tb_offset2 - tb_offset,
+                        ((int64_t)tb_offset2 - tb_offset) / tb->freq);
+
+    CPU_FOREACH(cpu) {
+        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
+        if (!pcpu->env.tb_env) {
+            error_report("No timebase object");
+            return -1;
+        }
+        pcpu->env.tb_env->tb_offset = tb_offset2;
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_ppc_timebase = {
+    .name = "timebase",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = timebase_pre_save,
+    .pre_load = timebase_pre_load,
+    .post_load = timebase_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32_EQUAL(freq, PPCTimebaseOffset),
+        VMSTATE_UINT64(guest_timebase, PPCTimebaseOffset),
+        VMSTATE_UINT64(time_of_the_day, PPCTimebaseOffset),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 /* Set up (once) timebase frequency (in Hz) */
 clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
 {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 451c473..9b0681b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -818,7 +818,7 @@ static int spapr_vga_init(PCIBus *pci_bus)
 
 static const VMStateDescription vmstate_spapr = {
     .name = "spapr",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
@@ -827,6 +827,7 @@ static const VMStateDescription vmstate_spapr = {
         /* RTC offset */
         VMSTATE_UINT64(rtc_offset, sPAPREnvironment),
 
+        VMSTATE_PPC_TIMEBASE(tb_offset, sPAPREnvironment),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5fdac1e..dbaa439 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -3,6 +3,7 @@
 
 #include "sysemu/dma.h"
 #include "hw/ppc/xics.h"
+#include "cpu-qom.h"
 
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
@@ -29,6 +30,7 @@ typedef struct sPAPREnvironment {
     target_ulong entry_point;
     uint32_t next_irq;
     uint64_t rtc_offset;
+    struct PPCTimebaseOffset tb_offset;
     bool has_graphics;
 
     uint32_t epow_irq;
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..1fc91e2 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -120,6 +120,22 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
                                int cpuid, void *opaque);
 #ifndef CONFIG_USER_ONLY
 extern const struct VMStateDescription vmstate_ppc_cpu;
+
+typedef struct PPCTimebaseOffset {
+    uint32_t freq;
+    uint64_t guest_timebase;
+    uint64_t time_of_the_day;
+} PPCTimebaseOffset;
+
+extern const struct VMStateDescription vmstate_ppc_timebase;
+
+#define VMSTATE_PPC_TIMEBASE(_field, _state) {                              \
+    .name       = (stringify(_field)),                                      \
+    .size       = sizeof(PPCTimebaseOffset),                                \
+    .vmsd       = &vmstate_ppc_timebase,                                    \
+    .flags      = VMS_STRUCT,                                               \
+    .offset     = vmstate_offset_value(_state, _field, PPCTimebaseOffset),  \
+}
 #endif
 
 #endif
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index ead69fa..a80faba 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -35,6 +35,7 @@
 #include "hw/sysbus.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
+#include "hw/ppc/ppc.h"
 #include "sysemu/watchdog.h"
 #include "trace.h"
 
@@ -890,6 +891,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
             }
         }
+
+        kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
 #endif /* TARGET_PPC64 */
     }
 
@@ -1133,6 +1136,8 @@ int kvm_arch_get_registers(CPUState *cs)
                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
             }
         }
+
+        kvm_get_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
 #endif
     }
 
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 691071d..8c59cbb 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -1,5 +1,6 @@
 #include "hw/hw.h"
 #include "hw/boards.h"
+#include "hw/ppc/ppc.h"
 #include "sysemu/kvm.h"
 #include "helper_regs.h"
 
@@ -495,7 +496,7 @@ static const VMStateDescription vmstate_tlbmas = {
 
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
-    .version_id = 5,
+    .version_id = 6,
     .minimum_version_id = 5,
     .minimum_version_id_old = 4,
     .load_state_old = cpu_load_old,
@@ -532,6 +533,7 @@ const VMStateDescription vmstate_ppc_cpu = {
         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
         VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
         VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
+
         VMSTATE_END_OF_LIST()
     },
     .subsections = (VMStateSubsection []) {
diff --git a/trace-events b/trace-events
index 3df3f32..c284d09 100644
--- a/trace-events
+++ b/trace-events
@@ -1161,6 +1161,9 @@ spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liob
 spapr_iommu_xlate(uint64_t liobn, uint64_t ioba, uint64_t tce, unsigned perm, unsigned pgsize) "liobn=%"PRIx64" 0x%"PRIx64" -> 0x%"PRIx64" perm=%u mask=%x"
 spapr_iommu_new_table(uint64_t liobn, void *tcet, void *table, int fd) "liobn=%"PRIx64" tcet=%p table=%p fd=%d"
 
+# hw/ppc/ppc.c
+ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t seconds) "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64" (%"PRId64"s)"
+
 # util/hbitmap.c
 hbitmap_iter_skip_words(const void *hb, void *hbi, uint64_t pos, unsigned long cur) "hb %p hbi %p pos %"PRId64" cur 0x%lx"
 hbitmap_reset(void *hb, uint64_t start, uint64_t count, uint64_t sbit, uint64_t ebit) "hb %p items %"PRIu64",%"PRIu64" bits %"PRIu64"..%"PRIu64
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexander Graf]

On 03.04.14 15:14, Alexey Kardashevskiy wrote:
> This advertises Data Address Breakpoint Register Extension (DABRX) to
> the guest via hyperrtas list and enables it to migrate.

Do all CPUs we support (970 anyone) have DABRX support? Also who handles 
this hcall in the TCG case? What about older host kernels that don't 
support xdabr yet? What about PR KVM?


Alex

>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/ppc/spapr.c              | 1 +
>   target-ppc/translate_init.c | 4 ++++
>   2 files changed, 5 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a11e121..451c473 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>       uint32_t start_prop = cpu_to_be32(initrd_base);
>       uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>       char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
> +        "\0hcall-xdabr"
>           "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
>       char qemu_hypertas_prop[] = "hcall-memop1";
>       uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index d07e186..1627bb0 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>                        SPR_NOACCESS, SPR_NOACCESS,
>                        &spr_read_generic, &spr_write_generic,
>                        KVM_REG_PPC_PMC6, 0x00000000);
> +    spr_register_kvm(env, SPR_DABRX, "DABRX",
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     KVM_REG_PPC_DABRX, 0x00000000);
>   #endif /* !CONFIG_USER_ONLY */
>       gen_spr_amr(env);
>       /* XXX : not implemented */

Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Alexander Graf]

On 03.04.14 15:14, Alexey Kardashevskiy wrote:
> This enabled KVM and migration support for a number of POWER8 registers:
> * Program Prioirty Register (PPR)

Typo

> * Sampled Instruction Address Register (SIAR)
> * Sampled Data Address Register (SDAR)
> * Vector Registers Save Register (VRSAVE)
>
> This enables save/restore of transactional state if MSR_TS is set
> in the MSR.
>
> This adds new POWER8 registers support required for migration.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Tom, please have a look through this as well :).

> ---
>   target-ppc/cpu.h            | 18 ++++++++++++++++++
>   target-ppc/kvm.c            | 38 ++++++++++++++++++++++++++++++++++++++
>   target-ppc/machine.c        | 34 ++++++++++++++++++++++++++++++++++
>   target-ppc/translate_init.c | 24 ++++++++++++++++--------
>   4 files changed, 106 insertions(+), 8 deletions(-)
>
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2719c08..2b476b4 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -426,6 +426,8 @@ struct ppc_slb_t {
>   #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
>   #define MSR_SHV  60 /* hypervisor state                               hflags */
> +#define MSR_TS   33 /* Transactional state, 2 bits (Book3s)                  */

2 bits means you want to add another define or at least a comment at bit 
34. I find it rather counterintuitive to declare bit 33 MSR_TS as 2-bit 
wide too - you'd expect (3 << MSR_TS) gives you the right mask, but then 
it'd have to be 34, no?

> +#define MSR_TM   32 /* Transactional Memory Available (Book3s)               */
>   #define MSR_CM   31 /* Computation mode for BookE                     hflags */
>   #define MSR_ICM  30 /* Interrupt computation mode for BookE                  */
>   #define MSR_THV  29 /* hypervisor state for 32 bits PowerPC           hflags */
> @@ -1081,6 +1083,20 @@ struct CPUPPCState {
>        */
>       uint8_t fit_period[4];
>       uint8_t wdt_period[4];
> +
> +    /* Transactional memory state migration */

No migration, we're laying the foundation for TM emulation.

> +    target_ulong tm_gpr[32];
> +    ppc_avr_t tm_vsr[64];
> +    uint64_t tm_cr;
> +    uint64_t tm_lr;
> +    uint64_t tm_ctr;
> +    uint64_t tm_fpscr;
> +    uint64_t tm_amr;
> +    uint64_t tm_ppr;
> +    uint64_t tm_vrsave;
> +    uint32_t tm_vscr;
> +    uint64_t tm_dscr;
> +    uint64_t tm_tar;
>   };
>   
>   #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
> @@ -1479,6 +1495,8 @@ static inline int cpu_mmu_index (CPUPPCState *env)
>   #define SPR_MPC_MD_EPN        (0x30B)
>   #define SPR_PERFC             (0x30C)
>   #define SPR_MPC_MD_TWB        (0x30C)
> +#define SPR_BOOK3S_SIAR       (0x30C)
> +#define SPR_BOOK3S_SDAR       (0x30D)
>   #define SPR_PERFD             (0x30D)
>   #define SPR_MPC_MD_TWC        (0x30D)
>   #define SPR_PERFE             (0x30E)
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9974b10..ead69fa 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>           }
>   
>   #ifdef TARGET_PPC64
> +        if ((cpu->env.msr >> MSR_TS) & 3) {

Ah, it works because you're shifting the other direction. That works. 
How about we just introduce an msr_ts() helper similar to the other 
lower case helpers to make this obvious?

> +            for (i = 0; i < sizeof(env->tm_gpr)/sizeof(env->tm_gpr[0]); i++) {

ARRAY_SIZE?


Alex

> +                kvm_set_one_reg(cs, KVM_REG_PPC_TM_GPR(i), &env->tm_gpr[i]);
> +            }
> +            for (i = 0; i < sizeof(env->tm_vsr)/sizeof(env->tm_vsr[0]); i++) {
> +                kvm_set_one_reg(cs, KVM_REG_PPC_TM_VSR(i), &env->tm_vsr[i]);
> +            }
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_CR, &env->tm_cr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_LR, &env->tm_lr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_CTR, &env->tm_ctr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_FPSCR, &env->tm_fpscr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_AMR, &env->tm_amr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_PPR, &env->tm_ppr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_VRSAVE, &env->tm_vrsave);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_VSCR, &env->tm_vscr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_DSCR, &env->tm_dscr);
> +            kvm_set_one_reg(cs, KVM_REG_PPC_TM_TAR, &env->tm_tar);
> +        }
> +
>           if (cap_papr) {
>               if (kvm_put_vpa(cs) < 0) {
>                   DPRINTF("Warning: Unable to set VPA information to KVM\n");
> @@ -1090,6 +1109,25 @@ int kvm_arch_get_registers(CPUState *cs)
>           }
>   
>   #ifdef TARGET_PPC64
> +        if ((cpu->env.msr >> MSR_TS) & 3) {
> +            for (i = 0; i < sizeof(env->tm_gpr)/sizeof(env->tm_gpr[0]); i++) {
> +                kvm_get_one_reg(cs, KVM_REG_PPC_TM_GPR(i), &env->tm_gpr[i]);
> +            }
> +            for (i = 0; i < sizeof(env->tm_vsr)/sizeof(env->tm_vsr[0]); i++) {
> +                kvm_get_one_reg(cs, KVM_REG_PPC_TM_VSR(i), &env->tm_vsr[i]);
> +            }
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_CR, &env->tm_cr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_LR, &env->tm_lr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_CTR, &env->tm_ctr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_FPSCR, &env->tm_fpscr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_AMR, &env->tm_amr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_PPR, &env->tm_ppr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_VRSAVE, &env->tm_vrsave);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_VSCR, &env->tm_vscr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_DSCR, &env->tm_dscr);
> +            kvm_get_one_reg(cs, KVM_REG_PPC_TM_TAR, &env->tm_tar);
> +        }
> +
>           if (cap_papr) {
>               if (kvm_get_vpa(cs) < 0) {
>                   DPRINTF("Warning: Unable to get VPA information from KVM\n");
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 063b379..691071d 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -247,6 +247,37 @@ static const VMStateDescription vmstate_vsx = {
>       },
>   };
>   
> +#ifdef TARGET_PPC64
> +/* Transactional memory state */
> +static bool tm_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    return (cpu->env.msr >> MSR_TS) & 3;
> +}
> +
> +static const VMStateDescription vmstate_tm = {
> +    .name = "cpu/tm",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINTTL_ARRAY(env.tm_gpr, PowerPCCPU, 32),
> +        VMSTATE_AVR_ARRAY(env.tm_vsr, PowerPCCPU, 64),
> +        VMSTATE_UINT64(env.tm_cr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_lr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_ctr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_fpscr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_amr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_ppr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_vrsave, PowerPCCPU),
> +        VMSTATE_UINT32(env.tm_vscr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_dscr, PowerPCCPU),
> +        VMSTATE_UINT64(env.tm_tar, PowerPCCPU),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +#endif
> +
>   static bool sr_needed(void *opaque)
>   {
>   #ifdef TARGET_PPC64
> @@ -518,6 +549,9 @@ const VMStateDescription vmstate_ppc_cpu = {
>               .needed = sr_needed,
>           } , {
>   #ifdef TARGET_PPC64
> +            .vmsd = &vmstate_tm,
> +            .needed = tm_needed,
> +        } , {
>               .vmsd = &vmstate_slb,
>               .needed = slb_needed,
>           } , {
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 1627bb0..4b20c29 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env)
>                    SPR_NOACCESS, SPR_NOACCESS,
>                    &spr_read_generic, SPR_NOACCESS,
>                    0x80800000);
> -    spr_register(env, SPR_VRSAVE, "SPR_VRSAVE",
> -                 &spr_read_generic, &spr_write_generic,
> -                 &spr_read_generic, &spr_write_generic,
> -                 0x00000000);
> -    spr_register(env, SPR_PPR, "PPR",
> -                 &spr_read_generic, &spr_write_generic,
> -                 &spr_read_generic, &spr_write_generic,
> -                 0x00000000);
> +    spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_VRSAVE, 0x00000000);
> +    spr_register_kvm(env, SPR_PPR, "PPR",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_PPR, 0x00000000);
> +    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_SIAR, 0x00000000);
> +    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
> +                     &spr_read_generic, &spr_write_generic,
> +                     &spr_read_generic, &spr_write_generic,
> +                     KVM_REG_PPC_SDAR, 0x00000000);
>       /* Logical partitionning */
>       spr_register_kvm(env, SPR_LPCR, "LPCR",
>                        SPR_NOACCESS, SPR_NOACCESS,

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Tom Musta]

On 4/3/2014 8:14 AM, Alexey Kardashevskiy wrote:
> This advertises Data Address Breakpoint Register Extension (DABRX) to
> the guest via hyperrtas list and enables it to migrate.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/ppc/spapr.c              | 1 +
>  target-ppc/translate_init.c | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a11e121..451c473 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>      uint32_t start_prop = cpu_to_be32(initrd_base);
>      uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>      char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
> +        "\0hcall-xdabr"
>          "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
>      char qemu_hypertas_prop[] = "hcall-memop1";
>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};


It isn't clear to me what is enabled with this.  Alexey: can you provide a little explanation
of what adding this string actually does?  And I assume the spelling of "xdabr" (versus "dabrx")
is intentional?

> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index d07e186..1627bb0 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>                       SPR_NOACCESS, SPR_NOACCESS,
>                       &spr_read_generic, &spr_write_generic,
>                       KVM_REG_PPC_PMC6, 0x00000000);
> +    spr_register_kvm(env, SPR_DABRX, "DABRX",
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     SPR_NOACCESS, SPR_NOACCESS,
> +                     KVM_REG_PPC_DABRX, 0x00000000);
>  #endif /* !CONFIG_USER_ONLY */
>      gen_spr_amr(env);
>      /* XXX : not implemented */
> 

I see a problem with this and it is caused by some of the P8 code that I had added a while back.
The P8 init code  (init_proc_POWER8) calls this init_proc_POWER7 routine.  So by adding DABRX
to the P7 code means P8 gets it for free.  Unfortunately, P8 doesn't have DABRX ... it supports
the new debug facilities (DAWR[X]).  The DABR and IABR registers are in the same boat.   I think
the init_proc_POWER8 code needs to become a self-sufficienct version.

I can fix and send to you or you can fix yourself ... let me know.

Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Tom Musta]

On 4/3/2014 8:33 AM, Alexander Graf wrote:
> 
> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>> This enabled KVM and migration support for a number of POWER8 registers:

<snip>

> 
> Tom, please have a look through this as well :).
> 
>> ---

<snip>

>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -426,6 +426,8 @@ struct ppc_slb_t {
>>   #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
>>   #define MSR_SHV  60 /* hypervisor state                               hflags */
>> +#define MSR_TS   33 /* Transactional state, 2 bits (Book3s)                  */
> 
> 2 bits means you want to add another define or at least a comment at bit 34. I find it rather counterintuitive to declare bit 33 MSR_TS as 2-bit wide too - you'd expect (3 << MSR_TS) gives you the right mask, but then it'd have to be 34, no?

Is this better?

    #define MSR_TS0 34
    #define MSR_TS1 33

You should also add a decoder:

    #define msr_ts ((env->msr >> MSR_TS1) & 3)

> 
>> +    target_ulong tm_gpr[32];
>> +    ppc_avr_t tm_vsr[64];
>> +    uint64_t tm_cr;
>> +    uint64_t tm_lr;
>> +    uint64_t tm_ctr;
>> +    uint64_t tm_fpscr;
>> +    uint64_t tm_amr;
>> +    uint64_t tm_ppr;
>> +    uint64_t tm_vrsave;
>> +    uint32_t tm_vscr;
>> +    uint64_t tm_dscr;
>> +    uint64_t tm_tar;
>>   };

If vscr is declared as 32 bits, should CR and VRSAVE also be 32-bits?

>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 9974b10..ead69fa 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>           }
>>     #ifdef TARGET_PPC64
>> +        if ((cpu->env.msr >> MSR_TS) & 3) {
> 
> Ah, it works because you're shifting the other direction. That works. How about we just introduce an msr_ts() helper similar to the other lower case helpers to make this obvious?
> 

Agreed.


>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index 1627bb0..4b20c29 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>                    SPR_NOACCESS, SPR_NOACCESS,
>>                    &spr_read_generic, SPR_NOACCESS,
>>                    0x80800000);
>> -    spr_register(env, SPR_VRSAVE, "SPR_VRSAVE",
>> -                 &spr_read_generic, &spr_write_generic,
>> -                 &spr_read_generic, &spr_write_generic,
>> -                 0x00000000);
>> -    spr_register(env, SPR_PPR, "PPR",
>> -                 &spr_read_generic, &spr_write_generic,
>> -                 &spr_read_generic, &spr_write_generic,
>> -                 0x00000000);
>> +    spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     KVM_REG_PPC_VRSAVE, 0x00000000);
>> +    spr_register_kvm(env, SPR_PPR, "PPR",
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     KVM_REG_PPC_PPR, 0x00000000);
>> +    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     KVM_REG_PPC_SIAR, 0x00000000);
>> +    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     &spr_read_generic, &spr_write_generic,
>> +                     KVM_REG_PPC_SDAR, 0x00000000);
>>       /* Logical partitionning */
>>       spr_register_kvm(env, SPR_LPCR, "LPCR",
>>                        SPR_NOACCESS, SPR_NOACCESS,
> 

These need to go into P8 as well? (see my comment for patch 2).

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexey Kardashevskiy]

On 04/04/2014 05:42 AM, Tom Musta wrote:
> On 4/3/2014 8:14 AM, Alexey Kardashevskiy wrote:
>> This advertises Data Address Breakpoint Register Extension (DABRX) to
>> the guest via hyperrtas list and enables it to migrate.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  hw/ppc/spapr.c              | 1 +
>>  target-ppc/translate_init.c | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index a11e121..451c473 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>      uint32_t start_prop = cpu_to_be32(initrd_base);
>>      uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>>      char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
>> +        "\0hcall-xdabr"
>>          "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
>>      char qemu_hypertas_prop[] = "hcall-memop1";
>>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
> 
> 
> It isn't clear to me what is enabled with this.  Alexey: can you provide a little explanation
> of what adding this string actually does?  

Yes, I will. I just got this patch from someone else and did not really try
to understand it :)

> And I assume the spelling of "xdabr" (versus "dabrx")
> is intentional?

Sure:

SPAPR 2.7:
H_SET_XDABR / 14.5.4.3.7 0x134 Normal If Extended DABR
option is implemented hcall-xdabr



>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index d07e186..1627bb0 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>                       SPR_NOACCESS, SPR_NOACCESS,
>>                       &spr_read_generic, &spr_write_generic,
>>                       KVM_REG_PPC_PMC6, 0x00000000);
>> +    spr_register_kvm(env, SPR_DABRX, "DABRX",
>> +                     SPR_NOACCESS, SPR_NOACCESS,
>> +                     SPR_NOACCESS, SPR_NOACCESS,
>> +                     KVM_REG_PPC_DABRX, 0x00000000);
>>  #endif /* !CONFIG_USER_ONLY */
>>      gen_spr_amr(env);
>>      /* XXX : not implemented */
>>
> 
> I see a problem with this and it is caused by some of the P8 code that I had added a while back.
> The P8 init code  (init_proc_POWER8) calls this init_proc_POWER7 routine.  So by adding DABRX
> to the P7 code means P8 gets it for free.  Unfortunately, P8 doesn't have DABRX ... it supports
> the new debug facilities (DAWR[X]).  The DABR and IABR registers are in the same boat.   I think
> the init_proc_POWER8 code needs to become a self-sufficienct version.

> 
> I can fix and send to you or you can fix yourself ... let me know.

Since I'll be touching this code soon, I can make copy content of
init_proc_POWER7 to init_proc_POWER8 and remove DABRX if this is what you
mean. Ok?



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexey Kardashevskiy]

On 04/04/2014 12:19 AM, Alexander Graf wrote:
> 
> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>> This advertises Data Address Breakpoint Register Extension (DABRX) to
>> the guest via hyperrtas list and enables it to migrate.
> 
> Do all CPUs we support (970 anyone) have DABRX support?

970MP and 970FX do. Support them too? Who cares? :)

> Also who handles this hcall in the TCG case?

Good point...

> What about older host kernels that don't
> support xdabr yet? 

They will ignore FW_FEATURE_XDABR, no?

> What about PR KVM?

Oh. Nothing. And we do not want to make this "hcall-xdabr" conditional,
right? Drop the whole patch? I am really confused now.


> 
> 
> Alex
> 
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/ppc/spapr.c              | 1 +
>>   target-ppc/translate_init.c | 4 ++++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index a11e121..451c473 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>       uint32_t start_prop = cpu_to_be32(initrd_base);
>>       uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>>       char hypertas_prop[] =
>> "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
>> +        "\0hcall-xdabr"
>>          
>> "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
>>       char qemu_hypertas_prop[] = "hcall-memop1";
>>       uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index d07e186..1627bb0 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>                        SPR_NOACCESS, SPR_NOACCESS,
>>                        &spr_read_generic, &spr_write_generic,
>>                        KVM_REG_PPC_PMC6, 0x00000000);
>> +    spr_register_kvm(env, SPR_DABRX, "DABRX",
>> +                     SPR_NOACCESS, SPR_NOACCESS,
>> +                     SPR_NOACCESS, SPR_NOACCESS,
>> +                     KVM_REG_PPC_DABRX, 0x00000000);
>>   #endif /* !CONFIG_USER_ONLY */
>>       gen_spr_amr(env);
>>       /* XXX : not implemented */
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Alexey Kardashevskiy]

On 04/04/2014 06:12 AM, Tom Musta wrote:
> On 4/3/2014 8:33 AM, Alexander Graf wrote:
>>
>> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>>> This enabled KVM and migration support for a number of POWER8 registers:
> 
> <snip>
> 
>>
>> Tom, please have a look through this as well :).
>>
>>> ---
> 
> <snip>
> 
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -426,6 +426,8 @@ struct ppc_slb_t {
>>>   #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>>>   #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
>>>   #define MSR_SHV  60 /* hypervisor state                               hflags */
>>> +#define MSR_TS   33 /* Transactional state, 2 bits (Book3s)                  */
>>
>> 2 bits means you want to add another define or at least a comment at bit 34. I find it rather counterintuitive to declare bit 33 MSR_TS as 2-bit wide too - you'd expect (3 << MSR_TS) gives you the right mask, but then it'd have to be 34, no?
> 
> Is this better?
> 
>     #define MSR_TS0 34
>     #define MSR_TS1 33
> 
> You should also add a decoder:
> 
>     #define msr_ts ((env->msr >> MSR_TS1) & 3)

Yes, this is better. Thanks!


>>
>>> +    target_ulong tm_gpr[32];
>>> +    ppc_avr_t tm_vsr[64];
>>> +    uint64_t tm_cr;
>>> +    uint64_t tm_lr;
>>> +    uint64_t tm_ctr;
>>> +    uint64_t tm_fpscr;
>>> +    uint64_t tm_amr;
>>> +    uint64_t tm_ppr;
>>> +    uint64_t tm_vrsave;
>>> +    uint32_t tm_vscr;
>>> +    uint64_t tm_dscr;
>>> +    uint64_t tm_tar;
>>>   };
> 
> If vscr is declared as 32 bits, should CR and VRSAVE also be 32-bits?

Nope.

linux-headers/asm-powerpc/kvm.h:
#define KVM_REG_PPC_TM_CR   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x60)
#define KVM_REG_PPC_TM_LR   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x61)
#define KVM_REG_PPC_TM_CTR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x62)
#define KVM_REG_PPC_TM_FPSCR    (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x63)
#define KVM_REG_PPC_TM_AMR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x64)
#define KVM_REG_PPC_TM_PPR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x65)
#define KVM_REG_PPC_TM_VRSAVE   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x66)
#define KVM_REG_PPC_TM_VSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U32 | 0x67)
#define KVM_REG_PPC_TM_DSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x68)
#define KVM_REG_PPC_TM_TAR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x69)

For the reason unknown (Paul is not in the office to ask) all TM-related
registers are defined as 64bit and only VSCR is 32bit. And this is in the
host kernel already.


>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 9974b10..ead69fa 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>>           }
>>>     #ifdef TARGET_PPC64
>>> +        if ((cpu->env.msr >> MSR_TS) & 3) {
>>
>> Ah, it works because you're shifting the other direction. That works. How about we just introduce an msr_ts() helper similar to the other lower case helpers to make this obvious?
>>
> 
> Agreed.
> 
> 
>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>> index 1627bb0..4b20c29 100644
>>> --- a/target-ppc/translate_init.c
>>> +++ b/target-ppc/translate_init.c
>>> @@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>>                    SPR_NOACCESS, SPR_NOACCESS,
>>>                    &spr_read_generic, SPR_NOACCESS,
>>>                    0x80800000);
>>> -    spr_register(env, SPR_VRSAVE, "SPR_VRSAVE",
>>> -                 &spr_read_generic, &spr_write_generic,
>>> -                 &spr_read_generic, &spr_write_generic,
>>> -                 0x00000000);
>>> -    spr_register(env, SPR_PPR, "PPR",
>>> -                 &spr_read_generic, &spr_write_generic,
>>> -                 &spr_read_generic, &spr_write_generic,
>>> -                 0x00000000);
>>> +    spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     KVM_REG_PPC_VRSAVE, 0x00000000);
>>> +    spr_register_kvm(env, SPR_PPR, "PPR",
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     KVM_REG_PPC_PPR, 0x00000000);
>>> +    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     KVM_REG_PPC_SIAR, 0x00000000);
>>> +    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     &spr_read_generic, &spr_write_generic,
>>> +                     KVM_REG_PPC_SDAR, 0x00000000);
>>>       /* Logical partitionning */
>>>       spr_register_kvm(env, SPR_LPCR, "LPCR",
>>>                        SPR_NOACCESS, SPR_NOACCESS,
>>
> 
> These need to go into P8 as well? (see my comment for patch 2).

Yes. VRSAVE, SIAR, SDAR are even defined for 970 (Alex, should I add them
to 970 definitions?), PPR is not defined in any 970 spec but is in 2.04..2.07.


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexander Graf]

On 04/04/2014 08:13 AM, Alexey Kardashevskiy wrote:
> On 04/04/2014 12:19 AM, Alexander Graf wrote:
>> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>>> This advertises Data Address Breakpoint Register Extension (DABRX) to
>>> the guest via hyperrtas list and enables it to migrate.
>> Do all CPUs we support (970 anyone) have DABRX support?
> 970MP and 970FX do. Support them too? Who cares? :)

Well, we do support running KVM on these and for KVM we default to -cpu 
host, so they're important to keep in mind.

>
>> Also who handles this hcall in the TCG case?
> Good point...
>
>> What about older host kernels that don't
>> support xdabr yet?
> They will ignore FW_FEATURE_XDABR, no?

How does a host kernel ignore anything here? We're telling the guest 
that we support an hcall without asking the host at all.

>
>> What about PR KVM?
> Oh. Nothing. And we do not want to make this "hcall-xdabr" conditional,
> right? Drop the whole patch? I am really confused now.

I think we should properly check whether we can handle this hcall and/or 
implement a handler for TCG and hosts which don't handle it themselves.


Alex

Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Alexander Graf]

On 04/04/2014 08:58 AM, Alexey Kardashevskiy wrote:
> On 04/04/2014 06:12 AM, Tom Musta wrote:
>> On 4/3/2014 8:33 AM, Alexander Graf wrote:
>>> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>>>> This enabled KVM and migration support for a number of POWER8 registers:
>> <snip>
>>
>>> Tom, please have a look through this as well :).
>>>
>>>> ---
>> <snip>
>>
>>>> --- a/target-ppc/cpu.h
>>>> +++ b/target-ppc/cpu.h
>>>> @@ -426,6 +426,8 @@ struct ppc_slb_t {
>>>>    #define MSR_TAG  62 /* Tag-active mode (POWERx ?)                            */
>>>>    #define MSR_ISF  61 /* Sixty-four-bit interrupt mode on 630                  */
>>>>    #define MSR_SHV  60 /* hypervisor state                               hflags */
>>>> +#define MSR_TS   33 /* Transactional state, 2 bits (Book3s)                  */
>>> 2 bits means you want to add another define or at least a comment at bit 34. I find it rather counterintuitive to declare bit 33 MSR_TS as 2-bit wide too - you'd expect (3 << MSR_TS) gives you the right mask, but then it'd have to be 34, no?
>> Is this better?
>>
>>      #define MSR_TS0 34
>>      #define MSR_TS1 33
>>
>> You should also add a decoder:
>>
>>      #define msr_ts ((env->msr >> MSR_TS1) & 3)
> Yes, this is better. Thanks!
>
>
>>>> +    target_ulong tm_gpr[32];
>>>> +    ppc_avr_t tm_vsr[64];
>>>> +    uint64_t tm_cr;
>>>> +    uint64_t tm_lr;
>>>> +    uint64_t tm_ctr;
>>>> +    uint64_t tm_fpscr;
>>>> +    uint64_t tm_amr;
>>>> +    uint64_t tm_ppr;
>>>> +    uint64_t tm_vrsave;
>>>> +    uint32_t tm_vscr;
>>>> +    uint64_t tm_dscr;
>>>> +    uint64_t tm_tar;
>>>>    };
>> If vscr is declared as 32 bits, should CR and VRSAVE also be 32-bits?
> Nope.
>
> linux-headers/asm-powerpc/kvm.h:
> #define KVM_REG_PPC_TM_CR   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x60)
> #define KVM_REG_PPC_TM_LR   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x61)
> #define KVM_REG_PPC_TM_CTR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x62)
> #define KVM_REG_PPC_TM_FPSCR    (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x63)
> #define KVM_REG_PPC_TM_AMR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x64)
> #define KVM_REG_PPC_TM_PPR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x65)
> #define KVM_REG_PPC_TM_VRSAVE   (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x66)
> #define KVM_REG_PPC_TM_VSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U32 | 0x67)
> #define KVM_REG_PPC_TM_DSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x68)
> #define KVM_REG_PPC_TM_TAR  (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x69)
>
> For the reason unknown (Paul is not in the office to ask) all TM-related
> registers are defined as 64bit and only VSCR is 32bit. And this is in the
> host kernel already.
>
>
>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>> index 9974b10..ead69fa 100644
>>>> --- a/target-ppc/kvm.c
>>>> +++ b/target-ppc/kvm.c
>>>> @@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>>>            }
>>>>      #ifdef TARGET_PPC64
>>>> +        if ((cpu->env.msr >> MSR_TS) & 3) {
>>> Ah, it works because you're shifting the other direction. That works. How about we just introduce an msr_ts() helper similar to the other lower case helpers to make this obvious?
>>>
>> Agreed.
>>
>>
>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>> index 1627bb0..4b20c29 100644
>>>> --- a/target-ppc/translate_init.c
>>>> +++ b/target-ppc/translate_init.c
>>>> @@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>>>                     SPR_NOACCESS, SPR_NOACCESS,
>>>>                     &spr_read_generic, SPR_NOACCESS,
>>>>                     0x80800000);
>>>> -    spr_register(env, SPR_VRSAVE, "SPR_VRSAVE",
>>>> -                 &spr_read_generic, &spr_write_generic,
>>>> -                 &spr_read_generic, &spr_write_generic,
>>>> -                 0x00000000);
>>>> -    spr_register(env, SPR_PPR, "PPR",
>>>> -                 &spr_read_generic, &spr_write_generic,
>>>> -                 &spr_read_generic, &spr_write_generic,
>>>> -                 0x00000000);
>>>> +    spr_register_kvm(env, SPR_VRSAVE, "VRSAVE",
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     KVM_REG_PPC_VRSAVE, 0x00000000);
>>>> +    spr_register_kvm(env, SPR_PPR, "PPR",
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     KVM_REG_PPC_PPR, 0x00000000);
>>>> +    spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR",
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     KVM_REG_PPC_SIAR, 0x00000000);
>>>> +    spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR",
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     &spr_read_generic, &spr_write_generic,
>>>> +                     KVM_REG_PPC_SDAR, 0x00000000);
>>>>        /* Logical partitionning */
>>>>        spr_register_kvm(env, SPR_LPCR, "LPCR",
>>>>                         SPR_NOACCESS, SPR_NOACCESS,
>> These need to go into P8 as well? (see my comment for patch 2).
> Yes. VRSAVE, SIAR, SDAR are even defined for 970 (Alex, should I add them
> to 970 definitions?), PPR is not defined in any 970 spec but is in 2.04..2.07.

Anything that 970 can work with should be handled in the 970 case as 
well, yes. Keep in mind that we also have POWER5 that can run PR KVM.

Maybe it'd be better to extract those into helper functions that we just 
call from the respective proc init functions? That way we at least stay 
consistent.


Alex

Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Tom Musta]

On 4/3/2014 7:51 PM, Alexey Kardashevskiy wrote:
> Since I'll be touching this code soon, I can make copy content of
> init_proc_POWER7 to init_proc_POWER8 and remove DABRX if this is what you
> mean. Ok?

Yes it is.  Thanks, Alexey.

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexander Graf]

On 03.04.14 15:14, Alexey Kardashevskiy wrote:
> This allows guests to have a different timebase origin from the host.
>
> This is needed for migration, where a guest can migrate from one host
> to another and the two hosts might have a different timebase origin.
> However, the timebase seen by the guest must not go backwards, and
> should go forwards only by a small amount corresponding to the time
> taken for the migration.
>
> This is only supported for recent POWER hardware which has the TBU40
> (timebase upper 40 bits) register. That includes POWER6, 7, 8 but not
> 970.
>
> This adds kvm_access_one_reg() to access a special register which is not
> in env->spr.
>
> The feature must be present in the host kernel.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v4:
> * made it per machine timebase offser rather than per CPU
>
> v3:
> * kvm_access_one_reg moved out to a separate patch
> * tb_offset and host_timebase were replaced with guest_timebase as
> the destionation does not really care of offset on the source
>
> v2:
> * bumped the vmstate_ppc_cpu version
> * defined version for the env.tb_env field
> ---
>   hw/ppc/ppc.c           | 120 +++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/spapr.c         |   3 +-
>   include/hw/ppc/spapr.h |   2 +
>   target-ppc/cpu-qom.h   |  16 +++++++
>   target-ppc/kvm.c       |   5 +++
>   target-ppc/machine.c   |   4 +-
>   trace-events           |   3 ++
>   7 files changed, 151 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 9c2a132..b51db1b 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -29,9 +29,11 @@
>   #include "sysemu/cpus.h"
>   #include "hw/timer/m48t59.h"
>   #include "qemu/log.h"
> +#include "qemu/error-report.h"
>   #include "hw/loader.h"
>   #include "sysemu/kvm.h"
>   #include "kvm_ppc.h"
> +#include "trace.h"
>   
>   //#define PPC_DEBUG_IRQ
>   //#define PPC_DEBUG_TB
> @@ -797,6 +799,124 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
>       cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>   }
>   
> +/*
> + * Calculate timebase on the destination side of migration
> + *
> + * We calculate new timebase offset as shown below:
> + * 1) Gtb2 = Gtb1 + max(tod2 - tod1, 0)
> + *    Gtb2 = tb2 + off2
> + * 2) tb2 + off2 = Gtb1 + max(tod2 - tod1, 0)
> + * 3) off2 = Gtb1 - tb2 + max(tod2 - tod1, 0)
> + *
> + * where:
> + * Gtb2 - destination guest timebase
> + * tb2 - destination host timebase
> + * off2 - destination timebase offset
> + * tod2 - destination time of the day
> + * Gtb1 - source guest timebase
> + * tod1 - source time of the day
> + *
> + * The result we want is in @off2
> + *
> + * Two conditions must be met for @off2:
> + * 1) off2 must be multiple of 2^24 ticks as it will be set via TBU40 SPR
> + * 2) Gtb2 >= Gtb1
> + */
> +static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
> +{
> +    uint64_t tb2, tod2;
> +    int64_t off2;
> +    int ratio = tb->freq / 1000000;
> +    struct timeval tv;
> +
> +    tb2 = cpu_get_real_ticks();
> +    gettimeofday(&tv, NULL);
> +    tod2 = tv.tv_sec * 1000000 + tv.tv_usec;
> +
> +    off2 = tb->guest_timebase - tb2;
> +    if ((tod2 > tb->time_of_the_day) &&
> +        (tod2 - tb->time_of_the_day < 1000000)) {
> +        off2 += (tod2 - tb->time_of_the_day) * ratio;
> +    }
> +    off2 = ROUND_UP(off2, 1 << 24);
> +
> +    return off2;
> +}

I *think* what you're trying to say here is that you want

assert(source_timebase_freq == timebase_freq);

migration_duration_ns = host_ns - source_host_ns;
guest_tb = source_guest_tb + ns_scaled_to_tb(min(0, migration_duration_ns);
kvm_set_guest_tb(guest_tb);
   -> kvm_set_one_reg(KVM_REG_PPC_TB_OFFSET, guest_tb - mftb());

But I honestly have not managed to read that from the code. Either this 
really is what you're trying to do and the code is just very hard to 
read (which means it needs to be written more easily) or you're doing 
something different which I don't understand.

We also designed the PPC_TB_OFFSET ONE_REG in a way that it always 
rounds up to its 40 bit granularity, so no need to do this in QEMU. In 
fact, we don't want to do it in QEMU in case there will be a more 
fine-grained SPR in the future.

And from all I understand the timebase frequency is now architecturally 
specified, so it won't change for newer cores, no? And if we migrate TCG 
guests it will be the same between two hosts.


Alex

> +
> +static void timebase_pre_save(void *opaque)
> +{
> +    PPCTimebaseOffset *tb = opaque;
> +    struct timeval tv;
> +    uint64_t ticks = cpu_get_real_ticks();
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +
> +    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
> +
> +    gettimeofday(&tv, NULL);
> +    tb->time_of_the_day = tv.tv_sec * 1000000 + tv.tv_usec;
> +    /*
> +     * tb_offset is only expected to be changed by migration so
> +     * there is no need to update it from KVM here
> +     */
> +    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> +}
> +
> +static int timebase_pre_load(void *opaque)
> +{
> +    PPCTimebaseOffset *tb = opaque;
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +
> +    if (!first_ppc_cpu->env.tb_env) {
> +        error_report("No timebase object");
> +        return -1;
> +    }
> +
> +    /* Initialize @freq so it will be checked during migration */
> +    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
> +
> +    return 0;
> +}
> +
> +static int timebase_post_load(void *opaque, int version_id)
> +{
> +    PPCTimebaseOffset *tb = opaque;
> +    CPUState *cpu;
> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> +    int64_t tb_offset = first_ppc_cpu->env.tb_env->tb_offset;
> +    int64_t tb_offset2 = cpu_ppc_adjust_tb_offset(tb);
> +
> +    trace_ppc_tb_adjust(tb_offset, tb_offset2,
> +                        (int64_t)tb_offset2 - tb_offset,
> +                        ((int64_t)tb_offset2 - tb_offset) / tb->freq);
> +
> +    CPU_FOREACH(cpu) {
> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> +        if (!pcpu->env.tb_env) {
> +            error_report("No timebase object");
> +            return -1;
> +        }
> +        pcpu->env.tb_env->tb_offset = tb_offset2;
> +    }
> +
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_ppc_timebase = {
> +    .name = "timebase",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = timebase_pre_save,
> +    .pre_load = timebase_pre_load,
> +    .post_load = timebase_post_load,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT32_EQUAL(freq, PPCTimebaseOffset),
> +        VMSTATE_UINT64(guest_timebase, PPCTimebaseOffset),
> +        VMSTATE_UINT64(time_of_the_day, PPCTimebaseOffset),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>   /* Set up (once) timebase frequency (in Hz) */
>   clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>   {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 451c473..9b0681b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -818,7 +818,7 @@ static int spapr_vga_init(PCIBus *pci_bus)
>   
>   static const VMStateDescription vmstate_spapr = {
>       .name = "spapr",
> -    .version_id = 1,
> +    .version_id = 2,
>       .minimum_version_id = 1,
>       .minimum_version_id_old = 1,
>       .fields      = (VMStateField []) {
> @@ -827,6 +827,7 @@ static const VMStateDescription vmstate_spapr = {
>           /* RTC offset */
>           VMSTATE_UINT64(rtc_offset, sPAPREnvironment),
>   
> +        VMSTATE_PPC_TIMEBASE(tb_offset, sPAPREnvironment),
>           VMSTATE_END_OF_LIST()
>       },
>   };
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5fdac1e..dbaa439 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -3,6 +3,7 @@
>   
>   #include "sysemu/dma.h"
>   #include "hw/ppc/xics.h"
> +#include "cpu-qom.h"
>   
>   struct VIOsPAPRBus;
>   struct sPAPRPHBState;
> @@ -29,6 +30,7 @@ typedef struct sPAPREnvironment {
>       target_ulong entry_point;
>       uint32_t next_irq;
>       uint64_t rtc_offset;
> +    struct PPCTimebaseOffset tb_offset;
>       bool has_graphics;
>   
>       uint32_t epow_irq;
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 47dc8e6..1fc91e2 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -120,6 +120,22 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>                                  int cpuid, void *opaque);
>   #ifndef CONFIG_USER_ONLY
>   extern const struct VMStateDescription vmstate_ppc_cpu;
> +
> +typedef struct PPCTimebaseOffset {
> +    uint32_t freq;
> +    uint64_t guest_timebase;
> +    uint64_t time_of_the_day;
> +} PPCTimebaseOffset;
> +
> +extern const struct VMStateDescription vmstate_ppc_timebase;
> +
> +#define VMSTATE_PPC_TIMEBASE(_field, _state) {                              \
> +    .name       = (stringify(_field)),                                      \
> +    .size       = sizeof(PPCTimebaseOffset),                                \
> +    .vmsd       = &vmstate_ppc_timebase,                                    \
> +    .flags      = VMS_STRUCT,                                               \
> +    .offset     = vmstate_offset_value(_state, _field, PPCTimebaseOffset),  \
> +}
>   #endif
>   
>   #endif
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index ead69fa..a80faba 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -35,6 +35,7 @@
>   #include "hw/sysbus.h"
>   #include "hw/ppc/spapr.h"
>   #include "hw/ppc/spapr_vio.h"
> +#include "hw/ppc/ppc.h"
>   #include "sysemu/watchdog.h"
>   #include "trace.h"
>   
> @@ -890,6 +891,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>                   DPRINTF("Warning: Unable to set VPA information to KVM\n");
>               }
>           }
> +
> +        kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
>   #endif /* TARGET_PPC64 */
>       }
>   
> @@ -1133,6 +1136,8 @@ int kvm_arch_get_registers(CPUState *cs)
>                   DPRINTF("Warning: Unable to get VPA information from KVM\n");
>               }
>           }
> +
> +        kvm_get_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &env->tb_env->tb_offset);
>   #endif
>       }
>   
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 691071d..8c59cbb 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -1,5 +1,6 @@
>   #include "hw/hw.h"
>   #include "hw/boards.h"
> +#include "hw/ppc/ppc.h"
>   #include "sysemu/kvm.h"
>   #include "helper_regs.h"
>   
> @@ -495,7 +496,7 @@ static const VMStateDescription vmstate_tlbmas = {
>   
>   const VMStateDescription vmstate_ppc_cpu = {
>       .name = "cpu",
> -    .version_id = 5,
> +    .version_id = 6,
>       .minimum_version_id = 5,
>       .minimum_version_id_old = 4,
>       .load_state_old = cpu_load_old,
> @@ -532,6 +533,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>           VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>           VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>           VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> +
>           VMSTATE_END_OF_LIST()
>       },
>       .subsections = (VMStateSubsection []) {
> diff --git a/trace-events b/trace-events
> index 3df3f32..c284d09 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1161,6 +1161,9 @@ spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liob
>   spapr_iommu_xlate(uint64_t liobn, uint64_t ioba, uint64_t tce, unsigned perm, unsigned pgsize) "liobn=%"PRIx64" 0x%"PRIx64" -> 0x%"PRIx64" perm=%u mask=%x"
>   spapr_iommu_new_table(uint64_t liobn, void *tcet, void *table, int fd) "liobn=%"PRIx64" tcet=%p table=%p fd=%d"
>   
> +# hw/ppc/ppc.c
> +ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t seconds) "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64" (%"PRId64"s)"
> +
>   # util/hbitmap.c
>   hbitmap_iter_skip_words(const void *hb, void *hbi, uint64_t pos, unsigned long cur) "hb %p hbi %p pos %"PRId64" cur 0x%lx"
>   hbitmap_reset(void *hb, uint64_t start, uint64_t count, uint64_t sbit, uint64_t ebit) "hb %p items %"PRIu64",%"PRIu64" bits %"PRIu64"..%"PRIu64

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexey Kardashevskiy]

On 04/10/2014 10:34 PM, Alexander Graf wrote:
> 
> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>> This allows guests to have a different timebase origin from the host.
>>
>> This is needed for migration, where a guest can migrate from one host
>> to another and the two hosts might have a different timebase origin.
>> However, the timebase seen by the guest must not go backwards, and
>> should go forwards only by a small amount corresponding to the time
>> taken for the migration.
>>
>> This is only supported for recent POWER hardware which has the TBU40
>> (timebase upper 40 bits) register. That includes POWER6, 7, 8 but not
>> 970.
>>
>> This adds kvm_access_one_reg() to access a special register which is not
>> in env->spr.
>>
>> The feature must be present in the host kernel.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v4:
>> * made it per machine timebase offser rather than per CPU
>>
>> v3:
>> * kvm_access_one_reg moved out to a separate patch
>> * tb_offset and host_timebase were replaced with guest_timebase as
>> the destionation does not really care of offset on the source
>>
>> v2:
>> * bumped the vmstate_ppc_cpu version
>> * defined version for the env.tb_env field
>> ---
>>   hw/ppc/ppc.c           | 120
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/ppc/spapr.c         |   3 +-
>>   include/hw/ppc/spapr.h |   2 +
>>   target-ppc/cpu-qom.h   |  16 +++++++
>>   target-ppc/kvm.c       |   5 +++
>>   target-ppc/machine.c   |   4 +-
>>   trace-events           |   3 ++
>>   7 files changed, 151 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index 9c2a132..b51db1b 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -29,9 +29,11 @@
>>   #include "sysemu/cpus.h"
>>   #include "hw/timer/m48t59.h"
>>   #include "qemu/log.h"
>> +#include "qemu/error-report.h"
>>   #include "hw/loader.h"
>>   #include "sysemu/kvm.h"
>>   #include "kvm_ppc.h"
>> +#include "trace.h"
>>     //#define PPC_DEBUG_IRQ
>>   //#define PPC_DEBUG_TB
>> @@ -797,6 +799,124 @@ static void cpu_ppc_set_tb_clk (void *opaque,
>> uint32_t freq)
>>       cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>>   }
>>   +/*
>> + * Calculate timebase on the destination side of migration
>> + *
>> + * We calculate new timebase offset as shown below:
>> + * 1) Gtb2 = Gtb1 + max(tod2 - tod1, 0)
>> + *    Gtb2 = tb2 + off2
>> + * 2) tb2 + off2 = Gtb1 + max(tod2 - tod1, 0)
>> + * 3) off2 = Gtb1 - tb2 + max(tod2 - tod1, 0)
>> + *
>> + * where:
>> + * Gtb2 - destination guest timebase
>> + * tb2 - destination host timebase
>> + * off2 - destination timebase offset
>> + * tod2 - destination time of the day
>> + * Gtb1 - source guest timebase
>> + * tod1 - source time of the day
>> + *
>> + * The result we want is in @off2
>> + *
>> + * Two conditions must be met for @off2:
>> + * 1) off2 must be multiple of 2^24 ticks as it will be set via TBU40 SPR
>> + * 2) Gtb2 >= Gtb1
>> + */
>> +static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
>> +{
>> +    uint64_t tb2, tod2;
>> +    int64_t off2;
>> +    int ratio = tb->freq / 1000000;
>> +    struct timeval tv;
>> +
>> +    tb2 = cpu_get_real_ticks();
>> +    gettimeofday(&tv, NULL);
>> +    tod2 = tv.tv_sec * 1000000 + tv.tv_usec;
>> +
>> +    off2 = tb->guest_timebase - tb2;
>> +    if ((tod2 > tb->time_of_the_day) &&
>> +        (tod2 - tb->time_of_the_day < 1000000)) {
>> +        off2 += (tod2 - tb->time_of_the_day) * ratio;
>> +    }
>> +    off2 = ROUND_UP(off2, 1 << 24);
>> +
>> +    return off2;
>> +}
> 
> I *think* what you're trying to say here is that you want
> 
> assert(source_timebase_freq == timebase_freq);
> 
> migration_duration_ns = host_ns - source_host_ns;
> guest_tb = source_guest_tb + ns_scaled_to_tb(min(0, migration_duration_ns);
> kvm_set_guest_tb(guest_tb);
>   -> kvm_set_one_reg(KVM_REG_PPC_TB_OFFSET, guest_tb - mftb());
> 
> But I honestly have not managed to read that from the code. Either this
> really is what you're trying to do and the code is just very hard to read
> (which means it needs to be written more easily) or you're doing something
> different which I don't understand.


Is this any better?

static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
{
	struct timeval tv;
	int64_t migration_duration_ns, migration_duration_tb;
	int64_t guest_tb, host_ns;
	int ratio = tb->freq / 1000000;
	int64_t off;

	gettimeofday(&tv, NULL);
	host_ns = tv.tv_sec * 1000000 + tv.tv_usec;
	migration_duration_ns = MIN(1000000,
		host_ns - tb->time_of_the_day);
	migration_duration_tb = migration_duration_ns * ratio;

	guest_tb = tb->guest_timebase + MIN(0, migration_duration_tb);

	off = guest_tb - cpu_get_real_ticks();

	return off;
}


> We also designed the PPC_TB_OFFSET ONE_REG in a way that it always rounds
> up to its 40 bit granularity, so no need to do this in QEMU. In fact, we
> don't want to do it in QEMU in case there will be a more fine-grained SPR
> in the future.

I believe rounding was not in the kernel when I started making this...


> And from all I understand the timebase frequency is now architecturally
> specified, so it won't change for newer cores, no?

I asked people in our lab. Everyone says that it should not change but
noone would bet on it too much.


> And if we migrate TCG
> guests it will be the same between two hosts.

And G5 uses 33333333. I really do not understand why it is bad to
send-and-check timer frequency. Why?


Is the rest ok? Thanks for review!


> 
> Alex
> 
>> +
>> +static void timebase_pre_save(void *opaque)
>> +{
>> +    PPCTimebaseOffset *tb = opaque;
>> +    struct timeval tv;
>> +    uint64_t ticks = cpu_get_real_ticks();
>> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>> +
>> +    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
>> +
>> +    gettimeofday(&tv, NULL);
>> +    tb->time_of_the_day = tv.tv_sec * 1000000 + tv.tv_usec;
>> +    /*
>> +     * tb_offset is only expected to be changed by migration so
>> +     * there is no need to update it from KVM here
>> +     */
>> +    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
>> +}
>> +
>> +static int timebase_pre_load(void *opaque)
>> +{
>> +    PPCTimebaseOffset *tb = opaque;
>> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>> +
>> +    if (!first_ppc_cpu->env.tb_env) {
>> +        error_report("No timebase object");
>> +        return -1;
>> +    }
>> +
>> +    /* Initialize @freq so it will be checked during migration */
>> +    tb->freq = first_ppc_cpu->env.tb_env->tb_freq;
>> +
>> +    return 0;
>> +}
>> +
>> +static int timebase_post_load(void *opaque, int version_id)
>> +{
>> +    PPCTimebaseOffset *tb = opaque;
>> +    CPUState *cpu;
>> +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>> +    int64_t tb_offset = first_ppc_cpu->env.tb_env->tb_offset;
>> +    int64_t tb_offset2 = cpu_ppc_adjust_tb_offset(tb);
>> +
>> +    trace_ppc_tb_adjust(tb_offset, tb_offset2,
>> +                        (int64_t)tb_offset2 - tb_offset,
>> +                        ((int64_t)tb_offset2 - tb_offset) / tb->freq);
>> +
>> +    CPU_FOREACH(cpu) {
>> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
>> +        if (!pcpu->env.tb_env) {
>> +            error_report("No timebase object");
>> +            return -1;
>> +        }
>> +        pcpu->env.tb_env->tb_offset = tb_offset2;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +const VMStateDescription vmstate_ppc_timebase = {
>> +    .name = "timebase",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .pre_save = timebase_pre_save,
>> +    .pre_load = timebase_pre_load,
>> +    .post_load = timebase_post_load,
>> +    .fields      = (VMStateField []) {
>> +        VMSTATE_UINT32_EQUAL(freq, PPCTimebaseOffset),
>> +        VMSTATE_UINT64(guest_timebase, PPCTimebaseOffset),
>> +        VMSTATE_UINT64(time_of_the_day, PPCTimebaseOffset),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>>   /* Set up (once) timebase frequency (in Hz) */
>>   clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>   {
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 451c473..9b0681b 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -818,7 +818,7 @@ static int spapr_vga_init(PCIBus *pci_bus)
>>     static const VMStateDescription vmstate_spapr = {
>>       .name = "spapr",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .minimum_version_id = 1,
>>       .minimum_version_id_old = 1,
>>       .fields      = (VMStateField []) {
>> @@ -827,6 +827,7 @@ static const VMStateDescription vmstate_spapr = {
>>           /* RTC offset */
>>           VMSTATE_UINT64(rtc_offset, sPAPREnvironment),
>>   +        VMSTATE_PPC_TIMEBASE(tb_offset, sPAPREnvironment),
>>           VMSTATE_END_OF_LIST()
>>       },
>>   };
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 5fdac1e..dbaa439 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -3,6 +3,7 @@
>>     #include "sysemu/dma.h"
>>   #include "hw/ppc/xics.h"
>> +#include "cpu-qom.h"
>>     struct VIOsPAPRBus;
>>   struct sPAPRPHBState;
>> @@ -29,6 +30,7 @@ typedef struct sPAPREnvironment {
>>       target_ulong entry_point;
>>       uint32_t next_irq;
>>       uint64_t rtc_offset;
>> +    struct PPCTimebaseOffset tb_offset;
>>       bool has_graphics;
>>         uint32_t epow_irq;
>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
>> index 47dc8e6..1fc91e2 100644
>> --- a/target-ppc/cpu-qom.h
>> +++ b/target-ppc/cpu-qom.h
>> @@ -120,6 +120,22 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction
>> f, CPUState *cs,
>>                                  int cpuid, void *opaque);
>>   #ifndef CONFIG_USER_ONLY
>>   extern const struct VMStateDescription vmstate_ppc_cpu;
>> +
>> +typedef struct PPCTimebaseOffset {
>> +    uint32_t freq;
>> +    uint64_t guest_timebase;
>> +    uint64_t time_of_the_day;
>> +} PPCTimebaseOffset;
>> +
>> +extern const struct VMStateDescription vmstate_ppc_timebase;
>> +
>> +#define VMSTATE_PPC_TIMEBASE(_field, _state)
>> {                              \
>> +    .name       =
>> (stringify(_field)),                                      \
>> +    .size       =
>> sizeof(PPCTimebaseOffset),                                \
>> +    .vmsd       =
>> &vmstate_ppc_timebase,                                    \
>> +    .flags      =
>> VMS_STRUCT,                                               \
>> +    .offset     = vmstate_offset_value(_state, _field,
>> PPCTimebaseOffset),  \
>> +}
>>   #endif
>>     #endif
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index ead69fa..a80faba 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -35,6 +35,7 @@
>>   #include "hw/sysbus.h"
>>   #include "hw/ppc/spapr.h"
>>   #include "hw/ppc/spapr_vio.h"
>> +#include "hw/ppc/ppc.h"
>>   #include "sysemu/watchdog.h"
>>   #include "trace.h"
>>   @@ -890,6 +891,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>                   DPRINTF("Warning: Unable to set VPA information to
>> KVM\n");
>>               }
>>           }
>> +
>> +        kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET,
>> &env->tb_env->tb_offset);
>>   #endif /* TARGET_PPC64 */
>>       }
>>   @@ -1133,6 +1136,8 @@ int kvm_arch_get_registers(CPUState *cs)
>>                   DPRINTF("Warning: Unable to get VPA information from
>> KVM\n");
>>               }
>>           }
>> +
>> +        kvm_get_one_reg(cs, KVM_REG_PPC_TB_OFFSET,
>> &env->tb_env->tb_offset);
>>   #endif
>>       }
>>   diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index 691071d..8c59cbb 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -1,5 +1,6 @@
>>   #include "hw/hw.h"
>>   #include "hw/boards.h"
>> +#include "hw/ppc/ppc.h"
>>   #include "sysemu/kvm.h"
>>   #include "helper_regs.h"
>>   @@ -495,7 +496,7 @@ static const VMStateDescription vmstate_tlbmas = {
>>     const VMStateDescription vmstate_ppc_cpu = {
>>       .name = "cpu",
>> -    .version_id = 5,
>> +    .version_id = 6,
>>       .minimum_version_id = 5,
>>       .minimum_version_id_old = 4,
>>       .load_state_old = cpu_load_old,
>> @@ -532,6 +533,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>>           VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>>           VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>>           VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>> +
>>           VMSTATE_END_OF_LIST()
>>       },
>>       .subsections = (VMStateSubsection []) {
>> diff --git a/trace-events b/trace-events
>> index 3df3f32..c284d09 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1161,6 +1161,9 @@ spapr_iommu_get(uint64_t liobn, uint64_t ioba,
>> uint64_t ret, uint64_t tce) "liob
>>   spapr_iommu_xlate(uint64_t liobn, uint64_t ioba, uint64_t tce, unsigned
>> perm, unsigned pgsize) "liobn=%"PRIx64" 0x%"PRIx64" -> 0x%"PRIx64"
>> perm=%u mask=%x"
>>   spapr_iommu_new_table(uint64_t liobn, void *tcet, void *table, int fd)
>> "liobn=%"PRIx64" tcet=%p table=%p fd=%d"
>>   +# hw/ppc/ppc.c
>> +ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t
>> seconds) "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64"
>> (%"PRId64"s)"
>> +
>>   # util/hbitmap.c
>>   hbitmap_iter_skip_words(const void *hb, void *hbi, uint64_t pos,
>> unsigned long cur) "hb %p hbi %p pos %"PRId64" cur 0x%lx"
>>   hbitmap_reset(void *hb, uint64_t start, uint64_t count, uint64_t sbit,
>> uint64_t ebit) "hb %p items %"PRIu64",%"PRIu64" bits %"PRIu64"..%"PRIu64
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexander Graf]

On 10.04.14 16:31, Alexey Kardashevskiy wrote:
> On 04/10/2014 10:34 PM, Alexander Graf wrote:
>> On 03.04.14 15:14, Alexey Kardashevskiy wrote:
>>> This allows guests to have a different timebase origin from the host.
>>>
>>> This is needed for migration, where a guest can migrate from one host
>>> to another and the two hosts might have a different timebase origin.
>>> However, the timebase seen by the guest must not go backwards, and
>>> should go forwards only by a small amount corresponding to the time
>>> taken for the migration.
>>>
>>> This is only supported for recent POWER hardware which has the TBU40
>>> (timebase upper 40 bits) register. That includes POWER6, 7, 8 but not
>>> 970.
>>>
>>> This adds kvm_access_one_reg() to access a special register which is not
>>> in env->spr.
>>>
>>> The feature must be present in the host kernel.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v4:
>>> * made it per machine timebase offser rather than per CPU
>>>
>>> v3:
>>> * kvm_access_one_reg moved out to a separate patch
>>> * tb_offset and host_timebase were replaced with guest_timebase as
>>> the destionation does not really care of offset on the source
>>>
>>> v2:
>>> * bumped the vmstate_ppc_cpu version
>>> * defined version for the env.tb_env field
>>> ---
>>>    hw/ppc/ppc.c           | 120
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>    hw/ppc/spapr.c         |   3 +-
>>>    include/hw/ppc/spapr.h |   2 +
>>>    target-ppc/cpu-qom.h   |  16 +++++++
>>>    target-ppc/kvm.c       |   5 +++
>>>    target-ppc/machine.c   |   4 +-
>>>    trace-events           |   3 ++
>>>    7 files changed, 151 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>>> index 9c2a132..b51db1b 100644
>>> --- a/hw/ppc/ppc.c
>>> +++ b/hw/ppc/ppc.c
>>> @@ -29,9 +29,11 @@
>>>    #include "sysemu/cpus.h"
>>>    #include "hw/timer/m48t59.h"
>>>    #include "qemu/log.h"
>>> +#include "qemu/error-report.h"
>>>    #include "hw/loader.h"
>>>    #include "sysemu/kvm.h"
>>>    #include "kvm_ppc.h"
>>> +#include "trace.h"
>>>      //#define PPC_DEBUG_IRQ
>>>    //#define PPC_DEBUG_TB
>>> @@ -797,6 +799,124 @@ static void cpu_ppc_set_tb_clk (void *opaque,
>>> uint32_t freq)
>>>        cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>>>    }
>>>    +/*
>>> + * Calculate timebase on the destination side of migration
>>> + *
>>> + * We calculate new timebase offset as shown below:
>>> + * 1) Gtb2 = Gtb1 + max(tod2 - tod1, 0)
>>> + *    Gtb2 = tb2 + off2
>>> + * 2) tb2 + off2 = Gtb1 + max(tod2 - tod1, 0)
>>> + * 3) off2 = Gtb1 - tb2 + max(tod2 - tod1, 0)
>>> + *
>>> + * where:
>>> + * Gtb2 - destination guest timebase
>>> + * tb2 - destination host timebase
>>> + * off2 - destination timebase offset
>>> + * tod2 - destination time of the day
>>> + * Gtb1 - source guest timebase
>>> + * tod1 - source time of the day
>>> + *
>>> + * The result we want is in @off2
>>> + *
>>> + * Two conditions must be met for @off2:
>>> + * 1) off2 must be multiple of 2^24 ticks as it will be set via TBU40 SPR
>>> + * 2) Gtb2 >= Gtb1
>>> + */
>>> +static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
>>> +{
>>> +    uint64_t tb2, tod2;
>>> +    int64_t off2;
>>> +    int ratio = tb->freq / 1000000;
>>> +    struct timeval tv;
>>> +
>>> +    tb2 = cpu_get_real_ticks();
>>> +    gettimeofday(&tv, NULL);
>>> +    tod2 = tv.tv_sec * 1000000 + tv.tv_usec;
>>> +
>>> +    off2 = tb->guest_timebase - tb2;
>>> +    if ((tod2 > tb->time_of_the_day) &&
>>> +        (tod2 - tb->time_of_the_day < 1000000)) {
>>> +        off2 += (tod2 - tb->time_of_the_day) * ratio;
>>> +    }
>>> +    off2 = ROUND_UP(off2, 1 << 24);
>>> +
>>> +    return off2;
>>> +}
>> I *think* what you're trying to say here is that you want
>>
>> assert(source_timebase_freq == timebase_freq);
>>
>> migration_duration_ns = host_ns - source_host_ns;
>> guest_tb = source_guest_tb + ns_scaled_to_tb(min(0, migration_duration_ns);
>> kvm_set_guest_tb(guest_tb);
>>    -> kvm_set_one_reg(KVM_REG_PPC_TB_OFFSET, guest_tb - mftb());
>>
>> But I honestly have not managed to read that from the code. Either this
>> really is what you're trying to do and the code is just very hard to read
>> (which means it needs to be written more easily) or you're doing something
>> different which I don't understand.
>
> Is this any better?
>
> static int64_t cpu_ppc_adjust_tb_offset(PPCTimebaseOffset *tb)
> {
> 	struct timeval tv;
> 	int64_t migration_duration_ns, migration_duration_tb;

If I read the code correctly you're operating in us, not ns, no?

> 	int64_t guest_tb, host_ns;
> 	int ratio = tb->freq / 1000000;

#define USEC_PER_SEC 1000000

You're also losing quite a bit of precision here, no?

> 	int64_t off;
>
> 	gettimeofday(&tv, NULL);
> 	host_ns = tv.tv_sec * 1000000 + tv.tv_usec;

host_us = get_clock_realtime() / 1000; ?

> 	migration_duration_ns = MIN(1000000,

Why is it MIN(1000000)? Is a migration supposed to last at least 1sec? Why?

> 		host_ns - tb->time_of_the_day);
> 	migration_duration_tb = migration_duration_ns * ratio;
>
> 	guest_tb = tb->guest_timebase + MIN(0, migration_duration_tb);
>
> 	off = guest_tb - cpu_get_real_ticks();

It's probably easier to read when you create one function that just 
returns a guest TB value adjusted by the time the last measurement 
happened. The fact that the KVM register wants an offset is a KVM 
implementation detail. The TB adjustment should happen generically.

>
> 	return off;
> }
>
>
>> We also designed the PPC_TB_OFFSET ONE_REG in a way that it always rounds
>> up to its 40 bit granularity, so no need to do this in QEMU. In fact, we
>> don't want to do it in QEMU in case there will be a more fine-grained SPR
>> in the future.
> I believe rounding was not in the kernel when I started making this...
>
>
>> And from all I understand the timebase frequency is now architecturally
>> specified, so it won't change for newer cores, no?
> I asked people in our lab. Everyone says that it should not change but
> noone would bet on it too much.

When it changes and you want to live migrate, you'll need to implement a 
guest TB scale register and the whole idea of a "TB offset" ONE_REG is 
absurd.

The more I think about this the more I realize we should have created a 
"guest TB value", not a "guest TB offset" ONE_REG.

>
>
>> And if we migrate TCG
>> guests it will be the same between two hosts.
> And G5 uses 33333333. I really do not understand why it is bad to
> send-and-check timer frequency. Why?

Because the guest will continue to run at a different TB frequency on 
the new host and break.

>
>
> Is the rest ok? Thanks for review!

Not sure. Please rework everything according to the comments, make the 
code readable enough that your wife understands it and then resend it :).


Alex

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Benjamin Herrenschmidt]

On Fri, 2014-04-11 at 11:40 +0200, Alexander Graf wrote:
> > And G5 uses 33333333. I really do not understand why it is bad to
> > send-and-check timer frequency. Why?
> 
> Because the guest will continue to run at a different TB frequency on 
> the new host and break.

Right, which is why we should send it accross so it can be checked and
we can barf accordingly rather than having random odd things happen :-)

Ben.

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexander Graf]

> Am 11.04.2014 um 23:55 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> 
> On Fri, 2014-04-11 at 11:40 +0200, Alexander Graf wrote:
>>> And G5 uses 33333333. I really do not understand why it is bad to
>>> send-and-check timer frequency. Why?
>> 
>> Because the guest will continue to run at a different TB frequency on 
>> the new host and break.
> 
> Right, which is why we should send it accross so it can be checked and
> we can barf accordingly rather than having random odd things happen :-)

Would the TB frequency ever be different inside the same generation? We can't properly support cross-family migration, so we need to bail out on that anyway. No need for an additional check here, no?

Alex

> 
> Ben.
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexander Graf]

> Am 12.04.2014 um 00:59 schrieb Alexander Graf <agraf@suse.de>:
> 
> 
> 
>> Am 11.04.2014 um 23:55 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
>> 
>> On Fri, 2014-04-11 at 11:40 +0200, Alexander Graf wrote:
>>>> And G5 uses 33333333. I really do not understand why it is bad to
>>>> send-and-check timer frequency. Why?
>>> 
>>> Because the guest will continue to run at a different TB frequency on 
>>> the new host and break.
>> 
>> Right, which is why we should send it accross so it can be checked and
>> we can barf accordingly rather than having random odd things happen :-)
> 
> Would the TB frequency ever be different inside the same generation? We can't properly support cross-family migration, so we need to bail out on that anyway. No need for an additional check here, no?

Unless we're trying to do PR migration.

Ok, an assert() would make a lot of sense. Or is there a better way to fail migration?


Alex

> 
> Alex
> 
>> 
>> Ben.
> 

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexey Kardashevskiy]

On 04/12/2014 07:55 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2014-04-11 at 11:40 +0200, Alexander Graf wrote:
>>> And G5 uses 33333333. I really do not understand why it is bad to
>>> send-and-check timer frequency. Why?
>>
>> Because the guest will continue to run at a different TB frequency on 
>> the new host and break.
> 
> Right, which is why we should send it accross so it can be checked and
> we can barf accordingly rather than having random odd things happen :-)

As I was explained several times already, QEMU does not transfer
configuration (yet) and we only should implement correct transfer of
parameters (without checking) and specify in the QEMU command line the ones
which cannot be transferred.

So with timebase frequency, it should be a command line switch (-machine
option?) which would take frequency and fail if it is POWER7/8 and not
500MHz. And then libvirt should take care of it (always pass it or have XML
tag for it). Same story as with migrating IRQs (which I do now in a hacky
way but I am going to changed it to work via the command line).


-- 
Alexey

Re: [Qemu-devel] [PATCH 4/4] spapr: Add support for time base offset migration

[Alexander Graf]

> Am 12.04.2014 um 05:44 schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:
> 
>> On 04/12/2014 07:55 AM, Benjamin Herrenschmidt wrote:
>> On Fri, 2014-04-11 at 11:40 +0200, Alexander Graf wrote:
>>>> And G5 uses 33333333. I really do not understand why it is bad to
>>>> send-and-check timer frequency. Why?
>>> 
>>> Because the guest will continue to run at a different TB frequency on 
>>> the new host and break.
>> 
>> Right, which is why we should send it accross so it can be checked and
>> we can barf accordingly rather than having random odd things happen :-)
> 
> As I was explained several times already, QEMU does not transfer
> configuration (yet) and we only
> should implement correct transfer of
> parameters (without checking) and specify in the QEMU command line the ones
> which cannot be transferred.

Exactly. We should try to migrate only state that the user doesn't specify on the command line.

> 
> So with timebase frequency, it should be a command line switch (-machine
> option?) which would take frequency and fail if it is POWER7/8 and not
> 500MHz. And then libvirt should take care of it (always pass it or have XML
> tag for it).

We can also declare everything before p7 non-migratable. That would also solve the issue.

> Same story as with migrating IRQs (which I do now in a hacky
> way but I am going to changed it to work via the command line).

It's slightly different, but similar. The tb frequency is a hardware constraint, IRQ numbering isn't.

But either way works for me really. I would be happy to make pre-p7 cpus non-migratable. It'd definitely reduce the test matrix.


Alex

Re: [Qemu-devel] [PATCH 1/4] kvm: Add set_one_reg/get_one_reg helpers

[Alexander Graf]

On 04/03/2014 03:14 PM, Alexey Kardashevskiy wrote:
> This adds QEMU wrappers for KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctls.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Please merge with the s390 variant of this.


Alex

Re: [Qemu-devel] [PATCH 1/4] kvm: Add set_one_reg/get_one_reg helpers

[Alexey Kardashevskiy]

On 05/08/2014 10:27 PM, Alexander Graf wrote:
> On 04/03/2014 03:14 PM, Alexey Kardashevskiy wrote:
>> This adds QEMU wrappers for KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctls.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Please merge with the s390 variant of this.


How? Why?

I posted mine 19 september 2013. What you refer to is from April this year.
How is s390 patch better than mine? Did I "ping" not often enough? Not funny :(

Do you want me to move kvm_set_one_reg/kvm_get_one_reg from
target-s390x/kvm.c to kvm-all.c?


-- 
Alexey

[Qemu-devel] [PATCH] kvm: make one_reg helpers available for everyone

[Cornelia Huck]

s390x introduced helper functions for getting/setting one_regs with
commit 860643bc. However, nothing about these is s390-specific.

Alexey Kardashevskiy had already posted a general version, so let's
merge the two patches and massage the code a bit.

CC: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 include/sysemu/kvm.h | 20 ++++++++++++++++++++
 kvm-all.c            | 28 ++++++++++++++++++++++++++++
 target-s390x/kvm.c   | 29 -----------------------------
 trace-events         |  6 ++----
 4 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 5ad4e0e..a6c2823 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -383,4 +383,24 @@ void kvm_init_irq_routing(KVMState *s);
  *          > 0: irq chip was created
  */
 int kvm_arch_irqchip_create(KVMState *s);
+
+/**
+ * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
+ * @id: The register ID
+ * @source: The pointer to the value to be set. It must point to a variable
+ *          of the correct type/size for the register being accessed.
+ *
+ * Returns: 0 on success, or a negative errno on failure.
+ */
+int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
+
+/**
+ * kvm_get_one_reg - get a register value from KVM via KVM_GET_ONE_REG ioctl
+ * @id: The register ID
+ * @target: The pointer where the value is to be stored. It must point to a
+ *          variable of the correct type/size for the register being accessed.
+ *
+ * Returns: 0 on success, or a negative errno on failure.
+ */
+int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
 #endif
diff --git a/kvm-all.c b/kvm-all.c
index 5cb7f26..94520e5 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2114,3 +2114,31 @@ int kvm_create_device(KVMState *s, uint64_t type, bool test)
 
     return test ? 0 : create_dev.fd;
 }
+
+int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
+{
+    struct kvm_one_reg reg;
+    int r;
+
+    reg.id = id;
+    reg.addr = (uintptr_t) source;
+    r = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (r) {
+        trace_kvm_failed_reg_set(id, strerror(r));
+    }
+    return r;
+}
+
+int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
+{
+    struct kvm_one_reg reg;
+    int r;
+
+    reg.id = id;
+    reg.addr = (uintptr_t) target;
+    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (r) {
+        trace_kvm_failed_reg_get(id, strerror(r));
+    }
+    return r;
+}
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index b7b0edc..ba2dffe 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -129,35 +129,6 @@ void kvm_arch_reset_vcpu(CPUState *cpu)
     }
 }
 
-static int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
-{
-    struct kvm_one_reg reg;
-    int r;
-
-    reg.id = id;
-    reg.addr = (uint64_t) source;
-    r = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
-    if (r) {
-        trace_kvm_failed_reg_set(id, strerror(errno));
-    }
-    return r;
-}
-
-static int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
-{
-    struct kvm_one_reg reg;
-    int r;
-
-    reg.id = id;
-    reg.addr = (uint64_t) target;
-    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
-    if (r) {
-        trace_kvm_failed_reg_get(id, strerror(errno));
-    }
-    return r;
-}
-
-
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     S390CPU *cpu = S390_CPU(cs);
diff --git a/trace-events b/trace-events
index af4449d..2c5b307 100644
--- a/trace-events
+++ b/trace-events
@@ -1230,6 +1230,8 @@ kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
 kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
 kvm_failed_spr_set(int str, const char *msg) "Warning: Unable to set SPR %d to KVM: %s"
 kvm_failed_spr_get(int str, const char *msg) "Warning: Unable to retrieve SPR %d from KVM: %s"
+kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
+kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
 
 # memory.c
 memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
@@ -1246,7 +1248,3 @@ xen_pv_mmio_write(uint64_t addr) "WARNING: write to Xen PV Device MMIO space (ad
 # hw/pci/pci_host.c
 pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x"
 pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x"
-
-# target-s390/kvm.c
-kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
-kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH] kvm: make one_reg helpers available for everyone

[Alexander Graf]

On 09.05.14 10:06, Cornelia Huck wrote:
> s390x introduced helper functions for getting/setting one_regs with
> commit 860643bc. However, nothing about these is s390-specific.
>
> Alexey Kardashevskiy had already posted a general version, so let's
> merge the two patches and massage the code a bit.
>
> CC: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Thanks a lot for the patch Conny. Unfortunately Paolo seems to have 
missed this and queued Alexey's patch instead.

Does s390x even compile still with Alexey's patch applied? If it does, 
please post a simple follow-up patch removing your own s390 
implementation. If it doesn't compile, NACK on the current KVM pull request.


Alex

> ---
>   include/sysemu/kvm.h | 20 ++++++++++++++++++++
>   kvm-all.c            | 28 ++++++++++++++++++++++++++++
>   target-s390x/kvm.c   | 29 -----------------------------
>   trace-events         |  6 ++----
>   4 files changed, 50 insertions(+), 33 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 5ad4e0e..a6c2823 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -383,4 +383,24 @@ void kvm_init_irq_routing(KVMState *s);
>    *          > 0: irq chip was created
>    */
>   int kvm_arch_irqchip_create(KVMState *s);
> +
> +/**
> + * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
> + * @id: The register ID
> + * @source: The pointer to the value to be set. It must point to a variable
> + *          of the correct type/size for the register being accessed.
> + *
> + * Returns: 0 on success, or a negative errno on failure.
> + */
> +int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
> +
> +/**
> + * kvm_get_one_reg - get a register value from KVM via KVM_GET_ONE_REG ioctl
> + * @id: The register ID
> + * @target: The pointer where the value is to be stored. It must point to a
> + *          variable of the correct type/size for the register being accessed.
> + *
> + * Returns: 0 on success, or a negative errno on failure.
> + */
> +int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
>   #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index 5cb7f26..94520e5 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2114,3 +2114,31 @@ int kvm_create_device(KVMState *s, uint64_t type, bool test)
>   
>       return test ? 0 : create_dev.fd;
>   }
> +
> +int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
> +{
> +    struct kvm_one_reg reg;
> +    int r;
> +
> +    reg.id = id;
> +    reg.addr = (uintptr_t) source;
> +    r = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (r) {
> +        trace_kvm_failed_reg_set(id, strerror(r));
> +    }
> +    return r;
> +}
> +
> +int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
> +{
> +    struct kvm_one_reg reg;
> +    int r;
> +
> +    reg.id = id;
> +    reg.addr = (uintptr_t) target;
> +    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (r) {
> +        trace_kvm_failed_reg_get(id, strerror(r));
> +    }
> +    return r;
> +}
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index b7b0edc..ba2dffe 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -129,35 +129,6 @@ void kvm_arch_reset_vcpu(CPUState *cpu)
>       }
>   }
>   
> -static int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
> -{
> -    struct kvm_one_reg reg;
> -    int r;
> -
> -    reg.id = id;
> -    reg.addr = (uint64_t) source;
> -    r = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> -    if (r) {
> -        trace_kvm_failed_reg_set(id, strerror(errno));
> -    }
> -    return r;
> -}
> -
> -static int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
> -{
> -    struct kvm_one_reg reg;
> -    int r;
> -
> -    reg.id = id;
> -    reg.addr = (uint64_t) target;
> -    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> -    if (r) {
> -        trace_kvm_failed_reg_get(id, strerror(errno));
> -    }
> -    return r;
> -}
> -
> -
>   int kvm_arch_put_registers(CPUState *cs, int level)
>   {
>       S390CPU *cpu = S390_CPU(cs);
> diff --git a/trace-events b/trace-events
> index af4449d..2c5b307 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1230,6 +1230,8 @@ kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
>   kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>   kvm_failed_spr_set(int str, const char *msg) "Warning: Unable to set SPR %d to KVM: %s"
>   kvm_failed_spr_get(int str, const char *msg) "Warning: Unable to retrieve SPR %d from KVM: %s"
> +kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
> +kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>   
>   # memory.c
>   memory_region_ops_read(void *mr, uint64_t addr, uint64_t value, unsigned size) "mr %p addr %#"PRIx64" value %#"PRIx64" size %u"
> @@ -1246,7 +1248,3 @@ xen_pv_mmio_write(uint64_t addr) "WARNING: write to Xen PV Device MMIO space (ad
>   # hw/pci/pci_host.c
>   pci_cfg_read(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x -> 0x%x"
>   pci_cfg_write(const char *dev, unsigned devid, unsigned fnid, unsigned offs, unsigned val) "%s %02u:%u @0x%x <- 0x%x"
> -
> -# target-s390/kvm.c
> -kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
> -kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"