[Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

This is a rebased and revised version of my patches revising CPU
compatiblity mode handling on ppc, last posted in November.  Since
then, many of the patches have already been merged (some for 2.9, some
since).  This is what's left.

 * There was conceptual confusion about what a compatibility mode
   means, and how it interacts with the machine type.  This cleans
   that up, clarifying that a compatibility mode (as an externally set
   option) only makes sense on machine types that don't permit the
   guest hypervisor privilege (i.e. 'pseries')

 * It was previously the user's (or management layer's) responsibility
   to determine compatibility of CPUs on either end for migration.
   This uses the compatibility modes to check that properly during an
   incoming migration.

This hasn't been extensively tested yet.  There are quite a few
migration cases to consider, for example:

Basic:

1) Boot guest with -cpu host
        Should go into POWER8 compat mode after CAS
        Previously would have been raw mode

2) Boot guest with -machine pseries,max-cpu-compat=power7 -cpu host
        Should go into POWER7 compat mode

3) Boot guest with -cpu host,compat=power7
        Should act as (2), but print a warning

4) Boot guest via libvirt with power7 compat mode specified in XML
        Should act as (3), (2) once we fix libvirt

5) Hack guest to only advertise power7 compatibility, boot with -cpu host
        Should go into POWER7 compat mode after CAS

6) Hack guest to only advertise real PVRs
        Should remain in POWER8 raw mode after CAS

7) Hack guest to only advertise real PVRs
   Boot with -machine pseries,max-cpu-compat=power8
        Should fail at CAS time

8) Hack guest to only advertise power7 compatibility, boot with -cpu host
   Reboot to normal guest
        Should go to power7 compat mode after CAS of boot 1
        Should revert to raw mode on reboot
        SHould go to power8 compat mode after CAS of boot 2

Migration:

9) Boot guest with qemu-2.6 -machine pseries-2.6 -cpu host
   Migrate to qemu-2.8 -machine pseries-2.6 -cpu host
        Should work, end up running in power8 raw mode

10) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host
        Should work, end up running in power8 raw mode

11) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host,compat=power7
        Should work, be running in POWER7 compat after, but give warning like
        (3)

12) Boot guest with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7,max-cpu-compat=power7 -cpu host
        Should work, be running in POWER7 compat after, no warning

13) Boot to SLOF with qemu-2.6 -machine pseries-2.6 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.6 -cpu host

        ?

14) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host
        ?

15) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7 -cpu host,compat=power7
        ?

16) Boot to SLOF with qemu-2.7 -machine pseries-2.7 -cpu host,compat=power7
    Migrate to qemu-2.8 -machine pseries-2.7,max-cpu-compat=power7 -cpu host
        ?

17) Boot guest with qemu-2.6 -machine pseries-2.6 -cpu host
    Migrate to qemu-2.7.z -machine pseries-2.6 -cpu host
        Should work

18) Hack guest to only advertise power7 compatibility, boot with -cpu host
    Boot with qemu-2.8, migrate to qemu-2.8
        Should be in power7 compat mode after CAS on source, and still
        in power7 compat mode on destination


Changes since v3:
  * Backwards compatible -cpu handling now removes compat= option from
    options passed on to the cpu, so it doesn't trigger further warnings
  * Add a migration fix make cpu_synchronize_state() safe in post_load
    handlers, which in turn fixes a bug in 5/5.
  * A number of bugfixes and other tweaks suggested by feedback on v2.

Changes since RFCv2:
  * Many patches dropped, since they're already merged
  * Rebased, fixed conflicts
  * Restored support for backwards migration (wasn't as complicated as
    I thought)
  * Updated final patch's description to more accurately reflect the
    logic

Changes since RFCv1:
  * Change CAS logic to prefer compatibility modes over raw mode
  * Simplified by giving up on half-hearted attempts to maintain
    backwards migration
  * Folded migration stream changes into a single patch
  * Removed some preliminary patches which are already merged

David Gibson (4):
  migration: Mark CPU states dirty before incoming migration/loadvm
  pseries: Move CPU compatibility property to machine
  pseries: Reset CPU compatibility mode
  ppc: Rework CPU compatibility testing across migration

Greg Kurz (1):
  qapi: add explicit null to string input and output visitors

 cpus.c                       |   9 ++++
 hw/ppc/spapr.c               |   8 +++-
 hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
 hw/ppc/spapr_hcall.c         |   8 ++--
 include/hw/ppc/spapr.h       |  12 +++--
 include/sysemu/cpus.h        |   1 +
 include/sysemu/hax.h         |   1 +
 include/sysemu/hw_accel.h    |  10 +++++
 include/sysemu/kvm.h         |   1 +
 kvm-all.c                    |  10 +++++
 migration/savevm.c           |   2 +
 qapi/string-input-visitor.c  |  11 +++++
 qapi/string-output-visitor.c |  14 ++++++
 target/i386/hax-all.c        |  10 +++++
 target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
 target/ppc/cpu.h             |   5 ++-
 target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
 target/ppc/translate_init.c  |  86 +++++++++++-------------------------
 18 files changed, 340 insertions(+), 84 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCHv4 1/5] qapi: add explicit null to string input and output visitors

[David Gibson]

From: Greg Kurz <groug@kaod.org>

This may be used for deprecated object properties that are kept for
backwards compatibility.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 qapi/string-input-visitor.c  | 11 +++++++++++
 qapi/string-output-visitor.c | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index c089491..63ae115 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -326,6 +326,16 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     *obj = val;
 }
 
+static void parse_type_null(Visitor *v, const char *name, Error **errp)
+{
+    StringInputVisitor *siv = to_siv(v);
+
+    if (!siv->string || siv->string[0]) {
+        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+                   "null");
+    }
+}
+
 static void string_input_free(Visitor *v)
 {
     StringInputVisitor *siv = to_siv(v);
@@ -349,6 +359,7 @@ Visitor *string_input_visitor_new(const char *str)
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
+    v->visitor.type_null = parse_type_null;
     v->visitor.start_list = start_list;
     v->visitor.next_list = next_list;
     v->visitor.check_list = check_list;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 53c2175..af649e1 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -256,6 +256,19 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }
 
+static void print_type_null(Visitor *v, const char *name, Error **errp)
+{
+    StringOutputVisitor *sov = to_sov(v);
+    char *out;
+
+    if (sov->human) {
+        out = g_strdup("<null>");
+    } else {
+        out = g_strdup("");
+    }
+    string_output_set(sov, out);
+}
+
 static void
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -341,6 +354,7 @@ Visitor *string_output_visitor_new(bool human, char **result)
     v->visitor.type_bool = print_type_bool;
     v->visitor.type_str = print_type_str;
     v->visitor.type_number = print_type_number;
+    v->visitor.type_null = print_type_null;
     v->visitor.start_list = start_list;
     v->visitor.next_list = next_list;
     v->visitor.end_list = end_list;
-- 
2.9.4

[Qemu-devel] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm

[David Gibson]

As a rule, CPU internal state should never be updated when
!cpu->kvm_vcpu_dirty (or the HAX equivalent).  If that is done, then
subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
will clobber state.

However, we routinely do this during a loadvm or incoming migration.
Usually this is called shortly after a reset, which will clear all the cpu
dirty flags with cpu_synchronize_all_post_reset().  Nothing is expected
to set the dirty flags again before the cpu state is loaded from the
incoming stream.

This means that it isn't safe to call cpu_synchronize_state() from a
post_load handler, which is non-obvious and potentially inconvenient.

We could cpu_synchronize_all_state() before the loadvm, but that would be
overkill since a) we expect the state to already be synchronized from the
reset and b) we expect to completely rewrite the state with a call to
cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().

To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
associated helpers, which simply marks the cpu state as dirty without
actually changing anything.  i.e. it says we want to discard any existing
KVM (or HAX) state and replace it with what we're going to load.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dave Gilbert <dgilbert@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 cpus.c                    |  9 +++++++++
 include/sysemu/cpus.h     |  1 +
 include/sysemu/hax.h      |  1 +
 include/sysemu/hw_accel.h | 10 ++++++++++
 include/sysemu/kvm.h      |  1 +
 kvm-all.c                 | 10 ++++++++++
 migration/savevm.c        |  2 ++
 target/i386/hax-all.c     | 10 ++++++++++
 8 files changed, 44 insertions(+)

diff --git a/cpus.c b/cpus.c
index 516e5cb..6398439 100644
--- a/cpus.c
+++ b/cpus.c
@@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void)
     }
 }
 
+void cpu_synchronize_all_pre_loadvm(void)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        cpu_synchronize_pre_loadvm(cpu);
+    }
+}
+
 static int do_vm_stop(RunState state)
 {
     int ret = 0;
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index a8053f1..731756d 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
 void cpu_synchronize_all_states(void);
 void cpu_synchronize_all_post_reset(void);
 void cpu_synchronize_all_post_init(void);
+void cpu_synchronize_all_pre_loadvm(void);
 
 void qtest_clock_warp(int64_t dest);
 
diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h
index d9f0239..232a68a 100644
--- a/include/sysemu/hax.h
+++ b/include/sysemu/hax.h
@@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size);
 void hax_cpu_synchronize_state(CPUState *cpu);
 void hax_cpu_synchronize_post_reset(CPUState *cpu);
 void hax_cpu_synchronize_post_init(CPUState *cpu);
+void hax_cpu_synchronize_pre_loadvm(CPUState *cpu);
 
 #ifdef CONFIG_HAX
 
diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
index c9b3105..469ffda 100644
--- a/include/sysemu/hw_accel.h
+++ b/include/sysemu/hw_accel.h
@@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
     }
 }
 
+static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
+{
+    if (kvm_enabled()) {
+        kvm_cpu_synchronize_pre_loadvm(cpu);
+    }
+    if (hax_enabled()) {
+        hax_cpu_synchronize_pre_loadvm(cpu);
+    }
+}
+
 #endif /* QEMU_HW_ACCEL_H */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 5cc83f2..a45c145 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
 void kvm_cpu_synchronize_state(CPUState *cpu);
 void kvm_cpu_synchronize_post_reset(CPUState *cpu);
 void kvm_cpu_synchronize_post_init(CPUState *cpu);
+void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
 
 void kvm_init_cpu_signals(CPUState *cpu);
 
diff --git a/kvm-all.c b/kvm-all.c
index 90b8573..a8485bd 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu)
     run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
 
+static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
+{
+    cpu->kvm_vcpu_dirty = true;
+}
+
+void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
+{
+    run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
+}
+
 #ifdef KVM_HAVE_MCE_INJECTION
 static __thread void *pending_sigbus_addr;
 static __thread int pending_sigbus_code;
diff --git a/migration/savevm.c b/migration/savevm.c
index d971e5e..6391131 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2011,6 +2011,8 @@ int qemu_loadvm_state(QEMUFile *f)
         }
     }
 
+    cpu_synchronize_all_pre_loadvm();
+
     ret = qemu_loadvm_state_main(f, mis);
     qemu_event_set(&mis->main_thread_load_event);
 
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index ef13015..1306722 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -635,6 +635,16 @@ void hax_cpu_synchronize_post_init(CPUState *cpu)
     run_on_cpu(cpu, do_hax_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
 
+static void do_hax_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
+{
+    cpu->hax_vcpu_dirty = true;
+}
+
+void hax_cpu_synchronize_pre_loadvm(CPUState *cpu)
+{
+    run_on_cpu(cpu, do_hax_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
+}
+
 int hax_smp_cpu_exec(CPUState *cpu)
 {
     CPUArchState *env = (CPUArchState *) (cpu->env_ptr);
-- 
2.9.4

[Qemu-devel] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[David Gibson]

Server class POWER CPUs have a "compat" property, which is used to set the
backwards compatibility mode for the processor.  However, this only makes
sense for machine types which don't give the guest access to hypervisor
privilege - otherwise the compatibility level is under the guest's control.

To reflect this, this removes the CPU 'compat' property and instead
creates a 'max-cpu-compat' property on the pseries machine.  Strictly
speaking this breaks compatibility, but AFAIK the 'compat' option was
never (directly) used with -device or device_add.

The option was used with -cpu.  So, to maintain compatibility, this
patch adds a hack to the cpu option parsing to strip out any compat
options supplied with -cpu and set them on the machine property
instead of the now deprecated cpu property.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |   6 ++-
 hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
 hw/ppc/spapr_hcall.c        |   8 ++--
 include/hw/ppc/spapr.h      |  12 ++++--
 target/ppc/compat.c         | 102 ++++++++++++++++++++++++++++++++++++++++++++
 target/ppc/cpu.h            |   5 ++-
 target/ppc/translate_init.c |  86 +++++++++++--------------------------
 7 files changed, 202 insertions(+), 73 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ab3aab1..3c4e88f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState *machine)
         machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
     }
 
-    ppc_cpu_parse_features(machine->cpu_model);
+    spapr_cpu_parse_features(spapr);
 
     spapr_init_cpus(spapr);
 
@@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
                                     " place of standard EPOW events when possible"
                                     " (required for memory hot-unplug support)",
                                     NULL);
+
+    ppc_compat_add_property(obj, "max-cpu-compat", &spapr->max_compat_pvr,
+                            "Maximum permitted CPU compatibility mode",
+                            &error_fatal);
 }
 
 static void spapr_machine_finalizefn(Object *obj)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ff7058e..ab4102b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -20,6 +20,58 @@
 #include "sysemu/numa.h"
 #include "qemu/error-report.h"
 
+void spapr_cpu_parse_features(sPAPRMachineState *spapr)
+{
+    /*
+     * Backwards compatibility hack:
+     *
+     *   CPUs had a "compat=" property which didn't make sense for
+     *   anything except pseries.  It was replaced by "max-cpu-compat"
+     *   machine option.  This supports old command lines like
+     *       -cpu POWER8,compat=power7
+     *   By stripping the compat option and applying it to the machine
+     *   before passing it on to the cpu level parser.
+     */
+    gchar **inpieces;
+    int i, j;
+    gchar *compat_str = NULL;
+
+    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
+
+    /* inpieces[0] is the actual model string */
+    i = 1;
+    j = 1;
+    while (inpieces[i]) {
+        if (g_str_has_prefix(inpieces[i], "compat=")) {
+            /* in case of multiple compat= optipons */
+            g_free(compat_str);
+            compat_str = inpieces[i];
+        } else {
+            j++;
+        }
+
+        /* Excise compat options from list */
+        inpieces[j] = inpieces[i];
+        i++;
+    }
+    inpieces[j] = NULL;
+
+    if (compat_str) {
+        char *val = compat_str + strlen("compat=");
+        gchar *newprops = g_strjoinv(",", inpieces);
+
+        object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
+                                &error_fatal);
+
+        ppc_cpu_parse_features(newprops);
+        g_free(newprops);
+    } else {
+        ppc_cpu_parse_features(MACHINE(spapr)->cpu_model);
+    }
+
+    g_strfreev(inpieces);
+}
+
 static void spapr_cpu_reset(void *opaque)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -70,10 +122,10 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     /* Enable PAPR mode in TCG or KVM */
     cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
 
-    if (cpu->max_compat) {
+    if (spapr->max_compat_pvr) {
         Error *local_err = NULL;
 
-        ppc_set_compat(cpu, cpu->max_compat, &local_err);
+        ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 77d2d66..8129959 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1044,11 +1044,11 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
     }
 }
 
-static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
-                              Error **errp)
+static uint32_t cas_check_pvr(sPAPRMachineState *spapr, PowerPCCPU *cpu,
+                              target_ulong *addr, Error **errp)
 {
     bool explicit_match = false; /* Matched the CPU's real PVR */
-    uint32_t max_compat = cpu->max_compat;
+    uint32_t max_compat = spapr->max_compat_pvr;
     uint32_t best_compat = 0;
     int i;
 
@@ -1104,7 +1104,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     bool guest_radix;
     Error *local_err = NULL;
 
-    cas_pvr = cas_check_pvr(cpu, &addr, &local_err);
+    cas_pvr = cas_check_pvr(spapr, cpu, &addr, &local_err);
     if (local_err) {
         error_report_err(local_err);
         return H_HARDWARE;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 98fb78b..4da92e2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -87,16 +87,19 @@ struct sPAPRMachineState {
     uint64_t rtc_offset; /* Now used only during incoming migration */
     struct PPCTimebase tb;
     bool has_graphics;
-    sPAPROptionVector *ov5;         /* QEMU-supported option vectors */
-    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
-    bool cas_reboot;
-    bool cas_legacy_guest_workaround;
 
     Notifier epow_notifier;
     QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
     bool use_hotplug_event_source;
     sPAPREventSource *event_sources;
 
+    /* ibm,client-architecture-support option negotiation */
+    bool cas_reboot;
+    bool cas_legacy_guest_workaround;
+    sPAPROptionVector *ov5;         /* QEMU-supported option vectors */
+    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
+    uint32_t max_compat_pvr;
+
     /* Migration state */
     int htab_save_index;
     bool htab_first_pass;
@@ -639,6 +642,7 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
                                             uint32_t count, uint32_t index);
 void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
                                                uint32_t count, uint32_t index);
+void spapr_cpu_parse_features(sPAPRMachineState *spapr);
 void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
                                     sPAPRMachineState *spapr);
 
diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index e8ec1e1..e72839f 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -24,9 +24,11 @@
 #include "sysemu/cpus.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
 #include "cpu-models.h"
 
 typedef struct {
+    const char *name;
     uint32_t pvr;
     uint64_t pcr;
     uint64_t pcr_level;
@@ -38,6 +40,7 @@ static const CompatInfo compat_table[] = {
      * Ordered from oldest to newest - the code relies on this
      */
     { /* POWER6, ISA2.05 */
+        .name = "power6",
         .pvr = CPU_POWERPC_LOGICAL_2_05,
         .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
                PCR_COMPAT_2_05 | PCR_TM_DIS | PCR_VSX_DIS,
@@ -45,24 +48,28 @@ static const CompatInfo compat_table[] = {
         .max_threads = 2,
     },
     { /* POWER7, ISA2.06 */
+        .name = "power7",
         .pvr = CPU_POWERPC_LOGICAL_2_06,
         .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
         .pcr_level = PCR_COMPAT_2_06,
         .max_threads = 4,
     },
     {
+        .name = "power7+",
         .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
         .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
         .pcr_level = PCR_COMPAT_2_06,
         .max_threads = 4,
     },
     { /* POWER8, ISA2.07 */
+        .name = "power8",
         .pvr = CPU_POWERPC_LOGICAL_2_07,
         .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07,
         .pcr_level = PCR_COMPAT_2_07,
         .max_threads = 8,
     },
     { /* POWER9, ISA3.00 */
+        .name = "power9",
         .pvr = CPU_POWERPC_LOGICAL_3_00,
         .pcr = PCR_COMPAT_3_00,
         .pcr_level = PCR_COMPAT_3_00,
@@ -189,3 +196,98 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
 
     return n_threads;
 }
+
+static void ppc_compat_prop_get(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    uint32_t compat_pvr = *((uint32_t *)opaque);
+    const char *value;
+
+    if (!compat_pvr) {
+        value = "";
+    } else {
+        const CompatInfo *compat = compat_by_pvr(compat_pvr);
+
+        g_assert(compat);
+
+        value = compat->name;
+    }
+
+    visit_type_str(v, name, (char **)&value, errp);
+}
+
+static void ppc_compat_prop_set(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    Error *error = NULL;
+    char *value;
+    uint32_t compat_pvr;
+
+    visit_type_str(v, name, &value, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    if (strcmp(value, "") == 0) {
+        compat_pvr = 0;
+    } else {
+        int i;
+        const CompatInfo *compat = NULL;
+
+        for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
+            if (strcmp(value, compat_table[i].name) == 0) {
+                compat = &compat_table[i];
+                break;
+
+            }
+        }
+
+        if (!compat) {
+            error_setg(errp, "Invalid compatibility mode \"%s\"", value);
+            goto out;
+        }
+        compat_pvr = compat->pvr;
+    }
+
+    *((uint32_t *)opaque) = compat_pvr;
+
+out:
+    g_free(value);
+}
+
+void ppc_compat_add_property(Object *obj, const char *name,
+                             uint32_t *compat_pvr, const char *basedesc,
+                             Error **errp)
+{
+    Error *local_err = NULL;
+    gchar *namesv[ARRAY_SIZE(compat_table) + 1];
+    gchar *names, *desc;
+    int i;
+
+    object_property_add(obj, name, "string",
+                        ppc_compat_prop_get, ppc_compat_prop_set, NULL,
+                        compat_pvr, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
+        /*
+         * Have to discard const here, because g_strjoinv() takes
+         * (gchar **), not (const gchar **) :(
+         */
+        namesv[i] = (gchar *)compat_table[i].name;
+    }
+    namesv[ARRAY_SIZE(compat_table)] = NULL;
+
+    names = g_strjoinv(", ", namesv);
+    desc = g_strdup_printf("%s. Valid values are %s.", basedesc, names);
+    object_property_set_description(obj, name, desc, &local_err);
+
+    g_free(names);
+    g_free(desc);
+
+out:
+    error_propagate(errp, local_err);
+}
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 401e10e..4517b4b 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1189,7 +1189,6 @@ typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
  * PowerPCCPU:
  * @env: #CPUPPCState
  * @cpu_dt_id: CPU index used in the device tree. KVM uses this index too
- * @max_compat: Maximal supported logical PVR from the command line
  * @compat_pvr: Current logical PVR, zero if in "raw" mode
  *
  * A PowerPC CPU.
@@ -1201,7 +1200,6 @@ struct PowerPCCPU {
 
     CPUPPCState env;
     int cpu_dt_id;
-    uint32_t max_compat;
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
     Object *intc;
@@ -1374,6 +1372,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
 void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
 #endif
 int ppc_compat_max_threads(PowerPCCPU *cpu);
+void ppc_compat_add_property(Object *obj, const char *name,
+                             uint32_t *compat_pvr, const char *basedesc,
+                             Error **errp);
 #endif /* defined(TARGET_PPC64) */
 
 #include "exec/cpu-all.h"
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 56a0ab2..e837cd2 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -33,6 +33,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc.h"
 #include "mmu-book3s-v3.h"
+#include "sysemu/qtest.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -8413,73 +8414,38 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
     pcc->l1_icache_size = 0x10000;
 }
 
-static void powerpc_get_compat(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
-{
-    char *value = (char *)"";
-    Property *prop = opaque;
-    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
-
-    switch (*max_compat) {
-    case CPU_POWERPC_LOGICAL_2_05:
-        value = (char *)"power6";
-        break;
-    case CPU_POWERPC_LOGICAL_2_06:
-        value = (char *)"power7";
-        break;
-    case CPU_POWERPC_LOGICAL_2_07:
-        value = (char *)"power8";
-        break;
-    case 0:
-        break;
-    default:
-        error_report("Internal error: compat is set to %x", *max_compat);
-        abort();
-        break;
-    }
-
-    visit_type_str(v, name, &value, errp);
-}
-
-static void powerpc_set_compat(Object *obj, Visitor *v, const char *name,
-                               void *opaque, Error **errp)
+/*
+ * The CPU used to have a "compat" property which set the
+ * compatibility mode PVR.  However, this was conceptually broken - it
+ * only makes sense on the pseries machine type (otherwise the guest
+ * owns the PCR and can control the compatibility mode itself).  It's
+ * been replaced with the 'max-cpu-compat' property on the pseries
+ * machine type.  For backwards compatibility, pseries specially
+ * parses the -cpu parameter and converts old compat= parameters into
+ * the appropriate machine parameters.  This stub implementation of
+ * the parameter catches any uses on explicitly created CPUs.
+ */
+static void getset_compat_deprecated(Object *obj, Visitor *v, const char *name,
+                                     void *opaque, Error **errp)
 {
-    Error *error = NULL;
-    char *value = NULL;
-    Property *prop = opaque;
-    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
-
-    visit_type_str(v, name, &value, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-
-    if (strcmp(value, "power6") == 0) {
-        *max_compat = CPU_POWERPC_LOGICAL_2_05;
-    } else if (strcmp(value, "power7") == 0) {
-        *max_compat = CPU_POWERPC_LOGICAL_2_06;
-    } else if (strcmp(value, "power8") == 0) {
-        *max_compat = CPU_POWERPC_LOGICAL_2_07;
-    } else {
-        error_setg(errp, "Invalid compatibility mode \"%s\"", value);
+    if (!qtest_enabled()) {
+        error_report("CPU 'compat' property is deprecated and has no effect; "
+                     "use max-cpu-compat machine property instead");
     }
-
-    g_free(value);
+    visit_type_null(v, name, NULL);
 }
 
-static PropertyInfo powerpc_compat_propinfo = {
+static PropertyInfo ppc_compat_deprecated_propinfo = {
     .name = "str",
-    .description = "compatibility mode, power6/power7/power8",
-    .get = powerpc_get_compat,
-    .set = powerpc_set_compat,
+    .description = "compatibility mode (deprecated)",
+    .get = getset_compat_deprecated,
+    .set = getset_compat_deprecated,
 };
-
-#define DEFINE_PROP_POWERPC_COMPAT(_n, _s, _f) \
-    DEFINE_PROP(_n, _s, _f, powerpc_compat_propinfo, uint32_t)
-
 static Property powerpc_servercpu_properties[] = {
-    DEFINE_PROP_POWERPC_COMPAT("compat", PowerPCCPU, max_compat),
+    {
+        .name = "compat",
+        .info = &ppc_compat_deprecated_propinfo,
+    },
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.9.4

[Qemu-devel] [PATCHv4 4/5] pseries: Reset CPU compatibility mode

[David Gibson]

Currently, the CPU compatibility mode is set when the cpu is initialized,
then again when the guest negotiates features.  This means if a guest
negotiates a compatibility mode, then reboots, that compatibility mode
will be retained across the reset.

Usually that will get overridden when features are negotiated on the next
boot, but it's still not really correct.  This patch moves the initial set
up of the compatibility mode from cpu init to reset time.  The mode *is*
retained if the reboot was caused by the feature negotiation (it might
be important in that case, though it's unlikely).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c          |  2 ++
 hw/ppc/spapr_cpu_core.c | 10 ----------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3c4e88f..2821b7e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1348,6 +1348,8 @@ static void ppc_spapr_reset(void)
     if (!spapr->cas_reboot) {
         spapr_ovec_cleanup(spapr->ov5_cas);
         spapr->ov5_cas = spapr_ovec_new();
+
+        ppc_set_compat_all(spapr->max_compat_pvr, &error_abort);
     }
 
     fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ab4102b..7cc7320 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -122,16 +122,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     /* Enable PAPR mode in TCG or KVM */
     cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
 
-    if (spapr->max_compat_pvr) {
-        Error *local_err = NULL;
-
-        ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-    }
-
     qemu_register_reset(spapr_cpu_reset, cpu);
     spapr_cpu_reset(cpu);
 }
-- 
2.9.4

[Qemu-devel] [PATCHv4 5/5] ppc: Rework CPU compatibility testing across migration

[David Gibson]

Migrating between different CPU versions is a bit complicated for ppc.
A long time ago, we ensured identical CPU versions at either end by
checking the PVR had the same value.  However, this breaks under KVM
HV, because we always have to use the host's PVR - it's not
virtualized.  That would mean we couldn't migrate between hosts with
different PVRs, even if the CPUs are close enough to compatible in
practice (sometimes identical cores with different surrounding logic
have different PVRs, so this happens in practice quite often).

So, we removed the PVR check, but instead checked that several flags
indicating supported instructions matched.  This turns out to be a bad
idea, because those instruction masks are not architected information, but
essentially a TCG implementation detail.  So changes to qemu internal CPU
modelling can break migration - this happened between qemu-2.6 and
qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
removal of old migration mistakes".

Now, verification of CPU compatibility across a migration basically doesn't
happen.  We simply ignore the PVR of the incoming migration, and hope the
cpu on the destination is close enough to work.

Now that we've cleaned up handling of processor compatibility modes for
pseries machine type, we can do better.  We allow migration if:

    * The source and destination PVRs are for the same type of CPU, as
      determined by CPU class's pvr_match function
OR  * When the source was in a compatibility mode, and the destination CPU
      supports the same compatibility mode

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/machine.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 3 deletions(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 6cb3a48..2c6d9dc 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -8,6 +8,7 @@
 #include "helper_regs.h"
 #include "mmu-hash64.h"
 #include "migration/cpu.h"
+#include "qapi/error.h"
 
 static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
 {
@@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
     }
 }
 
+/*
+ * Determine if a given PVR is a "close enough" match to the CPU
+ * object.  For TCG and KVM PR it would probably be sufficient to
+ * require an exact PVR match.  However for KVM HV the user is
+ * restricted to a PVR exactly matching the host CPU.  The correct way
+ * to handle this is to put the guest into an architected
+ * compatibility mode.  However, to allow a more forgiving transition
+ * and migration from before this was widely done, we allow migration
+ * between sufficiently similar PVRs, as determined by the CPU class's
+ * pvr_match() hook.
+ */
+static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    if (pvr == pcc->pvr) {
+        return true;
+    }
+    if (pcc->pvr_match) {
+        return pcc->pvr_match(pcc, pvr);
+    }
+    return false;
+}
+
 static int cpu_post_load(void *opaque, int version_id)
 {
     PowerPCCPU *cpu = opaque;
@@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
     target_ulong msr;
 
     /*
-     * We always ignore the source PVR. The user or management
-     * software has to take care of running QEMU in a compatible mode.
+     * If we're operating in compat mode, we should be ok as long as
+     * the destination supports the same compatiblity mode.
+     *
+     * Otherwise, however, we require that the destination has exactly
+     * the same CPU model as the source.
      */
-    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
+
+#if defined(TARGET_PPC64)
+    if (cpu->compat_pvr) {
+        Error *local_err = NULL;
+
+        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(local_err);
+            return -1;
+        }
+    } else
+#endif
+    {
+        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
+            return -1;
+        }
+    }
+
     env->lr = env->spr[SPR_LR];
     env->ctr = env->spr[SPR_CTR];
     cpu_write_xer(env, env->spr[SPR_XER]);
@@ -560,6 +606,25 @@ static const VMStateDescription vmstate_tlbmas = {
     }
 };
 
+static bool compat_needed(void *opaque)
+{
+    PowerPCCPU *cpu = opaque;
+
+    assert(!(cpu->compat_pvr && !cpu->vhyp));
+    return (cpu->compat_pvr != 0);
+}
+
+static const VMStateDescription vmstate_compat = {
+    .name = "cpu/compat",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = compat_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
     .version_id = 5,
@@ -613,6 +678,7 @@ const VMStateDescription vmstate_ppc_cpu = {
         &vmstate_tlb6xx,
         &vmstate_tlbemb,
         &vmstate_tlbmas,
+        &vmstate_compat,
         NULL
     }
 };
-- 
2.9.4

Re: [Qemu-devel] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm

[Greg Kurz]

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

On Fri, 26 May 2017 15:23:16 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> As a rule, CPU internal state should never be updated when
> !cpu->kvm_vcpu_dirty (or the HAX equivalent).  If that is done, then
> subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
> will clobber state.
> 
> However, we routinely do this during a loadvm or incoming migration.
> Usually this is called shortly after a reset, which will clear all the cpu
> dirty flags with cpu_synchronize_all_post_reset().  Nothing is expected
> to set the dirty flags again before the cpu state is loaded from the
> incoming stream.
> 
> This means that it isn't safe to call cpu_synchronize_state() from a
> post_load handler, which is non-obvious and potentially inconvenient.
> 
> We could cpu_synchronize_all_state() before the loadvm, but that would be
> overkill since a) we expect the state to already be synchronized from the
> reset and b) we expect to completely rewrite the state with a call to
> cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().
> 
> To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
> associated helpers, which simply marks the cpu state as dirty without
> actually changing anything.  i.e. it says we want to discard any existing
> KVM (or HAX) state and replace it with what we're going to load.
> 

This makes sense and looks nicer than adding a post-load specific path to
ppc_set_compat() indeed.

Just one remark below.

> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dave Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  cpus.c                    |  9 +++++++++
>  include/sysemu/cpus.h     |  1 +
>  include/sysemu/hax.h      |  1 +
>  include/sysemu/hw_accel.h | 10 ++++++++++
>  include/sysemu/kvm.h      |  1 +
>  kvm-all.c                 | 10 ++++++++++
>  migration/savevm.c        |  2 ++
>  target/i386/hax-all.c     | 10 ++++++++++
>  8 files changed, 44 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 516e5cb..6398439 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void)
>      }
>  }
>  
> +void cpu_synchronize_all_pre_loadvm(void)
> +{
> +    CPUState *cpu;
> +
> +    CPU_FOREACH(cpu) {
> +        cpu_synchronize_pre_loadvm(cpu);
> +    }
> +}
> +
>  static int do_vm_stop(RunState state)
>  {
>      int ret = 0;
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index a8053f1..731756d 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
>  void cpu_synchronize_all_states(void);
>  void cpu_synchronize_all_post_reset(void);
>  void cpu_synchronize_all_post_init(void);
> +void cpu_synchronize_all_pre_loadvm(void);
>  
>  void qtest_clock_warp(int64_t dest);
>  
> diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h
> index d9f0239..232a68a 100644
> --- a/include/sysemu/hax.h
> +++ b/include/sysemu/hax.h
> @@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size);
>  void hax_cpu_synchronize_state(CPUState *cpu);
>  void hax_cpu_synchronize_post_reset(CPUState *cpu);
>  void hax_cpu_synchronize_post_init(CPUState *cpu);
> +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu);
>  
>  #ifdef CONFIG_HAX
>  
> diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
> index c9b3105..469ffda 100644
> --- a/include/sysemu/hw_accel.h
> +++ b/include/sysemu/hw_accel.h
> @@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
>      }
>  }
>  
> +static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
> +{
> +    if (kvm_enabled()) {
> +        kvm_cpu_synchronize_pre_loadvm(cpu);
> +    }
> +    if (hax_enabled()) {
> +        hax_cpu_synchronize_pre_loadvm(cpu);
> +    }
> +}
> +
>  #endif /* QEMU_HW_ACCEL_H */
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 5cc83f2..a45c145 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>  void kvm_cpu_synchronize_state(CPUState *cpu);
>  void kvm_cpu_synchronize_post_reset(CPUState *cpu);
>  void kvm_cpu_synchronize_post_init(CPUState *cpu);
> +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
>  
>  void kvm_init_cpu_signals(CPUState *cpu);
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index 90b8573..a8485bd 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu)
>      run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
>  }
>  
> +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
> +{
> +    cpu->kvm_vcpu_dirty = true;
> +}
> +
> +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
> +{
> +    run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);

Do we really need to run_on_cpu() since we only set the dirty flag ?

