* [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
@ 2017-04-27 7:28 David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors David Gibson
` (7 more replies)
0 siblings, 8 replies; 30+ messages in thread
From: David Gibson @ 2017-04-27 7:28 UTC (permalink / raw)
To: groug, clg, aik, mdroth, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc, 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 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 (3):
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
hw/ppc/spapr.c | 8 ++++-
hw/ppc/spapr_cpu_core.c | 47 +++++++++++++++++++------
hw/ppc/spapr_hcall.c | 2 +-
include/hw/ppc/spapr.h | 12 ++++---
qapi/string-input-visitor.c | 11 ++++++
qapi/string-output-visitor.c | 14 ++++++++
target/ppc/compat.c | 65 ++++++++++++++++++++++++++++++++++
target/ppc/cpu.h | 6 ++--
target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++--
target/ppc/translate_init.c | 84 ++++++++++++--------------------------------
10 files changed, 238 insertions(+), 82 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
@ 2017-04-27 7:28 ` David Gibson
2017-05-02 11:48 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
` (6 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-04-27 7:28 UTC (permalink / raw)
To: groug, clg, aik, mdroth, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc, 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 94ac821..5ec5352 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -266,6 +266,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)
@@ -351,6 +364,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.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors David Gibson
@ 2017-04-27 7:28 ` David Gibson
2017-04-27 17:23 ` Michael Roth
` (3 more replies)
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode David Gibson
` (5 subsequent siblings)
7 siblings, 4 replies; 30+ messages in thread
From: David Gibson @ 2017-04-27 7:28 UTC (permalink / raw)
To: groug, clg, aik, mdroth, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc, 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 new
removed cpu property.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 6 +++-
hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
hw/ppc/spapr_hcall.c | 2 +-
include/hw/ppc/spapr.h | 12 ++++---
target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
target/ppc/cpu.h | 6 ++--
target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
7 files changed, 145 insertions(+), 71 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 80d12d0..547fa27 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2117,7 +2117,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);
@@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
" place of standard EPOW events when possible"
" (required for memory hot-unplug support)",
NULL);
+
+ object_property_add(obj, "max-cpu-compat", "str",
+ ppc_compat_prop_get, ppc_compat_prop_set,
+ NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -20,6 +20,43 @@
#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 n, i;
+ gchar *compat_str = NULL;
+
+ inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
+ n = g_strv_length(inpieces);
+
+ /* inpieces[0] is the actual model string */
+ for (i = 0; i < n; i++) {
+ if (g_str_has_prefix(inpieces[i], "compat=")) {
+ compat_str = inpieces[i];
+ }
+ }
+
+ if (compat_str) {
+ char *val = compat_str + strlen("compat=");
+ object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
+ &error_fatal);
+ }
+
+ 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 +107,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 9f18f75..d4dc12b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
target_ulong list = ppc64_phys_to_real(args[0]);
target_ulong ov_table;
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;
sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5802f88..40d5f89 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -86,16 +86,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;
@@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
@@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
return n_threads;
}
+
+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);
+}
+
+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);
+}
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e0ff041..e953e75 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1185,7 +1185,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.
@@ -1197,7 +1196,6 @@ struct PowerPCCPU {
CPUPPCState env;
int cpu_dt_id;
- uint32_t max_compat;
uint32_t compat_pvr;
PPCVirtualHypervisor *vhyp;
Object *intc;
@@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
+ const char *name, void *opaque, Error **err);
+void ppc_compat_prop_set(Object *obj, Visitor *v,
+ const char *name, void *opaque, Error **err);
#endif /* defined(TARGET_PPC64) */
#include "exec/cpu-all.h"
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index e82e3e6..a92c825 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8413,73 +8413,35 @@ 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);
- }
-
- g_free(value);
+ error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
+ visit_type_null(v, name, errp);
}
-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.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
@ 2017-04-27 7:28 ` David Gibson
2017-04-27 18:08 ` Michael Roth
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration David Gibson
` (4 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-04-27 7:28 UTC (permalink / raw)
To: groug, clg, aik, mdroth, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc, 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>
---
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 547fa27..67f5106 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1332,6 +1332,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 ba610bc..e810431 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -107,16 +107,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.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
` (2 preceding siblings ...)
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode David Gibson
@ 2017-04-27 7:28 ` David Gibson
2017-04-27 19:51 ` Michael Roth
2017-05-04 10:07 ` Greg Kurz
2017-04-27 8:04 ` [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling no-reply
` (3 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: David Gibson @ 2017-04-27 7:28 UTC (permalink / raw)
To: groug, clg, aik, mdroth, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc, 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 68 insertions(+), 3 deletions(-)
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 6cb3a48..20a46c9 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,24 @@ static const VMStateDescription vmstate_tlbmas = {
}
};
+static bool compat_needed(void *opaque)
+{
+ PowerPCCPU *cpu = opaque;
+
+ return cpu->vhyp != NULL;
+}
+
+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 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
&vmstate_tlb6xx,
&vmstate_tlbemb,
&vmstate_tlbmas,
+ &vmstate_compat,
NULL
}
};
--
2.9.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
` (3 preceding siblings ...)
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration David Gibson
@ 2017-04-27 8:04 ` no-reply
2017-04-28 9:29 ` Greg Kurz
` (2 subsequent siblings)
7 siblings, 0 replies; 30+ messages in thread
From: no-reply @ 2017-04-27 8:04 UTC (permalink / raw)
To: david
Cc: famz, groug, clg, aik, mdroth, nikunj, agraf, qemu-devel, armbru,
qemu-ppc, abologna
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20170427072843.8089-1-david@gibson.dropbear.id.au
Subject: [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
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
From https://github.com/patchew-project/qemu
- [tag update] patchew/20170426221046.4596-1-eblake@redhat.com -> patchew/20170426221046.4596-1-eblake@redhat.com
Switched to a new branch 'test'
28f8147 ppc: Rework CPU compatibility testing across migration
07b83df pseries: Reset CPU compatibility mode
f9b3835 pseries: Move CPU compatibility property to machine
504c501 qapi: add explicit null to string input and output visitors
=== OUTPUT BEGIN ===
Checking PATCH 1/4: qapi: add explicit null to string input and output visitors...
Checking PATCH 2/4: pseries: Move CPU compatibility property to machine...
ERROR: line over 90 characters
#372: FILE: target/ppc/translate_init.c:8430:
+ error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
total: 1 errors, 0 warnings, 332 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/4: pseries: Reset CPU compatibility mode...
Checking PATCH 4/4: ppc: Rework CPU compatibility testing across migration...
ERROR: braces {} are necessary for all arms of this statement
#96: FILE: target/ppc/machine.c:239:
+ if (cpu->compat_pvr) {
[...]
+ } else
[...]
total: 1 errors, 0 warnings, 102 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
@ 2017-04-27 17:23 ` Michael Roth
2017-05-01 2:33 ` David Gibson
2017-05-02 14:24 ` Greg Kurz
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Michael Roth @ 2017-04-27 17:23 UTC (permalink / raw)
To: David Gibson, aik, clg, groug, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc
Quoting David Gibson (2017-04-27 02:28:41)
> 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 new
> removed cpu property.
"now-deprecated cpu property" maybe?
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 6 +++-
> hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
> hw/ppc/spapr_hcall.c | 2 +-
> include/hw/ppc/spapr.h | 12 ++++---
> target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
> target/ppc/cpu.h | 6 ++--
> target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
> 7 files changed, 145 insertions(+), 71 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..547fa27 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2117,7 +2117,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);
>
> @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events when possible"
> " (required for memory hot-unplug support)",
> NULL);
> +
> + object_property_add(obj, "max-cpu-compat", "str",
> + ppc_compat_prop_get, ppc_compat_prop_set,
> + NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,43 @@
> #include "sysemu/numa.h"
> #include "qemu/error-report.h"
>
> +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> +{
> + /*
> + * Backwards compatibility hack:
> +
Missing "*"
> + * 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 n, i;
> + gchar *compat_str = NULL;
> +
> + inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> + n = g_strv_length(inpieces);
> +
> + /* inpieces[0] is the actual model string */
The comment seems a bit out of place unless we start with i = 1
> + for (i = 0; i < n; i++) {
> + if (g_str_has_prefix(inpieces[i], "compat=")) {
> + compat_str = inpieces[i];
> + }
> + }
> +
> + if (compat_str) {
> + char *val = compat_str + strlen("compat=");
> + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> + &error_fatal);
> + }
> +
> + 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 +107,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 9f18f75..d4dc12b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> target_ulong list = ppc64_phys_to_real(args[0]);
> target_ulong ov_table;
> 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;
> sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f88..40d5f89 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -86,16 +86,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;
> @@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
> @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
>
> return n_threads;
> }
> +
> +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);
> +}
> +
> +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);
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e0ff041..e953e75 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1185,7 +1185,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.
> @@ -1197,7 +1196,6 @@ struct PowerPCCPU {
>
> CPUPPCState env;
> int cpu_dt_id;
> - uint32_t max_compat;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> Object *intc;
> @@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> +void ppc_compat_prop_set(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> #endif /* defined(TARGET_PPC64) */
>
> #include "exec/cpu-all.h"
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index e82e3e6..a92c825 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8413,73 +8413,35 @@ 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);
> - }
> -
> - g_free(value);
> + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
Isn't the "and has no effect" portion a bit misleading? AFAICT it does
have an effect still, it just shouldn't be used anymore.
> + visit_type_null(v, name, errp);
> }
>
> -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.3
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode David Gibson
@ 2017-04-27 18:08 ` Michael Roth
0 siblings, 0 replies; 30+ messages in thread
From: Michael Roth @ 2017-04-27 18:08 UTC (permalink / raw)
To: David Gibson, aik, clg, groug, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc
Quoting David Gibson (2017-04-27 02:28:42)
> 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 547fa27..67f5106 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1332,6 +1332,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 ba610bc..e810431 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -107,16 +107,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.3
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration David Gibson
@ 2017-04-27 19:51 ` Michael Roth
2017-05-01 6:48 ` David Gibson
2017-05-26 3:40 ` David Gibson
2017-05-04 10:07 ` Greg Kurz
1 sibling, 2 replies; 30+ messages in thread
From: Michael Roth @ 2017-04-27 19:51 UTC (permalink / raw)
To: David Gibson, aik, clg, groug, nikunj
Cc: agraf, abologna, armbru, qemu-devel, qemu-ppc
Quoting David Gibson (2017-04-27 02:28:43)
> 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..20a46c9 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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> }
> };
>
> +static bool compat_needed(void *opaque)
> +{
> + PowerPCCPU *cpu = opaque;
> +
> + return cpu->vhyp != NULL;
> +}
Would it make sense to relax this to:
cpu->vhyp != NULL && cpu->compat_pvr
? This would at least allow cross-version migration in cases where we
weren't previously running in a compatibility mode. As it stands it
seems like this would rule out both new->old and old->new migration.
Another possibility might be to only enable migration/checking of
compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
stuff.
> +
> +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 +677,7 @@ const VMStateDescription vmstate_ppc_cpu = {
> &vmstate_tlb6xx,
> &vmstate_tlbemb,
> &vmstate_tlbmas,
> + &vmstate_compat,
> NULL
> }
> };
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
` (4 preceding siblings ...)
2017-04-27 8:04 ` [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling no-reply
@ 2017-04-28 9:29 ` Greg Kurz
2017-05-03 18:03 ` Greg Kurz
2017-05-04 14:32 ` Andrea Bolognani
7 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-04-28 9:29 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 5680 bytes --]
On Thu, 27 Apr 2017 17:28:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> 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:
>
Hi David,
I'll rerun the tests shortly.
Cheers,
--
Greg
> 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 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 (3):
> 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
>
> hw/ppc/spapr.c | 8 ++++-
> hw/ppc/spapr_cpu_core.c | 47 +++++++++++++++++++------
> hw/ppc/spapr_hcall.c | 2 +-
> include/hw/ppc/spapr.h | 12 ++++---
> qapi/string-input-visitor.c | 11 ++++++
> qapi/string-output-visitor.c | 14 ++++++++
> target/ppc/compat.c | 65 ++++++++++++++++++++++++++++++++++
> target/ppc/cpu.h | 6 ++--
> target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++--
> target/ppc/translate_init.c | 84 ++++++++++++--------------------------------
> 10 files changed, 238 insertions(+), 82 deletions(-)
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 17:23 ` Michael Roth
@ 2017-05-01 2:33 ` David Gibson
2017-05-02 11:23 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-05-01 2:33 UTC (permalink / raw)
To: Michael Roth
Cc: aik, clg, groug, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 17106 bytes --]
On Thu, Apr 27, 2017 at 12:23:51PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-04-27 02:28:41)
> > 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 new
> > removed cpu property.
>
> "now-deprecated cpu property" maybe?
Good idea.
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c | 6 +++-
> > hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
> > hw/ppc/spapr_hcall.c | 2 +-
> > include/hw/ppc/spapr.h | 12 ++++---
> > target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
> > target/ppc/cpu.h | 6 ++--
> > target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
> > 7 files changed, 145 insertions(+), 71 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 80d12d0..547fa27 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2117,7 +2117,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);
> >
> > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> > " place of standard EPOW events when possible"
> > " (required for memory hot-unplug support)",
> > NULL);
> > +
> > + object_property_add(obj, "max-cpu-compat", "str",
> > + ppc_compat_prop_get, ppc_compat_prop_set,
> > + NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -20,6 +20,43 @@
> > #include "sysemu/numa.h"
> > #include "qemu/error-report.h"
> >
> > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > +{
> > + /*
> > + * Backwards compatibility hack:
> > +
>
> Missing "*"
Oops, fixed.
> > + * 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 n, i;
> > + gchar *compat_str = NULL;
> > +
> > + inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > + n = g_strv_length(inpieces);
> > +
> > + /* inpieces[0] is the actual model string */
>
> The comment seems a bit out of place unless we start with i = 1
A good point. Yes, it should start from i=1.
> > + for (i = 0; i < n; i++) {
> > + if (g_str_has_prefix(inpieces[i], "compat=")) {
> > + compat_str = inpieces[i];
> > + }
> > + }
> > +
> > + if (compat_str) {
> > + char *val = compat_str + strlen("compat=");
> > + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> > + &error_fatal);
> > + }
> > +
> > + 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 +107,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 9f18f75..d4dc12b 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > target_ulong list = ppc64_phys_to_real(args[0]);
> > target_ulong ov_table;
> > 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;
> > sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 5802f88..40d5f89 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -86,16 +86,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;
> > @@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
> > @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
> >
> > return n_threads;
> > }
> > +
> > +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);
> > +}
> > +
> > +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);
> > +}
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index e0ff041..e953e75 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1185,7 +1185,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.
> > @@ -1197,7 +1196,6 @@ struct PowerPCCPU {
> >
> > CPUPPCState env;
> > int cpu_dt_id;
> > - uint32_t max_compat;
> > uint32_t compat_pvr;
> > PPCVirtualHypervisor *vhyp;
> > Object *intc;
> > @@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
> > + const char *name, void *opaque, Error **err);
> > +void ppc_compat_prop_set(Object *obj, Visitor *v,
> > + const char *name, void *opaque, Error **err);
> > #endif /* defined(TARGET_PPC64) */
> >
> > #include "exec/cpu-all.h"
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index e82e3e6..a92c825 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8413,73 +8413,35 @@ 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);
> > - }
> > -
> > - g_free(value);
> > + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
>
> Isn't the "and has no effect" portion a bit misleading? AFAICT it does
> have an effect still, it just shouldn't be used anymore.
No. Used as a property on the CPU object it really does have no
effect. It's just that if you attach it to -cpu XXX, rather than to a
-device specific-cpu-type, then it gets automatically translated to
the new machine property. These calls should only be triggered in the
second case.
> > + visit_type_null(v, name, errp);
> > }
> >
> > -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(),
> > };
> >
>
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-04-27 19:51 ` Michael Roth
@ 2017-05-01 6:48 ` David Gibson
2017-05-26 3:40 ` David Gibson
1 sibling, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-05-01 6:48 UTC (permalink / raw)
To: Michael Roth
Cc: aik, clg, groug, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 6972 bytes --]
On Thu, Apr 27, 2017 at 02:51:31PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-04-27 02:28:43)
> > 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 68 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > }
> > };
> >
> > +static bool compat_needed(void *opaque)
> > +{
> > + PowerPCCPU *cpu = opaque;
> > +
> > + return cpu->vhyp != NULL;
> > +}
>
> Would it make sense to relax this to:
>
> cpu->vhyp != NULL && cpu->compat_pvr
Hm, so that's equivalent to just cpu->compat_pvr != 0, since
compat_pvr should never be set when !vhyp.
> ? This would at least allow cross-version migration in cases where we
> weren't previously running in a compatibility mode. As it stands it
> seems like this would rule out both new->old and old->new migration.
Hrm. I thought ther section needed test just controlled whether the
section was created on outgoing migration, not whether it was
mandatory on an incoming migration. So old->new migration should I
think work (well, no worse than it ever did).
new->old would be broken by this though. So I guess I need some sort
of compat flag so the older machine types don't generate this section.
> Another possibility might be to only enable migration/checking of
> compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
> stuff.
>
> > +
> > +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 +677,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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-05-01 2:33 ` David Gibson
@ 2017-05-02 11:23 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-05-02 11:23 UTC (permalink / raw)
To: David Gibson
Cc: Michael Roth, aik, clg, nikunj, agraf, abologna, armbru,
qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 18733 bytes --]
On Mon, 1 May 2017 12:33:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Apr 27, 2017 at 12:23:51PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2017-04-27 02:28:41)
> > > 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 new
> > > removed cpu property.
> >
> > "now-deprecated cpu property" maybe?
>
> Good idea.
>
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > > hw/ppc/spapr.c | 6 +++-
> > > hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
> > > hw/ppc/spapr_hcall.c | 2 +-
> > > include/hw/ppc/spapr.h | 12 ++++---
> > > target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
> > > target/ppc/cpu.h | 6 ++--
> > > target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
> > > 7 files changed, 145 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 80d12d0..547fa27 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2117,7 +2117,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);
> > >
> > > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> > > " place of standard EPOW events when possible"
> > > " (required for memory hot-unplug support)",
> > > NULL);
> > > +
> > > + object_property_add(obj, "max-cpu-compat", "str",
> > > + ppc_compat_prop_get, ppc_compat_prop_set,
> > > + NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -20,6 +20,43 @@
> > > #include "sysemu/numa.h"
> > > #include "qemu/error-report.h"
> > >
> > > +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> > > +{
> > > + /*
> > > + * Backwards compatibility hack:
> > > +
> >
> > Missing "*"
>
> Oops, fixed.
>
> > > + * 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 n, i;
> > > + gchar *compat_str = NULL;
> > > +
> > > + inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> > > + n = g_strv_length(inpieces);
> > > +
> > > + /* inpieces[0] is the actual model string */
> >
> > The comment seems a bit out of place unless we start with i = 1
>
> A good point. Yes, it should start from i=1.
>
> > > + for (i = 0; i < n; i++) {
> > > + if (g_str_has_prefix(inpieces[i], "compat=")) {
> > > + compat_str = inpieces[i];
> > > + }
> > > + }
> > > +
> > > + if (compat_str) {
> > > + char *val = compat_str + strlen("compat=");
> > > + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> > > + &error_fatal);
> > > + }
> > > +
> > > + 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 +107,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 9f18f75..d4dc12b 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > > target_ulong list = ppc64_phys_to_real(args[0]);
> > > target_ulong ov_table;
> > > 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;
> > > sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 5802f88..40d5f89 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -86,16 +86,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;
> > > @@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
> > > @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
> > >
> > > return n_threads;
> > > }
> > > +
> > > +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);
> > > +}
> > > +
> > > +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);
> > > +}
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index e0ff041..e953e75 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -1185,7 +1185,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.
> > > @@ -1197,7 +1196,6 @@ struct PowerPCCPU {
> > >
> > > CPUPPCState env;
> > > int cpu_dt_id;
> > > - uint32_t max_compat;
> > > uint32_t compat_pvr;
> > > PPCVirtualHypervisor *vhyp;
> > > Object *intc;
> > > @@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
> > > + const char *name, void *opaque, Error **err);
> > > +void ppc_compat_prop_set(Object *obj, Visitor *v,
> > > + const char *name, void *opaque, Error **err);
> > > #endif /* defined(TARGET_PPC64) */
> > >
> > > #include "exec/cpu-all.h"
> > > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > > index e82e3e6..a92c825 100644
> > > --- a/target/ppc/translate_init.c
> > > +++ b/target/ppc/translate_init.c
> > > @@ -8413,73 +8413,35 @@ 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);
> > > - }
> > > -
> > > - g_free(value);
> > > + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
> >
> > Isn't the "and has no effect" portion a bit misleading? AFAICT it does
> > have an effect still, it just shouldn't be used anymore.
>
> No. Used as a property on the CPU object it really does have no
> effect. It's just that if you attach it to -cpu XXX, rather than to a
> -device specific-cpu-type, then it gets automatically translated to
> the new machine property. These calls should only be triggered in the
> second case.
>
But they also get triggered in the -cpu case... When passing
-cpu host,compat=power7 -machine type=pseries,accel=kvm
one gets:
qemu-system-ppc64: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=power7: Invalid parameter type for 'compat', expected: null
> > > + visit_type_null(v, name, errp);
Because the null string input visitor from patch 1/4 expects an empty
string:
+ if (!siv->string || siv->string[0]) {
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+ "null");
+ }
and the error gets propagated all the way up.
I had suggested to break the propagation with:
+ visit_type_null(v, name, NULL);
You can find the discussion here:
http://patchwork.ozlabs.org/patch/705734/
> > > }
> > >
> > > -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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors David Gibson
@ 2017-05-02 11:48 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-05-02 11:48 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, qemu-devel, armbru, qemu-ppc, abologna
[-- Attachment #1: Type: text/plain, Size: 3210 bytes --]
On Thu, 27 Apr 2017 17:28:40 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> 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");
I had boldly copied the error_setg() line from qobject_input_type_null(), but
I guess the 3rd argument ("null") isn't really appropriate to describe the
expected input in this case.
What about something like below ?
+ error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
+ "empty string");
> + }
> +}
> +
> 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 94ac821..5ec5352 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -266,6 +266,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)
> @@ -351,6 +364,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;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
2017-04-27 17:23 ` Michael Roth
@ 2017-05-02 14:24 ` Greg Kurz
2017-05-26 1:24 ` David Gibson
2017-05-04 10:06 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2017-05-04 17:09 ` [Qemu-devel] " Andrea Bolognani
3 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-05-02 14:24 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 16113 bytes --]
On Thu, 27 Apr 2017 17:28:41 +1000
David Gibson <david@gibson.dropbear.id.au> 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 new
> removed cpu property.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c | 6 +++-
> hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
> hw/ppc/spapr_hcall.c | 2 +-
> include/hw/ppc/spapr.h | 12 ++++---
> target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
> target/ppc/cpu.h | 6 ++--
> target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
> 7 files changed, 145 insertions(+), 71 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..547fa27 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2117,7 +2117,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);
>
> @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events when possible"
> " (required for memory hot-unplug support)",
> NULL);
> +
> + object_property_add(obj, "max-cpu-compat", "str",
> + ppc_compat_prop_get, ppc_compat_prop_set,
> + NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,43 @@
> #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 n, i;
> + gchar *compat_str = NULL;
> +
> + inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> + n = g_strv_length(inpieces);
> +
> + /* inpieces[0] is the actual model string */
> + for (i = 0; i < n; i++) {
> + if (g_str_has_prefix(inpieces[i], "compat=")) {
> + compat_str = inpieces[i];
> + }
> + }
> +
> + if (compat_str) {
> + char *val = compat_str + strlen("compat=");
> + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> + &error_fatal);
> + }
> +
> + 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 +107,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 9f18f75..d4dc12b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> target_ulong list = ppc64_phys_to_real(args[0]);
> target_ulong ov_table;
> 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;
> sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f88..40d5f89 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -86,16 +86,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;
> @@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
And now we also have POWER9 in the list, so:
.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 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
>
> return n_threads;
> }
> +
> +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);
> +}
> +
> +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;
The current implementation in powerpc_get_compat() considers "" to be an
invalid compatibility mode. Is there a reason to behave differently with
max-cpu-compat ?
> + } 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);
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e0ff041..e953e75 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1185,7 +1185,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.
> @@ -1197,7 +1196,6 @@ struct PowerPCCPU {
>
> CPUPPCState env;
> int cpu_dt_id;
> - uint32_t max_compat;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> Object *intc;
> @@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> +void ppc_compat_prop_set(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> #endif /* defined(TARGET_PPC64) */
>
> #include "exec/cpu-all.h"
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index e82e3e6..a92c825 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8413,73 +8413,35 @@ 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);
> - }
> -
> - g_free(value);
> + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
> + visit_type_null(v, name, errp);
> }
>
As suggested in another mail, maybe NULL should be passed instead of errp if
we really want to implement the "has no effect". Otherwise, it has the effect
of terminating QEMU when passing a compat prop that isn't "".
> -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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
` (5 preceding siblings ...)
2017-04-28 9:29 ` Greg Kurz
@ 2017-05-03 18:03 ` Greg Kurz
2017-05-04 14:32 ` Andrea Bolognani
7 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-05-03 18:03 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 8389 bytes --]
On Thu, 27 Apr 2017 17:28:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> 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:
>
I tested with the following change squashed into patch 2:
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8428,7 +8428,7 @@ static void getset_compat_deprecated(Object *obj, Visitor
void *opaque, Error **errp)
{
error_report("CPU 'compat' property is deprecated and has no effect; use ma
- visit_type_null(v, name, errp);
+ visit_type_null(v, name, NULL);
}
static PropertyInfo ppc_compat_deprecated_propinfo = {
> Basic:
>
> 1) Boot guest with -cpu host
> Should go into POWER8 compat mode after CAS
> Previously would have been raw mode
>
== QEMU ==
spapr_cas_pvr current=0, explicit_match=1, new=f000004
== guest ==
cpu : POWER8 (architected), altivec supported
> 2) Boot guest with -machine pseries,max-cpu-compat=power7 -cpu host
> Should go into POWER7 compat mode
>
== QEMU ==
spapr_cas_pvr current=f000003, explicit_match=1, new=f000003
== guest ==
cpu : POWER7 (architected), altivec supported
> 3) Boot guest with -cpu host,compat=power7
> Should act as (2), but print a warning
>
== QEMU ==
We get the following warning as expected:
CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
We actually get one per vcpu. This may means a lot of lines if there are
many of them. But I can't think of any simple way to avoid that... the
user will have a motivation to update its command line :)
spapr_cas_pvr current=f000003, explicit_match=1, new=f000003
== guest ==
cpu : POWER7 (architected), altivec supported
> 4) Boot guest via libvirt with power7 compat mode specified in XML
> Should act as (3), (2) once we fix libvirt
>
Not tested.
> 5) Hack guest to only advertise power7 compatibility, boot with -cpu host
> Should go into POWER7 compat mode after CAS
>
== QEMU ==
spapr_cas_pvr current=0, explicit_match=1, new=f000003
== guest ==
cpu : POWER7 (architected), altivec supported
> 6) Hack guest to only advertise real PVRs
> Should remain in POWER8 raw mode after CAS
>
== QEMU ==
spapr_cas_pvr current=0, explicit_match=1, new=0
== guest ==
cpu : POWER8 (raw), altivec supported
> 7) Hack guest to only advertise real PVRs
> Boot with -machine pseries,max-cpu-compat=power8
> Should fail at CAS time
>
Calling ibm,client-architecture-support...
WARNING: ibm,client-architecture-support call FAILED!
But as with your RFCv2, the guest boots anyway in compat
mode, even if the guest didn't advertise it:
cpu : POWER8 (architected), altivec supported
We had discussed about two possible options at the time:
1) terminate the guest (LoPAPR section B.6.2.3)
or
2) just add an error_report()
You seemed to favor 2) at the time, but Sam Bobroff's recent work to
allow MMU mode selection via CAS (commit 9fb4541f5803) explicitly
has QEMU to "terminate the guest if it requests an unavailable mode,
as required by the architecture." (ie, QEMU exits).
Shouldn't we do the same when we asked for compat mode but the client
doesn't advertise any compat PVR ?
> 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
>
== QEMU ==
boot 1: spapr_cas_pvr current=0, explicit_match=1, new=f000003
boot 2: spapr_cas_pvr current=0, explicit_match=1, new=f000004
== guest ==
boot 1: cpu : POWER7 (architected), altivec supported
boot 2: cpu : POWER8 (architected), altivec supported
> Migration:
>
I'll work on testing migration ASAP.
Cheers,
--
Greg
> 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 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 (3):
> 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
>
> hw/ppc/spapr.c | 8 ++++-
> hw/ppc/spapr_cpu_core.c | 47 +++++++++++++++++++------
> hw/ppc/spapr_hcall.c | 2 +-
> include/hw/ppc/spapr.h | 12 ++++---
> qapi/string-input-visitor.c | 11 ++++++
> qapi/string-output-visitor.c | 14 ++++++++
> target/ppc/compat.c | 65 ++++++++++++++++++++++++++++++++++
> target/ppc/cpu.h | 6 ++--
> target/ppc/machine.c | 71 +++++++++++++++++++++++++++++++++++--
> target/ppc/translate_init.c | 84 ++++++++++++--------------------------------
> 10 files changed, 238 insertions(+), 82 deletions(-)
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
2017-04-27 17:23 ` Michael Roth
2017-05-02 14:24 ` Greg Kurz
@ 2017-05-04 10:06 ` Greg Kurz
2017-05-04 17:09 ` [Qemu-devel] " Andrea Bolognani
3 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-05-04 10:06 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, qemu-devel, armbru, qemu-ppc, abologna
[-- Attachment #1: Type: text/plain, Size: 17475 bytes --]
On Thu, 27 Apr 2017 17:28:41 +1000
David Gibson <david@gibson.dropbear.id.au> 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 new
> removed cpu property.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
Yet another comment about the impact on tests/qom-test, as already
mentioned during RFCv2.
> hw/ppc/spapr.c | 6 +++-
> hw/ppc/spapr_cpu_core.c | 41 ++++++++++++++++++++--
> hw/ppc/spapr_hcall.c | 2 +-
> include/hw/ppc/spapr.h | 12 ++++---
> target/ppc/compat.c | 65 +++++++++++++++++++++++++++++++++++
> target/ppc/cpu.h | 6 ++--
> target/ppc/translate_init.c | 84 +++++++++++++--------------------------------
> 7 files changed, 145 insertions(+), 71 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..547fa27 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2117,7 +2117,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);
>
> @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events when possible"
> " (required for memory hot-unplug support)",
> NULL);
> +
> + object_property_add(obj, "max-cpu-compat", "str",
> + ppc_compat_prop_get, ppc_compat_prop_set,
> + NULL, &spapr->max_compat_pvr, &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 4389ef4..ba610bc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,43 @@
> #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 n, i;
> + gchar *compat_str = NULL;
> +
> + inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> + n = g_strv_length(inpieces);
> +
> + /* inpieces[0] is the actual model string */
> + for (i = 0; i < n; i++) {
> + if (g_str_has_prefix(inpieces[i], "compat=")) {
> + compat_str = inpieces[i];
> + }
> + }
> +
> + if (compat_str) {
> + char *val = compat_str + strlen("compat=");
> + object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> + &error_fatal);
> + }
> +
> + 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 +107,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 9f18f75..d4dc12b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1059,7 +1059,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> target_ulong list = ppc64_phys_to_real(args[0]);
> target_ulong ov_table;
> 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;
> sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f88..40d5f89 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -86,16 +86,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;
> @@ -633,6 +636,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..476dead 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,18 +48,21 @@ 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,
> @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
>
> return n_threads;
> }
> +
> +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);
> +}
> +
> +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);
> +}
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e0ff041..e953e75 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1185,7 +1185,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.
> @@ -1197,7 +1196,6 @@ struct PowerPCCPU {
>
> CPUPPCState env;
> int cpu_dt_id;
> - uint32_t max_compat;
> uint32_t compat_pvr;
> PPCVirtualHypervisor *vhyp;
> Object *intc;
> @@ -1369,6 +1367,10 @@ 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_prop_get(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> +void ppc_compat_prop_set(Object *obj, Visitor *v,
> + const char *name, void *opaque, Error **err);
> #endif /* defined(TARGET_PPC64) */
>
> #include "exec/cpu-all.h"
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index e82e3e6..a92c825 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -8413,73 +8413,35 @@ 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);
> - }
> -
> - g_free(value);
> + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
This causes some extra verbosity in qom-test:
/ppc64/qom/ref405ep: OK
/ppc64/qom/none: OK
/ppc64/qom/virtex-ml507: OK
/ppc64/qom/powernv: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/ppce500: OK
/ppc64/qom/mpc8544ds: OK
/ppc64/qom/bamboo: OK
/ppc64/qom/g3beige: OK
/ppc64/qom/pseries-2.10: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/prep: OK
/ppc64/qom/pseries-2.9: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/mac99: OK
/ppc64/qom/pseries-2.6: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.7: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.8: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.4: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.5: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.2: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/taihu: OK
/ppc64/qom/pseries-2.3: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/pseries-2.1: CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead
OK
/ppc64/qom/40p: OK
This can be avoided with a trivial check:
if (!qtest_enabled()) {
error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
}
> + visit_type_null(v, name, errp);
> }
>
> -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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration David Gibson
2017-04-27 19:51 ` Michael Roth
@ 2017-05-04 10:07 ` Greg Kurz
2017-05-26 4:16 ` David Gibson
1 sibling, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-05-04 10:07 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 8007 bytes --]
On Thu, 27 Apr 2017 17:28:43 +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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 6cb3a48..20a46c9 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);
As already mentioned during the review of RFCv2, this calls
cpu_synchronize_state(CPU(cpu)) and trashes the registers.
The following changes avoid that:
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
return true;
}
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
+ Error **errp)
{
const CompatInfo *compat = compat_by_pvr(compat_pvr);
CPUPPCState *env = &cpu->env;
@@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
pcr = compat->pcr;
}
- cpu_synchronize_state(CPU(cpu));
+ if (sync_needed) {
+ cpu_synchronize_state(CPU(cpu));
+ }
cpu->compat_pvr = compat_pvr;
env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
@@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
PowerPCCPU *cpu = POWERPC_CPU(cs);
SetCompatState *s = arg.host_ptr;
- ppc_set_compat(cpu, s->compat_pvr, &s->err);
+ ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
}
void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1d8f2fcd4a46..057785347820 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
#if defined(TARGET_PPC64)
bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
uint32_t min_compat_pvr, uint32_t max_compat_pvr);
-void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
+ Error **errp);
#if !defined(CONFIG_USER_ONLY)
void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
#endif
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 20a46c95a596..fda63532b041 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
if (cpu->compat_pvr) {
Error *local_err = NULL;
- ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
+ ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
if (local_err) {
error_report_err(local_err);
error_free(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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> }
> };
>
> +static bool compat_needed(void *opaque)
> +{
> + PowerPCCPU *cpu = opaque;
> +
> + return cpu->vhyp != NULL;
> +}
> +
> +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 +677,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 --]
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
` (6 preceding siblings ...)
2017-05-03 18:03 ` Greg Kurz
@ 2017-05-04 14:32 ` Andrea Bolognani
2017-05-04 19:22 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
7 siblings, 1 reply; 30+ messages in thread
From: Andrea Bolognani @ 2017-05-04 14:32 UTC (permalink / raw)
To: David Gibson, groug, clg, aik, mdroth, nikunj
Cc: agraf, armbru, qemu-devel, qemu-ppc
On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> 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.
I started playing with this, mostly with libvirt integration
in mind of course, and quickly ran into a behavior that I
believe was not intentional and unfortunately managed to slip
into 2.9, I assume through the patches which were initially
part of this series (mentioned above).
More specifically, when I run a guest with
-M pseries-2.8 -cpu host
using QEMU 2.8, the CPU in the guest is reported as
POWER8E (raw), altivec supported
However, when using QEMU 2.9 with the very same command line,
including explicitly using the pseries-2.8 machine type, I
get
POWER8 (architected), altivec supported
instead. The same happens with current master + your patches
applied on top.
I'm not sure how much real trouble that will actually cause
for guests, but it's a guest-visible change as a result of
upgrading QEMU, which should just not happen.
I'll keep testing your series and get back to you as soon as
I have more feedback or questions.
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
` (2 preceding siblings ...)
2017-05-04 10:06 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2017-05-04 17:09 ` Andrea Bolognani
2017-05-04 18:50 ` Greg Kurz
2017-05-26 2:10 ` David Gibson
3 siblings, 2 replies; 30+ messages in thread
From: Andrea Bolognani @ 2017-05-04 17:09 UTC (permalink / raw)
To: David Gibson, groug, clg, aik, mdroth, nikunj
Cc: agraf, armbru, qemu-devel, qemu-ppc
On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events when possible"
> " (required for memory hot-unplug support)",
> NULL);
> +
> + object_property_add(obj, "max-cpu-compat", "str",
> + ppc_compat_prop_get, ppc_compat_prop_set,
> + NULL, &spapr->max_compat_pvr, &error_fatal);
I'm not familiar with QEMU's object system, but shouldn't
you be using object_property_add_str() instead? It looks
like you're doing more than the straightforward wrapper
would do, so maybe that's just not possible.
In any case, all other string properties look like
pseries-2.10.kvm-type=string
whereas this one ends up looking like
pseries-2.10.max-cpu-compat=str
which I think should be fixed - object_property_add_str()
passes "string" instead of "str" to object_property_add().
You should also add a sensible description for the property,
preferably spelling out all the accepted values.
Speaking of properties...
$ qemu-system-ppc64 -cpu host,compat=whatever
Segmentation fault
You might want to look into that ;)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-05-04 17:09 ` [Qemu-devel] " Andrea Bolognani
@ 2017-05-04 18:50 ` Greg Kurz
2017-05-12 7:08 ` David Gibson
2017-05-26 2:10 ` David Gibson
1 sibling, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-05-04 18:50 UTC (permalink / raw)
To: Andrea Bolognani
Cc: David Gibson, clg, aik, mdroth, nikunj, agraf, armbru,
qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
On Thu, 04 May 2017 19:09:11 +0200
Andrea Bolognani <abologna@redhat.com> wrote:
> On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> > " place of standard EPOW events when possible"
> > " (required for memory hot-unplug support)",
> > NULL);
> > +
> > + object_property_add(obj, "max-cpu-compat", "str",
> > + ppc_compat_prop_get, ppc_compat_prop_set,
> > + NULL, &spapr->max_compat_pvr, &error_fatal);
>
> I'm not familiar with QEMU's object system, but shouldn't
> you be using object_property_add_str() instead? It looks
> like you're doing more than the straightforward wrapper
> would do, so maybe that's just not possible.
>
>
> In any case, all other string properties look like
>
> pseries-2.10.kvm-type=string
>
> whereas this one ends up looking like
>
> pseries-2.10.max-cpu-compat=str
>
> which I think should be fixed - object_property_add_str()
> passes "string" instead of "str" to object_property_add().
>
> You should also add a sensible description for the property,
> preferably spelling out all the accepted values.
>
>
> Speaking of properties...
>
> $ qemu-system-ppc64 -cpu host,compat=whatever
> Segmentation fault
>
> You might want to look into that ;)
>
This happens because patch 2 is missing a change for the recently added POWER9:
.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,
> --
> Andrea Bolognani / Red Hat / Virtualization
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv3 0/4] Clean up compatibility mode handling
2017-05-04 14:32 ` Andrea Bolognani
@ 2017-05-04 19:22 ` Greg Kurz
2017-05-12 7:33 ` David Gibson
0 siblings, 1 reply; 30+ messages in thread
From: Greg Kurz @ 2017-05-04 19:22 UTC (permalink / raw)
To: Andrea Bolognani
Cc: David Gibson, clg, aik, mdroth, nikunj, qemu-devel, qemu-ppc, armbru
[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]
On Thu, 04 May 2017 16:32:59 +0200
Andrea Bolognani <abologna@redhat.com> wrote:
> On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > 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.
>
> I started playing with this, mostly with libvirt integration
> in mind of course, and quickly ran into a behavior that I
> believe was not intentional and unfortunately managed to slip
> into 2.9, I assume through the patches which were initially
> part of this series (mentioned above).
>
> More specifically, when I run a guest with
>
> -M pseries-2.8 -cpu host
>
> using QEMU 2.8, the CPU in the guest is reported as
>
> POWER8E (raw), altivec supported
>
> However, when using QEMU 2.9 with the very same command line,
> including explicitly using the pseries-2.8 machine type, I
> get
>
> POWER8 (architected), altivec supported
>
The goal of this series is indeed to switch from raw to architected but I
agree that it shouldn't affect existing machine types. This probably calls
for some compat prop.
> instead. The same happens with current master + your patches
> applied on top.
>
> I'm not sure how much real trouble that will actually cause
> for guests, but it's a guest-visible change as a result of
> upgrading QEMU, which should just not happen.
>
>
> I'll keep testing your series and get back to you as soon as
> I have more feedback or questions.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-05-04 18:50 ` Greg Kurz
@ 2017-05-12 7:08 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-05-12 7:08 UTC (permalink / raw)
To: Greg Kurz
Cc: Andrea Bolognani, clg, aik, mdroth, nikunj, agraf, armbru,
qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]
On Thu, May 04, 2017 at 08:50:47PM +0200, Greg Kurz wrote:
> On Thu, 04 May 2017 19:09:11 +0200
> Andrea Bolognani <abologna@redhat.com> wrote:
>
> > On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> > > " place of standard EPOW events when possible"
> > > " (required for memory hot-unplug support)",
> > > NULL);
> > > +
> > > + object_property_add(obj, "max-cpu-compat", "str",
> > > + ppc_compat_prop_get, ppc_compat_prop_set,
> > > + NULL, &spapr->max_compat_pvr, &error_fatal);
> >
> > I'm not familiar with QEMU's object system, but shouldn't
> > you be using object_property_add_str() instead? It looks
> > like you're doing more than the straightforward wrapper
> > would do, so maybe that's just not possible.
> >
> >
> > In any case, all other string properties look like
> >
> > pseries-2.10.kvm-type=string
> >
> > whereas this one ends up looking like
> >
> > pseries-2.10.max-cpu-compat=str
> >
> > which I think should be fixed - object_property_add_str()
> > passes "string" instead of "str" to object_property_add().
> >
> > You should also add a sensible description for the property,
> > preferably spelling out all the accepted values.
> >
> >
> > Speaking of properties...
> >
> > $ qemu-system-ppc64 -cpu host,compat=whatever
> > Segmentation fault
> >
> > You might want to look into that ;)
> >
>
> This happens because patch 2 is missing a change for the recently added POWER9:
>
> .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,
>
Right, I have that fixed in my tree so it will be in the next spin.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv3 0/4] Clean up compatibility mode handling
2017-05-04 19:22 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2017-05-12 7:33 ` David Gibson
2017-05-12 8:33 ` Andrea Bolognani
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-05-12 7:33 UTC (permalink / raw)
To: Greg Kurz
Cc: Andrea Bolognani, clg, aik, mdroth, nikunj, qemu-devel, qemu-ppc, armbru
[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]
On Thu, May 04, 2017 at 09:22:37PM +0200, Greg Kurz wrote:
> On Thu, 04 May 2017 16:32:59 +0200
> Andrea Bolognani <abologna@redhat.com> wrote:
>
> > On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > > 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.
> >
> > I started playing with this, mostly with libvirt integration
> > in mind of course, and quickly ran into a behavior that I
> > believe was not intentional and unfortunately managed to slip
> > into 2.9, I assume through the patches which were initially
> > part of this series (mentioned above).
> >
> > More specifically, when I run a guest with
> >
> > -M pseries-2.8 -cpu host
> >
> > using QEMU 2.8, the CPU in the guest is reported as
> >
> > POWER8E (raw), altivec supported
> >
> > However, when using QEMU 2.9 with the very same command line,
> > including explicitly using the pseries-2.8 machine type, I
> > get
> >
> > POWER8 (architected), altivec supported
> >
>
> The goal of this series is indeed to switch from raw to architected but I
> agree that it shouldn't affect existing machine types. This probably calls
> for some compat prop.
So, I have mixed feelings about this.
1. Yes, the series was intended to switch from preferring raw to
preferring architected mode.
2. No, it shouldn't have affected older machine types - I didn't think
that through.
But, having made the mistake, I'm not sure it's worth correcting. I
don't actually expect this to break guests, and fixing it now that
it's already there will be messy, and raises the possibility of new
behavioural change between older and newer subversions of 2.9.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCHv3 0/4] Clean up compatibility mode handling
2017-05-12 7:33 ` David Gibson
@ 2017-05-12 8:33 ` Andrea Bolognani
0 siblings, 0 replies; 30+ messages in thread
From: Andrea Bolognani @ 2017-05-12 8:33 UTC (permalink / raw)
To: David Gibson, Greg Kurz
Cc: clg, aik, mdroth, nikunj, qemu-devel, qemu-ppc, armbru
On Fri, 2017-05-12 at 17:33 +1000, David Gibson wrote:
> > The goal of this series is indeed to switch from raw to architected but I
> > agree that it shouldn't affect existing machine types. This probably calls
> > for some compat prop.
>
> So, I have mixed feelings about this.
>
> 1. Yes, the series was intended to switch from preferring raw to
> preferring architected mode.
>
> 2. No, it shouldn't have affected older machine types - I didn't think
> that through.
>
> But, having made the mistake, I'm not sure it's worth correcting. I
> don't actually expect this to break guests, and fixing it now that
> it's already there will be messy, and raises the possibility of new
> behavioural change between older and newer subversions of 2.9.
I'm not sure how QEMU handles maintenance branches, but it
seems to me that it would be better to confine this issue
to a single buggy release rather than carrying it forward
forever.
Downstream releases can just backport the relevant commits
and pretend nothing ever happened :)
--
Andrea Bolognani / Red Hat / Virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-05-02 14:24 ` Greg Kurz
@ 2017-05-26 1:24 ` David Gibson
0 siblings, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-05-26 1:24 UTC (permalink / raw)
To: Greg Kurz
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]
On Tue, May 02, 2017 at 04:24:55PM +0200, Greg Kurz wrote:
> On Thu, 27 Apr 2017 17:28:41 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > @@ -45,18 +48,21 @@ 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,
>
> And now we also have POWER9 in the list, so:
>
> .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,
Updated for the next spin.
> > @@ -189,3 +195,62 @@ int ppc_compat_max_threads(PowerPCCPU *cpu)
> >
> > return n_threads;
> > }
> > +
> > +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);
> > +}
> > +
> > +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;
>
> The current implementation in powerpc_get_compat() considers "" to be an
> invalid compatibility mode. Is there a reason to behave differently with
> max-cpu-compat ?
Hrm. Symmetry, really. In ppc_compat_prop_get() we represent no
compatibility mode set as an empty string. Setting the same value
back should have the corresponding effect.
[snip]
> > +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);
> > - }
> > -
> > - g_free(value);
> > + error_report("CPU 'compat' property is deprecated and has no effect; use max-cpu-compat machine property instead");
> > + visit_type_null(v, name, errp);
> > }
> >
>
> As suggested in another mail, maybe NULL should be passed instead of errp if
> we really want to implement the "has no effect". Otherwise, it has the effect
> of terminating QEMU when passing a compat prop that isn't "".
Good point. I'll change that.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine
2017-05-04 17:09 ` [Qemu-devel] " Andrea Bolognani
2017-05-04 18:50 ` Greg Kurz
@ 2017-05-26 2:10 ` David Gibson
1 sibling, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-05-26 2:10 UTC (permalink / raw)
To: Andrea Bolognani
Cc: groug, clg, aik, mdroth, nikunj, agraf, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]
On Thu, May 04, 2017 at 07:09:11PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-04-27 at 17:28 +1000, David Gibson wrote:
> > @@ -2480,6 +2480,10 @@ static void spapr_machine_initfn(Object *obj)
> > " place of standard EPOW events when possible"
> > " (required for memory hot-unplug support)",
> > NULL);
> > +
> > + object_property_add(obj, "max-cpu-compat", "str",
> > + ppc_compat_prop_get, ppc_compat_prop_set,
> > + NULL, &spapr->max_compat_pvr, &error_fatal);
>
> I'm not familiar with QEMU's object system, but shouldn't
> you be using object_property_add_str() instead? It looks
> like you're doing more than the straightforward wrapper
> would do, so maybe that's just not possible.
Right. I can't use object_property_add_str() for two reasons. First,
I need the opaque parameter which it lacks, second it assumes that
you're storing the property
opaque parameter (which the _str() variant lacks) to pass in the
destination variable for the compat pvr.
> In any case, all other string properties look like
>
> pseries-2.10.kvm-type=string
>
> whereas this one ends up looking like
>
> pseries-2.10.max-cpu-compat=str
>
> which I think should be fixed - object_property_add_str()
> passes "string" instead of "str" to object_property_add().
Ah, yes, that's a bug, I'll fix.
> You should also add a sensible description for the property,
> preferably spelling out all the accepted values.
I'll add that.
> Speaking of properties...
>
> $ qemu-system-ppc64 -cpu host,compat=whatever
> Segmentation fault
>
> You might want to look into that ;)
Uh.. yeah. I think I've fixed that.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-04-27 19:51 ` Michael Roth
2017-05-01 6:48 ` David Gibson
@ 2017-05-26 3:40 ` David Gibson
1 sibling, 0 replies; 30+ messages in thread
From: David Gibson @ 2017-05-26 3:40 UTC (permalink / raw)
To: Michael Roth
Cc: aik, clg, groug, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 6675 bytes --]
On Thu, Apr 27, 2017 at 02:51:31PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-04-27 02:28:43)
> > 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 68 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > }
> > };
> >
> > +static bool compat_needed(void *opaque)
> > +{
> > + PowerPCCPU *cpu = opaque;
> > +
> > + return cpu->vhyp != NULL;
> > +}
>
> Would it make sense to relax this to:
>
> cpu->vhyp != NULL && cpu->compat_pvr
So, that's equivalent to just cpu->compat_pvr, since it can't be
non-zero if cpu->vhyp isn't set.
> ? This would at least allow cross-version migration in cases where we
> weren't previously running in a compatibility mode. As it stands it
> seems like this would rule out both new->old and old->new migration.
Ah, yes that's a good idea.
> Another possibility might be to only enable migration/checking of
> compat_pvr for 2.10 and later, similar to the cpu_pre_2_8_migration
> stuff.
True.. and that might be a bit more robust, too. I'll look into it.
>
> > +
> > +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 +677,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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-05-04 10:07 ` Greg Kurz
@ 2017-05-26 4:16 ` David Gibson
2017-05-29 10:51 ` Greg Kurz
0 siblings, 1 reply; 30+ messages in thread
From: David Gibson @ 2017-05-26 4:16 UTC (permalink / raw)
To: Greg Kurz
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 8913 bytes --]
On Thu, May 04, 2017 at 12:07:47PM +0200, Greg Kurz wrote:
> On Thu, 27 Apr 2017 17:28:43 +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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 68 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 6cb3a48..20a46c9 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);
>
> As already mentioned during the review of RFCv2, this calls
> cpu_synchronize_state(CPU(cpu)) and trashes the registers.
>
> The following changes avoid that:
This is a really ugly fix, and I think it misses the point.
If a synchronize_state() trashes state here, it means we've already
altered register state while not synchronized, which is a pre-existing
bug.
>
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> return true;
> }
>
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> + Error **errp)
> {
> const CompatInfo *compat = compat_by_pvr(compat_pvr);
> CPUPPCState *env = &cpu->env;
> @@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> pcr = compat->pcr;
> }
>
> - cpu_synchronize_state(CPU(cpu));
> + if (sync_needed) {
> + cpu_synchronize_state(CPU(cpu));
> + }
>
> cpu->compat_pvr = compat_pvr;
> env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> @@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> SetCompatState *s = arg.host_ptr;
>
> - ppc_set_compat(cpu, s->compat_pvr, &s->err);
> + ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
> }
>
> void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 1d8f2fcd4a46..057785347820 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
> #if defined(TARGET_PPC64)
> bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> uint32_t min_compat_pvr, uint32_t max_compat_pvr);
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> + Error **errp);
> #if !defined(CONFIG_USER_ONLY)
> void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
> #endif
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 20a46c95a596..fda63532b041 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
> if (cpu->compat_pvr) {
> Error *local_err = NULL;
>
> - ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> + ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
> if (local_err) {
> error_report_err(local_err);
> error_free(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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > }
> > };
> >
> > +static bool compat_needed(void *opaque)
> > +{
> > + PowerPCCPU *cpu = opaque;
> > +
> > + return cpu->vhyp != NULL;
> > +}
> > +
> > +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 +677,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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration
2017-05-26 4:16 ` David Gibson
@ 2017-05-29 10:51 ` Greg Kurz
0 siblings, 0 replies; 30+ messages in thread
From: Greg Kurz @ 2017-05-29 10:51 UTC (permalink / raw)
To: David Gibson
Cc: clg, aik, mdroth, nikunj, agraf, abologna, armbru, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 9755 bytes --]
On Fri, 26 May 2017 14:16:30 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, May 04, 2017 at 12:07:47PM +0200, Greg Kurz wrote:
> > On Thu, 27 Apr 2017 17:28:43 +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 | 71 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 68 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > > index 6cb3a48..20a46c9 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);
> >
> > As already mentioned during the review of RFCv2, this calls
> > cpu_synchronize_state(CPU(cpu)) and trashes the registers.
> >
> > The following changes avoid that:
>
> This is a really ugly fix, and I think it misses the point.
>
> If a synchronize_state() trashes state here, it means we've already
> altered register state while not synchronized, which is a pre-existing
> bug.
>
This is exactly what happens when processing an incoming migration:
1) reset the cpu (clears the cpu dirty flags)
2) "alter register state" according to the migration stream
3) synchronize all registers with cpu_synchronize_all_post_init()
So I'm not sure where we have a pre-existing bug... or maybe document
that cpu_synchronize_state() shouldn't be called when processing
incoming migration (and why should it be since we synchronize all
registers at the end?).
> >
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -118,7 +118,8 @@ bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> > return true;
> > }
> >
> > -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> > +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> > + Error **errp)
> > {
> > const CompatInfo *compat = compat_by_pvr(compat_pvr);
> > CPUPPCState *env = &cpu->env;
> > @@ -138,7 +139,9 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> > pcr = compat->pcr;
> > }
> >
> > - cpu_synchronize_state(CPU(cpu));
> > + if (sync_needed) {
> > + cpu_synchronize_state(CPU(cpu));
> > + }
> >
> > cpu->compat_pvr = compat_pvr;
> > env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> > @@ -162,7 +165,7 @@ static void do_set_compat(CPUState *cs, run_on_cpu_data arg)
> > PowerPCCPU *cpu = POWERPC_CPU(cs);
> > SetCompatState *s = arg.host_ptr;
> >
> > - ppc_set_compat(cpu, s->compat_pvr, &s->err);
> > + ppc_set_compat(cpu, s->compat_pvr, true, &s->err);
> > }
> >
> > void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 1d8f2fcd4a46..057785347820 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1364,7 +1364,8 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
> > #if defined(TARGET_PPC64)
> > bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> > uint32_t min_compat_pvr, uint32_t max_compat_pvr);
> > -void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> > +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, bool sync_needed,
> > + Error **errp);
> > #if !defined(CONFIG_USER_ONLY)
> > void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
> > #endif
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 20a46c95a596..fda63532b041 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -239,7 +239,7 @@ static int cpu_post_load(void *opaque, int version_id)
> > if (cpu->compat_pvr) {
> > Error *local_err = NULL;
> >
> > - ppc_set_compat(cpu, cpu->compat_pvr, &local_err);
> > + ppc_set_compat(cpu, cpu->compat_pvr, false, &local_err);
> > if (local_err) {
> > error_report_err(local_err);
> > error_free(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,24 @@ static const VMStateDescription vmstate_tlbmas = {
> > > }
> > > };
> > >
> > > +static bool compat_needed(void *opaque)
> > > +{
> > > + PowerPCCPU *cpu = opaque;
> > > +
> > > + return cpu->vhyp != NULL;
> > > +}
> > > +
> > > +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 +677,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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-05-29 10:52 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-27 7:28 [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 1/4] qapi: add explicit null to string input and output visitors David Gibson
2017-05-02 11:48 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 2/4] pseries: Move CPU compatibility property to machine David Gibson
2017-04-27 17:23 ` Michael Roth
2017-05-01 2:33 ` David Gibson
2017-05-02 11:23 ` Greg Kurz
2017-05-02 14:24 ` Greg Kurz
2017-05-26 1:24 ` David Gibson
2017-05-04 10:06 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2017-05-04 17:09 ` [Qemu-devel] " Andrea Bolognani
2017-05-04 18:50 ` Greg Kurz
2017-05-12 7:08 ` David Gibson
2017-05-26 2:10 ` David Gibson
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 3/4] pseries: Reset CPU compatibility mode David Gibson
2017-04-27 18:08 ` Michael Roth
2017-04-27 7:28 ` [Qemu-devel] [PATCHv3 4/4] ppc: Rework CPU compatibility testing across migration David Gibson
2017-04-27 19:51 ` Michael Roth
2017-05-01 6:48 ` David Gibson
2017-05-26 3:40 ` David Gibson
2017-05-04 10:07 ` Greg Kurz
2017-05-26 4:16 ` David Gibson
2017-05-29 10:51 ` Greg Kurz
2017-04-27 8:04 ` [Qemu-devel] [PATCHv3 0/4] Clean up compatibility mode handling no-reply
2017-04-28 9:29 ` Greg Kurz
2017-05-03 18:03 ` Greg Kurz
2017-05-04 14:32 ` Andrea Bolognani
2017-05-04 19:22 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2017-05-12 7:33 ` David Gibson
2017-05-12 8:33 ` Andrea Bolognani
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.