| Message ID | 1465492263-28472-5-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote: [...] > -static void cpu_common_parse_features(CPUState *cpu, char *features, > +static void cpu_common_parse_features(const char *typename, char *features, > Error **errp) > { > char *featurestr; /* Single "key=value" string being parsed */ > char *val; > - Error *err = NULL; > + static bool cpu_globals_initialized; > + > + /* TODO: all callers of ->parse_features() need to be changed to > + * call it only once, so we can remove this check (or change it > + * to assert(!cpu_globals_initialized). > + * Current callers of ->parse_features() are: > + * - machvirt_init() > + * - cpu_generic_init() > + * - cpu_x86_create() > + */ > + if (cpu_globals_initialized) { > + return; > + } > + cpu_globals_initialized = true; > > featurestr = features ? strtok(features, ",") : NULL; > > while (featurestr) { > val = strchr(featurestr, '='); > if (val) { > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > *val = 0; > val++; > - object_property_parse(OBJECT(cpu), val, featurestr, &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > + prop->driver = typename; > + prop->property = g_strdup(featurestr); > + prop->value = g_strdup(val); > + qdev_prop_register_global(prop); This allows the user to trigger an assert: $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on qemu-system-x86_64: hw/core/qdev-properties.c:1087: qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed. Aborted (core dumped) but even if we fix the assert by setting prop->user_provided=true, we have a problem. Previous behavior was: $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on qemu-system-x86_64: Property '.INVALID' not found $ after this patch, and setting prop->user_provided=true, we have: $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored: Property '.INVALID' not found [QEMU keeps running] QEMU needs to refuse to run if an invalid property is specified on -cpu. It is an important mechanism to prevent VMs from running if the user is requesting for a unsupported feature that requires newer QEMU. Any suggestions on how to fix that? Maybe qdev_prop_set_globals() can collect errors in a list in DeviceState, and we can check for them in code that creates device objects (like cpu_generic_init(), qdev_device_add()), or in the beginning of device_set_realized().
----- Original Message ----- > From: "Eduardo Habkost" <ehabkost@redhat.com> > To: "Igor Mammedov" <imammedo@redhat.com> > Cc: qemu-devel@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, "mark cave-ayland" > <mark.cave-ayland@ilande.co.uk>, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, > "Markus Armbruster" <armbru@redhat.com> > Sent: Tuesday, June 14, 2016 9:47:18 PM > Subject: Handling errors caused by -global (was Re: [Qemu-devel] [PATCH v2 4/6] cpu: use CPUClass->parse_features() > as convertor to global properties) > > On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote: > [...] > > -static void cpu_common_parse_features(CPUState *cpu, char *features, > > +static void cpu_common_parse_features(const char *typename, char > > *features, > > Error **errp) > > { > > char *featurestr; /* Single "key=value" string being parsed */ > > char *val; > > - Error *err = NULL; > > + static bool cpu_globals_initialized; > > + > > + /* TODO: all callers of ->parse_features() need to be changed to > > + * call it only once, so we can remove this check (or change it > > + * to assert(!cpu_globals_initialized). > > + * Current callers of ->parse_features() are: > > + * - machvirt_init() > > + * - cpu_generic_init() > > + * - cpu_x86_create() > > + */ > > + if (cpu_globals_initialized) { > > + return; > > + } > > + cpu_globals_initialized = true; > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > while (featurestr) { > > val = strchr(featurestr, '='); > > if (val) { > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > *val = 0; > > val++; > > - object_property_parse(OBJECT(cpu), val, featurestr, &err); > > - if (err) { > > - error_propagate(errp, err); > > - return; > > - } > > + prop->driver = typename; > > + prop->property = g_strdup(featurestr); > > + prop->value = g_strdup(val); > > + qdev_prop_register_global(prop); > > This allows the user to trigger an assert: > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > qemu-system-x86_64: hw/core/qdev-properties.c:1087: > qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed. > Aborted (core dumped) > > but even if we fix the assert by setting > prop->user_provided=true, we have a problem. Previous behavior > was: > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > qemu-system-x86_64: Property '.INVALID' not found > $ > > after this patch, and setting prop->user_provided=true, we have: > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored: > Property '.INVALID' not found > [QEMU keeps running] > > QEMU needs to refuse to run if an invalid property is specified > on -cpu. It is an important mechanism to prevent VMs from running > if the user is requesting for a unsupported feature that requires > newer QEMU. > > Any suggestions on how to fix that? Replace user_provided with a Error* argument to qdev_prop_register_global. It can be &error_abort and NULL for the current users of the function, while -cpu can use &error_fatal. Paolo > Maybe qdev_prop_set_globals() can collect errors in a list in > DeviceState, and we can check for them in code that creates > device objects (like cpu_generic_init(), qdev_device_add()), or > in the beginning of device_set_realized(). > > -- > Eduardo >
On Tue, Jun 14, 2016 at 05:41:03PM -0400, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Eduardo Habkost" <ehabkost@redhat.com> > > To: "Igor Mammedov" <imammedo@redhat.com> > > Cc: qemu-devel@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, "mark cave-ayland" > > <mark.cave-ayland@ilande.co.uk>, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, > > "Markus Armbruster" <armbru@redhat.com> > > Sent: Tuesday, June 14, 2016 9:47:18 PM > > Subject: Handling errors caused by -global (was Re: [Qemu-devel] [PATCH v2 4/6] cpu: use CPUClass->parse_features() > > as convertor to global properties) > > > > On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote: > > [...] > > > -static void cpu_common_parse_features(CPUState *cpu, char *features, > > > +static void cpu_common_parse_features(const char *typename, char > > > *features, > > > Error **errp) > > > { > > > char *featurestr; /* Single "key=value" string being parsed */ > > > char *val; > > > - Error *err = NULL; > > > + static bool cpu_globals_initialized; > > > + > > > + /* TODO: all callers of ->parse_features() need to be changed to > > > + * call it only once, so we can remove this check (or change it > > > + * to assert(!cpu_globals_initialized). > > > + * Current callers of ->parse_features() are: > > > + * - machvirt_init() > > > + * - cpu_generic_init() > > > + * - cpu_x86_create() > > > + */ > > > + if (cpu_globals_initialized) { > > > + return; > > > + } > > > + cpu_globals_initialized = true; > > > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > > > while (featurestr) { > > > val = strchr(featurestr, '='); > > > if (val) { > > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > > *val = 0; > > > val++; > > > - object_property_parse(OBJECT(cpu), val, featurestr, &err); > > > - if (err) { > > > - error_propagate(errp, err); > > > - return; > > > - } > > > + prop->driver = typename; > > > + prop->property = g_strdup(featurestr); > > > + prop->value = g_strdup(val); > > > + qdev_prop_register_global(prop); > > > > This allows the user to trigger an assert: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: hw/core/qdev-properties.c:1087: > > qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed. > > Aborted (core dumped) > > > > but even if we fix the assert by setting > > prop->user_provided=true, we have a problem. Previous behavior > > was: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: Property '.INVALID' not found > > $ > > > > after this patch, and setting prop->user_provided=true, we have: > > > > $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on > > qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored: > > Property '.INVALID' not found > > [QEMU keeps running] > > > > QEMU needs to refuse to run if an invalid property is specified > > on -cpu. It is an important mechanism to prevent VMs from running > > if the user is requesting for a unsupported feature that requires > > newer QEMU. > > > > Any suggestions on how to fix that? > > Replace user_provided with a Error* argument to qdev_prop_register_global. > It can be &error_abort and NULL for the current users of the function, > while -cpu can use &error_fatal. Brilliant. This should work.
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 73113cf..3519c9f 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1198,6 +1198,7 @@ static void machvirt_init(MachineState *machine) for (n = 0; n < smp_cpus; n++) { ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]); + const char *typename = object_class_get_name(oc); CPUClass *cc = CPU_CLASS(oc); Object *cpuobj; Error *err = NULL; @@ -1207,10 +1208,10 @@ static void machvirt_init(MachineState *machine) error_report("Unable to find CPU definition"); exit(1); } - cpuobj = object_new(object_class_get_name(oc)); + /* convert -smp CPU options specified by the user into global props */ + cc->parse_features(typename, cpuopts, &err); + cpuobj = object_new(typename); - /* Handle any CPU options specified by the user */ - cc->parse_features(CPU(cpuobj), cpuopts, &err); g_free(cpuopts); if (err) { error_report_err(err); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 32f3af3..cacb100 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -134,7 +134,7 @@ typedef struct CPUClass { /*< public >*/ ObjectClass *(*class_by_name)(const char *cpu_model); - void (*parse_features)(CPUState *cpu, char *str, Error **errp); + void (*parse_features)(const char *typename, char *str, Error **errp); void (*reset)(CPUState *cpu); int reset_dump_flags; diff --git a/qom/cpu.c b/qom/cpu.c index 751e992..eee23db 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -28,6 +28,7 @@ #include "exec/log.h" #include "qemu/error-report.h" #include "sysemu/sysemu.h" +#include "hw/qdev-properties.h" bool cpu_exists(int64_t id) { @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id) CPUState *cpu_generic_init(const char *typename, const char *cpu_model) { char *str, *name, *featurestr; - CPUState *cpu; + CPUState *cpu = NULL; ObjectClass *oc; CPUClass *cc; Error *err = NULL; @@ -60,16 +61,18 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model) return NULL; } - cpu = CPU(object_new(object_class_get_name(oc))); - cc = CPU_GET_CLASS(cpu); - + cc = CPU_CLASS(oc); featurestr = strtok(NULL, ","); - cc->parse_features(cpu, featurestr, &err); + /* TODO: all callers of cpu_generic_init() need to be converted to + * call parse_features() only once, before calling cpu_generic_init(). + */ + cc->parse_features(object_class_get_name(oc), featurestr, &err); g_free(str); if (err != NULL) { goto out; } + cpu = CPU(object_new(object_class_get_name(oc))); object_property_set_bool(OBJECT(cpu), true, "realized", &err); out: @@ -282,25 +285,38 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model) return NULL; } -static void cpu_common_parse_features(CPUState *cpu, char *features, +static void cpu_common_parse_features(const char *typename, char *features, Error **errp) { char *featurestr; /* Single "key=value" string being parsed */ char *val; - Error *err = NULL; + static bool cpu_globals_initialized; + + /* TODO: all callers of ->parse_features() need to be changed to + * call it only once, so we can remove this check (or change it + * to assert(!cpu_globals_initialized). + * Current callers of ->parse_features() are: + * - machvirt_init() + * - cpu_generic_init() + * - cpu_x86_create() + */ + if (cpu_globals_initialized) { + return; + } + cpu_globals_initialized = true; featurestr = features ? strtok(features, ",") : NULL; while (featurestr) { val = strchr(featurestr, '='); if (val) { + GlobalProperty *prop = g_new0(typeof(*prop), 1); *val = 0; val++; - object_property_parse(OBJECT(cpu), val, featurestr, &err); - if (err) { - error_propagate(errp, err); - return; - } + prop->driver = typename; + prop->property = g_strdup(featurestr); + prop->value = g_strdup(val); + qdev_prop_register_global(prop); } else { error_setg(errp, "Expected key=value format, found %s.", featurestr); diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3f886a5..279f656 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1958,12 +1958,17 @@ static FeatureWordArray minus_features = { 0 }; /* Parse "+feature,-feature,feature=foo" CPU feature string */ -static void x86_cpu_parse_featurestr(CPUState *cs, char *features, +static void x86_cpu_parse_featurestr(const char *typename, char *features, Error **errp) { - X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ Error *local_err = NULL; + static bool cpu_globals_initialized; + + if (cpu_globals_initialized) { + return; + } + cpu_globals_initialized = true; if (!features) { return; @@ -1975,6 +1980,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, const char *name; const char *val = NULL; char *eq = NULL; + GlobalProperty *prop; /* Compatibility syntax: */ if (featurestr[0] == '+') { @@ -2019,7 +2025,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, name = "tsc-frequency"; } - object_property_parse(OBJECT(cpu), val, name, &local_err); + prop = g_new0(typeof(*prop), 1); + prop->driver = typename; + prop->property = g_strdup(name); + prop->value = g_strdup(val); + qdev_prop_register_global(prop); } if (local_err) { @@ -2208,9 +2218,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) { X86CPU *cpu = NULL; ObjectClass *oc; + CPUClass *cc; gchar **model_pieces; char *name, *features; Error *error = NULL; + const char *typename; model_pieces = g_strsplit(cpu_model, ",", 2); if (!model_pieces[0]) { @@ -2225,10 +2237,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) error_setg(&error, "Unable to find CPU definition: %s", name); goto out; } + cc = CPU_CLASS(oc); + typename = object_class_get_name(oc); - cpu = X86_CPU(object_new(object_class_get_name(oc))); - - x86_cpu_parse_featurestr(CPU(cpu), features, &error); + cc->parse_features(typename, features, &error); + cpu = X86_CPU(object_new(typename)); if (error) { goto out; }
Currently CPUClass->parse_features() is used to parse -cpu features string and set properties on created CPU instances. But considering that features specified by -cpu apply to every created CPU instance, it doesn't make sence to parse the same features string for every CPU created. It also makes every target that cares about parsing features string explicitly call CPUClass->parse_features() parser, which gets in a way if we consider using generic device_add for CPU hotplug as device_add has not a clue about CPU specific hooks. Turns out we can use global properties mechanism to set properties on every created CPU instance for a given type. That way it's possible to convert CPU features into a set of global properties for CPU type specified by -cpu cpu_model and common Device.device_post_init() will apply them to CPU of given type automatically regardless whether it's manually created CPU or CPU created with help of device_add. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- This patch only make CPUClass->parse_features() a global properties convertor and follow up patches will switch individual users to new behaviour v2: - add todo comments in cpu_generic_init/cpu_common_parse_features - set cpu_globals_initialized = true early --- hw/arm/virt.c | 7 ++++--- include/qom/cpu.h | 2 +- qom/cpu.c | 40 ++++++++++++++++++++++++++++------------ target-i386/cpu.c | 25 +++++++++++++++++++------ 4 files changed, 52 insertions(+), 22 deletions(-)