> +}
> +
>  #ifdef KVM_HAVE_MCE_INJECTION
>  static __thread void *pending_sigbus_addr;
>  static __thread int pending_sigbus_code;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d971e5e..6391131 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2011,6 +2011,8 @@ int qemu_loadvm_state(QEMUFile *f)
>          }
>      }
>  
> +    cpu_synchronize_all_pre_loadvm();
> +
>      ret = qemu_loadvm_state_main(f, mis);
>      qemu_event_set(&mis->main_thread_load_event);
>  
> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
> index ef13015..1306722 100644
> --- a/target/i386/hax-all.c
> +++ b/target/i386/hax-all.c
> @@ -635,6 +635,16 @@ void hax_cpu_synchronize_post_init(CPUState *cpu)
>      run_on_cpu(cpu, do_hax_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
>  }
>  
> +static void do_hax_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
> +{
> +    cpu->hax_vcpu_dirty = true;
> +}
> +
> +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu)
> +{
> +    run_on_cpu(cpu, do_hax_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
> +}
> +
>  int hax_smp_cpu_exec(CPUState *cpu)
>  {
>      CPUArchState *env = (CPUArchState *) (cpu->env_ptr);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Greg Kurz]

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

On Fri, 26 May 2017 15:23:14 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

[...]
> 
> 
> Changes since v3:
>   * Backwards compatible -cpu handling now removes compat= option from
>     options passed on to the cpu, so it doesn't trigger further warnings

This seems to also have another interesting effect.

getset_compat_deprecated() could be called either during CPU realization from:

object_property_parse()
{
    Visitor *v = string_input_visitor_new(string);
    object_property_set(obj, v, name, errp);
    ...
}

or during a QOM set operation from:

void object_property_set_qobject(Object *obj, QObject *value,
                                 const char *name, Error **errp)
{
    Visitor *v;

    v = qobject_input_visitor_new(value);
    object_property_set(obj, v, name, errp);
    ...
}

or similarly during a QOM get operation with a QObject output visitor.

The realization path no longer exists with patch 2, so you don't need
to implement a null string input visitor anymore.

This means that patch 1 is no longer needed if I get things right but
you probably want Markus to second that.

>   * Add a migration fix make cpu_synchronize_state() safe in post_load
>     handlers, which in turn fixes a bug in 5/5.
>   * A number of bugfixes and other tweaks suggested by feedback on v2.
> 
> Changes since RFCv2:
>   * Many patches dropped, since they're already merged
>   * Rebased, fixed conflicts
>   * Restored support for backwards migration (wasn't as complicated as
>     I thought)
>   * Updated final patch's description to more accurately reflect the
>     logic
> 
> Changes since RFCv1:
>   * Change CAS logic to prefer compatibility modes over raw mode
>   * Simplified by giving up on half-hearted attempts to maintain
>     backwards migration
>   * Folded migration stream changes into a single patch
>   * Removed some preliminary patches which are already merged
> 
> David Gibson (4):
>   migration: Mark CPU states dirty before incoming migration/loadvm
>   pseries: Move CPU compatibility property to machine
>   pseries: Reset CPU compatibility mode
>   ppc: Rework CPU compatibility testing across migration
> 
> Greg Kurz (1):
>   qapi: add explicit null to string input and output visitors
> 
>  cpus.c                       |   9 ++++
>  hw/ppc/spapr.c               |   8 +++-
>  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
>  hw/ppc/spapr_hcall.c         |   8 ++--
>  include/hw/ppc/spapr.h       |  12 +++--
>  include/sysemu/cpus.h        |   1 +
>  include/sysemu/hax.h         |   1 +
>  include/sysemu/hw_accel.h    |  10 +++++
>  include/sysemu/kvm.h         |   1 +
>  kvm-all.c                    |  10 +++++
>  migration/savevm.c           |   2 +
>  qapi/string-input-visitor.c  |  11 +++++
>  qapi/string-output-visitor.c |  14 ++++++
>  target/i386/hax-all.c        |  10 +++++
>  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
>  target/ppc/cpu.h             |   5 ++-
>  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
>  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
>  18 files changed, 340 insertions(+), 84 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm

[David Gibson]

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

On Mon, May 29, 2017 at 10:46:05PM +0200, Greg Kurz wrote:
> On Fri, 26 May 2017 15:23:16 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > As a rule, CPU internal state should never be updated when
> > !cpu->kvm_vcpu_dirty (or the HAX equivalent).  If that is done, then
> > subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
> > will clobber state.
> > 
> > However, we routinely do this during a loadvm or incoming migration.
> > Usually this is called shortly after a reset, which will clear all the cpu
> > dirty flags with cpu_synchronize_all_post_reset().  Nothing is expected
> > to set the dirty flags again before the cpu state is loaded from the
> > incoming stream.
> > 
> > This means that it isn't safe to call cpu_synchronize_state() from a
> > post_load handler, which is non-obvious and potentially inconvenient.
> > 
> > We could cpu_synchronize_all_state() before the loadvm, but that would be
> > overkill since a) we expect the state to already be synchronized from the
> > reset and b) we expect to completely rewrite the state with a call to
> > cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().
> > 
> > To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
> > associated helpers, which simply marks the cpu state as dirty without
> > actually changing anything.  i.e. it says we want to discard any existing
> > KVM (or HAX) state and replace it with what we're going to load.
> > 
> 
> This makes sense and looks nicer than adding a post-load specific path to
> ppc_set_compat() indeed.
> 
> Just one remark below.
> 
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Dave Gilbert <dgilbert@redhat.com>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  cpus.c                    |  9 +++++++++
> >  include/sysemu/cpus.h     |  1 +
> >  include/sysemu/hax.h      |  1 +
> >  include/sysemu/hw_accel.h | 10 ++++++++++
> >  include/sysemu/kvm.h      |  1 +
> >  kvm-all.c                 | 10 ++++++++++
> >  migration/savevm.c        |  2 ++
> >  target/i386/hax-all.c     | 10 ++++++++++
> >  8 files changed, 44 insertions(+)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 516e5cb..6398439 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void)
> >      }
> >  }
> >  
> > +void cpu_synchronize_all_pre_loadvm(void)
> > +{
> > +    CPUState *cpu;
> > +
> > +    CPU_FOREACH(cpu) {
> > +        cpu_synchronize_pre_loadvm(cpu);
> > +    }
> > +}
> > +
> >  static int do_vm_stop(RunState state)
> >  {
> >      int ret = 0;
> > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> > index a8053f1..731756d 100644
> > --- a/include/sysemu/cpus.h
> > +++ b/include/sysemu/cpus.h
> > @@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
> >  void cpu_synchronize_all_states(void);
> >  void cpu_synchronize_all_post_reset(void);
> >  void cpu_synchronize_all_post_init(void);
> > +void cpu_synchronize_all_pre_loadvm(void);
> >  
> >  void qtest_clock_warp(int64_t dest);
> >  
> > diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h
> > index d9f0239..232a68a 100644
> > --- a/include/sysemu/hax.h
> > +++ b/include/sysemu/hax.h
> > @@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size);
> >  void hax_cpu_synchronize_state(CPUState *cpu);
> >  void hax_cpu_synchronize_post_reset(CPUState *cpu);
> >  void hax_cpu_synchronize_post_init(CPUState *cpu);
> > +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu);
> >  
> >  #ifdef CONFIG_HAX
> >  
> > diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
> > index c9b3105..469ffda 100644
> > --- a/include/sysemu/hw_accel.h
> > +++ b/include/sysemu/hw_accel.h
> > @@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
> >      }
> >  }
> >  
> > +static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
> > +{
> > +    if (kvm_enabled()) {
> > +        kvm_cpu_synchronize_pre_loadvm(cpu);
> > +    }
> > +    if (hax_enabled()) {
> > +        hax_cpu_synchronize_pre_loadvm(cpu);
> > +    }
> > +}
> > +
> >  #endif /* QEMU_HW_ACCEL_H */
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 5cc83f2..a45c145 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
> >  void kvm_cpu_synchronize_state(CPUState *cpu);
> >  void kvm_cpu_synchronize_post_reset(CPUState *cpu);
> >  void kvm_cpu_synchronize_post_init(CPUState *cpu);
> > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
> >  
> >  void kvm_init_cpu_signals(CPUState *cpu);
> >  
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 90b8573..a8485bd 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu)
> >      run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
> >  }
> >  
> > +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
> > +{
> > +    cpu->kvm_vcpu_dirty = true;
> > +}
> > +
> > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
> > +{
> > +    run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
> 
> Do we really need to run_on_cpu() since we only set the dirty flag ?

Um.. yeah.. I'm not sure.  I left it in out of paranoia, because I
wasn't sure if there could be memory ordering issues between the qemu
threads if we just set it from the migration thread.

I'm hoping Dave or Juan will have a better idea.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Tue, May 30, 2017 at 01:14:16AM +0200, Greg Kurz wrote:
> On Fri, 26 May 2017 15:23:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> [...]
> > 
> > 
> > Changes since v3:
> >   * Backwards compatible -cpu handling now removes compat= option from
> >     options passed on to the cpu, so it doesn't trigger further warnings
> 
> This seems to also have another interesting effect.
> 
> getset_compat_deprecated() could be called either during CPU realization from:
> 
> object_property_parse()
> {
>     Visitor *v = string_input_visitor_new(string);
>     object_property_set(obj, v, name, errp);
>     ...
> }
> 
> or during a QOM set operation from:
> 
> void object_property_set_qobject(Object *obj, QObject *value,
>                                  const char *name, Error **errp)
> {
>     Visitor *v;
> 
>     v = qobject_input_visitor_new(value);
>     object_property_set(obj, v, name, errp);
>     ...
> }
> 
> or similarly during a QOM get operation with a QObject output visitor.
> 
> The realization path no longer exists with patch 2, so you don't need
> to implement a null string input visitor anymore.

s/patch 2/patch 3/?

Is that true though?  It shouldn't get called through that path in
practice, because we strip the compat property from the cpu object
properties.  But it could get called if either a) the user explicitly
creates a cpu object with -device CPU,compat=whatever or b) if the
user uses the compat= property with a machine type other than pseries.

Of course the user *shouldn't* do either of those things, but
providing a meaningful error if they do is pretty much the whole
purpose of this getter/setter method.

> 
> This means that patch 1 is no longer needed if I get things right but
> you probably want Markus to second that.
> 
> >   * Add a migration fix make cpu_synchronize_state() safe in post_load
> >     handlers, which in turn fixes a bug in 5/5.
> >   * A number of bugfixes and other tweaks suggested by feedback on v2.
> > 
> > Changes since RFCv2:
> >   * Many patches dropped, since they're already merged
> >   * Rebased, fixed conflicts
> >   * Restored support for backwards migration (wasn't as complicated as
> >     I thought)
> >   * Updated final patch's description to more accurately reflect the
> >     logic
> > 
> > Changes since RFCv1:
> >   * Change CAS logic to prefer compatibility modes over raw mode
> >   * Simplified by giving up on half-hearted attempts to maintain
> >     backwards migration
> >   * Folded migration stream changes into a single patch
> >   * Removed some preliminary patches which are already merged
> > 
> > David Gibson (4):
> >   migration: Mark CPU states dirty before incoming migration/loadvm
> >   pseries: Move CPU compatibility property to machine
> >   pseries: Reset CPU compatibility mode
> >   ppc: Rework CPU compatibility testing across migration
> > 
> > Greg Kurz (1):
> >   qapi: add explicit null to string input and output visitors
> > 
> >  cpus.c                       |   9 ++++
> >  hw/ppc/spapr.c               |   8 +++-
> >  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
> >  hw/ppc/spapr_hcall.c         |   8 ++--
> >  include/hw/ppc/spapr.h       |  12 +++--
> >  include/sysemu/cpus.h        |   1 +
> >  include/sysemu/hax.h         |   1 +
> >  include/sysemu/hw_accel.h    |  10 +++++
> >  include/sysemu/kvm.h         |   1 +
> >  kvm-all.c                    |  10 +++++
> >  migration/savevm.c           |   2 +
> >  qapi/string-input-visitor.c  |  11 +++++
> >  qapi/string-output-visitor.c |  14 ++++++
> >  target/i386/hax-all.c        |  10 +++++
> >  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/cpu.h             |   5 ++-
> >  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
> >  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
> >  18 files changed, 340 insertions(+), 84 deletions(-)
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Greg Kurz]

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

On Tue, 30 May 2017 16:18:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, May 30, 2017 at 01:14:16AM +0200, Greg Kurz wrote:
> > On Fri, 26 May 2017 15:23:14 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > [...]  
> > > 
> > > 
> > > Changes since v3:
> > >   * Backwards compatible -cpu handling now removes compat= option from
> > >     options passed on to the cpu, so it doesn't trigger further warnings  
> > 
> > This seems to also have another interesting effect.
> > 
> > getset_compat_deprecated() could be called either during CPU realization from:
> > 
> > object_property_parse()
> > {
> >     Visitor *v = string_input_visitor_new(string);
> >     object_property_set(obj, v, name, errp);
> >     ...
> > }
> > 
> > or during a QOM set operation from:
> > 
> > void object_property_set_qobject(Object *obj, QObject *value,
> >                                  const char *name, Error **errp)
> > {
> >     Visitor *v;
> > 
> >     v = qobject_input_visitor_new(value);
> >     object_property_set(obj, v, name, errp);
> >     ...
> > }
> > 
> > or similarly during a QOM get operation with a QObject output visitor.
> > 
> > The realization path no longer exists with patch 2, so you don't need
> > to implement a null string input visitor anymore.  
> 
> s/patch 2/patch 3/?
> 

Yes indeed, sorry :)

> Is that true though?  It shouldn't get called through that path in
> practice, because we strip the compat property from the cpu object
> properties.  But it could get called if either a) the user explicitly
> creates a cpu object with -device CPU,compat=whatever or b) if the

Unless I'm missing something, properties of the CPU objects aren't
exposed by CPU devices:

qemu-system-ppc64 -machine pseries \
                  -device POWER8_v2.0-spapr-cpu-core,help
POWER8_v2.0-spapr-cpu-core.core-id=int
POWER8_v2.0-spapr-cpu-core.node-id=int32
POWER8_v2.0-spapr-cpu-core.nr-threads=int

and creation of CPU objects with -object isn't possible either since
they don't inherit from the TYPE_USER_CREATABLE class.

> user uses the compat= property with a machine type other than pseries.
> 

But yes, it is still possible to go through the object_property_parse()
path with another machine type indeed.

> Of course the user *shouldn't* do either of those things, but
> providing a meaningful error if they do is pretty much the whole
> purpose of this getter/setter method.
> 

All old non-pseries machine types already complain when started with
a POWER7 or newer CPU. Providing the extra error message looks weird:

qemu-system-ppc64 -machine ppce500 \
                  -cpu POWER7,compat=power6
qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
 use max-cpu-compat machine property instead
MMU model 983043 not supported by this machine.

but I guess it's better than crashing. :)

> > 
> > This means that patch 1 is no longer needed if I get things right but
> > you probably want Markus to second that.
> >   
> > >   * Add a migration fix make cpu_synchronize_state() safe in post_load
> > >     handlers, which in turn fixes a bug in 5/5.
> > >   * A number of bugfixes and other tweaks suggested by feedback on v2.
> > > 
> > > Changes since RFCv2:
> > >   * Many patches dropped, since they're already merged
> > >   * Rebased, fixed conflicts
> > >   * Restored support for backwards migration (wasn't as complicated as
> > >     I thought)
> > >   * Updated final patch's description to more accurately reflect the
> > >     logic
> > > 
> > > Changes since RFCv1:
> > >   * Change CAS logic to prefer compatibility modes over raw mode
> > >   * Simplified by giving up on half-hearted attempts to maintain
> > >     backwards migration
> > >   * Folded migration stream changes into a single patch
> > >   * Removed some preliminary patches which are already merged
> > > 
> > > David Gibson (4):
> > >   migration: Mark CPU states dirty before incoming migration/loadvm
> > >   pseries: Move CPU compatibility property to machine
> > >   pseries: Reset CPU compatibility mode
> > >   ppc: Rework CPU compatibility testing across migration
> > > 
> > > Greg Kurz (1):
> > >   qapi: add explicit null to string input and output visitors
> > > 
> > >  cpus.c                       |   9 ++++
> > >  hw/ppc/spapr.c               |   8 +++-
> > >  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
> > >  hw/ppc/spapr_hcall.c         |   8 ++--
> > >  include/hw/ppc/spapr.h       |  12 +++--
> > >  include/sysemu/cpus.h        |   1 +
> > >  include/sysemu/hax.h         |   1 +
> > >  include/sysemu/hw_accel.h    |  10 +++++
> > >  include/sysemu/kvm.h         |   1 +
> > >  kvm-all.c                    |  10 +++++
> > >  migration/savevm.c           |   2 +
> > >  qapi/string-input-visitor.c  |  11 +++++
> > >  qapi/string-output-visitor.c |  14 ++++++
> > >  target/i386/hax-all.c        |  10 +++++
> > >  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
> > >  target/ppc/cpu.h             |   5 ++-
> > >  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
> > >  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
> > >  18 files changed, 340 insertions(+), 84 deletions(-)
> > >   
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm

[Dr. David Alan Gilbert]

* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Mon, May 29, 2017 at 10:46:05PM +0200, Greg Kurz wrote:
> > On Fri, 26 May 2017 15:23:16 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > As a rule, CPU internal state should never be updated when
> > > !cpu->kvm_vcpu_dirty (or the HAX equivalent).  If that is done, then
> > > subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
> > > will clobber state.
> > > 
> > > However, we routinely do this during a loadvm or incoming migration.
> > > Usually this is called shortly after a reset, which will clear all the cpu
> > > dirty flags with cpu_synchronize_all_post_reset().  Nothing is expected
> > > to set the dirty flags again before the cpu state is loaded from the
> > > incoming stream.
> > > 
> > > This means that it isn't safe to call cpu_synchronize_state() from a
> > > post_load handler, which is non-obvious and potentially inconvenient.
> > > 
> > > We could cpu_synchronize_all_state() before the loadvm, but that would be
> > > overkill since a) we expect the state to already be synchronized from the
> > > reset and b) we expect to completely rewrite the state with a call to
> > > cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().
> > > 
> > > To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
> > > associated helpers, which simply marks the cpu state as dirty without
> > > actually changing anything.  i.e. it says we want to discard any existing
> > > KVM (or HAX) state and replace it with what we're going to load.
> > > 
> > 
> > This makes sense and looks nicer than adding a post-load specific path to
> > ppc_set_compat() indeed.
> > 
> > Just one remark below.
> > 
> > > Cc: Juan Quintela <quintela@redhat.com>
> > > Cc: Dave Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  cpus.c                    |  9 +++++++++
> > >  include/sysemu/cpus.h     |  1 +
> > >  include/sysemu/hax.h      |  1 +
> > >  include/sysemu/hw_accel.h | 10 ++++++++++
> > >  include/sysemu/kvm.h      |  1 +
> > >  kvm-all.c                 | 10 ++++++++++
> > >  migration/savevm.c        |  2 ++
> > >  target/i386/hax-all.c     | 10 ++++++++++
> > >  8 files changed, 44 insertions(+)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 516e5cb..6398439 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void)
> > >      }
> > >  }
> > >  
> > > +void cpu_synchronize_all_pre_loadvm(void)
> > > +{
> > > +    CPUState *cpu;
> > > +
> > > +    CPU_FOREACH(cpu) {
> > > +        cpu_synchronize_pre_loadvm(cpu);
> > > +    }
> > > +}
> > > +
> > >  static int do_vm_stop(RunState state)
> > >  {
> > >      int ret = 0;
> > > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> > > index a8053f1..731756d 100644
> > > --- a/include/sysemu/cpus.h
> > > +++ b/include/sysemu/cpus.h
> > > @@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
> > >  void cpu_synchronize_all_states(void);
> > >  void cpu_synchronize_all_post_reset(void);
> > >  void cpu_synchronize_all_post_init(void);
> > > +void cpu_synchronize_all_pre_loadvm(void);
> > >  
> > >  void qtest_clock_warp(int64_t dest);
> > >  
> > > diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h
> > > index d9f0239..232a68a 100644
> > > --- a/include/sysemu/hax.h
> > > +++ b/include/sysemu/hax.h
> > > @@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size);
> > >  void hax_cpu_synchronize_state(CPUState *cpu);
> > >  void hax_cpu_synchronize_post_reset(CPUState *cpu);
> > >  void hax_cpu_synchronize_post_init(CPUState *cpu);
> > > +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu);
> > >  
> > >  #ifdef CONFIG_HAX
> > >  
> > > diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
> > > index c9b3105..469ffda 100644
> > > --- a/include/sysemu/hw_accel.h
> > > +++ b/include/sysemu/hw_accel.h
> > > @@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
> > >      }
> > >  }
> > >  
> > > +static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
> > > +{
> > > +    if (kvm_enabled()) {
> > > +        kvm_cpu_synchronize_pre_loadvm(cpu);
> > > +    }
> > > +    if (hax_enabled()) {
> > > +        hax_cpu_synchronize_pre_loadvm(cpu);
> > > +    }
> > > +}
> > > +
> > >  #endif /* QEMU_HW_ACCEL_H */
> > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > > index 5cc83f2..a45c145 100644
> > > --- a/include/sysemu/kvm.h
> > > +++ b/include/sysemu/kvm.h
> > > @@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
> > >  void kvm_cpu_synchronize_state(CPUState *cpu);
> > >  void kvm_cpu_synchronize_post_reset(CPUState *cpu);
> > >  void kvm_cpu_synchronize_post_init(CPUState *cpu);
> > > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
> > >  
> > >  void kvm_init_cpu_signals(CPUState *cpu);
> > >  
> > > diff --git a/kvm-all.c b/kvm-all.c
> > > index 90b8573..a8485bd 100644
> > > --- a/kvm-all.c
> > > +++ b/kvm-all.c
> > > @@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu)
> > >      run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
> > >  }
> > >  
> > > +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
> > > +{
> > > +    cpu->kvm_vcpu_dirty = true;
> > > +}
> > > +
> > > +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
> > > +{
> > > +    run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
> > 
> > Do we really need to run_on_cpu() since we only set the dirty flag ?
> 
> Um.. yeah.. I'm not sure.  I left it in out of paranoia, because I
> wasn't sure if there could be memory ordering issues between the qemu
> threads if we just set it from the migration thread.
> 
> I'm hoping Dave or Juan will have a better idea.

I don't know the kvm cpu sync stuff well enough.

Dave

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCHv4 2/5] migration: Mark CPU states dirty before incoming migration/loadvm

[Juan Quintela]

David Gibson <david@gibson.dropbear.id.au> wrote:
> As a rule, CPU internal state should never be updated when
> !cpu->kvm_vcpu_dirty (or the HAX equivalent).  If that is done, then
> subsequent calls to cpu_synchronize_state() - usually safe and idempotent -
> will clobber state.
>
> However, we routinely do this during a loadvm or incoming migration.
> Usually this is called shortly after a reset, which will clear all the cpu
> dirty flags with cpu_synchronize_all_post_reset().  Nothing is expected
> to set the dirty flags again before the cpu state is loaded from the
> incoming stream.
>
> This means that it isn't safe to call cpu_synchronize_state() from a
> post_load handler, which is non-obvious and potentially inconvenient.
>
> We could cpu_synchronize_all_state() before the loadvm, but that would be
> overkill since a) we expect the state to already be synchronized from the
> reset and b) we expect to completely rewrite the state with a call to
> cpu_synchronize_all_post_init() at the end of qemu_loadvm_state().
>
> To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and
> associated helpers, which simply marks the cpu state as dirty without
> actually changing anything.  i.e. it says we want to discard any existing
> KVM (or HAX) state and replace it with what we're going to load.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dave Gilbert <dgilbert@redhat.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Juan Quintela <quintela@redhat.com>

>  
> +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)
> +{
> +    cpu->kvm_vcpu_dirty = true;
> +}
> +
> +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
> +{
> +    run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
> +}

They are exactly the same, does it make sense to only have a copy?

I don't really know, so I do the reviewed-by anyways.

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Tue, May 30, 2017 at 10:01:36AM +0200, Greg Kurz wrote:
> On Tue, 30 May 2017 16:18:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, May 30, 2017 at 01:14:16AM +0200, Greg Kurz wrote:
> > > On Fri, 26 May 2017 15:23:14 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > [...]  
> > > > 
> > > > 
> > > > Changes since v3:
> > > >   * Backwards compatible -cpu handling now removes compat= option from
> > > >     options passed on to the cpu, so it doesn't trigger further warnings  
> > > 
> > > This seems to also have another interesting effect.
> > > 
> > > getset_compat_deprecated() could be called either during CPU realization from:
> > > 
> > > object_property_parse()
> > > {
> > >     Visitor *v = string_input_visitor_new(string);
> > >     object_property_set(obj, v, name, errp);
> > >     ...
> > > }
> > > 
> > > or during a QOM set operation from:
> > > 
> > > void object_property_set_qobject(Object *obj, QObject *value,
> > >                                  const char *name, Error **errp)
> > > {
> > >     Visitor *v;
> > > 
> > >     v = qobject_input_visitor_new(value);
> > >     object_property_set(obj, v, name, errp);
> > >     ...
> > > }
> > > 
> > > or similarly during a QOM get operation with a QObject output visitor.
> > > 
> > > The realization path no longer exists with patch 2, so you don't need
> > > to implement a null string input visitor anymore.  
> > 
> > s/patch 2/patch 3/?
> > 
> 
> Yes indeed, sorry :)
> 
> > Is that true though?  It shouldn't get called through that path in
> > practice, because we strip the compat property from the cpu object
> > properties.  But it could get called if either a) the user explicitly
> > creates a cpu object with -device CPU,compat=whatever or b) if the
> 
> Unless I'm missing something, properties of the CPU objects aren't
> exposed by CPU devices:
> 
> qemu-system-ppc64 -machine pseries \
>                   -device POWER8_v2.0-spapr-cpu-core,help
> POWER8_v2.0-spapr-cpu-core.core-id=int
> POWER8_v2.0-spapr-cpu-core.node-id=int32
> POWER8_v2.0-spapr-cpu-core.nr-threads=int
> 
> and creation of CPU objects with -object isn't possible either since
> they don't inherit from the TYPE_USER_CREATABLE class.

Ah, true, I hadn't considered that.

> > user uses the compat= property with a machine type other than pseries.
> > 
> 
> But yes, it is still possible to go through the object_property_parse()
> path with another machine type indeed.

> > Of course the user *shouldn't* do either of those things, but
> > providing a meaningful error if they do is pretty much the whole
> > purpose of this getter/setter method.
> > 
> 
> All old non-pseries machine types already complain when started with
> a POWER7 or newer CPU. Providing the extra error message looks weird:
> 
> qemu-system-ppc64 -machine ppce500 \
>                   -cpu POWER7,compat=power6
> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
>  use max-cpu-compat machine property instead
> MMU model 983043 not supported by this machine.
> 
> but I guess it's better than crashing. :)

Well, sure POWER7 doesn't make sense for an e500 machine for other
reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
compat= doesn't.

> 
> > > 
> > > This means that patch 1 is no longer needed if I get things right but
> > > you probably want Markus to second that.
> > >   
> > > >   * Add a migration fix make cpu_synchronize_state() safe in post_load
> > > >     handlers, which in turn fixes a bug in 5/5.
> > > >   * A number of bugfixes and other tweaks suggested by feedback on v2.
> > > > 
> > > > Changes since RFCv2:
> > > >   * Many patches dropped, since they're already merged
> > > >   * Rebased, fixed conflicts
> > > >   * Restored support for backwards migration (wasn't as complicated as
> > > >     I thought)
> > > >   * Updated final patch's description to more accurately reflect the
> > > >     logic
> > > > 
> > > > Changes since RFCv1:
> > > >   * Change CAS logic to prefer compatibility modes over raw mode
> > > >   * Simplified by giving up on half-hearted attempts to maintain
> > > >     backwards migration
> > > >   * Folded migration stream changes into a single patch
> > > >   * Removed some preliminary patches which are already merged
> > > > 
> > > > David Gibson (4):
> > > >   migration: Mark CPU states dirty before incoming migration/loadvm
> > > >   pseries: Move CPU compatibility property to machine
> > > >   pseries: Reset CPU compatibility mode
> > > >   ppc: Rework CPU compatibility testing across migration
> > > > 
> > > > Greg Kurz (1):
> > > >   qapi: add explicit null to string input and output visitors
> > > > 
> > > >  cpus.c                       |   9 ++++
> > > >  hw/ppc/spapr.c               |   8 +++-
> > > >  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
> > > >  hw/ppc/spapr_hcall.c         |   8 ++--
> > > >  include/hw/ppc/spapr.h       |  12 +++--
> > > >  include/sysemu/cpus.h        |   1 +
> > > >  include/sysemu/hax.h         |   1 +
> > > >  include/sysemu/hw_accel.h    |  10 +++++
> > > >  include/sysemu/kvm.h         |   1 +
> > > >  kvm-all.c                    |  10 +++++
> > > >  migration/savevm.c           |   2 +
> > > >  qapi/string-input-visitor.c  |  11 +++++
> > > >  qapi/string-output-visitor.c |  14 ++++++
> > > >  target/i386/hax-all.c        |  10 +++++
> > > >  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
> > > >  target/ppc/cpu.h             |   5 ++-
> > > >  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
> > > >  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
> > > >  18 files changed, 340 insertions(+), 84 deletions(-)
> > > >   
> > >   
> > 
> > 
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Greg Kurz]

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

On Wed, 31 May 2017 12:57:48 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> [...]
> > All old non-pseries machine types already complain when started with
> > a POWER7 or newer CPU. Providing the extra error message looks weird:
> > 
> > qemu-system-ppc64 -machine ppce500 \
> >                   -cpu POWER7,compat=power6
> > qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> >  use max-cpu-compat machine property instead
> > MMU model 983043 not supported by this machine.
> > 
> > but I guess it's better than crashing. :)  
> 
> Well, sure POWER7 doesn't make sense for an e500 machine for other
> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> compat= doesn't.
> 

