| Message ID | 20191001125845.8793-2-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm/kvm: enable SVE in guests | expand |
On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > Add support for the query-cpu-model-expansion QMP command to Arm. We > do this selectively, only exposing CPU properties which represent > optional CPU features which the user may want to enable/disable. > Additionally we restrict the list of queryable cpu models to 'max', > 'host', or the current type when KVM is in use. And, finally, we only > implement expansion type 'full', as Arm does not yet have a "base" > CPU type. More details and example queries are described in a new > document (docs/arm-cpu-features.rst). > > Note, certainly more features may be added to the list of advertised > features, e.g. 'vfp' and 'neon'. The only requirement is that we can > detect invalid configurations and emit failures at QMP query time. > For 'vfp' and 'neon' this will require some refactoring to share a > validation function between the QMP query and the CPU realize > functions. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > --- > docs/arm-cpu-features.rst | 137 +++++++++++++++++++++++++++++++++++ > qapi/machine-target.json | 6 +- > target/arm/monitor.c | 145 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 285 insertions(+), 3 deletions(-) > create mode 100644 docs/arm-cpu-features.rst > > diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst > new file mode 100644 > index 000000000000..c79dcffb5556 > --- /dev/null > +++ b/docs/arm-cpu-features.rst > @@ -0,0 +1,137 @@ > +================ > +ARM CPU Features > +================ > + > +Examples of probing and using ARM CPU features > + > +Introduction > +============ > + > +CPU features are optional features that a CPU of supporting type may > +choose to implement or not. In QEMU, optional CPU features have > +corresponding boolean CPU proprieties that, when enabled, indicate > +that the feature is implemented, and, conversely, when disabled, > +indicate that it is not implemented. An example of an ARM CPU feature > +is the Performance Monitoring Unit (PMU). CPU types such as the > +Cortex-A15 and the Cortex-A57, which respectively implement ARM > +architecture reference manuals ARMv7-A and ARMv8-A, may both optionally > +implement PMUs. For example, if a user wants to use a Cortex-A15 without > +a PMU, then the `-cpu` parameter should contain `pmu=off` on the QEMU > +command line, i.e. `-cpu cortex-a15,pmu=off`. > + > +As not all CPU types support all optional CPU features, then whether or > +not a CPU property exists depends on the CPU type. For example, CPUs > +that implement the ARMv8-A architecture reference manual may optionally > +support the AArch32 CPU feature, which may be enabled by disabling the > +`aarch64` CPU property. A CPU type such as the Cortex-A15, which does > +not implement ARMv8-A, will not have the `aarch64` CPU property. > + > +QEMU's support may be limited for some CPU features, only partially > +supporting the feature or only supporting the feature under certain > +configurations. For example, the `aarch64` CPU feature, which, when > +disabled, enables the optional AArch32 CPU feature, is only supported > +when using the KVM accelerator and when running on a host CPU type that > +supports the feature. > + > +CPU Feature Probing > +=================== > + > +Determining which CPU features are available and functional for a given > +CPU type is possible with the `query-cpu-model-expansion` QMP command. > +Below are some examples where `scripts/qmp/qmp-shell` (see the top comment > +block in the script for usage) is used to issue the QMP commands. > + > +(1) Determine which CPU features are available for the `max` CPU type > + (Note, we started QEMU with qemu-system-aarch64, so `max` is > + implementing the ARMv8-A reference manual in this case):: > + > + (QEMU) query-cpu-model-expansion type=full model={"name":"max"} > + { "return": { > + "model": { "name": "max", "props": { > + "pmu": true, "aarch64": true > + }}}} > + > +We see that the `max` CPU type has the `pmu` and `aarch64` CPU features. > +We also see that the CPU features are enabled, as they are all `true`. > + > +(2) Let's try to disable the PMU:: > + > + (QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"pmu":false}} > + { "return": { > + "model": { "name": "max", "props": { > + "pmu": false, "aarch64": true > + }}}} > + > +We see it worked, as `pmu` is now `false`. > + > +(3) Let's try to disable `aarch64`, which enables the AArch32 CPU feature:: > + > + (QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"aarch64":false}} > + {"error": { > + "class": "GenericError", "desc": > + "'aarch64' feature cannot be disabled unless KVM is enabled and 32-bit EL1 is supported" > + }} > + > +It looks like this feature is limited to a configuration we do not > +currently have. > + > +(4) Let's try probing CPU features for the Cortex-A15 CPU type:: > + > + (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"} > + {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}} > + > +Only the `pmu` CPU feature is available. > + > +A note about CPU feature dependencies > +------------------------------------- > + > +It's possible for features to have dependencies on other features. I.e. > +it may be possible to change one feature at a time without error, but > +when attempting to change all features at once an error could occur > +depending on the order they are processed. It's also possible changing > +all at once doesn't generate an error, because a feature's dependencies > +are satisfied with other features, but the same feature cannot be changed > +independently without error. For these reasons callers should always > +attempt to make their desired changes all at once in order to ensure the > +collection is valid. > + > +A note about CPU models and KVM > +------------------------------- > + > +Named CPU models generally do not work with KVM. There are a few cases > +that do work, e.g. using the named CPU model `cortex-a57` with KVM on a > +seattle host, but mostly if KVM is enabled the `host` CPU type must be > +used. This means the guest is provided all the same CPU features as the > +host CPU type has. And, for this reason, the `host` CPU type should > +enable all CPU features that the host has by default. Indeed it's even > +a bit strange to allow disabling CPU features that the host has when using > +the `host` CPU type, but in the absence of CPU models it's the best we can > +do if we want to launch guests without all the host's CPU features enabled. > + > +Enabling KVM also affects the `query-cpu-model-expansion` QMP command. The > +affect is not only limited to specific features, as pointed out in example > +(3) of "CPU Feature Probing", but also to which CPU types may be expanded. > +When KVM is enabled, only the `max`, `host`, and current CPU type may be > +expanded. This restriction is necessary as it's not possible to know all > +CPU types that may work with KVM, but it does impose a small risk of users > +experiencing unexpected errors. For example on a seattle, as mentioned > +above, the `cortex-a57` CPU type is also valid when KVM is enabled. > +Therefore a user could use the `host` CPU type for the current type, but > +then attempt to query `cortex-a57`, however that query will fail with our > +restrictions. This shouldn't be an issue though as management layers and > +users have been preferring the `host` CPU type for use with KVM for quite > +some time. Additionally, if the KVM-enabled QEMU instance running on a > +seattle host is using the `cortex-a57` CPU type, then querying `cortex-a57` > +will work. > + > +Using CPU Features > +================== > + > +After determining which CPU features are available and supported for a > +given CPU type, then they may be selectively enabled or disabled on the > +QEMU command line with that CPU type:: > + > + $ qemu-system-aarch64 -M virt -cpu max,pmu=off > + > +The example above disables the PMU for the `max` CPU type. > + > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index 55310a6aa226..04623224720d 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -212,7 +212,7 @@ > ## > { 'struct': 'CpuModelExpansionInfo', > 'data': { 'model': 'CpuModelInfo' }, > - 'if': 'defined(TARGET_S390X) || defined(TARGET_I386)' } > + 'if': 'defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM)' } > > ## > # @query-cpu-model-expansion: > @@ -237,7 +237,7 @@ > # query-cpu-model-expansion while using these is not advised. > # > # Some architectures may not support all expansion types. s390x supports > -# "full" and "static". > +# "full" and "static". Arm only supports "full". > # > # Returns: a CpuModelExpansionInfo. Returns an error if expanding CPU models is > # not supported, if the model cannot be expanded, if the model contains > @@ -251,7 +251,7 @@ > 'data': { 'type': 'CpuModelExpansionType', > 'model': 'CpuModelInfo' }, > 'returns': 'CpuModelExpansionInfo', > - 'if': 'defined(TARGET_S390X) || defined(TARGET_I386)' } > + 'if': 'defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM)' } > > ## > # @CpuDefinitionInfo: > diff --git a/target/arm/monitor.c b/target/arm/monitor.c > index 6457c3c87f7c..edca8aa885f0 100644 > --- a/target/arm/monitor.c > +++ b/target/arm/monitor.c > @@ -21,8 +21,16 @@ > */ > > #include "qemu/osdep.h" > +#include "hw/boards.h" > #include "kvm_arm.h" > +#include "qapi/error.h" > +#include "qapi/visitor.h" > +#include "qapi/qobject-input-visitor.h" > +#include "qapi/qapi-commands-machine-target.h" > #include "qapi/qapi-commands-misc-target.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/qmp/qdict.h" > +#include "qom/qom-qobject.h" > > static GICCapability *gic_cap_new(int version) > { > @@ -81,3 +89,140 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > > return head; > } > + > +/* > + * These are cpu model features we want to advertise. The order here > + * matters as this is the order in which qmp_query_cpu_model_expansion > + * will attempt to set them. If there are dependencies between features, > + * then the order that considers those dependencies must be used. > + */ > +static const char *cpu_model_advertised_features[] = { > + "aarch64", "pmu", > + NULL > +}; > + > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > + CpuModelInfo *model, > + Error **errp) > +{ > + CpuModelExpansionInfo *expansion_info; > + const QDict *qdict_in = NULL; > + QDict *qdict_out; > + ObjectClass *oc; > + Object *obj; > + const char *name; > + int i; > + > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > + error_setg(errp, "The requested expansion type is not supported"); > + return NULL; > + } > + > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > + return NULL; > + } > + > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > + if (!oc) { > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > + model->name); > + return NULL; > + } > + > + if (kvm_enabled()) { > + const char *cpu_type = current_machine->cpu_type; > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > + bool supported = false; > + > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > + /* These are kvmarm's recommended cpu types */ > + supported = true; > + } else if (strlen(model->name) == len && > + !strncmp(model->name, cpu_type, len)) { > + /* KVM is enabled and we're using this type, so it works. */ > + supported = true; > + } > + if (!supported) { > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > + "with KVM on this host", model->name); > + return NULL; > + } > + } > + > + if (model->props) { > + qdict_in = qobject_to(QDict, model->props); > + if (!qdict_in) { > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > + return NULL; > + } > + } > + > + obj = object_new(object_class_get_name(oc)); > + > + if (qdict_in) { > + Visitor *visitor; > + Error *err = NULL; > + > + visitor = qobject_input_visitor_new(model->props); > + visit_start_struct(visitor, NULL, NULL, 0, &err); > + if (err) { > + object_unref(obj); Shouldn't we free the 'visitor' here as well ? > + error_propagate(errp, err); > + return NULL; > + } > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + if (qdict_get(qdict_in, name)) { > + object_property_set(obj, visitor, name, &err); > + if (err) { > + break; > + } > + } > + } > + > + if (!err) { > + visit_check_struct(visitor, &err); > + } > + visit_end_struct(visitor, NULL); > + visit_free(visitor); > + if (err) { > + object_unref(obj); > + error_propagate(errp, err); > + return NULL; > + } > + } > + > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > + expansion_info->model->name = g_strdup(model->name); > + > + qdict_out = qdict_new(); > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + ObjectProperty *prop = object_property_find(obj, name, NULL); > + if (prop) { > + Error *err = NULL; > + QObject *value; > + > + assert(prop->get); > + value = object_property_get_qobject(obj, name, &err); > + assert(!err); > + > + qdict_put_obj(qdict_out, name, value); > + } > + } > + > + if (!qdict_size(qdict_out)) { > + qobject_unref(qdict_out); > + } else { > + expansion_info->model->props = QOBJECT(qdict_out); > + expansion_info->model->has_props = true; > + } > + > + object_unref(obj); > + > + return expansion_info; > +} > -- > 2.20.1 > >
On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > + > > + obj = object_new(object_class_get_name(oc)); > > + > > + if (qdict_in) { > > + Visitor *visitor; > > + Error *err = NULL; > > + > > + visitor = qobject_input_visitor_new(model->props); > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > + if (err) { > > + object_unref(obj); > > Shouldn't we free the 'visitor' here as well ? Yes. Good catch. So we also need to fix target/s390x/cpu_models.c:cpu_model_from_info(), which has the same construction (the construction from which I derived this) > > > + error_propagate(errp, err); > > + return NULL; > > + } > > + What about the rest of the patch? With that fixed for v6 can I add your r-b? Thanks, drew
On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > + > > > + obj = object_new(object_class_get_name(oc)); > > > + > > > + if (qdict_in) { > > > + Visitor *visitor; > > > + Error *err = NULL; > > > + > > > + visitor = qobject_input_visitor_new(model->props); > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > + if (err) { > > > + object_unref(obj); > > > > Shouldn't we free the 'visitor' here as well ? > > Yes. Good catch. So we also need to fix > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > construction (the construction from which I derived this) > > > > > > + error_propagate(errp, err); > > > + return NULL; > > > + } > > > + > > What about the rest of the patch? With that fixed for v6 can I > add your r-b? > I still got this feeling that we could optimize that a bit - which I'm currently on, so hopefully I'll be able to add more comments soon if that proves to be the case. BR Beata > Thanks, > drew
On Tue, 15 Oct 2019 at 12:56, Beata Michalska <beata.michalska@linaro.org> wrote: > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > > + > > > > + obj = object_new(object_class_get_name(oc)); > > > > + > > > > + if (qdict_in) { > > > > + Visitor *visitor; > > > > + Error *err = NULL; > > > > + > > > > + visitor = qobject_input_visitor_new(model->props); > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > + if (err) { > > > > + object_unref(obj); > > > > > > Shouldn't we free the 'visitor' here as well ? > > > > Yes. Good catch. So we also need to fix > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > > construction (the construction from which I derived this) > > > > > > > > > + error_propagate(errp, err); > > > > + return NULL; > > > > + } > > > > + > > > > What about the rest of the patch? With that fixed for v6 can I > > add your r-b? > > > > I still got this feeling that we could optimize that a bit - which I'm > currently on, so hopefully I'll be able to add more comments soon if > that proves to be the case. > > BR > Beata I think there are few options that might be considered though the gain is not huge .. but it's always smth: > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > + CpuModelInfo *model, > + Error **errp) > +{ > + CpuModelExpansionInfo *expansion_info; > + const QDict *qdict_in = NULL; > + QDict *qdict_out; > + ObjectClass *oc; > + Object *obj; > + const char *name; > + int i; > + > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > + error_setg(errp, "The requested expansion type is not supported"); > + return NULL; > + } > + > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > + return NULL; > + } > + > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > + if (!oc) { > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > + model->name); > + return NULL; > + } > + > + if (kvm_enabled()) { > + const char *cpu_type = current_machine->cpu_type; > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > + bool supported = false; > + > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > + /* These are kvmarm's recommended cpu types */ > + supported = true; > + } else if (strlen(model->name) == len && > + !strncmp(model->name, cpu_type, len)) { > + /* KVM is enabled and we're using this type, so it works. */ > + supported = true; > + } > + if (!supported) { > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > + "with KVM on this host", model->name); > + return NULL; > + } > + } > + The above section can be slightly reduced and rearranged - preferably moved to a separate function -> get_cpu_model (...) ? * You can check the 'host' model first and then validate the accelerator -> if ( !strcmp(model->name, "host") if (!kvm_enabled()) log_error & leave else goto cpu_class_by_name /*cpu_class_by_name moved after the final model check @see below */ * the kvm_enabled section can be than slightly improved (dropping the second compare against 'host') if (kvm_enabled() && strcmp(model->name, "max") { /*Validate the current_machine->cpu_type against the model->name and report error case mismatch /* otherwise just fall through */ } * cpu_class_by_name moved here ... > + if (model->props) { MInor: the CPUModelInfo seems to have dedicated field for that verification -> has_props > + qdict_in = qobject_to(QDict, model->props); > + if (!qdict_in) { > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > + return NULL; > + } > + } > + > + obj = object_new(object_class_get_name(oc)); > + > + if (qdict_in) { > + Visitor *visitor; > + Error *err = NULL; > + > + visitor = qobject_input_visitor_new(model->props); > + visit_start_struct(visitor, NULL, NULL, 0, &err); > + if (err) { > + object_unref(obj); > + error_propagate(errp, err); > + return NULL; > + } > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + if (qdict_get(qdict_in, name)) { > + object_property_set(obj, visitor, name, &err); > + if (err) { > + break; > + } > + } > + } > + > + if (!err) { > + visit_check_struct(visitor, &err); > + } > + visit_end_struct(visitor, NULL); > + visit_free(visitor); > + if (err) { > + object_unref(obj); > + error_propagate(errp, err); > + return NULL; > + } > + } The both >> if (err) << blocks could be extracted and moved at the end of the function to mark a 'cleanup section' and both here and few lines before (with the visit_start_struct failure) could use goto. Easier to maintain and to make sure we make the proper cleanup in any case. > + > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > + expansion_info->model->name = g_strdup(model->name); > + > + qdict_out = qdict_new(); > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + ObjectProperty *prop = object_property_find(obj, name, NULL); > + if (prop) { > + Error *err = NULL; > + QObject *value; > + > + assert(prop->get); > + value = object_property_get_qobject(obj, name, &err); > + assert(!err); > + > + qdict_put_obj(qdict_out, name, value); > + } > + } > + This could be merged with the first iteration over the features, smth like: while () { if ((value = qdict_get(qdict_in, name))) { object_property_set ... if (!err) qobject_ref(value) /* we have the weak reference */ else break; } else { value = object_property_get_qobject() } if (value) qdict_put_object(qdict_out, name, value) } This way you iterate over the table once and you do not query for the same property twice by getting the value from the qdict_in. If the value is not acceptable we will fail either way so should be all good. > + if (!qdict_size(qdict_out)) { > + qobject_unref(qdict_out); > + } else { > + expansion_info->model->props = QOBJECT(qdict_out); > + expansion_info->model->has_props = true; > + } > + > + object_unref(obj); > + > + return expansion_info; Mentioned earlier cleanup section: cleanup: object_unref(obj); qobject_unref(qdict_out) ; /* if single loop is used */ error_propagate(errp,err); return NULL; > +} > -- > 2.20.1 > Hope I haven't missed anything. What do you think ? BR Beata > > Thanks, > > drew
On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote: > On Tue, 15 Oct 2019 at 12:56, Beata Michalska > <beata.michalska@linaro.org> wrote: > > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > > > + > > > > > + obj = object_new(object_class_get_name(oc)); > > > > > + > > > > > + if (qdict_in) { > > > > > + Visitor *visitor; > > > > > + Error *err = NULL; > > > > > + > > > > > + visitor = qobject_input_visitor_new(model->props); > > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > > + if (err) { > > > > > + object_unref(obj); > > > > > > > > Shouldn't we free the 'visitor' here as well ? > > > > > > Yes. Good catch. So we also need to fix > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > > > construction (the construction from which I derived this) > > > > > > > > > > > > + error_propagate(errp, err); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > > What about the rest of the patch? With that fixed for v6 can I > > > add your r-b? > > > > > > > I still got this feeling that we could optimize that a bit - which I'm > > currently on, so hopefully I'll be able to add more comments soon if > > that proves to be the case. > > > > BR > > Beata > > I think there are few options that might be considered though the gain > is not huge .. but it's always smth: > > > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > + CpuModelInfo *model, > > + Error **errp) > > +{ > > + CpuModelExpansionInfo *expansion_info; > > + const QDict *qdict_in = NULL; > > + QDict *qdict_out; > > + ObjectClass *oc; > > + Object *obj; > > + const char *name; > > + int i; > > + > > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > > + error_setg(errp, "The requested expansion type is not supported"); > > + return NULL; > > + } > > + > > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > + return NULL; > > + } > > + > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > + if (!oc) { > > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > > + model->name); > > + return NULL; > > + } > > + > > + if (kvm_enabled()) { > > + const char *cpu_type = current_machine->cpu_type; > > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > > + bool supported = false; > > + > > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > > + /* These are kvmarm's recommended cpu types */ > > + supported = true; > > + } else if (strlen(model->name) == len && > > + !strncmp(model->name, cpu_type, len)) { > > + /* KVM is enabled and we're using this type, so it works. */ > > + supported = true; > > + } > > + if (!supported) { > > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > + "with KVM on this host", model->name); > > + return NULL; > > + } > > + } > > + > > The above section can be slightly reduced and rearranged - preferably > moved to a separate function > -> get_cpu_model (...) ? > > * You can check the 'host' model first and then validate the accelerator -> > if ( !strcmp(model->name, "host") > if (!kvm_enabled()) > log_error & leave > else > goto cpu_class_by_name /*cpu_class_by_name moved after the > final model check @see below */ > > * the kvm_enabled section can be than slightly improved (dropping the > second compare against 'host') > > if (kvm_enabled() && strcmp(model->name, "max") { > /*Validate the current_machine->cpu_type against the > model->name and report error case mismatch > /* otherwise just fall through */ > } > * cpu_class_by_name moved here ... > > + if (model->props) { > MInor: the CPUModelInfo seems to have dedicated field for that > verification -> has_props > > > + qdict_in = qobject_to(QDict, model->props); > > + if (!qdict_in) { > > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > > + return NULL; > > + } > > + } > > + > > + obj = object_new(object_class_get_name(oc)); > > + > > + if (qdict_in) { > > + Visitor *visitor; > > + Error *err = NULL; > > + > > + visitor = qobject_input_visitor_new(model->props); > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > + if (err) { > > + object_unref(obj); > > + error_propagate(errp, err); > > + return NULL; > > + } > > + > > + i = 0; > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > + if (qdict_get(qdict_in, name)) { > > + object_property_set(obj, visitor, name, &err); > > + if (err) { > > + break; > > + } > > + } > > + } > > + > > + if (!err) { > > + visit_check_struct(visitor, &err); > > + } > > + visit_end_struct(visitor, NULL); > > + visit_free(visitor); > > + if (err) { > > + object_unref(obj); > > + error_propagate(errp, err); > > + return NULL; > > + } > > + } > > The both >> if (err) << blocks could be extracted and moved at the end > of the function > to mark a 'cleanup section' and both here and few lines before > (with the visit_start_struct failure) could use goto. > Easier to maintain and to make sure we make the proper cleanup in any case. > > > + > > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > > + expansion_info->model->name = g_strdup(model->name); > > + > > + qdict_out = qdict_new(); > > + > > + i = 0; > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > + ObjectProperty *prop = object_property_find(obj, name, NULL); > > + if (prop) { > > + Error *err = NULL; > > + QObject *value; > > + > > + assert(prop->get); > > + value = object_property_get_qobject(obj, name, &err); > > + assert(!err); > > + > > + qdict_put_obj(qdict_out, name, value); > > + } > > + } > > + > > This could be merged with the first iteration over the features, > smth like: > > while () { > if ((value = qdict_get(qdict_in, name))) { > object_property_set ... > if (!err) > qobject_ref(value) /* we have the weak reference */ > else > break; > } else { > value = object_property_get_qobject() > } > if (value) qdict_put_object(qdict_out, name, value) > } > > This way you iterate over the table once and you do not query > for the same property twice by getting the value from the qdict_in. > If the value is not acceptable we will fail either way so should be all good. > > > > + if (!qdict_size(qdict_out)) { > > + qobject_unref(qdict_out); > > + } else { > > + expansion_info->model->props = QOBJECT(qdict_out); > > + expansion_info->model->has_props = true; > > + } > > + > > + object_unref(obj); > > + > > + return expansion_info; > > Mentioned earlier cleanup section: > cleanup: > object_unref(obj); > qobject_unref(qdict_out) ; /* if single loop is used */ > error_propagate(errp,err); > return NULL; > > > +} > > -- > > 2.20.1 > > > > Hope I haven't missed anything. > What do you think ? > I think you need to post an entire function that incorporates all the proposed changes, or at least a diff that I can apply in order to get the entirely changed function. I also think that it's fine the way it is, so it would take a justification stronger than a potential micro optimization to get me to change it. Thanks, drew
On Wed, 16 Oct 2019 at 14:50, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote: > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska > > <beata.michalska@linaro.org> wrote: > > > > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > > > > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > > > > + > > > > > > + obj = object_new(object_class_get_name(oc)); > > > > > > + > > > > > > + if (qdict_in) { > > > > > > + Visitor *visitor; > > > > > > + Error *err = NULL; > > > > > > + > > > > > > + visitor = qobject_input_visitor_new(model->props); > > > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > > > + if (err) { > > > > > > + object_unref(obj); > > > > > > > > > > Shouldn't we free the 'visitor' here as well ? > > > > > > > > Yes. Good catch. So we also need to fix > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > > > > construction (the construction from which I derived this) > > > > > > > > > > > > > > > + error_propagate(errp, err); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > > > What about the rest of the patch? With that fixed for v6 can I > > > > add your r-b? > > > > > > > > > > I still got this feeling that we could optimize that a bit - which I'm > > > currently on, so hopefully I'll be able to add more comments soon if > > > that proves to be the case. > > > > > > BR > > > Beata > > > > I think there are few options that might be considered though the gain > > is not huge .. but it's always smth: > > > > > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > > + CpuModelInfo *model, > > > + Error **errp) > > > +{ > > > + CpuModelExpansionInfo *expansion_info; > > > + const QDict *qdict_in = NULL; > > > + QDict *qdict_out; > > > + ObjectClass *oc; > > > + Object *obj; > > > + const char *name; > > > + int i; > > > + > > > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > > > + error_setg(errp, "The requested expansion type is not supported"); > > > + return NULL; > > > + } > > > + > > > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > > > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > > + return NULL; > > > + } > > > + > > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > > + if (!oc) { > > > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > > > + model->name); > > > + return NULL; > > > + } > > > + > > > + if (kvm_enabled()) { > > > + const char *cpu_type = current_machine->cpu_type; > > > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > > > + bool supported = false; > > > + > > > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > > > + /* These are kvmarm's recommended cpu types */ > > > + supported = true; > > > + } else if (strlen(model->name) == len && > > > + !strncmp(model->name, cpu_type, len)) { > > > + /* KVM is enabled and we're using this type, so it works. */ > > > + supported = true; > > > + } > > > + if (!supported) { > > > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > > + "with KVM on this host", model->name); > > > + return NULL; > > > + } > > > + } > > > + > > > > The above section can be slightly reduced and rearranged - preferably > > moved to a separate function > > -> get_cpu_model (...) ? > > > > * You can check the 'host' model first and then validate the accelerator -> > > if ( !strcmp(model->name, "host") > > if (!kvm_enabled()) > > log_error & leave > > else > > goto cpu_class_by_name /*cpu_class_by_name moved after the > > final model check @see below */ > > > > * the kvm_enabled section can be than slightly improved (dropping the > > second compare against 'host') > > > > if (kvm_enabled() && strcmp(model->name, "max") { > > /*Validate the current_machine->cpu_type against the > > model->name and report error case mismatch > > /* otherwise just fall through */ > > } > > * cpu_class_by_name moved here ... > > > + if (model->props) { > > MInor: the CPUModelInfo seems to have dedicated field for that > > verification -> has_props > > > > > + qdict_in = qobject_to(QDict, model->props); > > > + if (!qdict_in) { > > > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > > > + return NULL; > > > + } > > > + } > > > + > > > + obj = object_new(object_class_get_name(oc)); > > > + > > > + if (qdict_in) { > > > + Visitor *visitor; > > > + Error *err = NULL; > > > + > > > + visitor = qobject_input_visitor_new(model->props); > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > + if (err) { > > > + object_unref(obj); > > > + error_propagate(errp, err); > > > + return NULL; > > > + } > > > + > > > + i = 0; > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > + if (qdict_get(qdict_in, name)) { > > > + object_property_set(obj, visitor, name, &err); > > > + if (err) { > > > + break; > > > + } > > > + } > > > + } > > > + > > > + if (!err) { > > > + visit_check_struct(visitor, &err); > > > + } > > > + visit_end_struct(visitor, NULL); > > > + visit_free(visitor); > > > + if (err) { > > > + object_unref(obj); > > > + error_propagate(errp, err); > > > + return NULL; > > > + } > > > + } > > > > The both >> if (err) << blocks could be extracted and moved at the end > > of the function > > to mark a 'cleanup section' and both here and few lines before > > (with the visit_start_struct failure) could use goto. > > Easier to maintain and to make sure we make the proper cleanup in any case. > > > > > + > > > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > > > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > > > + expansion_info->model->name = g_strdup(model->name); > > > + > > > + qdict_out = qdict_new(); > > > + > > > + i = 0; > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > + ObjectProperty *prop = object_property_find(obj, name, NULL); > > > + if (prop) { > > > + Error *err = NULL; > > > + QObject *value; > > > + > > > + assert(prop->get); > > > + value = object_property_get_qobject(obj, name, &err); > > > + assert(!err); > > > + > > > + qdict_put_obj(qdict_out, name, value); > > > + } > > > + } > > > + > > > > This could be merged with the first iteration over the features, > > smth like: > > > > while () { > > if ((value = qdict_get(qdict_in, name))) { > > object_property_set ... > > if (!err) > > qobject_ref(value) /* we have the weak reference */ > > else > > break; > > } else { > > value = object_property_get_qobject() > > } > > if (value) qdict_put_object(qdict_out, name, value) > > } > > > > This way you iterate over the table once and you do not query > > for the same property twice by getting the value from the qdict_in. > > If the value is not acceptable we will fail either way so should be all good. > > > > > > > + if (!qdict_size(qdict_out)) { > > > + qobject_unref(qdict_out); > > > + } else { > > > + expansion_info->model->props = QOBJECT(qdict_out); > > > + expansion_info->model->has_props = true; > > > + } > > > + > > > + object_unref(obj); > > > + > > > + return expansion_info; > > > > Mentioned earlier cleanup section: > > cleanup: > > object_unref(obj); > > qobject_unref(qdict_out) ; /* if single loop is used */ > > error_propagate(errp,err); > > return NULL; > > > > > +} > > > -- > > > 2.20.1 > > > > > > > Hope I haven't missed anything. > > What do you think ? > > > > I think you need to post an entire function that incorporates all the > proposed changes, or at least a diff that I can apply in order to get > the entirely changed function. I also think that it's fine the way > it is, so it would take a justification stronger than a potential > micro optimization to get me to change it. > The numbers I can pull out of it are not thrilling and this is not on a fast path so I will not be pushing for changes. Though extracting the clean-up might be worth considering - for improved maintenance. For a reference though:
On Wed, Oct 16, 2019 at 04:16:57PM +0100, Beata Michalska wrote: > On Wed, 16 Oct 2019 at 14:50, Andrew Jones <drjones@redhat.com> wrote: > > > > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote: > > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska > > > <beata.michalska@linaro.org> wrote: > > > > > > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > > > > > > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > > > > > + > > > > > > > + obj = object_new(object_class_get_name(oc)); > > > > > > > + > > > > > > > + if (qdict_in) { > > > > > > > + Visitor *visitor; > > > > > > > + Error *err = NULL; > > > > > > > + > > > > > > > + visitor = qobject_input_visitor_new(model->props); > > > > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > > > > + if (err) { > > > > > > > + object_unref(obj); > > > > > > > > > > > > Shouldn't we free the 'visitor' here as well ? > > > > > > > > > > Yes. Good catch. So we also need to fix > > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > > > > > construction (the construction from which I derived this) > > > > > > > > > > > > > > > > > > + error_propagate(errp, err); > > > > > > > + return NULL; > > > > > > > + } > > > > > > > + > > > > > > > > > > What about the rest of the patch? With that fixed for v6 can I > > > > > add your r-b? > > > > > > > > > > > > > I still got this feeling that we could optimize that a bit - which I'm > > > > currently on, so hopefully I'll be able to add more comments soon if > > > > that proves to be the case. > > > > > > > > BR > > > > Beata > > > > > > I think there are few options that might be considered though the gain > > > is not huge .. but it's always smth: > > > > > > > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > > > + CpuModelInfo *model, > > > > + Error **errp) > > > > +{ > > > > + CpuModelExpansionInfo *expansion_info; > > > > + const QDict *qdict_in = NULL; > > > > + QDict *qdict_out; > > > > + ObjectClass *oc; > > > > + Object *obj; > > > > + const char *name; > > > > + int i; > > > > + > > > > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > > > > + error_setg(errp, "The requested expansion type is not supported"); > > > > + return NULL; > > > > + } > > > > + > > > > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > > > > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > > > + return NULL; > > > > + } > > > > + > > > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > > > + if (!oc) { > > > > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > > > > + model->name); > > > > + return NULL; > > > > + } > > > > + > > > > + if (kvm_enabled()) { > > > > + const char *cpu_type = current_machine->cpu_type; > > > > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > > > > + bool supported = false; > > > > + > > > > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > > > > + /* These are kvmarm's recommended cpu types */ > > > > + supported = true; > > > > + } else if (strlen(model->name) == len && > > > > + !strncmp(model->name, cpu_type, len)) { > > > > + /* KVM is enabled and we're using this type, so it works. */ > > > > + supported = true; > > > > + } > > > > + if (!supported) { > > > > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > > > + "with KVM on this host", model->name); > > > > + return NULL; > > > > + } > > > > + } > > > > + > > > > > > The above section can be slightly reduced and rearranged - preferably > > > moved to a separate function > > > -> get_cpu_model (...) ? > > > > > > * You can check the 'host' model first and then validate the accelerator -> > > > if ( !strcmp(model->name, "host") > > > if (!kvm_enabled()) > > > log_error & leave > > > else > > > goto cpu_class_by_name /*cpu_class_by_name moved after the > > > final model check @see below */ > > > > > > * the kvm_enabled section can be than slightly improved (dropping the > > > second compare against 'host') > > > > > > if (kvm_enabled() && strcmp(model->name, "max") { > > > /*Validate the current_machine->cpu_type against the > > > model->name and report error case mismatch > > > /* otherwise just fall through */ > > > } > > > * cpu_class_by_name moved here ... > > > > + if (model->props) { > > > MInor: the CPUModelInfo seems to have dedicated field for that > > > verification -> has_props > > > > > > > + qdict_in = qobject_to(QDict, model->props); > > > > + if (!qdict_in) { > > > > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > > > > + return NULL; > > > > + } > > > > + } > > > > + > > > > + obj = object_new(object_class_get_name(oc)); > > > > + > > > > + if (qdict_in) { > > > > + Visitor *visitor; > > > > + Error *err = NULL; > > > > + > > > > + visitor = qobject_input_visitor_new(model->props); > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > + if (err) { > > > > + object_unref(obj); > > > > + error_propagate(errp, err); > > > > + return NULL; > > > > + } > > > > + > > > > + i = 0; > > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > > + if (qdict_get(qdict_in, name)) { > > > > + object_property_set(obj, visitor, name, &err); > > > > + if (err) { > > > > + break; > > > > + } > > > > + } > > > > + } > > > > + > > > > + if (!err) { > > > > + visit_check_struct(visitor, &err); > > > > + } > > > > + visit_end_struct(visitor, NULL); > > > > + visit_free(visitor); > > > > + if (err) { > > > > + object_unref(obj); > > > > + error_propagate(errp, err); > > > > + return NULL; > > > > + } > > > > + } > > > > > > The both >> if (err) << blocks could be extracted and moved at the end > > > of the function > > > to mark a 'cleanup section' and both here and few lines before > > > (with the visit_start_struct failure) could use goto. > > > Easier to maintain and to make sure we make the proper cleanup in any case. > > > > > > > + > > > > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > > > > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > > > > + expansion_info->model->name = g_strdup(model->name); > > > > + > > > > + qdict_out = qdict_new(); > > > > + > > > > + i = 0; > > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > > + ObjectProperty *prop = object_property_find(obj, name, NULL); > > > > + if (prop) { > > > > + Error *err = NULL; > > > > + QObject *value; > > > > + > > > > + assert(prop->get); > > > > + value = object_property_get_qobject(obj, name, &err); > > > > + assert(!err); > > > > + > > > > + qdict_put_obj(qdict_out, name, value); > > > > + } > > > > + } > > > > + > > > > > > This could be merged with the first iteration over the features, > > > smth like: > > > > > > while () { > > > if ((value = qdict_get(qdict_in, name))) { > > > object_property_set ... > > > if (!err) > > > qobject_ref(value) /* we have the weak reference */ > > > else > > > break; > > > } else { > > > value = object_property_get_qobject() > > > } > > > if (value) qdict_put_object(qdict_out, name, value) > > > } > > > > > > This way you iterate over the table once and you do not query > > > for the same property twice by getting the value from the qdict_in. > > > If the value is not acceptable we will fail either way so should be all good. > > > > > > > > > > + if (!qdict_size(qdict_out)) { > > > > + qobject_unref(qdict_out); > > > > + } else { > > > > + expansion_info->model->props = QOBJECT(qdict_out); > > > > + expansion_info->model->has_props = true; > > > > + } > > > > + > > > > + object_unref(obj); > > > > + > > > > + return expansion_info; > > > > > > Mentioned earlier cleanup section: > > > cleanup: > > > object_unref(obj); > > > qobject_unref(qdict_out) ; /* if single loop is used */ > > > error_propagate(errp,err); > > > return NULL; > > > > > > > +} > > > > -- > > > > 2.20.1 > > > > > > > > > > Hope I haven't missed anything. > > > What do you think ? > > > > > > > I think you need to post an entire function that incorporates all the > > proposed changes, or at least a diff that I can apply in order to get > > the entirely changed function. I also think that it's fine the way > > it is, so it would take a justification stronger than a potential > > micro optimization to get me to change it. > > > > The numbers I can pull out of it are not thrilling and this is not > on a fast path so I will not be pushing for changes. > Though extracting the clean-up might be worth considering - > for improved maintenance. > > For a reference though: It doesn't apply for me - even after fixing up the damager your mailer did to it. I'd be surprised if it worked though. Merging the two loops over features makes the output generation depend on the caller providing input. Did you try the arm-cpu-features test with these changes? drew > > _______________________________________________________ > > --- > target/arm/monitor.c | 100 +++++++++++++++++++++++++-------------------------- > 1 file changed, 50 insertions(+), 50 deletions(-) > > diff --git a/target/arm/monitor.c b/target/arm/monitor.c > index edca8aa..0d6bd42 100644 > --- a/target/arm/monitor.c > +++ b/target/arm/monitor.c > @@ -112,17 +112,40 @@ CpuModelExpansionInfo > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > Object *obj; > const char *name; > int i; > + Error *err = NULL; > > if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > error_setg(errp, "The requested expansion type is not supported"); > return NULL; > } > > - if (!kvm_enabled() && !strcmp(model->name, "host")) { > - error_setg(errp, "The CPU type '%s' requires KVM", model->name); > - return NULL; > + /* CPU type => 'host' */ > + if (!strcmp(model->name, "host")) { > + if (!kvm_enabled()) { > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > + return NULL; > + } else { > + goto valid; > + } > + } > + > + > + /* Case when KVM is enabled and the model is a specific cpu model ... */ > + if (kvm_enabled() && strcmp(model->name, "max")) { > + const char *cpu_type = current_machine->cpu_type; > + int len = strlen(cpu_type) - strlen("-" TYPE_ARM_CPU); > + > + if (strlen(model->name) == len > + && !strncmp(cpu_type, model->name, len)) { > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > + "with KVM on this host", model->name); > + return NULL; > + } > + > } > > +valid: > + > oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > if (!oc) { > error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > @@ -130,25 +153,6 @@ CpuModelExpansionInfo > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > return NULL; > } > > - if (kvm_enabled()) { > - const char *cpu_type = current_machine->cpu_type; > - int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > - bool supported = false; > - > - if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > - /* These are kvmarm's recommended cpu types */ > - supported = true; > - } else if (strlen(model->name) == len && > - !strncmp(model->name, cpu_type, len)) { > - /* KVM is enabled and we're using this type, so it works. */ > - supported = true; > - } > - if (!supported) { > - error_setg(errp, "We cannot guarantee the CPU type '%s' works " > - "with KVM on this host", model->name); > - return NULL; > - } > - } > > if (model->props) { > qdict_in = qobject_to(QDict, model->props); > @@ -159,62 +163,52 @@ CpuModelExpansionInfo > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > } > > obj = object_new(object_class_get_name(oc)); > + qdict_out = qdict_new(); > > if (qdict_in) { > Visitor *visitor; > - Error *err = NULL; > + QObject *value; > > visitor = qobject_input_visitor_new(model->props); > visit_start_struct(visitor, NULL, NULL, 0, &err); > if (err) { > - object_unref(obj); > - error_propagate(errp, err); > - return NULL; > + visit_free(visitor); > + goto cleanup; > } > - > i = 0; > while ((name = cpu_model_advertised_features[i++]) != NULL) { > - if (qdict_get(qdict_in, name)) { > + value = qdict_get(qdict_in, name); > + if (value) { > object_property_set(obj, visitor, name, &err); > - if (err) { > + if (!err) { > + qobject_ref(value); > + } else { > break; > } > + > + } else { > + value = object_property_get_qobject(obj, name, &err); > } > - } > > + if (value) { > + qdict_put_obj(qdict_out, name, value); > + } > + } > if (!err) { > visit_check_struct(visitor, &err); > } > visit_end_struct(visitor, NULL); > visit_free(visitor); > if (err) { > - object_unref(obj); > - error_propagate(errp, err); > - return NULL; > + goto cleanup; > } > + > } > > expansion_info = g_new0(CpuModelExpansionInfo, 1); > expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > expansion_info->model->name = g_strdup(model->name); > > - qdict_out = qdict_new(); > - > - i = 0; > - while ((name = cpu_model_advertised_features[i++]) != NULL) { > - ObjectProperty *prop = object_property_find(obj, name, NULL); > - if (prop) { > - Error *err = NULL; > - QObject *value; > - > - assert(prop->get); > - value = object_property_get_qobject(obj, name, &err); > - assert(!err); > - > - qdict_put_obj(qdict_out, name, value); > - } > - } > - > if (!qdict_size(qdict_out)) { > qobject_unref(qdict_out); > } else { > @@ -225,4 +219,10 @@ CpuModelExpansionInfo > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > object_unref(obj); > > return expansion_info; > + > +cleanup: > + object_unref(obj); > + qobject_unref(qdict_out); > + error_propagate(errp, err); > + return NULL; > } > -- > 2.7.4 > > BR > Beata > > > Thanks, > > drew >
On Wed, 16 Oct 2019 at 17:16, Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Oct 16, 2019 at 04:16:57PM +0100, Beata Michalska wrote: > > On Wed, 16 Oct 2019 at 14:50, Andrew Jones <drjones@redhat.com> wrote: > > > > > > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote: > > > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska > > > > <beata.michalska@linaro.org> wrote: > > > > > > > > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjones@redhat.com> wrote: > > > > > > > > > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote: > > > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjones@redhat.com> wrote: > > > > > > > > + > > > > > > > > + obj = object_new(object_class_get_name(oc)); > > > > > > > > + > > > > > > > > + if (qdict_in) { > > > > > > > > + Visitor *visitor; > > > > > > > > + Error *err = NULL; > > > > > > > > + > > > > > > > > + visitor = qobject_input_visitor_new(model->props); > > > > > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > > > > > + if (err) { > > > > > > > > + object_unref(obj); > > > > > > > > > > > > > > Shouldn't we free the 'visitor' here as well ? > > > > > > > > > > > > Yes. Good catch. So we also need to fix > > > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same > > > > > > construction (the construction from which I derived this) > > > > > > > > > > > > > > > > > > > > > + error_propagate(errp, err); > > > > > > > > + return NULL; > > > > > > > > + } > > > > > > > > + > > > > > > > > > > > > What about the rest of the patch? With that fixed for v6 can I > > > > > > add your r-b? > > > > > > > > > > > > > > > > I still got this feeling that we could optimize that a bit - which I'm > > > > > currently on, so hopefully I'll be able to add more comments soon if > > > > > that proves to be the case. > > > > > > > > > > BR > > > > > Beata > > > > > > > > I think there are few options that might be considered though the gain > > > > is not huge .. but it's always smth: > > > > > > > > > +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > > > > + CpuModelInfo *model, > > > > > + Error **errp) > > > > > +{ > > > > > + CpuModelExpansionInfo *expansion_info; > > > > > + const QDict *qdict_in = NULL; > > > > > + QDict *qdict_out; > > > > > + ObjectClass *oc; > > > > > + Object *obj; > > > > > + const char *name; > > > > > + int i; > > > > > + > > > > > + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > > > > > + error_setg(errp, "The requested expansion type is not supported"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + if (!kvm_enabled() && !strcmp(model->name, "host")) { > > > > > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > > > > + if (!oc) { > > > > > + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > > > > > + model->name); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + if (kvm_enabled()) { > > > > > + const char *cpu_type = current_machine->cpu_type; > > > > > + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > > > > > + bool supported = false; > > > > > + > > > > > + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > > > > > + /* These are kvmarm's recommended cpu types */ > > > > > + supported = true; > > > > > + } else if (strlen(model->name) == len && > > > > > + !strncmp(model->name, cpu_type, len)) { > > > > > + /* KVM is enabled and we're using this type, so it works. */ > > > > > + supported = true; > > > > > + } > > > > > + if (!supported) { > > > > > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > > > > + "with KVM on this host", model->name); > > > > > + return NULL; > > > > > + } > > > > > + } > > > > > + > > > > > > > > The above section can be slightly reduced and rearranged - preferably > > > > moved to a separate function > > > > -> get_cpu_model (...) ? > > > > > > > > * You can check the 'host' model first and then validate the accelerator -> > > > > if ( !strcmp(model->name, "host") > > > > if (!kvm_enabled()) > > > > log_error & leave > > > > else > > > > goto cpu_class_by_name /*cpu_class_by_name moved after the > > > > final model check @see below */ > > > > > > > > * the kvm_enabled section can be than slightly improved (dropping the > > > > second compare against 'host') > > > > > > > > if (kvm_enabled() && strcmp(model->name, "max") { > > > > /*Validate the current_machine->cpu_type against the > > > > model->name and report error case mismatch > > > > /* otherwise just fall through */ > > > > } > > > > * cpu_class_by_name moved here ... > > > > > + if (model->props) { > > > > MInor: the CPUModelInfo seems to have dedicated field for that > > > > verification -> has_props > > > > > > > > > + qdict_in = qobject_to(QDict, model->props); > > > > > + if (!qdict_in) { > > > > > + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); > > > > > + return NULL; > > > > > + } > > > > > + } > > > > > + > > > > > + obj = object_new(object_class_get_name(oc)); > > > > > + > > > > > + if (qdict_in) { > > > > > + Visitor *visitor; > > > > > + Error *err = NULL; > > > > > + > > > > > + visitor = qobject_input_visitor_new(model->props); > > > > > + visit_start_struct(visitor, NULL, NULL, 0, &err); > > > > > + if (err) { > > > > > + object_unref(obj); > > > > > + error_propagate(errp, err); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + i = 0; > > > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > > > + if (qdict_get(qdict_in, name)) { > > > > > + object_property_set(obj, visitor, name, &err); > > > > > + if (err) { > > > > > + break; > > > > > + } > > > > > + } > > > > > + } > > > > > + > > > > > + if (!err) { > > > > > + visit_check_struct(visitor, &err); > > > > > + } > > > > > + visit_end_struct(visitor, NULL); > > > > > + visit_free(visitor); > > > > > + if (err) { > > > > > + object_unref(obj); > > > > > + error_propagate(errp, err); > > > > > + return NULL; > > > > > + } > > > > > + } > > > > > > > > The both >> if (err) << blocks could be extracted and moved at the end > > > > of the function > > > > to mark a 'cleanup section' and both here and few lines before > > > > (with the visit_start_struct failure) could use goto. > > > > Easier to maintain and to make sure we make the proper cleanup in any case. > > > > > > > > > + > > > > > + expansion_info = g_new0(CpuModelExpansionInfo, 1); > > > > > + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > > > > > + expansion_info->model->name = g_strdup(model->name); > > > > > + > > > > > + qdict_out = qdict_new(); > > > > > + > > > > > + i = 0; > > > > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > > > > + ObjectProperty *prop = object_property_find(obj, name, NULL); > > > > > + if (prop) { > > > > > + Error *err = NULL; > > > > > + QObject *value; > > > > > + > > > > > + assert(prop->get); > > > > > + value = object_property_get_qobject(obj, name, &err); > > > > > + assert(!err); > > > > > + > > > > > + qdict_put_obj(qdict_out, name, value); > > > > > + } > > > > > + } > > > > > + > > > > > > > > This could be merged with the first iteration over the features, > > > > smth like: > > > > > > > > while () { > > > > if ((value = qdict_get(qdict_in, name))) { > > > > object_property_set ... > > > > if (!err) > > > > qobject_ref(value) /* we have the weak reference */ > > > > else > > > > break; > > > > } else { > > > > value = object_property_get_qobject() > > > > } > > > > if (value) qdict_put_object(qdict_out, name, value) > > > > } > > > > > > > > This way you iterate over the table once and you do not query > > > > for the same property twice by getting the value from the qdict_in. > > > > If the value is not acceptable we will fail either way so should be all good. > > > > > > > > > > > > > + if (!qdict_size(qdict_out)) { > > > > > + qobject_unref(qdict_out); > > > > > + } else { > > > > > + expansion_info->model->props = QOBJECT(qdict_out); > > > > > + expansion_info->model->has_props = true; > > > > > + } > > > > > + > > > > > + object_unref(obj); > > > > > + > > > > > + return expansion_info; > > > > > > > > Mentioned earlier cleanup section: > > > > cleanup: > > > > object_unref(obj); > > > > qobject_unref(qdict_out) ; /* if single loop is used */ > > > > error_propagate(errp,err); > > > > return NULL; > > > > > > > > > +} > > > > > -- > > > > > 2.20.1 > > > > > > > > > > > > > Hope I haven't missed anything. > > > > What do you think ? > > > > > > > > > > I think you need to post an entire function that incorporates all the > > > proposed changes, or at least a diff that I can apply in order to get > > > the entirely changed function. I also think that it's fine the way > > > it is, so it would take a justification stronger than a potential > > > micro optimization to get me to change it. > > > > > > > The numbers I can pull out of it are not thrilling and this is not > > on a fast path so I will not be pushing for changes. > > Though extracting the clean-up might be worth considering - > > for improved maintenance. > > > > For a reference though: > > It doesn't apply for me - even after fixing up the damager your mailer did > to it. I'd be surprised if it worked though. Merging the two loops over > features makes the output generation depend on the caller providing input. > Did you try the arm-cpu-features test with these changes? > Apologies for the late reply. Indeed, the patch got bit messed-up. Apologies for that as well. I have been testing manually but I did try the test you have provided and yes it fails - there is a slight problem with the case when qdict_in is empty,but this can be easily solved still keeping the single loop. Otherwise I have seen you have posted a new patchest so I guess we are dropping the idea of refactoring ? One more question: in case of querying a property which is not supported by given cpu model - we are returning properties that are actually valid (the test case for cortex-a15 and aarch64 prop). Shouldn't we return an error there? I honestly must admit I do not know what is the expected behaviour for the qmp query in such cases. Best Regards Beata > drew > > > > > _______________________________________________________ > > > > --- > > target/arm/monitor.c | 100 +++++++++++++++++++++++++-------------------------- > > 1 file changed, 50 insertions(+), 50 deletions(-) > > > > diff --git a/target/arm/monitor.c b/target/arm/monitor.c > > index edca8aa..0d6bd42 100644 > > --- a/target/arm/monitor.c > > +++ b/target/arm/monitor.c > > @@ -112,17 +112,40 @@ CpuModelExpansionInfo > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > Object *obj; > > const char *name; > > int i; > > + Error *err = NULL; > > > > if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { > > error_setg(errp, "The requested expansion type is not supported"); > > return NULL; > > } > > > > - if (!kvm_enabled() && !strcmp(model->name, "host")) { > > - error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > - return NULL; > > + /* CPU type => 'host' */ > > + if (!strcmp(model->name, "host")) { > > + if (!kvm_enabled()) { > > + error_setg(errp, "The CPU type '%s' requires KVM", model->name); > > + return NULL; > > + } else { > > + goto valid; > > + } > > + } > > + > > + > > + /* Case when KVM is enabled and the model is a specific cpu model ... */ > > + if (kvm_enabled() && strcmp(model->name, "max")) { > > + const char *cpu_type = current_machine->cpu_type; > > + int len = strlen(cpu_type) - strlen("-" TYPE_ARM_CPU); > > + > > + if (strlen(model->name) == len > > + && !strncmp(cpu_type, model->name, len)) { > > + error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > + "with KVM on this host", model->name); > > + return NULL; > > + } > > + > > } > > > > +valid: > > + > > oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > if (!oc) { > > error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", > > @@ -130,25 +153,6 @@ CpuModelExpansionInfo > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > return NULL; > > } > > > > - if (kvm_enabled()) { > > - const char *cpu_type = current_machine->cpu_type; > > - int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > > - bool supported = false; > > - > > - if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { > > - /* These are kvmarm's recommended cpu types */ > > - supported = true; > > - } else if (strlen(model->name) == len && > > - !strncmp(model->name, cpu_type, len)) { > > - /* KVM is enabled and we're using this type, so it works. */ > > - supported = true; > > - } > > - if (!supported) { > > - error_setg(errp, "We cannot guarantee the CPU type '%s' works " > > - "with KVM on this host", model->name); > > - return NULL; > > - } > > - } > > > > if (model->props) { > > qdict_in = qobject_to(QDict, model->props); > > @@ -159,62 +163,52 @@ CpuModelExpansionInfo > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > } > > > > obj = object_new(object_class_get_name(oc)); > > + qdict_out = qdict_new(); > > > > if (qdict_in) { > > Visitor *visitor; > > - Error *err = NULL; > > + QObject *value; > > > > visitor = qobject_input_visitor_new(model->props); > > visit_start_struct(visitor, NULL, NULL, 0, &err); > > if (err) { > > - object_unref(obj); > > - error_propagate(errp, err); > > - return NULL; > > + visit_free(visitor); > > + goto cleanup; > > } > > - > > i = 0; > > while ((name = cpu_model_advertised_features[i++]) != NULL) { > > - if (qdict_get(qdict_in, name)) { > > + value = qdict_get(qdict_in, name); > > + if (value) { > > object_property_set(obj, visitor, name, &err); > > - if (err) { > > + if (!err) { > > + qobject_ref(value); > > + } else { > > break; > > } > > + > > + } else { > > + value = object_property_get_qobject(obj, name, &err); > > } > > - } > > > > + if (value) { > > + qdict_put_obj(qdict_out, name, value); > > + } > > + } > > if (!err) { > > visit_check_struct(visitor, &err); > > } > > visit_end_struct(visitor, NULL); > > visit_free(visitor); > > if (err) { > > - object_unref(obj); > > - error_propagate(errp, err); > > - return NULL; > > + goto cleanup; > > } > > + > > } > > > > expansion_info = g_new0(CpuModelExpansionInfo, 1); > > expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); > > expansion_info->model->name = g_strdup(model->name); > > > > - qdict_out = qdict_new(); > > - > > - i = 0; > > - while ((name = cpu_model_advertised_features[i++]) != NULL) { > > - ObjectProperty *prop = object_property_find(obj, name, NULL); > > - if (prop) { > > - Error *err = NULL; > > - QObject *value; > > - > > - assert(prop->get); > > - value = object_property_get_qobject(obj, name, &err); > > - assert(!err); > > - > > - qdict_put_obj(qdict_out, name, value); > > - } > > - } > > - > > if (!qdict_size(qdict_out)) { > > qobject_unref(qdict_out); > > } else { > > @@ -225,4 +219,10 @@ CpuModelExpansionInfo > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > > object_unref(obj); > > > > return expansion_info; > > + > > +cleanup: > > + object_unref(obj); > > + qobject_unref(qdict_out); > > + error_propagate(errp, err); > > + return NULL; > > } > > -- > > 2.7.4 > > > > BR > > Beata > > > > > Thanks, > > > drew > >
On Mon, Oct 21, 2019 at 04:07:14PM +0100, Beata Michalska wrote: > Indeed, the patch got bit messed-up. Apologies for that as well. > I have been testing manually but I did try the test you have provided > and yes it fails - there is a slight problem with the case when qdict_in > is empty,but this can be easily solved still keeping the single loop. > Otherwise I have seen you have posted a new patchest so I guess we are > dropping the idea of refactoring ? Well, without a patch that applies, I couldn't really evaluate your proposal. And, TBH, I'd rather not hold this series up on a refactoring that doesn't provide measurable performance improvements, especially when it's not in a performance critical path. Indeed, I'd like to get this series merged as soon as possible, which is why I posted v6 with your visit_free() fix already. > > One more question: in case of querying a property which is not supported > by given cpu model - we are returning properties that are actually valid > (the test case for cortex-a15 and aarch64 prop). > Shouldn't we return an error there? I honestly must admit I do not know > what is the expected behaviour for the qmp query in such cases. We do generate an error for that case: (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"} {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}} (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15","props":{"aarch64":false}} {"error": {"class": "GenericError", "desc": "Property '.aarch64' not found"}} If you have any more comments on the series, please send them right away. I'd like Peter to be able to merge this soon, and I understand that he's waiting on your review. Thanks, drew
Hi Andrew, On Tue, 22 Oct 2019 at 14:43, Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Oct 21, 2019 at 04:07:14PM +0100, Beata Michalska wrote: > > Indeed, the patch got bit messed-up. Apologies for that as well. > > I have been testing manually but I did try the test you have provided > > and yes it fails - there is a slight problem with the case when qdict_in > > is empty,but this can be easily solved still keeping the single loop. > > Otherwise I have seen you have posted a new patchest so I guess we are > > dropping the idea of refactoring ? > > Well, without a patch that applies, I couldn't really evaluate your > proposal. And, TBH, I'd rather not hold this series up on a refactoring > that doesn't provide measurable performance improvements, especially when > it's not in a performance critical path. Indeed, I'd like to get this > series merged as soon as possible, which is why I posted v6 with your > visit_free() fix already. > > > > > One more question: in case of querying a property which is not supported > > by given cpu model - we are returning properties that are actually valid > > (the test case for cortex-a15 and aarch64 prop). > > Shouldn't we return an error there? I honestly must admit I do not know > > what is the expected behaviour for the qmp query in such cases. > > We do generate an error for that case: > > (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"} > {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}} > > (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15","props":{"aarch64":false}} > {"error": {"class": "GenericError", "desc": "Property '.aarch64' not found"}} > > > If you have any more comments on the series, please send them right away. > I'd like Peter to be able to merge this soon, and I understand that he's > waiting on your review. > I think we can proceed with the v6 as it is. Thanks a lot. BR Beata > Thanks, > drew >
diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst new file mode 100644 index 000000000000..c79dcffb5556 --- /dev/null +++ b/docs/arm-cpu-features.rst @@ -0,0 +1,137 @@ +================ +ARM CPU Features +================ + +Examples of probing and using ARM CPU features + +Introduction +============ + +CPU features are optional features that a CPU of supporting type may +choose to implement or not. In QEMU, optional CPU features have +corresponding boolean CPU proprieties that, when enabled, indicate +that the feature is implemented, and, conversely, when disabled, +indicate that it is not implemented. An example of an ARM CPU feature +is the Performance Monitoring Unit (PMU). CPU types such as the +Cortex-A15 and the Cortex-A57, which respectively implement ARM +architecture reference manuals ARMv7-A and ARMv8-A, may both optionally +implement PMUs. For example, if a user wants to use a Cortex-A15 without +a PMU, then the `-cpu` parameter should contain `pmu=off` on the QEMU +command line, i.e. `-cpu cortex-a15,pmu=off`. + +As not all CPU types support all optional CPU features, then whether or +not a CPU property exists depends on the CPU type. For example, CPUs +that implement the ARMv8-A architecture reference manual may optionally +support the AArch32 CPU feature, which may be enabled by disabling the +`aarch64` CPU property. A CPU type such as the Cortex-A15, which does +not implement ARMv8-A, will not have the `aarch64` CPU property. + +QEMU's support may be limited for some CPU features, only partially +supporting the feature or only supporting the feature under certain +configurations. For example, the `aarch64` CPU feature, which, when +disabled, enables the optional AArch32 CPU feature, is only supported +when using the KVM accelerator and when running on a host CPU type that +supports the feature. + +CPU Feature Probing +=================== + +Determining which CPU features are available and functional for a given +CPU type is possible with the `query-cpu-model-expansion` QMP command. +Below are some examples where `scripts/qmp/qmp-shell` (see the top comment +block in the script for usage) is used to issue the QMP commands. + +(1) Determine which CPU features are available for the `max` CPU type + (Note, we started QEMU with qemu-system-aarch64, so `max` is + implementing the ARMv8-A reference manual in this case):: + + (QEMU) query-cpu-model-expansion type=full model={"name":"max"} + { "return": { + "model": { "name": "max", "props": { + "pmu": true, "aarch64": true + }}}} + +We see that the `max` CPU type has the `pmu` and `aarch64` CPU features. +We also see that the CPU features are enabled, as they are all `true`. + +(2) Let's try to disable the PMU:: + + (QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"pmu":false}} + { "return": { + "model": { "name": "max", "props": { + "pmu": false, "aarch64": true + }}}} + +We see it worked, as `pmu` is now `false`. + +(3) Let's try to disable `aarch64`, which enables the AArch32 CPU feature:: + + (QEMU) query-cpu-model-expansion type=full model={"name":"max","props":{"aarch64":false}} + {"error": { + "class": "GenericError", "desc": + "'aarch64' feature cannot be disabled unless KVM is enabled and 32-bit EL1 is supported" + }} + +It looks like this feature is limited to a configuration we do not +currently have. + +(4) Let's try probing CPU features for the Cortex-A15 CPU type:: + + (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"} + {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}} + +Only the `pmu` CPU feature is available. + +A note about CPU feature dependencies +------------------------------------- + +It's possible for features to have dependencies on other features. I.e. +it may be possible to change one feature at a time without error, but +when attempting to change all features at once an error could occur +depending on the order they are processed. It's also possible changing +all at once doesn't generate an error, because a feature's dependencies +are satisfied with other features, but the same feature cannot be changed +independently without error. For these reasons callers should always +attempt to make their desired changes all at once in order to ensure the +collection is valid. + +A note about CPU models and KVM +------------------------------- + +Named CPU models generally do not work with KVM. There are a few cases +that do work, e.g. using the named CPU model `cortex-a57` with KVM on a +seattle host, but mostly if KVM is enabled the `host` CPU type must be +used. This means the guest is provided all the same CPU features as the +host CPU type has. And, for this reason, the `host` CPU type should +enable all CPU features that the host has by default. Indeed it's even +a bit strange to allow disabling CPU features that the host has when using +the `host` CPU type, but in the absence of CPU models it's the best we can +do if we want to launch guests without all the host's CPU features enabled. + +Enabling KVM also affects the `query-cpu-model-expansion` QMP command. The +affect is not only limited to specific features, as pointed out in example +(3) of "CPU Feature Probing", but also to which CPU types may be expanded. +When KVM is enabled, only the `max`, `host`, and current CPU type may be +expanded. This restriction is necessary as it's not possible to know all +CPU types that may work with KVM, but it does impose a small risk of users +experiencing unexpected errors. For example on a seattle, as mentioned +above, the `cortex-a57` CPU type is also valid when KVM is enabled. +Therefore a user could use the `host` CPU type for the current type, but +then attempt to query `cortex-a57`, however that query will fail with our +restrictions. This shouldn't be an issue though as management layers and +users have been preferring the `host` CPU type for use with KVM for quite +some time. Additionally, if the KVM-enabled QEMU instance running on a +seattle host is using the `cortex-a57` CPU type, then querying `cortex-a57` +will work. + +Using CPU Features +================== + +After determining which CPU features are available and supported for a +given CPU type, then they may be selectively enabled or disabled on the +QEMU command line with that CPU type:: + + $ qemu-system-aarch64 -M virt -cpu max,pmu=off + +The example above disables the PMU for the `max` CPU type. + diff --git a/qapi/machine-target.json b/qapi/machine-target.json index 55310a6aa226..04623224720d 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -212,7 +212,7 @@ ## { 'struct': 'CpuModelExpansionInfo', 'data': { 'model': 'CpuModelInfo' }, - 'if': 'defined(TARGET_S390X) || defined(TARGET_I386)' } + 'if': 'defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM)' } ## # @query-cpu-model-expansion: @@ -237,7 +237,7 @@ # query-cpu-model-expansion while using these is not advised. # # Some architectures may not support all expansion types. s390x supports -# "full" and "static". +# "full" and "static". Arm only supports "full". # # Returns: a CpuModelExpansionInfo. Returns an error if expanding CPU models is # not supported, if the model cannot be expanded, if the model contains @@ -251,7 +251,7 @@ 'data': { 'type': 'CpuModelExpansionType', 'model': 'CpuModelInfo' }, 'returns': 'CpuModelExpansionInfo', - 'if': 'defined(TARGET_S390X) || defined(TARGET_I386)' } + 'if': 'defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM)' } ## # @CpuDefinitionInfo: diff --git a/target/arm/monitor.c b/target/arm/monitor.c index 6457c3c87f7c..edca8aa885f0 100644 --- a/target/arm/monitor.c +++ b/target/arm/monitor.c @@ -21,8 +21,16 @@ */ #include "qemu/osdep.h" +#include "hw/boards.h" #include "kvm_arm.h" +#include "qapi/error.h" +#include "qapi/visitor.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-commands-machine-target.h" #include "qapi/qapi-commands-misc-target.h" +#include "qapi/qmp/qerror.h" +#include "qapi/qmp/qdict.h" +#include "qom/qom-qobject.h" static GICCapability *gic_cap_new(int version) { @@ -81,3 +89,140 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) return head; } + +/* + * These are cpu model features we want to advertise. The order here + * matters as this is the order in which qmp_query_cpu_model_expansion + * will attempt to set them. If there are dependencies between features, + * then the order that considers those dependencies must be used. + */ +static const char *cpu_model_advertised_features[] = { + "aarch64", "pmu", + NULL +}; + +CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, + CpuModelInfo *model, + Error **errp) +{ + CpuModelExpansionInfo *expansion_info; + const QDict *qdict_in = NULL; + QDict *qdict_out; + ObjectClass *oc; + Object *obj; + const char *name; + int i; + + if (type != CPU_MODEL_EXPANSION_TYPE_FULL) { + error_setg(errp, "The requested expansion type is not supported"); + return NULL; + } + + if (!kvm_enabled() && !strcmp(model->name, "host")) { + error_setg(errp, "The CPU type '%s' requires KVM", model->name); + return NULL; + } + + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); + if (!oc) { + error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU type", + model->name); + return NULL; + } + + if (kvm_enabled()) { + const char *cpu_type = current_machine->cpu_type; + int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); + bool supported = false; + + if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) { + /* These are kvmarm's recommended cpu types */ + supported = true; + } else if (strlen(model->name) == len && + !strncmp(model->name, cpu_type, len)) { + /* KVM is enabled and we're using this type, so it works. */ + supported = true; + } + if (!supported) { + error_setg(errp, "We cannot guarantee the CPU type '%s' works " + "with KVM on this host", model->name); + return NULL; + } + } + + if (model->props) { + qdict_in = qobject_to(QDict, model->props); + if (!qdict_in) { + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict"); + return NULL; + } + } + + obj = object_new(object_class_get_name(oc)); + + if (qdict_in) { + Visitor *visitor; + Error *err = NULL; + + visitor = qobject_input_visitor_new(model->props); + visit_start_struct(visitor, NULL, NULL, 0, &err); + if (err) { + object_unref(obj); + error_propagate(errp, err); + return NULL; + } + + i = 0; + while ((name = cpu_model_advertised_features[i++]) != NULL) { + if (qdict_get(qdict_in, name)) { + object_property_set(obj, visitor, name, &err); + if (err) { + break; + } + } + } + + if (!err) { + visit_check_struct(visitor, &err); + } + visit_end_struct(visitor, NULL); + visit_free(visitor); + if (err) { + object_unref(obj); + error_propagate(errp, err); + return NULL; + } + } + + expansion_info = g_new0(CpuModelExpansionInfo, 1); + expansion_info->model = g_malloc0(sizeof(*expansion_info->model)); + expansion_info->model->name = g_strdup(model->name); + + qdict_out = qdict_new(); + + i = 0; + while ((name = cpu_model_advertised_features[i++]) != NULL) { + ObjectProperty *prop = object_property_find(obj, name, NULL); + if (prop) { + Error *err = NULL; + QObject *value; + + assert(prop->get); + value = object_property_get_qobject(obj, name, &err); + assert(!err); + + qdict_put_obj(qdict_out, name, value); + } + } + + if (!qdict_size(qdict_out)) { + qobject_unref(qdict_out); + } else { + expansion_info->model->props = QOBJECT(qdict_out); + expansion_info->model->has_props = true; + } + + object_unref(obj); + + return expansion_info; +}