The powernv machine type doesn't even support CPU features at all:

    chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
    if (!object_class_by_name(chip_typename)) {
        error_report("invalid CPU model '%s' for %s machine",
                     machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
        exit(1);
    }

> >   
> > > > 
> > > > This means that patch 1 is no longer needed if I get things right but
> > > > you probably want Markus to second that.
> > > >     
> > > > >   * Add a migration fix make cpu_synchronize_state() safe in post_load
> > > > >     handlers, which in turn fixes a bug in 5/5.
> > > > >   * A number of bugfixes and other tweaks suggested by feedback on v2.
> > > > > 
> > > > > Changes since RFCv2:
> > > > >   * Many patches dropped, since they're already merged
> > > > >   * Rebased, fixed conflicts
> > > > >   * Restored support for backwards migration (wasn't as complicated as
> > > > >     I thought)
> > > > >   * Updated final patch's description to more accurately reflect the
> > > > >     logic
> > > > > 
> > > > > Changes since RFCv1:
> > > > >   * Change CAS logic to prefer compatibility modes over raw mode
> > > > >   * Simplified by giving up on half-hearted attempts to maintain
> > > > >     backwards migration
> > > > >   * Folded migration stream changes into a single patch
> > > > >   * Removed some preliminary patches which are already merged
> > > > > 
> > > > > David Gibson (4):
> > > > >   migration: Mark CPU states dirty before incoming migration/loadvm
> > > > >   pseries: Move CPU compatibility property to machine
> > > > >   pseries: Reset CPU compatibility mode
> > > > >   ppc: Rework CPU compatibility testing across migration
> > > > > 
> > > > > Greg Kurz (1):
> > > > >   qapi: add explicit null to string input and output visitors
> > > > > 
> > > > >  cpus.c                       |   9 ++++
> > > > >  hw/ppc/spapr.c               |   8 +++-
> > > > >  hw/ppc/spapr_cpu_core.c      |  62 +++++++++++++++++++++-----
> > > > >  hw/ppc/spapr_hcall.c         |   8 ++--
> > > > >  include/hw/ppc/spapr.h       |  12 +++--
> > > > >  include/sysemu/cpus.h        |   1 +
> > > > >  include/sysemu/hax.h         |   1 +
> > > > >  include/sysemu/hw_accel.h    |  10 +++++
> > > > >  include/sysemu/kvm.h         |   1 +
> > > > >  kvm-all.c                    |  10 +++++
> > > > >  migration/savevm.c           |   2 +
> > > > >  qapi/string-input-visitor.c  |  11 +++++
> > > > >  qapi/string-output-visitor.c |  14 ++++++
> > > > >  target/i386/hax-all.c        |  10 +++++
> > > > >  target/ppc/compat.c          | 102 +++++++++++++++++++++++++++++++++++++++++++
> > > > >  target/ppc/cpu.h             |   5 ++-
> > > > >  target/ppc/machine.c         |  72 ++++++++++++++++++++++++++++--
> > > > >  target/ppc/translate_init.c  |  86 +++++++++++-------------------------
> > > > >  18 files changed, 340 insertions(+), 84 deletions(-)
> > > > >     
> > > >     
> > > 
> > > 
> > >   
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[Suraj Jitindar Singh]

On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> Server class POWER CPUs have a "compat" property, which is used to
> set the
> backwards compatibility mode for the processor.  However, this only
> makes
> sense for machine types which don't give the guest access to
> hypervisor
> privilege - otherwise the compatibility level is under the guest's
> control.
> 
> To reflect this, this removes the CPU 'compat' property and instead
> creates a 'max-cpu-compat' property on the pseries machine.  Strictly
> speaking this breaks compatibility, but AFAIK the 'compat' option was
> never (directly) used with -device or device_add.
> 
> The option was used with -cpu.  So, to maintain compatibility, this
> patch adds a hack to the cpu option parsing to strip out any compat
> options supplied with -cpu and set them on the machine property
> instead of the now deprecated cpu property.

Generally looks good, a couple of comments below.

Suraj

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c              |   6 ++-
>  hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
>  hw/ppc/spapr_hcall.c        |   8 ++--
>  include/hw/ppc/spapr.h      |  12 ++++--
>  target/ppc/compat.c         | 102
> ++++++++++++++++++++++++++++++++++++++++++++
>  target/ppc/cpu.h            |   5 ++-
>  target/ppc/translate_init.c |  86 +++++++++++-----------------------
> ---
>  7 files changed, 202 insertions(+), 73 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..3c4e88f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> *machine)
>          machine->cpu_model = kvm_enabled() ? "host" : smc-
> >tcg_default_cpu;
>      }
>  
> -    ppc_cpu_parse_features(machine->cpu_model);
> +    spapr_cpu_parse_features(spapr);
>  
>      spapr_init_cpus(spapr);
>  
> @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
>                                      " place of standard EPOW events
> when possible"
>                                      " (required for memory hot-
> unplug support)",
>                                      NULL);
> +
> +    ppc_compat_add_property(obj, "max-cpu-compat", &spapr-
> >max_compat_pvr,
> +                            "Maximum permitted CPU compatibility
> mode",
> +                            &error_fatal);
>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index ff7058e..ab4102b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,58 @@
>  #include "sysemu/numa.h"
>  #include "qemu/error-report.h"
>  
> +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> +{
> +    /*
> +     * Backwards compatibility hack:
> +     *
> +     *   CPUs had a "compat=" property which didn't make sense for
> +     *   anything except pseries.  It was replaced by "max-cpu-
> compat"
> +     *   machine option.  This supports old command lines like
> +     *       -cpu POWER8,compat=power7
> +     *   By stripping the compat option and applying it to the
> machine
> +     *   before passing it on to the cpu level parser.
> +     */
> +    gchar **inpieces;
> +    int i, j;
> +    gchar *compat_str = NULL;
> +
> +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> +
> +    /* inpieces[0] is the actual model string */
> +    i = 1;
> +    j = 1;
> +    while (inpieces[i]) {
> +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> +            /* in case of multiple compat= optipons */

s/optipons/options?

> +            g_free(compat_str);
> +            compat_str = inpieces[i];
> +        } else {
> +            j++;
> +        }
> +
> +        /* Excise compat options from list */
> +        inpieces[j] = inpieces[i];

it's worth noting that where previously when specifying an invalid
option you got:

qemu-system-ppc64: Expected key=value format, found *blah*

You now get a segfault here.

> +        i++;
> +    }
> +    inpieces[j] = NULL;
> +
> +    if (compat_str) {
> +        char *val = compat_str + strlen("compat=");
> +        gchar *newprops = g_strjoinv(",", inpieces);
> +
> +        object_property_set_str(OBJECT(spapr), val, "max-cpu-
> compat",
> +                                &error_fatal);
> +
> +        ppc_cpu_parse_features(newprops);
> +        g_free(newprops);
> +    } else {
> +        ppc_cpu_parse_features(MACHINE(spapr)->cpu_model);
> +    }
> +
> +    g_strfreev(inpieces);
> +}
> +
>  static void spapr_cpu_reset(void *opaque)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -70,10 +122,10 @@ static void spapr_cpu_init(sPAPRMachineState
> *spapr, PowerPCCPU *cpu,
>      /* Enable PAPR mode in TCG or KVM */
>      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>  
> -    if (cpu->max_compat) {
> +    if (spapr->max_compat_pvr) {
>          Error *local_err = NULL;
>  
> -        ppc_set_compat(cpu, cpu->max_compat, &local_err);
> +        ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 77d2d66..8129959 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1044,11 +1044,11 @@ static target_ulong
> h_signal_sys_reset(PowerPCCPU *cpu,
>      }
>  }
>  
> -static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
> -                              Error **errp)
> +static uint32_t cas_check_pvr(sPAPRMachineState *spapr, PowerPCCPU
> *cpu,
> +                              target_ulong *addr, Error **errp)
>  {
>      bool explicit_match = false; /* Matched the CPU's real PVR */
> -    uint32_t max_compat = cpu->max_compat;
> +    uint32_t max_compat = spapr->max_compat_pvr;
>      uint32_t best_compat = 0;
>      int i;
>  
> @@ -1104,7 +1104,7 @@ static target_ulong
> h_client_architecture_support(PowerPCCPU *cpu,
>      bool guest_radix;
>      Error *local_err = NULL;
>  
> -    cas_pvr = cas_check_pvr(cpu, &addr, &local_err);
> +    cas_pvr = cas_check_pvr(spapr, cpu, &addr, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>          return H_HARDWARE;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 98fb78b..4da92e2 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -87,16 +87,19 @@ struct sPAPRMachineState {
>      uint64_t rtc_offset; /* Now used only during incoming migration
> */
>      struct PPCTimebase tb;
>      bool has_graphics;
> -    sPAPROptionVector *ov5;         /* QEMU-supported option vectors
> */
> -    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option
> vectors */
> -    bool cas_reboot;
> -    bool cas_legacy_guest_workaround;
>  
>      Notifier epow_notifier;
>      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
>      bool use_hotplug_event_source;
>      sPAPREventSource *event_sources;
>  
> +    /* ibm,client-architecture-support option negotiation */
> +    bool cas_reboot;
> +    bool cas_legacy_guest_workaround;
> +    sPAPROptionVector *ov5;         /* QEMU-supported option vectors
> */
> +    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option
> vectors */
> +    uint32_t max_compat_pvr;
> +
>      /* Migration state */
>      int htab_save_index;
>      bool htab_first_pass;
> @@ -639,6 +642,7 @@ void
> spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                              uint32_t count, uint32_t
> index);
>  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType
> drc_type,
>                                                 uint32_t count,
> uint32_t index);
> +void spapr_cpu_parse_features(sPAPRMachineState *spapr);
>  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
>                                      sPAPRMachineState *spapr);
>  
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index e8ec1e1..e72839f 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -24,9 +24,11 @@
>  #include "sysemu/cpus.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> +#include "qapi/visitor.h"
>  #include "cpu-models.h"
>  
>  typedef struct {
> +    const char *name;
>      uint32_t pvr;
>      uint64_t pcr;
>      uint64_t pcr_level;
> @@ -38,6 +40,7 @@ static const CompatInfo compat_table[] = {
>       * Ordered from oldest to newest - the code relies on this
>       */
>      { /* POWER6, ISA2.05 */
> +        .name = "power6",
>          .pvr = CPU_POWERPC_LOGICAL_2_05,
>          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
>                 PCR_COMPAT_2_05 | PCR_TM_DIS | PCR_VSX_DIS,
> @@ -45,24 +48,28 @@ static const CompatInfo compat_table[] = {
>          .max_threads = 2,
>      },
>      { /* POWER7, ISA2.06 */
> +        .name = "power7",
>          .pvr = CPU_POWERPC_LOGICAL_2_06,
>          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> PCR_TM_DIS,
>          .pcr_level = PCR_COMPAT_2_06,
>          .max_threads = 4,
>      },
>      {
> +        .name = "power7+",
>          .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
>          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> PCR_TM_DIS,
>          .pcr_level = PCR_COMPAT_2_06,
>          .max_threads = 4,
>      },
>      { /* POWER8, ISA2.07 */
> +        .name = "power8",
>          .pvr = CPU_POWERPC_LOGICAL_2_07,
>          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07,
>          .pcr_level = PCR_COMPAT_2_07,
>          .max_threads = 8,
>      },
>      { /* POWER9, ISA3.00 */
> +        .name = "power9",
>          .pvr = CPU_POWERPC_LOGICAL_3_00,
>          .pcr = PCR_COMPAT_3_00,
>          .pcr_level = PCR_COMPAT_3_00,
> @@ -189,3 +196,98 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
>  
>      return n_threads;
>  }
> +
> +static void ppc_compat_prop_get(Object *obj, Visitor *v, const char
> *name,
> +                                void *opaque, Error **errp)
> +{
> +    uint32_t compat_pvr = *((uint32_t *)opaque);
> +    const char *value;
> +
> +    if (!compat_pvr) {
> +        value = "";
> +    } else {
> +        const CompatInfo *compat = compat_by_pvr(compat_pvr);
> +
> +        g_assert(compat);
> +
> +        value = compat->name;
> +    }
> +
> +    visit_type_str(v, name, (char **)&value, errp);
> +}
> +
> +static void ppc_compat_prop_set(Object *obj, Visitor *v, const char
> *name,
> +                                void *opaque, Error **errp)
> +{
> +    Error *error = NULL;
> +    char *value;
> +    uint32_t compat_pvr;
> +
> +    visit_type_str(v, name, &value, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    if (strcmp(value, "") == 0) {
> +        compat_pvr = 0;
> +    } else {
> +        int i;
> +        const CompatInfo *compat = NULL;
> +
> +        for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
> +            if (strcmp(value, compat_table[i].name) == 0) {
> +                compat = &compat_table[i];
> +                break;
> +
> +            }
> +        }
> +
> +        if (!compat) {
> +            error_setg(errp, "Invalid compatibility mode \"%s\"",
> value);
> +            goto out;
> +        }
> +        compat_pvr = compat->pvr;
> +    }
> +
> +    *((uint32_t *)opaque) = compat_pvr;
> +
> +out:
> +    g_free(value);
> +}
> +
> +void ppc_compat_add_property(Object *obj, const char *name,
> +                             uint32_t *compat_pvr, const char
> *basedesc,
> +                             Error **errp)
> +{
> +    Error *local_err = NULL;
> +    gchar *namesv[ARRAY_SIZE(compat_table) + 1];
> +    gchar *names, *desc;
> +    int i;
> +
> +    object_property_add(obj, name, "string",
> +                        ppc_compat_prop_get, ppc_compat_prop_set,
> NULL,
> +                        compat_pvr, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
> +        /*
> +         * Have to discard const here, because g_strjoinv() takes
> +         * (gchar **), not (const gchar **) :(
> +         */
> +        namesv[i] = (gchar *)compat_table[i].name;
> +    }
> +    namesv[ARRAY_SIZE(compat_table)] = NULL;
> +
> +    names = g_strjoinv(", ", namesv);
> +    desc = g_strdup_printf("%s. Valid values are %s.", basedesc,
> names);
> +    object_property_set_description(obj, name, desc, &local_err);
> +
> +    g_free(names);
> +    g_free(desc);
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 401e10e..4517b4b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1189,7 +1189,6 @@ typedef struct PPCVirtualHypervisorClass
> PPCVirtualHypervisorClass;
>   * PowerPCCPU:
>   * @env: #CPUPPCState
>   * @cpu_dt_id: CPU index used in the device tree. KVM uses this
> index too
> - * @max_compat: Maximal supported logical PVR from the command line
>   * @compat_pvr: Current logical PVR, zero if in "raw" mode
>   *
>   * A PowerPC CPU.
> @@ -1201,7 +1200,6 @@ struct PowerPCCPU {
>  
>      CPUPPCState env;
>      int cpu_dt_id;
> -    uint32_t max_compat;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
>      Object *intc;
> @@ -1374,6 +1372,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
> compat_pvr, Error **errp);
>  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
>  #endif
>  int ppc_compat_max_threads(PowerPCCPU *cpu);
> +void ppc_compat_add_property(Object *obj, const char *name,
> +                             uint32_t *compat_pvr, const char
> *basedesc,
> +                             Error **errp);
>  #endif /* defined(TARGET_PPC64) */
>  
>  #include "exec/cpu-all.h"
> diff --git a/target/ppc/translate_init.c
> b/target/ppc/translate_init.c
> index 56a0ab2..e837cd2 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -33,6 +33,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/ppc/ppc.h"
>  #include "mmu-book3s-v3.h"
> +#include "sysemu/qtest.h"
>  
>  //#define PPC_DUMP_CPU
>  //#define PPC_DEBUG_SPR
> @@ -8413,73 +8414,38 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void
> *data)
>      pcc->l1_icache_size = 0x10000;
>  }
>  
> -static void powerpc_get_compat(Object *obj, Visitor *v, const char
> *name,
> -                               void *opaque, Error **errp)
> -{
> -    char *value = (char *)"";
> -    Property *prop = opaque;
> -    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
> -
> -    switch (*max_compat) {
> -    case CPU_POWERPC_LOGICAL_2_05:
> -        value = (char *)"power6";
> -        break;
> -    case CPU_POWERPC_LOGICAL_2_06:
> -        value = (char *)"power7";
> -        break;
> -    case CPU_POWERPC_LOGICAL_2_07:
> -        value = (char *)"power8";
> -        break;
> -    case 0:
> -        break;
> -    default:
> -        error_report("Internal error: compat is set to %x",
> *max_compat);
> -        abort();
> -        break;
> -    }
> -
> -    visit_type_str(v, name, &value, errp);
> -}
> -
> -static void powerpc_set_compat(Object *obj, Visitor *v, const char
> *name,
> -                               void *opaque, Error **errp)
> +/*
> + * The CPU used to have a "compat" property which set the
> + * compatibility mode PVR.  However, this was conceptually broken -
> it
> + * only makes sense on the pseries machine type (otherwise the guest
> + * owns the PCR and can control the compatibility mode
> itself).  It's
> + * been replaced with the 'max-cpu-compat' property on the pseries
> + * machine type.  For backwards compatibility, pseries specially
> + * parses the -cpu parameter and converts old compat= parameters
> into
> + * the appropriate machine parameters.  This stub implementation of
> + * the parameter catches any uses on explicitly created CPUs.
> + */
> +static void getset_compat_deprecated(Object *obj, Visitor *v, const
> char *name,
> +                                     void *opaque, Error **errp)
>  {
> -    Error *error = NULL;
> -    char *value = NULL;
> -    Property *prop = opaque;
> -    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
> -
> -    visit_type_str(v, name, &value, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -
> -    if (strcmp(value, "power6") == 0) {
> -        *max_compat = CPU_POWERPC_LOGICAL_2_05;
> -    } else if (strcmp(value, "power7") == 0) {
> -        *max_compat = CPU_POWERPC_LOGICAL_2_06;
> -    } else if (strcmp(value, "power8") == 0) {
> -        *max_compat = CPU_POWERPC_LOGICAL_2_07;
> -    } else {
> -        error_setg(errp, "Invalid compatibility mode \"%s\"",
> value);
> +    if (!qtest_enabled()) {
> +        error_report("CPU 'compat' property is deprecated and has no
> effect; "
> +                     "use max-cpu-compat machine property instead");
>      }
> -
> -    g_free(value);
> +    visit_type_null(v, name, NULL);
>  }
>  
> -static PropertyInfo powerpc_compat_propinfo = {
> +static PropertyInfo ppc_compat_deprecated_propinfo = {
>      .name = "str",
> -    .description = "compatibility mode, power6/power7/power8",
> -    .get = powerpc_get_compat,
> -    .set = powerpc_set_compat,
> +    .description = "compatibility mode (deprecated)",
> +    .get = getset_compat_deprecated,
> +    .set = getset_compat_deprecated,
>  };
> -
> -#define DEFINE_PROP_POWERPC_COMPAT(_n, _s, _f) \
> -    DEFINE_PROP(_n, _s, _f, powerpc_compat_propinfo, uint32_t)
> -
>  static Property powerpc_servercpu_properties[] = {
> -    DEFINE_PROP_POWERPC_COMPAT("compat", PowerPCCPU, max_compat),
> +    {
> +        .name = "compat",
> +        .info = &ppc_compat_deprecated_propinfo,
> +    },
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 5/5] ppc: Rework CPU compatibility testing across migration

[Suraj Jitindar Singh]

On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> Migrating between different CPU versions is a bit complicated for
> ppc.
> A long time ago, we ensured identical CPU versions at either end by
> checking the PVR had the same value.  However, this breaks under KVM
> HV, because we always have to use the host's PVR - it's not
> virtualized.  That would mean we couldn't migrate between hosts with
> different PVRs, even if the CPUs are close enough to compatible in
> practice (sometimes identical cores with different surrounding logic
> have different PVRs, so this happens in practice quite often).
> 
> So, we removed the PVR check, but instead checked that several flags
> indicating supported instructions matched.  This turns out to be a
> bad
> idea, because those instruction masks are not architected
> information, but
> essentially a TCG implementation detail.  So changes to qemu internal
> CPU
> modelling can break migration - this happened between qemu-2.6 and
> qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> removal of old migration mistakes".
> 
> Now, verification of CPU compatibility across a migration basically
> doesn't
> happen.  We simply ignore the PVR of the incoming migration, and hope
> the
> cpu on the destination is close enough to work.
> 
> Now that we've cleaned up handling of processor compatibility modes
> for
> pseries machine type, we can do better.  We allow migration if:
> 
>     * The source and destination PVRs are for the same type of CPU,
> as
>       determined by CPU class's pvr_match function
> OR  * When the source was in a compatibility mode, and the
> destination CPU
>       supports the same compatibility mode
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Looks good to me.

Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

> ---
>  target/ppc/machine.c | 72
> +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..2c6d9dc 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -8,6 +8,7 @@
>  #include "helper_regs.h"
>  #include "mmu-hash64.h"
>  #include "migration/cpu.h"
> +#include "qapi/error.h"
>  
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
> @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
>      }
>  }
>  
> +/*
> + * Determine if a given PVR is a "close enough" match to the CPU
> + * object.  For TCG and KVM PR it would probably be sufficient to
> + * require an exact PVR match.  However for KVM HV the user is
> + * restricted to a PVR exactly matching the host CPU.  The correct
> way
> + * to handle this is to put the guest into an architected
> + * compatibility mode.  However, to allow a more forgiving
> transition
> + * and migration from before this was widely done, we allow
> migration
> + * between sufficiently similar PVRs, as determined by the CPU
> class's
> + * pvr_match() hook.
> + */
> +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    if (pvr == pcc->pvr) {
> +        return true;
> +    }
> +    if (pcc->pvr_match) {
> +        return pcc->pvr_match(pcc, pvr);
> +    }
> +    return false;
> +}
> +
>  static int cpu_post_load(void *opaque, int version_id)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int
> version_id)
>      target_ulong msr;
>  
>      /*
> -     * We always ignore the source PVR. The user or management
> -     * software has to take care of running QEMU in a compatible
> mode.
> +     * If we're operating in compat mode, we should be ok as long as
> +     * the destination supports the same compatiblity mode.
> +     *
> +     * Otherwise, however, we require that the destination has
> exactly
> +     * the same CPU model as the source.
>       */
> -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> +
> +#if defined(TARGET_PPC64)
> +    if (cpu->compat_pvr) {
> +        Error *local_err = NULL;
> +
> +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(local_err);
> +            return -1;
> +        }
> +    } else
> +#endif
> +    {
> +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> +            return -1;
> +        }
> +    }
> +
>      env->lr = env->spr[SPR_LR];
>      env->ctr = env->spr[SPR_CTR];
>      cpu_write_xer(env, env->spr[SPR_XER]);
> @@ -560,6 +606,25 @@ static const VMStateDescription vmstate_tlbmas =
> {
>      }
>  };
>  
> +static bool compat_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    assert(!(cpu->compat_pvr && !cpu->vhyp));
> +    return (cpu->compat_pvr != 0);
> +}
> +
> +static const VMStateDescription vmstate_compat = {
> +    .name = "cpu/compat",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = compat_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -613,6 +678,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>          &vmstate_tlb6xx,
>          &vmstate_tlbemb,
>          &vmstate_tlbmas,
> +        &vmstate_compat,
>          NULL
>      }
>  };

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:
> On Wed, 31 May 2017 12:57:48 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > [...]
> > > All old non-pseries machine types already complain when started with
> > > a POWER7 or newer CPU. Providing the extra error message looks weird:
> > > 
> > > qemu-system-ppc64 -machine ppce500 \
> > >                   -cpu POWER7,compat=power6
> > > qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> > >  use max-cpu-compat machine property instead
> > > MMU model 983043 not supported by this machine.
> > > 
> > > but I guess it's better than crashing. :)  
> > 
> > Well, sure POWER7 doesn't make sense for an e500 machine for other
> > reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> > compat= doesn't.
> > 
> 
> The powernv machine type doesn't even support CPU features at all:
> 
>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
>     if (!object_class_by_name(chip_typename)) {
>         error_report("invalid CPU model '%s' for %s machine",
>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
>         exit(1);
>     }

Ah, well, that's another bug, but not one that's in scope for this
series.



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170526052319.28096-1-david@gibson.dropbear.id.au
Type: series
Subject: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
ab1e546 ppc: Rework CPU compatibility testing across migration
9506d9a pseries: Reset CPU compatibility mode
c974c40 pseries: Move CPU compatibility property to machine
ae5875d migration: Mark CPU states dirty before incoming migration/loadvm
e3081d0 qapi: add explicit null to string input and output visitors

=== OUTPUT BEGIN ===
Checking PATCH 1/5: qapi: add explicit null to string input and output visitors...
Checking PATCH 2/5: migration: Mark CPU states dirty before incoming migration/loadvm...
WARNING: line over 80 characters
#119: FILE: kvm-all.c:1899:
+static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)

WARNING: line over 80 characters
#153: FILE: target/i386/hax-all.c:638:
+static void do_hax_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg)

total: 0 errors, 2 warnings, 90 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/5: pseries: Move CPU compatibility property to machine...
Checking PATCH 4/5: pseries: Reset CPU compatibility mode...
Checking PATCH 5/5: ppc: Rework CPU compatibility testing across migration...
ERROR: braces {} are necessary for all arms of this statement
#97: FILE: target/ppc/machine.c:239:
+    if (cpu->compat_pvr) {
[...]
+    } else
[...]

total: 1 errors, 0 warnings, 103 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[David Gibson]

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

On Thu, Jun 01, 2017 at 03:44:40PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> > Server class POWER CPUs have a "compat" property, which is used to
> > set the
> > backwards compatibility mode for the processor.  However, this only
> > makes
> > sense for machine types which don't give the guest access to
> > hypervisor
> > privilege - otherwise the compatibility level is under the guest's
> > control.
> > 
> > To reflect this, this removes the CPU 'compat' property and instead
> > creates a 'max-cpu-compat' property on the pseries machine.  Strictly
> > speaking this breaks compatibility, but AFAIK the 'compat' option was
> > never (directly) used with -device or device_add.
> > 
> > The option was used with -cpu.  So, to maintain compatibility, this
> > patch adds a hack to the cpu option parsing to strip out any compat
> > options supplied with -cpu and set them on the machine property
> > instead of the now deprecated cpu property.
> 
> Generally looks good, a couple of comments below.
> 
> Suraj
> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |   6 ++-
> >  hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
> >  hw/ppc/spapr_hcall.c        |   8 ++--
> >  include/hw/ppc/spapr.h      |  12 ++++--
> >  target/ppc/compat.c         | 102
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/cpu.h            |   5 ++-
> >  target/ppc/translate_init.c |  86 +++++++++++-----------------------
> > ---
> >  7 files changed, 202 insertions(+), 73 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ab3aab1..3c4e88f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> > *machine)
> >          machine->cpu_model = kvm_enabled() ? "host" : smc-
> > >tcg_default_cpu;
> >      }
> >  
> > -    ppc_cpu_parse_features(machine->cpu_model);
> > +    spapr_cpu_parse_features(spapr);
> >  
> >      spapr_init_cpus(spapr);
> >  
> > @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events
> > when possible"
> >                                      " (required for memory hot-
> > unplug support)",
> >                                      NULL);
> > +
> > +    ppc_compat_add_property(obj, "max-cpu-compat", &spapr-
> > >max_compat_pvr,
> > +                            "Maximum permitted CPU compatibility
> > mode",
> > +                            &error_fatal);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index ff7058e..ab4102b 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -20,6 +20,58 @@
> >  #include "sysemu/numa.h"
> >  #include "qemu/error-report.h"
> >  
> > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > +{
> > +    /*
> > +     * Backwards compatibility hack:
> > +     *
> > +     *   CPUs had a "compat=" property which didn't make sense for
> > +     *   anything except pseries.  It was replaced by "max-cpu-
> > compat"
> > +     *   machine option.  This supports old command lines like
> > +     *       -cpu POWER8,compat=power7
> > +     *   By stripping the compat option and applying it to the
> > machine
> > +     *   before passing it on to the cpu level parser.
> > +     */
> > +    gchar **inpieces;
> > +    int i, j;
> > +    gchar *compat_str = NULL;
> > +
> > +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > +
> > +    /* inpieces[0] is the actual model string */
> > +    i = 1;
> > +    j = 1;
> > +    while (inpieces[i]) {
> > +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> > +            /* in case of multiple compat= optipons */
> 
> s/optipons/options?

Oops, fixed.

> > +            g_free(compat_str);
> > +            compat_str = inpieces[i];
> > +        } else {
> > +            j++;
> > +        }
> > +
> > +        /* Excise compat options from list */
> > +        inpieces[j] = inpieces[i];
> 
> it's worth noting that where previously when specifying an invalid
> option you got:
> 
> qemu-system-ppc64: Expected key=value format, found *blah*
> 
> You now get a segfault here.

Sod.  The joy of doing string manipulation in C.  Ok, I think I've
found and fixed that too.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[Greg Kurz]

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

On Thu, 01 Jun 2017 15:44:40 +1000
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> > Server class POWER CPUs have a "compat" property, which is used to
> > set the
> > backwards compatibility mode for the processor.  However, this only
> > makes
> > sense for machine types which don't give the guest access to
> > hypervisor
> > privilege - otherwise the compatibility level is under the guest's
> > control.
> > 
> > To reflect this, this removes the CPU 'compat' property and instead
> > creates a 'max-cpu-compat' property on the pseries machine.  Strictly
> > speaking this breaks compatibility, but AFAIK the 'compat' option was
> > never (directly) used with -device or device_add.
> > 
> > The option was used with -cpu.  So, to maintain compatibility, this
> > patch adds a hack to the cpu option parsing to strip out any compat
> > options supplied with -cpu and set them on the machine property
> > instead of the now deprecated cpu property.  
> 
> Generally looks good, a couple of comments below.
> 
> Suraj
> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c              |   6 ++-
> >  hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
> >  hw/ppc/spapr_hcall.c        |   8 ++--
> >  include/hw/ppc/spapr.h      |  12 ++++--
> >  target/ppc/compat.c         | 102
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/cpu.h            |   5 ++-
> >  target/ppc/translate_init.c |  86 +++++++++++-----------------------
> > ---
> >  7 files changed, 202 insertions(+), 73 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ab3aab1..3c4e88f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> > *machine)
> >          machine->cpu_model = kvm_enabled() ? "host" : smc-  
> > >tcg_default_cpu;  
> >      }
> >  
> > -    ppc_cpu_parse_features(machine->cpu_model);
> > +    spapr_cpu_parse_features(spapr);
> >  
> >      spapr_init_cpus(spapr);
> >  
> > @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
> >                                      " place of standard EPOW events
> > when possible"
> >                                      " (required for memory hot-
> > unplug support)",
> >                                      NULL);
> > +
> > +    ppc_compat_add_property(obj, "max-cpu-compat", &spapr-  
> > >max_compat_pvr,  
> > +                            "Maximum permitted CPU compatibility
> > mode",
> > +                            &error_fatal);
> >  }
> >  
> >  static void spapr_machine_finalizefn(Object *obj)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index ff7058e..ab4102b 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -20,6 +20,58 @@
> >  #include "sysemu/numa.h"
> >  #include "qemu/error-report.h"
> >  
> > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > +{
> > +    /*
> > +     * Backwards compatibility hack:
> > +     *
> > +     *   CPUs had a "compat=" property which didn't make sense for
> > +     *   anything except pseries.  It was replaced by "max-cpu-
> > compat"
> > +     *   machine option.  This supports old command lines like
> > +     *       -cpu POWER8,compat=power7
> > +     *   By stripping the compat option and applying it to the
> > machine
> > +     *   before passing it on to the cpu level parser.
> > +     */
> > +    gchar **inpieces;
> > +    int i, j;
> > +    gchar *compat_str = NULL;
> > +
> > +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > +
> > +    /* inpieces[0] is the actual model string */
> > +    i = 1;
> > +    j = 1;
> > +    while (inpieces[i]) {
> > +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> > +            /* in case of multiple compat= optipons */  
> 
> s/optipons/options?
> 
> > +            g_free(compat_str);
> > +            compat_str = inpieces[i];
> > +        } else {
> > +            j++;
> > +        }
> > +
> > +        /* Excise compat options from list */
> > +        inpieces[j] = inpieces[i];  
> 
> it's worth noting that where previously when specifying an invalid
> option you got:
> 
> qemu-system-ppc64: Expected key=value format, found *blah*
> 
> You now get a segfault here.
> 

Yeah. This basically does:

    inpieces[i + 1] = inpieces[i];

and we end up overwriting the terminal NULL pointer with a non-NULL
pointer.

What about simplifying the loop to:

    /* inpieces[0] is the actual model string */
    i = 1;
    while (inpieces[i]) {
        if (g_str_has_prefix(inpieces[i], "compat=")) {
            /* in case of multiple compat= optipons */
            g_free(compat_str);
            compat_str = inpieces[i];
            /* Excise compat options from list */
            inpieces[i] = inpieces[i + 1];
        }
        i++;
    }

> > +        i++;
> > +    }
> > +    inpieces[j] = NULL;
> > +
> > +    if (compat_str) {
> > +        char *val = compat_str + strlen("compat=");
> > +        gchar *newprops = g_strjoinv(",", inpieces);
> > +
> > +        object_property_set_str(OBJECT(spapr), val, "max-cpu-
> > compat",
> > +                                &error_fatal);
> > +
> > +        ppc_cpu_parse_features(newprops);
> > +        g_free(newprops);
> > +    } else {
> > +        ppc_cpu_parse_features(MACHINE(spapr)->cpu_model);
> > +    }
> > +
> > +    g_strfreev(inpieces);
> > +}
> > +
> >  static void spapr_cpu_reset(void *opaque)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > @@ -70,10 +122,10 @@ static void spapr_cpu_init(sPAPRMachineState
> > *spapr, PowerPCCPU *cpu,
> >      /* Enable PAPR mode in TCG or KVM */
> >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> >  
> > -    if (cpu->max_compat) {
> > +    if (spapr->max_compat_pvr) {
> >          Error *local_err = NULL;
> >  
> > -        ppc_set_compat(cpu, cpu->max_compat, &local_err);
> > +        ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> >              return;
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 77d2d66..8129959 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1044,11 +1044,11 @@ static target_ulong
> > h_signal_sys_reset(PowerPCCPU *cpu,
> >      }
> >  }
> >  
> > -static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
> > -                              Error **errp)
> > +static uint32_t cas_check_pvr(sPAPRMachineState *spapr, PowerPCCPU
> > *cpu,
> > +                              target_ulong *addr, Error **errp)
> >  {
> >      bool explicit_match = false; /* Matched the CPU's real PVR */
> > -    uint32_t max_compat = cpu->max_compat;
> > +    uint32_t max_compat = spapr->max_compat_pvr;
> >      uint32_t best_compat = 0;
> >      int i;
> >  
> > @@ -1104,7 +1104,7 @@ static target_ulong
> > h_client_architecture_support(PowerPCCPU *cpu,
> >      bool guest_radix;
> >      Error *local_err = NULL;
> >  
> > -    cas_pvr = cas_check_pvr(cpu, &addr, &local_err);
> > +    cas_pvr = cas_check_pvr(spapr, cpu, &addr, &local_err);
> >      if (local_err) {
> >          error_report_err(local_err);
> >          return H_HARDWARE;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 98fb78b..4da92e2 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -87,16 +87,19 @@ struct sPAPRMachineState {
> >      uint64_t rtc_offset; /* Now used only during incoming migration
> > */
> >      struct PPCTimebase tb;
> >      bool has_graphics;
> > -    sPAPROptionVector *ov5;         /* QEMU-supported option vectors
> > */
> > -    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option
> > vectors */
> > -    bool cas_reboot;
> > -    bool cas_legacy_guest_workaround;
> >  
> >      Notifier epow_notifier;
> >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> >      bool use_hotplug_event_source;
> >      sPAPREventSource *event_sources;
> >  
> > +    /* ibm,client-architecture-support option negotiation */
> > +    bool cas_reboot;
> > +    bool cas_legacy_guest_workaround;
> > +    sPAPROptionVector *ov5;         /* QEMU-supported option vectors
> > */
> > +    sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option
> > vectors */
> > +    uint32_t max_compat_pvr;
> > +
> >      /* Migration state */
> >      int htab_save_index;
> >      bool htab_first_pass;
> > @@ -639,6 +642,7 @@ void
> > spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
> >                                              uint32_t count, uint32_t
> > index);
> >  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType
> > drc_type,
> >                                                 uint32_t count,
> > uint32_t index);
> > +void spapr_cpu_parse_features(sPAPRMachineState *spapr);
> >  void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
> >                                      sPAPRMachineState *spapr);
> >  
> > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > index e8ec1e1..e72839f 100644
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -24,9 +24,11 @@
> >  #include "sysemu/cpus.h"
> >  #include "qemu/error-report.h"
> >  #include "qapi/error.h"
> > +#include "qapi/visitor.h"
> >  #include "cpu-models.h"
> >  
> >  typedef struct {
> > +    const char *name;
> >      uint32_t pvr;
> >      uint64_t pcr;
> >      uint64_t pcr_level;
> > @@ -38,6 +40,7 @@ static const CompatInfo compat_table[] = {
> >       * Ordered from oldest to newest - the code relies on this
> >       */
> >      { /* POWER6, ISA2.05 */
> > +        .name = "power6",
> >          .pvr = CPU_POWERPC_LOGICAL_2_05,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> >                 PCR_COMPAT_2_05 | PCR_TM_DIS | PCR_VSX_DIS,
> > @@ -45,24 +48,28 @@ static const CompatInfo compat_table[] = {
> >          .max_threads = 2,
> >      },
> >      { /* POWER7, ISA2.06 */
> > +        .name = "power7",
> >          .pvr = CPU_POWERPC_LOGICAL_2_06,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> > PCR_TM_DIS,
> >          .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      {
> > +        .name = "power7+",
> >          .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07 | PCR_COMPAT_2_06 |
> > PCR_TM_DIS,
> >          .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      { /* POWER8, ISA2.07 */
> > +        .name = "power8",
> >          .pvr = CPU_POWERPC_LOGICAL_2_07,
> >          .pcr = PCR_COMPAT_3_00 | PCR_COMPAT_2_07,
> >          .pcr_level = PCR_COMPAT_2_07,
> >          .max_threads = 8,
> >      },
> >      { /* POWER9, ISA3.00 */
> > +        .name = "power9",
> >          .pvr = CPU_POWERPC_LOGICAL_3_00,
> >          .pcr = PCR_COMPAT_3_00,
> >          .pcr_level = PCR_COMPAT_3_00,
> > @@ -189,3 +196,98 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
> >  
> >      return n_threads;
> >  }
> > +
> > +static void ppc_compat_prop_get(Object *obj, Visitor *v, const char
> > *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    uint32_t compat_pvr = *((uint32_t *)opaque);
> > +    const char *value;
> > +
> > +    if (!compat_pvr) {
> > +        value = "";
> > +    } else {
> > +        const CompatInfo *compat = compat_by_pvr(compat_pvr);
> > +
> > +        g_assert(compat);
> > +
> > +        value = compat->name;
> > +    }
> > +
> > +    visit_type_str(v, name, (char **)&value, errp);
> > +}
> > +
> > +static void ppc_compat_prop_set(Object *obj, Visitor *v, const char
> > *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    Error *error = NULL;
> > +    char *value;
> > +    uint32_t compat_pvr;
> > +
> > +    visit_type_str(v, name, &value, &error);
> > +    if (error) {
> > +        error_propagate(errp, error);
> > +        return;
> > +    }
> > +
> > +    if (strcmp(value, "") == 0) {
> > +        compat_pvr = 0;
> > +    } else {
> > +        int i;
> > +        const CompatInfo *compat = NULL;
> > +
> > +        for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
> > +            if (strcmp(value, compat_table[i].name) == 0) {
> > +                compat = &compat_table[i];
> > +                break;
> > +
> > +            }
> > +        }
> > +
> > +        if (!compat) {
> > +            error_setg(errp, "Invalid compatibility mode \"%s\"",
> > value);
> > +            goto out;
> > +        }
> > +        compat_pvr = compat->pvr;
> > +    }
> > +
> > +    *((uint32_t *)opaque) = compat_pvr;
> > +
> > +out:
> > +    g_free(value);
> > +}
> > +
> > +void ppc_compat_add_property(Object *obj, const char *name,
> > +                             uint32_t *compat_pvr, const char
> > *basedesc,
> > +                             Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    gchar *namesv[ARRAY_SIZE(compat_table) + 1];
> > +    gchar *names, *desc;
> > +    int i;
> > +
> > +    object_property_add(obj, name, "string",
> > +                        ppc_compat_prop_get, ppc_compat_prop_set,
> > NULL,
> > +                        compat_pvr, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    for (i = 0; i < ARRAY_SIZE(compat_table); i++) {
> > +        /*
> > +         * Have to discard const here, because g_strjoinv() takes
> > +         * (gchar **), not (const gchar **) :(
> > +         */
> > +        namesv[i] = (gchar *)compat_table[i].name;
> > +    }
> > +    namesv[ARRAY_SIZE(compat_table)] = NULL;
> > +
> > +    names = g_strjoinv(", ", namesv);
> > +    desc = g_strdup_printf("%s. Valid values are %s.", basedesc,
> > names);
> > +    object_property_set_description(obj, name, desc, &local_err);
> > +
> > +    g_free(names);
> > +    g_free(desc);
> > +
> > +out:
> > +    error_propagate(errp, local_err);
> > +}
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 401e10e..4517b4b 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1189,7 +1189,6 @@ typedef struct PPCVirtualHypervisorClass
> > PPCVirtualHypervisorClass;
> >   * PowerPCCPU:
> >   * @env: #CPUPPCState
> >   * @cpu_dt_id: CPU index used in the device tree. KVM uses this
> > index too
> > - * @max_compat: Maximal supported logical PVR from the command line
> >   * @compat_pvr: Current logical PVR, zero if in "raw" mode
> >   *
> >   * A PowerPC CPU.
> > @@ -1201,7 +1200,6 @@ struct PowerPCCPU {
> >  
> >      CPUPPCState env;
> >      int cpu_dt_id;
> > -    uint32_t max_compat;
> >      uint32_t compat_pvr;
> >      PPCVirtualHypervisor *vhyp;
> >      Object *intc;
> > @@ -1374,6 +1372,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
> > compat_pvr, Error **errp);
> >  void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
> >  #endif
> >  int ppc_compat_max_threads(PowerPCCPU *cpu);
> > +void ppc_compat_add_property(Object *obj, const char *name,
> > +                             uint32_t *compat_pvr, const char
> > *basedesc,
> > +                             Error **errp);
> >  #endif /* defined(TARGET_PPC64) */
> >  
> >  #include "exec/cpu-all.h"
> > diff --git a/target/ppc/translate_init.c
> > b/target/ppc/translate_init.c
> > index 56a0ab2..e837cd2 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -33,6 +33,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "mmu-book3s-v3.h"
> > +#include "sysemu/qtest.h"
> >  
> >  //#define PPC_DUMP_CPU
> >  //#define PPC_DEBUG_SPR
> > @@ -8413,73 +8414,38 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void
> > *data)
> >      pcc->l1_icache_size = 0x10000;
> >  }
> >  
> > -static void powerpc_get_compat(Object *obj, Visitor *v, const char
> > *name,
> > -                               void *opaque, Error **errp)
> > -{
> > -    char *value = (char *)"";
> > -    Property *prop = opaque;
> > -    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
> > -
> > -    switch (*max_compat) {
> > -    case CPU_POWERPC_LOGICAL_2_05:
> > -        value = (char *)"power6";
> > -        break;
> > -    case CPU_POWERPC_LOGICAL_2_06:
> > -        value = (char *)"power7";
> > -        break;
> > -    case CPU_POWERPC_LOGICAL_2_07:
> > -        value = (char *)"power8";
> > -        break;
> > -    case 0:
> > -        break;
> > -    default:
> > -        error_report("Internal error: compat is set to %x",
> > *max_compat);
> > -        abort();
> > -        break;
> > -    }
> > -
> > -    visit_type_str(v, name, &value, errp);
> > -}
> > -
> > -static void powerpc_set_compat(Object *obj, Visitor *v, const char
> > *name,
> > -                               void *opaque, Error **errp)
> > +/*
> > + * The CPU used to have a "compat" property which set the
> > + * compatibility mode PVR.  However, this was conceptually broken -
> > it
> > + * only makes sense on the pseries machine type (otherwise the guest
> > + * owns the PCR and can control the compatibility mode
> > itself).  It's
> > + * been replaced with the 'max-cpu-compat' property on the pseries
> > + * machine type.  For backwards compatibility, pseries specially
> > + * parses the -cpu parameter and converts old compat= parameters
> > into
> > + * the appropriate machine parameters.  This stub implementation of
> > + * the parameter catches any uses on explicitly created CPUs.
> > + */
> > +static void getset_compat_deprecated(Object *obj, Visitor *v, const
> > char *name,
> > +                                     void *opaque, Error **errp)
> >  {
> > -    Error *error = NULL;
> > -    char *value = NULL;
> > -    Property *prop = opaque;
> > -    uint32_t *max_compat = qdev_get_prop_ptr(DEVICE(obj), prop);
> > -
> > -    visit_type_str(v, name, &value, &error);
> > -    if (error) {
> > -        error_propagate(errp, error);
> > -        return;
> > -    }
> > -
> > -    if (strcmp(value, "power6") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_05;
> > -    } else if (strcmp(value, "power7") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_06;
> > -    } else if (strcmp(value, "power8") == 0) {
> > -        *max_compat = CPU_POWERPC_LOGICAL_2_07;
> > -    } else {
> > -        error_setg(errp, "Invalid compatibility mode \"%s\"",
> > value);
> > +    if (!qtest_enabled()) {
> > +        error_report("CPU 'compat' property is deprecated and has no
> > effect; "
> > +                     "use max-cpu-compat machine property instead");
> >      }
> > -
> > -    g_free(value);
> > +    visit_type_null(v, name, NULL);
> >  }
> >  
> > -static PropertyInfo powerpc_compat_propinfo = {
> > +static PropertyInfo ppc_compat_deprecated_propinfo = {
> >      .name = "str",
> > -    .description = "compatibility mode, power6/power7/power8",
> > -    .get = powerpc_get_compat,
> > -    .set = powerpc_set_compat,
> > +    .description = "compatibility mode (deprecated)",
> > +    .get = getset_compat_deprecated,
> > +    .set = getset_compat_deprecated,
> >  };
> > -
> > -#define DEFINE_PROP_POWERPC_COMPAT(_n, _s, _f) \
> > -    DEFINE_PROP(_n, _s, _f, powerpc_compat_propinfo, uint32_t)
> > -
> >  static Property powerpc_servercpu_properties[] = {
> > -    DEFINE_PROP_POWERPC_COMPAT("compat", PowerPCCPU, max_compat),
> > +    {
> > +        .name = "compat",
> > +        .info = &ppc_compat_deprecated_propinfo,
> > +    },
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >    


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 5/5] ppc: Rework CPU compatibility testing across migration

[Greg Kurz]

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

On Fri, 26 May 2017 15:23:19 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Migrating between different CPU versions is a bit complicated for ppc.
> A long time ago, we ensured identical CPU versions at either end by
> checking the PVR had the same value.  However, this breaks under KVM
> HV, because we always have to use the host's PVR - it's not
> virtualized.  That would mean we couldn't migrate between hosts with
> different PVRs, even if the CPUs are close enough to compatible in
> practice (sometimes identical cores with different surrounding logic
> have different PVRs, so this happens in practice quite often).
> 
> So, we removed the PVR check, but instead checked that several flags
> indicating supported instructions matched.  This turns out to be a bad
> idea, because those instruction masks are not architected information, but
> essentially a TCG implementation detail.  So changes to qemu internal CPU
> modelling can break migration - this happened between qemu-2.6 and
> qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> removal of old migration mistakes".
> 
> Now, verification of CPU compatibility across a migration basically doesn't
> happen.  We simply ignore the PVR of the incoming migration, and hope the
> cpu on the destination is close enough to work.
> 
> Now that we've cleaned up handling of processor compatibility modes for
> pseries machine type, we can do better.  We allow migration if:
> 
>     * The source and destination PVRs are for the same type of CPU, as
>       determined by CPU class's pvr_match function
> OR  * When the source was in a compatibility mode, and the destination CPU
>       supports the same compatibility mode
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/machine.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..2c6d9dc 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -8,6 +8,7 @@
>  #include "helper_regs.h"
>  #include "mmu-hash64.h"
>  #include "migration/cpu.h"
> +#include "qapi/error.h"
>  
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
> @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
>      }
>  }
>  
> +/*
> + * Determine if a given PVR is a "close enough" match to the CPU
> + * object.  For TCG and KVM PR it would probably be sufficient to
> + * require an exact PVR match.  However for KVM HV the user is
> + * restricted to a PVR exactly matching the host CPU.  The correct way
> + * to handle this is to put the guest into an architected
> + * compatibility mode.  However, to allow a more forgiving transition
> + * and migration from before this was widely done, we allow migration
> + * between sufficiently similar PVRs, as determined by the CPU class's
> + * pvr_match() hook.
> + */
> +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    if (pvr == pcc->pvr) {
> +        return true;
> +    }
> +    if (pcc->pvr_match) {
> +        return pcc->pvr_match(pcc, pvr);
> +    }
> +    return false;
> +}

The base class provides a fallback for pcc->pvr_match that does:

static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr)
{
    return pcc->pvr == pvr;
}

so I'm not sure this function is needed, but maybe I'm missing something.

> +
>  static int cpu_post_load(void *opaque, int version_id)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
>      target_ulong msr;
>  
>      /*
> -     * We always ignore the source PVR. The user or management
> -     * software has to take care of running QEMU in a compatible mode.
> +     * If we're operating in compat mode, we should be ok as long as
> +     * the destination supports the same compatiblity mode.
> +     *
> +     * Otherwise, however, we require that the destination has exactly
> +     * the same CPU model as the source.
>       */
> -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> +
> +#if defined(TARGET_PPC64)
> +    if (cpu->compat_pvr) {
> +        Error *local_err = NULL;
> +
> +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(local_err);
> +            return -1;
> +        }
> +    } else
> +#endif
> +    {
> +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> +            return -1;
> +        }
> +    }
> +
>      env->lr = env->spr[SPR_LR];
>      env->ctr = env->spr[SPR_CTR];
>      cpu_write_xer(env, env->spr[SPR_XER]);
> @@ -560,6 +606,25 @@ static const VMStateDescription vmstate_tlbmas = {
>      }
>  };
>  
> +static bool compat_needed(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    assert(!(cpu->compat_pvr && !cpu->vhyp));
> +    return (cpu->compat_pvr != 0);

Parenthesitis ?

> +}

This will break backward migration of pre-2.10 machine types. But it can be
fixed in a followup patch if really needed (or even addressed downstream).

Anyway, since you're the decision maker:

Reviewed-by: Greg Kurz <groug@kaod.org>

> +
> +static const VMStateDescription vmstate_compat = {
> +    .name = "cpu/compat",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = compat_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -613,6 +678,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>          &vmstate_tlb6xx,
>          &vmstate_tlbemb,
>          &vmstate_tlbmas,
> +        &vmstate_compat,
>          NULL
>      }
>  };


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Cédric Le Goater]

On 06/01/2017 08:52 AM, David Gibson wrote:
> On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:
>> On Wed, 31 May 2017 12:57:48 +1000
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>> [...]
>>>> All old non-pseries machine types already complain when started with
>>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
>>>>
>>>> qemu-system-ppc64 -machine ppce500 \
>>>>                   -cpu POWER7,compat=power6
>>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
>>>>  use max-cpu-compat machine property instead
>>>> MMU model 983043 not supported by this machine.
>>>>
>>>> but I guess it's better than crashing. :)  
>>>
>>> Well, sure POWER7 doesn't make sense for an e500 machine for other
>>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
>>> compat= doesn't.
>>>
>>
>> The powernv machine type doesn't even support CPU features at all:
>>
>>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
>>     if (!object_class_by_name(chip_typename)) {
>>         error_report("invalid CPU model '%s' for %s machine",
>>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
>>         exit(1);
>>     }
> 
> Ah, well, that's another bug, but not one that's in scope for this
> series.

PowerNV is still work in progress. I would not worry about it too much.

C. 

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[David Gibson]

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

On Thu, Jun 01, 2017 at 09:29:08AM +0200, Greg Kurz wrote:
> On Thu, 01 Jun 2017 15:44:40 +1000
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:
> 
> > On Fri, 2017-05-26 at 15:23 +1000, David Gibson wrote:
> > > Server class POWER CPUs have a "compat" property, which is used to
> > > set the
> > > backwards compatibility mode for the processor.  However, this only
> > > makes
> > > sense for machine types which don't give the guest access to
> > > hypervisor
> > > privilege - otherwise the compatibility level is under the guest's
> > > control.
> > > 
> > > To reflect this, this removes the CPU 'compat' property and instead
> > > creates a 'max-cpu-compat' property on the pseries machine.  Strictly
> > > speaking this breaks compatibility, but AFAIK the 'compat' option was
> > > never (directly) used with -device or device_add.
> > > 
> > > The option was used with -cpu.  So, to maintain compatibility, this
> > > patch adds a hack to the cpu option parsing to strip out any compat
> > > options supplied with -cpu and set them on the machine property
> > > instead of the now deprecated cpu property.  
> > 
> > Generally looks good, a couple of comments below.
> > 
> > Suraj
> > 
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr.c              |   6 ++-
> > >  hw/ppc/spapr_cpu_core.c     |  56 +++++++++++++++++++++++-
> > >  hw/ppc/spapr_hcall.c        |   8 ++--
> > >  include/hw/ppc/spapr.h      |  12 ++++--
> > >  target/ppc/compat.c         | 102
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  target/ppc/cpu.h            |   5 ++-
> > >  target/ppc/translate_init.c |  86 +++++++++++-----------------------
> > > ---
> > >  7 files changed, 202 insertions(+), 73 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index ab3aab1..3c4e88f 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> > > *machine)
> > >          machine->cpu_model = kvm_enabled() ? "host" : smc-  
> > > >tcg_default_cpu;  
> > >      }
> > >  
> > > -    ppc_cpu_parse_features(machine->cpu_model);
> > > +    spapr_cpu_parse_features(spapr);
> > >  
> > >      spapr_init_cpus(spapr);
> > >  
> > > @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
> > >                                      " place of standard EPOW events
> > > when possible"
> > >                                      " (required for memory hot-
> > > unplug support)",
> > >                                      NULL);
> > > +
> > > +    ppc_compat_add_property(obj, "max-cpu-compat", &spapr-  
> > > >max_compat_pvr,  
> > > +                            "Maximum permitted CPU compatibility
> > > mode",
> > > +                            &error_fatal);
> > >  }
> > >  
> > >  static void spapr_machine_finalizefn(Object *obj)
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index ff7058e..ab4102b 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -20,6 +20,58 @@
> > >  #include "sysemu/numa.h"
> > >  #include "qemu/error-report.h"
> > >  
> > > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > > +{
> > > +    /*
> > > +     * Backwards compatibility hack:
> > > +     *
> > > +     *   CPUs had a "compat=" property which didn't make sense for
> > > +     *   anything except pseries.  It was replaced by "max-cpu-
> > > compat"
> > > +     *   machine option.  This supports old command lines like
> > > +     *       -cpu POWER8,compat=power7
> > > +     *   By stripping the compat option and applying it to the
> > > machine
> > > +     *   before passing it on to the cpu level parser.
> > > +     */
> > > +    gchar **inpieces;
> > > +    int i, j;
> > > +    gchar *compat_str = NULL;
> > > +
> > > +    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > > +
> > > +    /* inpieces[0] is the actual model string */
> > > +    i = 1;
> > > +    j = 1;
> > > +    while (inpieces[i]) {
> > > +        if (g_str_has_prefix(inpieces[i], "compat=")) {
> > > +            /* in case of multiple compat= optipons */  
> > 
> > s/optipons/options?
> > 
> > > +            g_free(compat_str);
> > > +            compat_str = inpieces[i];
> > > +        } else {
> > > +            j++;
> > > +        }
> > > +
> > > +        /* Excise compat options from list */
> > > +        inpieces[j] = inpieces[i];  
> > 
> > it's worth noting that where previously when specifying an invalid
> > option you got:
> > 
> > qemu-system-ppc64: Expected key=value format, found *blah*
> > 
> > You now get a segfault here.
> > 
> 
> Yeah. This basically does:
> 
>     inpieces[i + 1] = inpieces[i];
> 
> and we end up overwriting the terminal NULL pointer with a non-NULL
> pointer.
> 
> What about simplifying the loop to:
> 
>     /* inpieces[0] is the actual model string */
>     i = 1;
>     while (inpieces[i]) {
>         if (g_str_has_prefix(inpieces[i], "compat=")) {
>             /* in case of multiple compat= optipons */
>             g_free(compat_str);
>             compat_str = inpieces[i];
>             /* Excise compat options from list */
>             inpieces[i] = inpieces[i + 1];
>         }
>         i++;
>     }

No.. that would duplicate the entry after the compat=, instead of
properly excising it.  I've already fixed this for my next draft.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Greg Kurz]

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

On Thu, 1 Jun 2017 13:59:14 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/01/2017 08:52 AM, David Gibson wrote:
> > On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:  
> >> On Wed, 31 May 2017 12:57:48 +1000
> >> David Gibson <david@gibson.dropbear.id.au> wrote:  
> >>> [...]  
> >>>> All old non-pseries machine types already complain when started with
> >>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
> >>>>
> >>>> qemu-system-ppc64 -machine ppce500 \
> >>>>                   -cpu POWER7,compat=power6
> >>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> >>>>  use max-cpu-compat machine property instead
> >>>> MMU model 983043 not supported by this machine.
> >>>>
> >>>> but I guess it's better than crashing. :)    
> >>>
> >>> Well, sure POWER7 doesn't make sense for an e500 machine for other
> >>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> >>> compat= doesn't.
> >>>  
> >>
> >> The powernv machine type doesn't even support CPU features at all:
> >>
> >>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> >>     if (!object_class_by_name(chip_typename)) {
> >>         error_report("invalid CPU model '%s' for %s machine",
> >>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
> >>         exit(1);
> >>     }  
> > 
> > Ah, well, that's another bug, but not one that's in scope for this
> > series.  
> 
> PowerNV is still work in progress. I would not worry about it too much.
> 

Of course and this isn't the purpose of the discussion actually. We were
talking about CPU features being relevant or not depending on the machine
type.

But I'm not even sure that CPU features are useful at all for ppc, not to
say very confusing (otherwise this series wouldn't be needed for example).

Speaking of PowerNV, just as an example, I guess the fix would be to
forbid machine->cpu_model if it contains features. And probably the same
for all other machine types, except pseries for backward compatibility
reasons.

> C. 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCHv4 3/5] pseries: Move CPU compatibility property to machine

[Greg Kurz]

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

On Thu, 1 Jun 2017 22:24:47 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
[...]
> > 
> > Yeah. This basically does:
> > 
> >     inpieces[i + 1] = inpieces[i];
> > 
> > and we end up overwriting the terminal NULL pointer with a non-NULL
> > pointer.
> > 
> > What about simplifying the loop to:
> > 
> >     /* inpieces[0] is the actual model string */
> >     i = 1;
> >     while (inpieces[i]) {
> >         if (g_str_has_prefix(inpieces[i], "compat=")) {
> >             /* in case of multiple compat= optipons */
> >             g_free(compat_str);
> >             compat_str = inpieces[i];
> >             /* Excise compat options from list */
> >             inpieces[i] = inpieces[i + 1];
> >         }
> >         i++;
> >     }  
> 
> No.. that would duplicate the entry after the compat=, instead of
> properly excising it.  I've already fixed this for my next draft.
> 

D'oh you're right... sorry for the noise :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Thu, Jun 01, 2017 at 01:59:14PM +0200, Cédric Le Goater wrote:
> On 06/01/2017 08:52 AM, David Gibson wrote:
> > On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:
> >> On Wed, 31 May 2017 12:57:48 +1000
> >> David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> [...]
> >>>> All old non-pseries machine types already complain when started with
> >>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
> >>>>
> >>>> qemu-system-ppc64 -machine ppce500 \
> >>>>                   -cpu POWER7,compat=power6
> >>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> >>>>  use max-cpu-compat machine property instead
> >>>> MMU model 983043 not supported by this machine.
> >>>>
> >>>> but I guess it's better than crashing. :)  
> >>>
> >>> Well, sure POWER7 doesn't make sense for an e500 machine for other
> >>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> >>> compat= doesn't.
> >>>
> >>
> >> The powernv machine type doesn't even support CPU features at all:
> >>
> >>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> >>     if (!object_class_by_name(chip_typename)) {
> >>         error_report("invalid CPU model '%s' for %s machine",
> >>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
> >>         exit(1);
> >>     }
> > 
> > Ah, well, that's another bug, but not one that's in scope for this
> > series.
> 
> PowerNV is still work in progress. I would not worry about it too much.

I wasn't intending to :).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Thu, Jun 01, 2017 at 03:09:15PM +0200, Greg Kurz wrote:
> On Thu, 1 Jun 2017 13:59:14 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 06/01/2017 08:52 AM, David Gibson wrote:
> > > On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:  
> > >> On Wed, 31 May 2017 12:57:48 +1000
> > >> David Gibson <david@gibson.dropbear.id.au> wrote:  
> > >>> [...]  
> > >>>> All old non-pseries machine types already complain when started with
> > >>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
> > >>>>
> > >>>> qemu-system-ppc64 -machine ppce500 \
> > >>>>                   -cpu POWER7,compat=power6
> > >>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> > >>>>  use max-cpu-compat machine property instead
> > >>>> MMU model 983043 not supported by this machine.
> > >>>>
> > >>>> but I guess it's better than crashing. :)    
> > >>>
> > >>> Well, sure POWER7 doesn't make sense for an e500 machine for other
> > >>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> > >>> compat= doesn't.
> > >>>  
> > >>
> > >> The powernv machine type doesn't even support CPU features at all:
> > >>
> > >>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> > >>     if (!object_class_by_name(chip_typename)) {
> > >>         error_report("invalid CPU model '%s' for %s machine",
> > >>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
> > >>         exit(1);
> > >>     }  
> > > 
> > > Ah, well, that's another bug, but not one that's in scope for this
> > > series.  
> > 
> > PowerNV is still work in progress. I would not worry about it too much.
> > 
> 
> Of course and this isn't the purpose of the discussion actually. We were
> talking about CPU features being relevant or not depending on the machine
> type.
> 
> But I'm not even sure that CPU features are useful at all for ppc, not to
> say very confusing (otherwise this series wouldn't be needed for example).
> 
> Speaking of PowerNV, just as an example, I guess the fix would be to
> forbid machine->cpu_model if it contains features. And probably the same
> for all other machine types, except pseries for backward compatibility
> reasons.

I don't think that's correct in principle.  I can imagine CPU
properties it might make sense to really set on the cpu, regardless of
machine type.  A quick look says we don't have any such at the moment,
but I don't think it's something we should prevent as a matter of policy.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 5/5] ppc: Rework CPU compatibility testing across migration

[David Gibson]

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

On Thu, Jun 01, 2017 at 10:23:18AM +0200, Greg Kurz wrote:
> On Fri, 26 May 2017 15:23:19 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Migrating between different CPU versions is a bit complicated for ppc.
> > A long time ago, we ensured identical CPU versions at either end by
> > checking the PVR had the same value.  However, this breaks under KVM
> > HV, because we always have to use the host's PVR - it's not
> > virtualized.  That would mean we couldn't migrate between hosts with
> > different PVRs, even if the CPUs are close enough to compatible in
> > practice (sometimes identical cores with different surrounding logic
> > have different PVRs, so this happens in practice quite often).
> > 
> > So, we removed the PVR check, but instead checked that several flags
> > indicating supported instructions matched.  This turns out to be a bad
> > idea, because those instruction masks are not architected information, but
> > essentially a TCG implementation detail.  So changes to qemu internal CPU
> > modelling can break migration - this happened between qemu-2.6 and
> > qemu-2.7.  That was addressed by 146c11f1 "target-ppc: Allow eventual
> > removal of old migration mistakes".
> > 
> > Now, verification of CPU compatibility across a migration basically doesn't
> > happen.  We simply ignore the PVR of the incoming migration, and hope the
> > cpu on the destination is close enough to work.
> > 
> > Now that we've cleaned up handling of processor compatibility modes for
> > pseries machine type, we can do better.  We allow migration if:
> > 
> >     * The source and destination PVRs are for the same type of CPU, as
> >       determined by CPU class's pvr_match function
> > OR  * When the source was in a compatibility mode, and the destination CPU
> >       supports the same compatibility mode
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target/ppc/machine.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 69 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..2c6d9dc 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -8,6 +8,7 @@
> >  #include "helper_regs.h"
> >  #include "mmu-hash64.h"
> >  #include "migration/cpu.h"
> > +#include "qapi/error.h"
> >  
> >  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> >  {
> > @@ -195,6 +196,30 @@ static void cpu_pre_save(void *opaque)
> >      }
> >  }
> >  
> > +/*
> > + * Determine if a given PVR is a "close enough" match to the CPU
> > + * object.  For TCG and KVM PR it would probably be sufficient to
> > + * require an exact PVR match.  However for KVM HV the user is
> > + * restricted to a PVR exactly matching the host CPU.  The correct way
> > + * to handle this is to put the guest into an architected
> > + * compatibility mode.  However, to allow a more forgiving transition
> > + * and migration from before this was widely done, we allow migration
> > + * between sufficiently similar PVRs, as determined by the CPU class's
> > + * pvr_match() hook.
> > + */
> > +static bool pvr_match(PowerPCCPU *cpu, uint32_t pvr)
> > +{
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +
> > +    if (pvr == pcc->pvr) {
> > +        return true;
> > +    }
> > +    if (pcc->pvr_match) {
> > +        return pcc->pvr_match(pcc, pvr);
> > +    }
> > +    return false;
> > +}
> 
> The base class provides a fallback for pcc->pvr_match that does:
> 
> static bool ppc_pvr_match_default(PowerPCCPUClass *pcc, uint32_t pvr)
> {
>     return pcc->pvr == pvr;
> }
> 
> so I'm not sure this function is needed, but maybe I'm missing something.

Ah, yes, I think you're right.  I've simplified accordingly.

> > +
> >  static int cpu_post_load(void *opaque, int version_id)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -203,10 +228,31 @@ static int cpu_post_load(void *opaque, int version_id)
> >      target_ulong msr;
> >  
> >      /*
> > -     * We always ignore the source PVR. The user or management
> > -     * software has to take care of running QEMU in a compatible mode.
> > +     * If we're operating in compat mode, we should be ok as long as
> > +     * the destination supports the same compatiblity mode.
> > +     *
> > +     * Otherwise, however, we require that the destination has exactly
> > +     * the same CPU model as the source.
> >       */
> > -    env->spr[SPR_PVR] = env->spr_cb[SPR_PVR].default_value;
> > +
> > +#if defined(TARGET_PPC64)
> > +    if (cpu->compat_pvr) {
> > +        Error *local_err = NULL;
> > +
> > +        ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> > +        if (local_err) {
> > +            error_report_err(local_err);
> > +            error_free(local_err);
> > +            return -1;
> > +        }
> > +    } else
> > +#endif
> > +    {
> > +        if (!pvr_match(cpu, env->spr[SPR_PVR])) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      env->lr = env->spr[SPR_LR];
> >      env->ctr = env->spr[SPR_CTR];
> >      cpu_write_xer(env, env->spr[SPR_XER]);
> > @@ -560,6 +606,25 @@ static const VMStateDescription vmstate_tlbmas = {
> >      }
> >  };
> >  
> > +static bool compat_needed(void *opaque)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    assert(!(cpu->compat_pvr && !cpu->vhyp));
> > +    return (cpu->compat_pvr != 0);
> 
> Parenthesitis ?
> 
> > +}
> 
> This will break backward migration of pre-2.10 machine types. But it can be
> fixed in a followup patch if really needed (or even addressed
> downstream).

Ah.. unless we force the source machine into raw mode somehow.
Bother.

Pity we can't mark new migration sections as "safe to ignore" for old
versions.

As you say this could be addressed downstream.  Usually that makes
things ugly, but it might be reasonable in this case.

Anyways, I think I'll address it in a followup, rather than delaying
this.

> 
> Anyway, since you're the decision maker:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +
> > +static const VMStateDescription vmstate_compat = {
> > +    .name = "cpu/compat",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = compat_needed,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(compat_pvr, PowerPCCPU),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  const VMStateDescription vmstate_ppc_cpu = {
> >      .name = "cpu",
> >      .version_id = 5,
> > @@ -613,6 +678,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          &vmstate_tlb6xx,
> >          &vmstate_tlbemb,
> >          &vmstate_tlbmas,
> > +        &vmstate_compat,
> >          NULL
> >      }
> >  };
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[Greg Kurz]

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

On Fri, 2 Jun 2017 12:00:07 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 01, 2017 at 03:09:15PM +0200, Greg Kurz wrote:
> > On Thu, 1 Jun 2017 13:59:14 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> > > On 06/01/2017 08:52 AM, David Gibson wrote:  
> > > > On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:    
> > > >> On Wed, 31 May 2017 12:57:48 +1000
> > > >> David Gibson <david@gibson.dropbear.id.au> wrote:    
> > > >>> [...]    
> > > >>>> All old non-pseries machine types already complain when started with
> > > >>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
> > > >>>>
> > > >>>> qemu-system-ppc64 -machine ppce500 \
> > > >>>>                   -cpu POWER7,compat=power6
> > > >>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> > > >>>>  use max-cpu-compat machine property instead
> > > >>>> MMU model 983043 not supported by this machine.
> > > >>>>
> > > >>>> but I guess it's better than crashing. :)      
> > > >>>
> > > >>> Well, sure POWER7 doesn't make sense for an e500 machine for other
> > > >>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> > > >>> compat= doesn't.
> > > >>>    
> > > >>
> > > >> The powernv machine type doesn't even support CPU features at all:
> > > >>
> > > >>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> > > >>     if (!object_class_by_name(chip_typename)) {
> > > >>         error_report("invalid CPU model '%s' for %s machine",
> > > >>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
> > > >>         exit(1);
> > > >>     }    
> > > > 
> > > > Ah, well, that's another bug, but not one that's in scope for this
> > > > series.    
> > > 
> > > PowerNV is still work in progress. I would not worry about it too much.
> > >   
> > 
> > Of course and this isn't the purpose of the discussion actually. We were
> > talking about CPU features being relevant or not depending on the machine
> > type.
> > 
> > But I'm not even sure that CPU features are useful at all for ppc, not to
> > say very confusing (otherwise this series wouldn't be needed for example).
> > 
> > Speaking of PowerNV, just as an example, I guess the fix would be to
> > forbid machine->cpu_model if it contains features. And probably the same
> > for all other machine types, except pseries for backward compatibility
> > reasons.  
> 
> I don't think that's correct in principle.  I can imagine CPU
> properties it might make sense to really set on the cpu, regardless of
> machine type.  A quick look says we don't have any such at the moment,
> but I don't think it's something we should prevent as a matter of policy.
> 

Fair enough. Then maybe all machine should parse CPU features and check which
one are valid before instantiating the CPUs ?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCHv4 0/5] Clean up compatibility mode handling

[David Gibson]

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

On Fri, Jun 02, 2017 at 10:15:25AM +0200, Greg Kurz wrote:
> On Fri, 2 Jun 2017 12:00:07 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 01, 2017 at 03:09:15PM +0200, Greg Kurz wrote:
> > > On Thu, 1 Jun 2017 13:59:14 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > > > On 06/01/2017 08:52 AM, David Gibson wrote:  
> > > > > On Wed, May 31, 2017 at 10:58:57AM +0200, Greg Kurz wrote:    
> > > > >> On Wed, 31 May 2017 12:57:48 +1000
> > > > >> David Gibson <david@gibson.dropbear.id.au> wrote:    
> > > > >>> [...]    
> > > > >>>> All old non-pseries machine types already complain when started with
> > > > >>>> a POWER7 or newer CPU. Providing the extra error message looks weird:
> > > > >>>>
> > > > >>>> qemu-system-ppc64 -machine ppce500 \
> > > > >>>>                   -cpu POWER7,compat=power6
> > > > >>>> qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect;
> > > > >>>>  use max-cpu-compat machine property instead
> > > > >>>> MMU model 983043 not supported by this machine.
> > > > >>>>
> > > > >>>> but I guess it's better than crashing. :)      
> > > > >>>
> > > > >>> Well, sure POWER7 doesn't make sense for an e500 machine for other
> > > > >>> reasons.  But POWER7 or POWER8 _would_ make sense for powernv, where
> > > > >>> compat= doesn't.
> > > > >>>    
> > > > >>
> > > > >> The powernv machine type doesn't even support CPU features at all:
> > > > >>
> > > > >>     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> > > > >>     if (!object_class_by_name(chip_typename)) {
> > > > >>         error_report("invalid CPU model '%s' for %s machine",
> > > > >>                      machine->cpu_model, MACHINE_GET_CLASS(machine)->name);
> > > > >>         exit(1);
> > > > >>     }    
> > > > > 
> > > > > Ah, well, that's another bug, but not one that's in scope for this
> > > > > series.    
> > > > 
> > > > PowerNV is still work in progress. I would not worry about it too much.
> > > >   
> > > 
> > > Of course and this isn't the purpose of the discussion actually. We were
> > > talking about CPU features being relevant or not depending on the machine
> > > type.
> > > 
> > > But I'm not even sure that CPU features are useful at all for ppc, not to
> > > say very confusing (otherwise this series wouldn't be needed for example).
> > > 
> > > Speaking of PowerNV, just as an example, I guess the fix would be to
> > > forbid machine->cpu_model if it contains features. And probably the same
> > > for all other machine types, except pseries for backward compatibility
> > > reasons.  
> > 
> > I don't think that's correct in principle.  I can imagine CPU
> > properties it might make sense to really set on the cpu, regardless of
> > machine type.  A quick look says we don't have any such at the moment,
> > but I don't think it's something we should prevent as a matter of policy.
> 
> Fair enough. Then maybe all machine should parse CPU features and check which
> one are valid before instantiating the CPUs ?

Well, CPu properties *should* be valid for all machine types.  The
fact that compat= wasn't was a mistake.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]