| Message ID | 20190621163422.6127-4-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm/kvm: enable SVE in guests | expand |
Hi Drew, On 6/21/19 6:34 PM, Andrew Jones 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. Also, > for simplicity, we restrict the list of queryable cpu models to 'max', > 'host', or the current type when KVM is in use, even though there > may exist KVM hosts where other types would also work. For example on a > seattle you could use 'host' for the current type, but then attempt to > query 'cortex-a57', which is also a valid CPU type to use with KVM on > seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not > yet have a "base" CPU type. Below are some example calls and results > (to save character clutter they're not in json, but are still json-ish > to give the idea) > > # expand the 'max' CPU model > query-cpu-model-expansion: type:full, model:{ name:max } > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} > > # attempt to expand the 'max' CPU model with pmu=off > query-cpu-model-expansion: > type:full, model:{ name:max, props:{ 'pmu': false }} > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} > > # attempt to expand the 'max' CPU model with aarch64=off > query-cpu-model-expansion: > type:full, model:{ name:max, props:{ 'aarch64': false }} > > error: "'aarch64' feature cannot be disabled unless KVM is enabled > and 32-bit EL1 is supported" > > In the last example KVM was not in use so an error was returned. > > Note1: 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. > > Note2: Certainly more features may be added to the list of > advertised features, e.g. 'vfp' and 'neon'. The only requirement > is that their property set accessors fail when invalid > configurations are detected. For vfp we would need something like > > set_vfp() > { > if (arm_feature(env, ARM_FEATURE_AARCH64) && > cpu->has_vfp != cpu->has_neon) > error("AArch64 CPUs must have both VFP and Neon or neither") > > in its set accessor, and the same for neon, rather than doing that > check at realize time, which isn't executed at qmp query time. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > qapi/target.json | 6 +- > target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 135 insertions(+), 3 deletions(-) > > diff --git a/qapi/target.json b/qapi/target.json > index 1d4d54b6002e..edfa2f82b916 100644 > --- a/qapi/target.json > +++ b/qapi/target.json > @@ -408,7 +408,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: > @@ -433,7 +433,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 > @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 > --- a/target/arm/monitor.c > +++ b/target/arm/monitor.c > @@ -23,7 +23,13 @@ > #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-target.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/qmp/qdict.h" > +#include "qom/qom-qobject.h" > > static GICCapability *gic_cap_new(int version) > { > @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > > return head; > } > + > +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 definition '%s' requires KVM", model->name); > + return NULL; > + } > + > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > + if (!oc) { > + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " use model name instead of CPU definition? > + "be used with KVM on this host", model->name); According to your commit mesg doesn't it mean that we fall into the simplification you mentionned and not necessarily that the model name cannot be used along with KVM? > seattle you could use 'host' for the current type, but then attempt to > query 'cortex-a57', which is also a valid CPU type to use with KVM on > seattle hosts, but that query will fail with our simplifications > + 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; > + > + visitor = qobject_input_visitor_new(model->props); > + visit_start_struct(visitor, NULL, NULL, 0, errp); > + if (*errp) { Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) Maybe you can guarantee that errp isn't NULL but ... > + object_unref(obj); > + return NULL; > + } > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + if (qdict_get(qdict_in, name)) { > + object_property_set(obj, visitor, name, errp); > + if (*errp) {> + break; I don't really get why we don't continue here instead of break. I see that later we read the props back and populate the qdict_out object > + } > + } > + } > + > + if (!*errp) {> + visit_check_struct(visitor, errp); > + } > + visit_end_struct(visitor, NULL); > + visit_free(visitor); > + if (*errp) { > + object_unref(obj); > + 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) { > + QObject *value; > + > + assert(prop->get); > + value = object_property_get_qobject(obj, name, errp); > + assert(!*errp); > + > + 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; > +} > Thanks Eric
On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: > Hi Drew, > > On 6/21/19 6:34 PM, Andrew Jones 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. Also, > > for simplicity, we restrict the list of queryable cpu models to 'max', > > 'host', or the current type when KVM is in use, even though there > > may exist KVM hosts where other types would also work. For example on a > > seattle you could use 'host' for the current type, but then attempt to > > query 'cortex-a57', which is also a valid CPU type to use with KVM on > > seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not > > yet have a "base" CPU type. Below are some example calls and results > > (to save character clutter they're not in json, but are still json-ish > > to give the idea) > > > > # expand the 'max' CPU model > > query-cpu-model-expansion: type:full, model:{ name:max } > > > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} > > > > # attempt to expand the 'max' CPU model with pmu=off > > query-cpu-model-expansion: > > type:full, model:{ name:max, props:{ 'pmu': false }} > > > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} > > > > # attempt to expand the 'max' CPU model with aarch64=off > > query-cpu-model-expansion: > > type:full, model:{ name:max, props:{ 'aarch64': false }} > > > > error: "'aarch64' feature cannot be disabled unless KVM is enabled > > and 32-bit EL1 is supported" > > > > In the last example KVM was not in use so an error was returned. > > > > Note1: 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. > > > > Note2: Certainly more features may be added to the list of > > advertised features, e.g. 'vfp' and 'neon'. The only requirement > > is that their property set accessors fail when invalid > > configurations are detected. For vfp we would need something like > > > > set_vfp() > > { > > if (arm_feature(env, ARM_FEATURE_AARCH64) && > > cpu->has_vfp != cpu->has_neon) > > error("AArch64 CPUs must have both VFP and Neon or neither") > > > > in its set accessor, and the same for neon, rather than doing that > > check at realize time, which isn't executed at qmp query time. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > qapi/target.json | 6 +- > > target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 135 insertions(+), 3 deletions(-) > > > > diff --git a/qapi/target.json b/qapi/target.json > > index 1d4d54b6002e..edfa2f82b916 100644 > > --- a/qapi/target.json > > +++ b/qapi/target.json > > @@ -408,7 +408,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: > > @@ -433,7 +433,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 > > @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 > > --- a/target/arm/monitor.c > > +++ b/target/arm/monitor.c > > @@ -23,7 +23,13 @@ > > #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-target.h" > > +#include "qapi/qmp/qerror.h" > > +#include "qapi/qmp/qdict.h" > > +#include "qom/qom-qobject.h" > > > > static GICCapability *gic_cap_new(int version) > > { > > @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > > > > return head; > > } > > + > > +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 definition '%s' requires KVM", model->name); > > + return NULL; > > + } > > + > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > > + if (!oc) { > > + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " > use model name instead of CPU definition? I took that wording from s390x, but maybe I prefer "The CPU type..." better. I'll change it for v3. > > + "be used with KVM on this host", model->name); > > According to your commit mesg doesn't it mean that we fall into the > simplification you mentionned and not necessarily that the model name > cannot be used along with KVM? There's no way to know that. The simplification is meant to avoid having to know which models will work with KVM, because most don't, but some do. Can you suggest wording you'd prefer if you don't want to make the error message so absolute? I think I prefer keeping it simple like this and just saying it doesn't work. > > > seattle you could use 'host' for the current type, but then attempt to > > query 'cortex-a57', which is also a valid CPU type to use with KVM on > > seattle hosts, but that query will fail with our simplifications > > + 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; > > + > > + visitor = qobject_input_visitor_new(model->props); > > + visit_start_struct(visitor, NULL, NULL, 0, errp); > > + if (*errp) { > Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h > I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) > Maybe you can guarantee that errp isn't NULL but ... Yeah, I know about the errp NULL thing, which is why I use local_err elsewhere. I decided to follow s390x here though because I'm guessing our QMP function will never be called with a NULL errp, it just wouldn't work that way. Would you be satisfied with an assert(errp) at the top of the function? Or should I switch all these to local_err and then propagate? > > + object_unref(obj); > > + return NULL; > > + } > > + > > + i = 0; > > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > > + if (qdict_get(qdict_in, name)) { > > + object_property_set(obj, visitor, name, errp); > > + if (*errp) {> + break; > I don't really get why we don't continue here instead of break. I see > that later we read the props back and populate the qdict_out object If we get an error here we're done and want to report it. If we continued we'd lose that error with the next object_property_set() call. See a few lines below where we free memory and return NULL due to this error. > > + } > > + } > > + } > > + > > + if (!*errp) {> + visit_check_struct(visitor, errp); > > + } > > + visit_end_struct(visitor, NULL); > > + visit_free(visitor); > > + if (*errp) { > > + object_unref(obj); > > + 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) { > > + QObject *value; > > + > > + assert(prop->get); > > + value = object_property_get_qobject(obj, name, errp); > > + assert(!*errp); > > + > > + 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; > > +} > > > Thanks > > Eric >
Hi Drew, On 6/26/19 3:26 PM, Andrew Jones wrote: > On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: >> Hi Drew, >> >> On 6/21/19 6:34 PM, Andrew Jones 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. Also, >>> for simplicity, we restrict the list of queryable cpu models to 'max', >>> 'host', or the current type when KVM is in use, even though there >>> may exist KVM hosts where other types would also work. For example on a >>> seattle you could use 'host' for the current type, but then attempt to >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>> seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not >>> yet have a "base" CPU type. Below are some example calls and results >>> (to save character clutter they're not in json, but are still json-ish >>> to give the idea) >>> >>> # expand the 'max' CPU model >>> query-cpu-model-expansion: type:full, model:{ name:max } >>> >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} >>> >>> # attempt to expand the 'max' CPU model with pmu=off >>> query-cpu-model-expansion: >>> type:full, model:{ name:max, props:{ 'pmu': false }} >>> >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} >>> >>> # attempt to expand the 'max' CPU model with aarch64=off >>> query-cpu-model-expansion: >>> type:full, model:{ name:max, props:{ 'aarch64': false }} >>> >>> error: "'aarch64' feature cannot be disabled unless KVM is enabled >>> and 32-bit EL1 is supported" >>> >>> In the last example KVM was not in use so an error was returned. >>> >>> Note1: 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. >>> >>> Note2: Certainly more features may be added to the list of >>> advertised features, e.g. 'vfp' and 'neon'. The only requirement >>> is that their property set accessors fail when invalid >>> configurations are detected. For vfp we would need something like >>> >>> set_vfp() >>> { >>> if (arm_feature(env, ARM_FEATURE_AARCH64) && >>> cpu->has_vfp != cpu->has_neon) >>> error("AArch64 CPUs must have both VFP and Neon or neither") >>> >>> in its set accessor, and the same for neon, rather than doing that >>> check at realize time, which isn't executed at qmp query time. >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> qapi/target.json | 6 +- >>> target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 135 insertions(+), 3 deletions(-) >>> >>> diff --git a/qapi/target.json b/qapi/target.json >>> index 1d4d54b6002e..edfa2f82b916 100644 >>> --- a/qapi/target.json >>> +++ b/qapi/target.json >>> @@ -408,7 +408,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: >>> @@ -433,7 +433,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 >>> @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 >>> --- a/target/arm/monitor.c >>> +++ b/target/arm/monitor.c >>> @@ -23,7 +23,13 @@ >>> #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-target.h" >>> +#include "qapi/qmp/qerror.h" >>> +#include "qapi/qmp/qdict.h" >>> +#include "qom/qom-qobject.h" >>> >>> static GICCapability *gic_cap_new(int version) >>> { >>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) >>> >>> return head; >>> } >>> + >>> +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 definition '%s' requires KVM", model->name); >>> + return NULL; >>> + } >>> + >>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); >>> + if (!oc) { >>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " >> use model name instead of CPU definition? > > I took that wording from s390x, but maybe I prefer "The CPU type..." > better. I'll change it for v3. This CPU type is not recognized as an ARM CPU type? > >>> + "be used with KVM on this host", model->name); >> >> According to your commit mesg doesn't it mean that we fall into the >> simplification you mentionned and not necessarily that the model name >> cannot be used along with KVM? > > There's no way to know that. The simplification is meant to avoid having > to know which models will work with KVM, because most don't, but some do. > Can you suggest wording you'd prefer if you don't want to make the error > message so absolute? I think I prefer keeping it simple like this and > just saying it doesn't work. Something like: "We cannot guarantee the CPU type %s works with KVM on this host" > >> >>> seattle you could use 'host' for the current type, but then attempt to >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>> seattle hosts, but that query will fail with our simplifications >>> + 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; >>> + >>> + visitor = qobject_input_visitor_new(model->props); >>> + visit_start_struct(visitor, NULL, NULL, 0, errp); >>> + if (*errp) { >> Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h >> I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) >> Maybe you can guarantee that errp isn't NULL but ... > > Yeah, I know about the errp NULL thing, which is why I use local_err > elsewhere. I decided to follow s390x here though because I'm guessing > our QMP function will never be called with a NULL errp, it just > wouldn't work that way. Would you be satisfied with an assert(errp) > at the top of the function? Or should I switch all these to local_err > and then propagate? well up to maintainers. If it is not that much a pain, just propagate ;-) > >>> + object_unref(obj); >>> + return NULL; >>> + } >>> + >>> + i = 0; >>> + while ((name = cpu_model_advertised_features[i++]) != NULL) { >>> + if (qdict_get(qdict_in, name)) { >>> + object_property_set(obj, visitor, name, errp); >>> + if (*errp) {> + break; >> I don't really get why we don't continue here instead of break. I see >> that later we read the props back and populate the qdict_out object > > If we get an error here we're done and want to report it. If we continued > we'd lose that error with the next object_property_set() call. See a few > lines below where we free memory and return NULL due to this error. I get it now. Thanks Eric > >>> + } >>> + } >>> + } >>> + >>> + if (!*errp) {> + visit_check_struct(visitor, errp); >>> + } >>> + visit_end_struct(visitor, NULL); >>> + visit_free(visitor); >>> + if (*errp) { >>> + object_unref(obj); >>> + 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) { >>> + QObject *value; >>> + >>> + assert(prop->get); >>> + value = object_property_get_qobject(obj, name, errp); >>> + assert(!*errp); >>> + >>> + 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; >>> +} >>> >> Thanks >> >> Eric >>
Hi, On 6/26/19 3:26 PM, Andrew Jones wrote: > On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: >> Hi Drew, >> >> On 6/21/19 6:34 PM, Andrew Jones 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. Also, >>> for simplicity, we restrict the list of queryable cpu models to 'max', >>> 'host', or the current type when KVM is in use, even though there >>> may exist KVM hosts where other types would also work. For example on a >>> seattle you could use 'host' for the current type, but then attempt to >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>> seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not >>> yet have a "base" CPU type. Below are some example calls and results >>> (to save character clutter they're not in json, but are still json-ish >>> to give the idea) >>> >>> # expand the 'max' CPU model >>> query-cpu-model-expansion: type:full, model:{ name:max } >>> >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} >>> >>> # attempt to expand the 'max' CPU model with pmu=off >>> query-cpu-model-expansion: >>> type:full, model:{ name:max, props:{ 'pmu': false }} >>> >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} >>> >>> # attempt to expand the 'max' CPU model with aarch64=off >>> query-cpu-model-expansion: >>> type:full, model:{ name:max, props:{ 'aarch64': false }} >>> >>> error: "'aarch64' feature cannot be disabled unless KVM is enabled >>> and 32-bit EL1 is supported" >>> >>> In the last example KVM was not in use so an error was returned. >>> >>> Note1: 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. >>> >>> Note2: Certainly more features may be added to the list of >>> advertised features, e.g. 'vfp' and 'neon'. The only requirement >>> is that their property set accessors fail when invalid >>> configurations are detected. For vfp we would need something like >>> >>> set_vfp() >>> { >>> if (arm_feature(env, ARM_FEATURE_AARCH64) && >>> cpu->has_vfp != cpu->has_neon) >>> error("AArch64 CPUs must have both VFP and Neon or neither") >>> >>> in its set accessor, and the same for neon, rather than doing that >>> check at realize time, which isn't executed at qmp query time. >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> qapi/target.json | 6 +- >>> target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 135 insertions(+), 3 deletions(-) >>> >>> diff --git a/qapi/target.json b/qapi/target.json >>> index 1d4d54b6002e..edfa2f82b916 100644 >>> --- a/qapi/target.json >>> +++ b/qapi/target.json >>> @@ -408,7 +408,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: >>> @@ -433,7 +433,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 >>> @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 >>> --- a/target/arm/monitor.c >>> +++ b/target/arm/monitor.c >>> @@ -23,7 +23,13 @@ >>> #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-target.h" >>> +#include "qapi/qmp/qerror.h" >>> +#include "qapi/qmp/qdict.h" >>> +#include "qom/qom-qobject.h" >>> >>> static GICCapability *gic_cap_new(int version) >>> { >>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) >>> >>> return head; >>> } >>> + >>> +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 definition '%s' requires KVM", model->name); >>> + return NULL; >>> + } >>> + >>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); >>> + if (!oc) { >>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " >> use model name instead of CPU definition? > > I took that wording from s390x, but maybe I prefer "The CPU type..." > better. I'll change it for v3. > >>> + "be used with KVM on this host", model->name); >> >> According to your commit mesg doesn't it mean that we fall into the >> simplification you mentionned and not necessarily that the model name >> cannot be used along with KVM? > > There's no way to know that. The simplification is meant to avoid having > to know which models will work with KVM, because most don't, but some do. > Can you suggest wording you'd prefer if you don't want to make the error > message so absolute? I think I prefer keeping it simple like this and > just saying it doesn't work. > >> >>> seattle you could use 'host' for the current type, but then attempt to >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>> seattle hosts, but that query will fail with our simplifications >>> + 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; >>> + >>> + visitor = qobject_input_visitor_new(model->props); >>> + visit_start_struct(visitor, NULL, NULL, 0, errp); >>> + if (*errp) { >> Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h >> I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) >> Maybe you can guarantee that errp isn't NULL but ... > > Yeah, I know about the errp NULL thing, which is why I use local_err > elsewhere. I decided to follow s390x here though because I'm guessing > our QMP function will never be called with a NULL errp, it just > wouldn't work that way. Would you be satisfied with an assert(errp) > at the top of the function? Or should I switch all these to local_err > and then propagate? > >>> + object_unref(obj); >>> + return NULL; >>> + } >>> + >>> + i = 0; >>> + while ((name = cpu_model_advertised_features[i++]) != NULL) { >>> + if (qdict_get(qdict_in, name)) { >>> + object_property_set(obj, visitor, name, errp); >>> + if (*errp) {> + break; >> I don't really get why we don't continue here instead of break. I see >> that later we read the props back and populate the qdict_out object > > If we get an error here we're done and want to report it. If we continued > we'd lose that error with the next object_property_set() call. See a few > lines below where we free memory and return NULL due to this error. By the way, if you were to use local_err, you could propagate them successively to errp and you wouldn't loose any of them. This would allow to report several errors at a time. Thanks Eric > >>> + } >>> + } >>> + } >>> + >>> + if (!*errp) {> + visit_check_struct(visitor, errp); >>> + } >>> + visit_end_struct(visitor, NULL); >>> + visit_free(visitor); >>> + if (*errp) { >>> + object_unref(obj); >>> + 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) { >>> + QObject *value; >>> + >>> + assert(prop->get); >>> + value = object_property_get_qobject(obj, name, errp); >>> + assert(!*errp); >>> + >>> + 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; >>> +} >>> >> Thanks >> >> Eric >> >
On Wed, Jul 24, 2019 at 02:51:08PM +0200, Auger Eric wrote: > Hi Drew, > > On 6/26/19 3:26 PM, Andrew Jones wrote: > > On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: > >> Hi Drew, > >> > >> On 6/21/19 6:34 PM, Andrew Jones 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. Also, > >>> for simplicity, we restrict the list of queryable cpu models to 'max', > >>> 'host', or the current type when KVM is in use, even though there > >>> may exist KVM hosts where other types would also work. For example on a > >>> seattle you could use 'host' for the current type, but then attempt to > >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on > >>> seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not > >>> yet have a "base" CPU type. Below are some example calls and results > >>> (to save character clutter they're not in json, but are still json-ish > >>> to give the idea) > >>> > >>> # expand the 'max' CPU model > >>> query-cpu-model-expansion: type:full, model:{ name:max } > >>> > >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} > >>> > >>> # attempt to expand the 'max' CPU model with pmu=off > >>> query-cpu-model-expansion: > >>> type:full, model:{ name:max, props:{ 'pmu': false }} > >>> > >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} > >>> > >>> # attempt to expand the 'max' CPU model with aarch64=off > >>> query-cpu-model-expansion: > >>> type:full, model:{ name:max, props:{ 'aarch64': false }} > >>> > >>> error: "'aarch64' feature cannot be disabled unless KVM is enabled > >>> and 32-bit EL1 is supported" > >>> > >>> In the last example KVM was not in use so an error was returned. > >>> > >>> Note1: 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. > >>> > >>> Note2: Certainly more features may be added to the list of > >>> advertised features, e.g. 'vfp' and 'neon'. The only requirement > >>> is that their property set accessors fail when invalid > >>> configurations are detected. For vfp we would need something like > >>> > >>> set_vfp() > >>> { > >>> if (arm_feature(env, ARM_FEATURE_AARCH64) && > >>> cpu->has_vfp != cpu->has_neon) > >>> error("AArch64 CPUs must have both VFP and Neon or neither") > >>> > >>> in its set accessor, and the same for neon, rather than doing that > >>> check at realize time, which isn't executed at qmp query time. > >>> > >>> Signed-off-by: Andrew Jones <drjones@redhat.com> > >>> --- > >>> qapi/target.json | 6 +- > >>> target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 135 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/qapi/target.json b/qapi/target.json > >>> index 1d4d54b6002e..edfa2f82b916 100644 > >>> --- a/qapi/target.json > >>> +++ b/qapi/target.json > >>> @@ -408,7 +408,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: > >>> @@ -433,7 +433,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 > >>> @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 > >>> --- a/target/arm/monitor.c > >>> +++ b/target/arm/monitor.c > >>> @@ -23,7 +23,13 @@ > >>> #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-target.h" > >>> +#include "qapi/qmp/qerror.h" > >>> +#include "qapi/qmp/qdict.h" > >>> +#include "qom/qom-qobject.h" > >>> > >>> static GICCapability *gic_cap_new(int version) > >>> { > >>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > >>> > >>> return head; > >>> } > >>> + > >>> +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 definition '%s' requires KVM", model->name); > >>> + return NULL; > >>> + } > >>> + > >>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > >>> + if (!oc) { > >>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " > >> use model name instead of CPU definition? > > > > I took that wording from s390x, but maybe I prefer "The CPU type..." > > better. I'll change it for v3. > This CPU type is not recognized as an ARM CPU type? That's not what this error message is stating. The CPU type may well be an ARM CPU type, but it's not one you can expect to use with KVM enabled. I currently have "The CPU type '%s' cannot " "be used with KVM on this host", model->name) queued up for v3. > > > >>> + "be used with KVM on this host", model->name); > >> > >> According to your commit mesg doesn't it mean that we fall into the > >> simplification you mentionned and not necessarily that the model name > >> cannot be used along with KVM? > > > > There's no way to know that. The simplification is meant to avoid having > > to know which models will work with KVM, because most don't, but some do. > > Can you suggest wording you'd prefer if you don't want to make the error > > message so absolute? I think I prefer keeping it simple like this and > > just saying it doesn't work. > Something like: > "We cannot guarantee the CPU type %s works with KVM on this host" OK, I can change to this one. > > > >> > >>> seattle you could use 'host' for the current type, but then attempt to > >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on > >>> seattle hosts, but that query will fail with our simplifications > >>> + 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; > >>> + > >>> + visitor = qobject_input_visitor_new(model->props); > >>> + visit_start_struct(visitor, NULL, NULL, 0, errp); > >>> + if (*errp) { > >> Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h > >> I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) > >> Maybe you can guarantee that errp isn't NULL but ... > > > > Yeah, I know about the errp NULL thing, which is why I use local_err > > elsewhere. I decided to follow s390x here though because I'm guessing > > our QMP function will never be called with a NULL errp, it just > > wouldn't work that way. Would you be satisfied with an assert(errp) > > at the top of the function? Or should I switch all these to local_err > > and then propagate? > well up to maintainers. If it is not that much a pain, just propagate ;-) > > OK, I'll just propagate. Thanks, drew
On Wed, Jul 24, 2019 at 02:55:39PM +0200, Auger Eric wrote: > Hi, > On 6/26/19 3:26 PM, Andrew Jones wrote: > > On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: > >> Hi Drew, > >> > >> On 6/21/19 6:34 PM, Andrew Jones 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. Also, > >>> for simplicity, we restrict the list of queryable cpu models to 'max', > >>> 'host', or the current type when KVM is in use, even though there > >>> may exist KVM hosts where other types would also work. For example on a > >>> seattle you could use 'host' for the current type, but then attempt to > >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on > >>> seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not > >>> yet have a "base" CPU type. Below are some example calls and results > >>> (to save character clutter they're not in json, but are still json-ish > >>> to give the idea) > >>> > >>> # expand the 'max' CPU model > >>> query-cpu-model-expansion: type:full, model:{ name:max } > >>> > >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} > >>> > >>> # attempt to expand the 'max' CPU model with pmu=off > >>> query-cpu-model-expansion: > >>> type:full, model:{ name:max, props:{ 'pmu': false }} > >>> > >>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} > >>> > >>> # attempt to expand the 'max' CPU model with aarch64=off > >>> query-cpu-model-expansion: > >>> type:full, model:{ name:max, props:{ 'aarch64': false }} > >>> > >>> error: "'aarch64' feature cannot be disabled unless KVM is enabled > >>> and 32-bit EL1 is supported" > >>> > >>> In the last example KVM was not in use so an error was returned. > >>> > >>> Note1: 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. > >>> > >>> Note2: Certainly more features may be added to the list of > >>> advertised features, e.g. 'vfp' and 'neon'. The only requirement > >>> is that their property set accessors fail when invalid > >>> configurations are detected. For vfp we would need something like > >>> > >>> set_vfp() > >>> { > >>> if (arm_feature(env, ARM_FEATURE_AARCH64) && > >>> cpu->has_vfp != cpu->has_neon) > >>> error("AArch64 CPUs must have both VFP and Neon or neither") > >>> > >>> in its set accessor, and the same for neon, rather than doing that > >>> check at realize time, which isn't executed at qmp query time. > >>> > >>> Signed-off-by: Andrew Jones <drjones@redhat.com> > >>> --- > >>> qapi/target.json | 6 +- > >>> target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 135 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/qapi/target.json b/qapi/target.json > >>> index 1d4d54b6002e..edfa2f82b916 100644 > >>> --- a/qapi/target.json > >>> +++ b/qapi/target.json > >>> @@ -408,7 +408,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: > >>> @@ -433,7 +433,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 > >>> @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 > >>> --- a/target/arm/monitor.c > >>> +++ b/target/arm/monitor.c > >>> @@ -23,7 +23,13 @@ > >>> #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-target.h" > >>> +#include "qapi/qmp/qerror.h" > >>> +#include "qapi/qmp/qdict.h" > >>> +#include "qom/qom-qobject.h" > >>> > >>> static GICCapability *gic_cap_new(int version) > >>> { > >>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > >>> > >>> return head; > >>> } > >>> + > >>> +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 definition '%s' requires KVM", model->name); > >>> + return NULL; > >>> + } > >>> + > >>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > >>> + if (!oc) { > >>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " > >> use model name instead of CPU definition? > > > > I took that wording from s390x, but maybe I prefer "The CPU type..." > > better. I'll change it for v3. > > > >>> + "be used with KVM on this host", model->name); > >> > >> According to your commit mesg doesn't it mean that we fall into the > >> simplification you mentionned and not necessarily that the model name > >> cannot be used along with KVM? > > > > There's no way to know that. The simplification is meant to avoid having > > to know which models will work with KVM, because most don't, but some do. > > Can you suggest wording you'd prefer if you don't want to make the error > > message so absolute? I think I prefer keeping it simple like this and > > just saying it doesn't work. > > > >> > >>> seattle you could use 'host' for the current type, but then attempt to > >>> query 'cortex-a57', which is also a valid CPU type to use with KVM on > >>> seattle hosts, but that query will fail with our simplifications > >>> + 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; > >>> + > >>> + visitor = qobject_input_visitor_new(model->props); > >>> + visit_start_struct(visitor, NULL, NULL, 0, errp); > >>> + if (*errp) { > >> Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h > >> I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) > >> Maybe you can guarantee that errp isn't NULL but ... > > > > Yeah, I know about the errp NULL thing, which is why I use local_err > > elsewhere. I decided to follow s390x here though because I'm guessing > > our QMP function will never be called with a NULL errp, it just > > wouldn't work that way. Would you be satisfied with an assert(errp) > > at the top of the function? Or should I switch all these to local_err > > and then propagate? > > > >>> + object_unref(obj); > >>> + return NULL; > >>> + } > >>> + > >>> + i = 0; > >>> + while ((name = cpu_model_advertised_features[i++]) != NULL) { > >>> + if (qdict_get(qdict_in, name)) { > >>> + object_property_set(obj, visitor, name, errp); > >>> + if (*errp) {> + break; > >> I don't really get why we don't continue here instead of break. I see > >> that later we read the props back and populate the qdict_out object > > > > If we get an error here we're done and want to report it. If we continued > > we'd lose that error with the next object_property_set() call. See a few > > lines below where we free memory and return NULL due to this error. > > By the way, if you were to use local_err, you could propagate them > successively to errp and you wouldn't loose any of them. This would > allow to report several errors at a time. > Interesting suggestion. I'll experiment with it, but I think the QMP error will only provide the caller the first one anyway. Thanks, drew
Hi Drew, On 7/24/19 4:05 PM, Andrew Jones wrote: > On Wed, Jul 24, 2019 at 02:51:08PM +0200, Auger Eric wrote: >> Hi Drew, >> >> On 6/26/19 3:26 PM, Andrew Jones wrote: >>> On Wed, Jun 26, 2019 at 09:43:09AM +0200, Auger Eric wrote: >>>> Hi Drew, >>>> >>>> On 6/21/19 6:34 PM, Andrew Jones 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. Also, >>>>> for simplicity, we restrict the list of queryable cpu models to 'max', >>>>> 'host', or the current type when KVM is in use, even though there >>>>> may exist KVM hosts where other types would also work. For example on a >>>>> seattle you could use 'host' for the current type, but then attempt to >>>>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>>>> seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not >>>>> yet have a "base" CPU type. Below are some example calls and results >>>>> (to save character clutter they're not in json, but are still json-ish >>>>> to give the idea) >>>>> >>>>> # expand the 'max' CPU model >>>>> query-cpu-model-expansion: type:full, model:{ name:max } >>>>> >>>>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} >>>>> >>>>> # attempt to expand the 'max' CPU model with pmu=off >>>>> query-cpu-model-expansion: >>>>> type:full, model:{ name:max, props:{ 'pmu': false }} >>>>> >>>>> return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} >>>>> >>>>> # attempt to expand the 'max' CPU model with aarch64=off >>>>> query-cpu-model-expansion: >>>>> type:full, model:{ name:max, props:{ 'aarch64': false }} >>>>> >>>>> error: "'aarch64' feature cannot be disabled unless KVM is enabled >>>>> and 32-bit EL1 is supported" >>>>> >>>>> In the last example KVM was not in use so an error was returned. >>>>> >>>>> Note1: 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. >>>>> >>>>> Note2: Certainly more features may be added to the list of >>>>> advertised features, e.g. 'vfp' and 'neon'. The only requirement >>>>> is that their property set accessors fail when invalid >>>>> configurations are detected. For vfp we would need something like >>>>> >>>>> set_vfp() >>>>> { >>>>> if (arm_feature(env, ARM_FEATURE_AARCH64) && >>>>> cpu->has_vfp != cpu->has_neon) >>>>> error("AArch64 CPUs must have both VFP and Neon or neither") >>>>> >>>>> in its set accessor, and the same for neon, rather than doing that >>>>> check at realize time, which isn't executed at qmp query time. >>>>> >>>>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>>>> --- >>>>> qapi/target.json | 6 +- >>>>> target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 135 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/qapi/target.json b/qapi/target.json >>>>> index 1d4d54b6002e..edfa2f82b916 100644 >>>>> --- a/qapi/target.json >>>>> +++ b/qapi/target.json >>>>> @@ -408,7 +408,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: >>>>> @@ -433,7 +433,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 >>>>> @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 >>>>> --- a/target/arm/monitor.c >>>>> +++ b/target/arm/monitor.c >>>>> @@ -23,7 +23,13 @@ >>>>> #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-target.h" >>>>> +#include "qapi/qmp/qerror.h" >>>>> +#include "qapi/qmp/qdict.h" >>>>> +#include "qom/qom-qobject.h" >>>>> >>>>> static GICCapability *gic_cap_new(int version) >>>>> { >>>>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) >>>>> >>>>> return head; >>>>> } >>>>> + >>>>> +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 definition '%s' requires KVM", model->name); >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); >>>>> + if (!oc) { >>>>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " >>>> use model name instead of CPU definition? >>> >>> I took that wording from s390x, but maybe I prefer "The CPU type..." >>> better. I'll change it for v3.>> This CPU type is not recognized as an ARM CPU type? > > That's not what this error message is stating. The CPU type may well be an > ARM CPU type, but it's not one you can expect to use with KVM enabled. I > currently have > > "The CPU type '%s' cannot " > "be used with KVM on this host", model->name) > > queued up for v3. decidedly, I meant the error message associated to: + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); + if (!oc) { + error_setg(errp, "The CPU definition '%s' is unknown.", model->name); + return NULL; + } Why am I always looking at your series when we suffer heat wave? Thanks Eric > >>> >>>>> + "be used with KVM on this host", model->name); >>>> >>>> According to your commit mesg doesn't it mean that we fall into the >>>> simplification you mentionned and not necessarily that the model name >>>> cannot be used along with KVM? >>> >>> There's no way to know that. The simplification is meant to avoid having >>> to know which models will work with KVM, because most don't, but some do. >>> Can you suggest wording you'd prefer if you don't want to make the error >>> message so absolute? I think I prefer keeping it simple like this and >>> just saying it doesn't work. >> Something like: >> "We cannot guarantee the CPU type %s works with KVM on this host" > > OK, I can change to this one. > >>> >>>> >>>>> seattle you could use 'host' for the current type, but then attempt to >>>>> query 'cortex-a57', which is also a valid CPU type to use with KVM on >>>>> seattle hosts, but that query will fail with our simplifications >>>>> + 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; >>>>> + >>>>> + visitor = qobject_input_visitor_new(model->props); >>>>> + visit_start_struct(visitor, NULL, NULL, 0, errp); >>>>> + if (*errp) { >>>> Normally we shouldn't do that as errp can be NULL. see /include/qapi/error.h >>>> I see the same in cpu_model_from_info() by the way (s390x/cpu_models.c) >>>> Maybe you can guarantee that errp isn't NULL but ... >>> >>> Yeah, I know about the errp NULL thing, which is why I use local_err >>> elsewhere. I decided to follow s390x here though because I'm guessing >>> our QMP function will never be called with a NULL errp, it just >>> wouldn't work that way. Would you be satisfied with an assert(errp) >>> at the top of the function? Or should I switch all these to local_err >>> and then propagate? >> well up to maintainers. If it is not that much a pain, just propagate ;-) >>> > > OK, I'll just propagate. > > Thanks, > drew >
On Wed, Jul 24, 2019 at 04:25:32PM +0200, Auger Eric wrote: > >>>>> diff --git a/target/arm/monitor.c b/target/arm/monitor.c > >>>>> index 41b32b94b258..19e3120eef95 100644 > >>>>> --- a/target/arm/monitor.c > >>>>> +++ b/target/arm/monitor.c > >>>>> @@ -23,7 +23,13 @@ > >>>>> #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-target.h" > >>>>> +#include "qapi/qmp/qerror.h" > >>>>> +#include "qapi/qmp/qdict.h" > >>>>> +#include "qom/qom-qobject.h" > >>>>> > >>>>> static GICCapability *gic_cap_new(int version) > >>>>> { > >>>>> @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > >>>>> > >>>>> return head; > >>>>> } > >>>>> + > >>>>> +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 definition '%s' requires KVM", model->name); > >>>>> + return NULL; > >>>>> + } > >>>>> + > >>>>> + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > >>>>> + if (!oc) { > >>>>> + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " > >>>> use model name instead of CPU definition? > >>> > >>> I took that wording from s390x, but maybe I prefer "The CPU type..." > >>> better. I'll change it for v3.>> This CPU type is not recognized as an ARM CPU type? > > > > That's not what this error message is stating. The CPU type may well be an > > ARM CPU type, but it's not one you can expect to use with KVM enabled. I > > currently have > > > > "The CPU type '%s' cannot " > > "be used with KVM on this host", model->name) > > > > queued up for v3. > > decidedly, I meant the error message associated to: > > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > + if (!oc) { > + error_setg(errp, "The CPU definition '%s' is unknown.", > model->name); > + return NULL; > + } Ah, OK. Yeah I can change that one too. Of course if we deviate from s390x's generic error messages for common errors, then we're assuming the messages aren't being parsed by upper layers using code that we'd like to easily adopt to ARM. But, I think that assumption is reasonable. > > Why am I always looking at your series when we suffer heat wave? > Climate change generates too many heat waves. Or I generate too much code that requires comments. Or both. Thanks, drew
Hi Drew, On 6/21/19 6:34 PM, Andrew Jones 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. Also, > for simplicity, we restrict the list of queryable cpu models to 'max', > 'host', or the current type when KVM is in use, even though there > may exist KVM hosts where other types would also work. For example on a > seattle you could use 'host' for the current type, but then attempt to > query 'cortex-a57', which is also a valid CPU type to use with KVM on > seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not > yet have a "base" CPU type. Below are some example calls and results > (to save character clutter they're not in json, but are still json-ish > to give the idea) > > # expand the 'max' CPU model > query-cpu-model-expansion: type:full, model:{ name:max } > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} > > # attempt to expand the 'max' CPU model with pmu=off > query-cpu-model-expansion: > type:full, model:{ name:max, props:{ 'pmu': false }} > > return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} > > # attempt to expand the 'max' CPU model with aarch64=off > query-cpu-model-expansion: > type:full, model:{ name:max, props:{ 'aarch64': false }} > > error: "'aarch64' feature cannot be disabled unless KVM is enabled > and 32-bit EL1 is supported" I struggled quite a lot to get the right syntax to test with qmp-shell (which I think is the most user friendly way to run those commands). In the commit message, you may add an example such as: (QEMU) query-cpu-model-expansion type=full model={"name":"host","props":{"aarch64":true,"pmu":true}} {"return": {"model": {"name": "host", "props": {"aarch64": true, "pmu": true}}}} (QEMU) query-cpu-model-expansion type=full model={"name":"host"} {"return": {"model": {"name": "host", "props": {"aarch64": true, "pmu": true}}}} Thanks Eric > > In the last example KVM was not in use so an error was returned. > > Note1: 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. > > Note2: Certainly more features may be added to the list of > advertised features, e.g. 'vfp' and 'neon'. The only requirement > is that their property set accessors fail when invalid > configurations are detected. For vfp we would need something like > > set_vfp() > { > if (arm_feature(env, ARM_FEATURE_AARCH64) && > cpu->has_vfp != cpu->has_neon) > error("AArch64 CPUs must have both VFP and Neon or neither") > > in its set accessor, and the same for neon, rather than doing that > check at realize time, which isn't executed at qmp query time. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > qapi/target.json | 6 +- > target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 135 insertions(+), 3 deletions(-) > > diff --git a/qapi/target.json b/qapi/target.json > index 1d4d54b6002e..edfa2f82b916 100644 > --- a/qapi/target.json > +++ b/qapi/target.json > @@ -408,7 +408,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: > @@ -433,7 +433,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 > @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 > --- a/target/arm/monitor.c > +++ b/target/arm/monitor.c > @@ -23,7 +23,13 @@ > #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-target.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/qmp/qdict.h" > +#include "qom/qom-qobject.h" > > static GICCapability *gic_cap_new(int version) > { > @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) > > return head; > } > + > +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 definition '%s' requires KVM", model->name); > + return NULL; > + } > + > + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); > + if (!oc) { > + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " > + "be used 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; > + > + visitor = qobject_input_visitor_new(model->props); > + visit_start_struct(visitor, NULL, NULL, 0, errp); > + if (*errp) { > + object_unref(obj); > + return NULL; > + } > + > + i = 0; > + while ((name = cpu_model_advertised_features[i++]) != NULL) { > + if (qdict_get(qdict_in, name)) { > + object_property_set(obj, visitor, name, errp); > + if (*errp) { > + break; > + } > + } > + } > + > + if (!*errp) { > + visit_check_struct(visitor, errp); > + } > + visit_end_struct(visitor, NULL); > + visit_free(visitor); > + if (*errp) { > + object_unref(obj); > + 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) { > + QObject *value; > + > + assert(prop->get); > + value = object_property_get_qobject(obj, name, errp); > + assert(!*errp); > + > + 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; > +} >
diff --git a/qapi/target.json b/qapi/target.json index 1d4d54b6002e..edfa2f82b916 100644 --- a/qapi/target.json +++ b/qapi/target.json @@ -408,7 +408,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: @@ -433,7 +433,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 @@ -447,7 +447,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 41b32b94b258..19e3120eef95 100644 --- a/target/arm/monitor.c +++ b/target/arm/monitor.c @@ -23,7 +23,13 @@ #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-target.h" +#include "qapi/qmp/qerror.h" +#include "qapi/qmp/qdict.h" +#include "qom/qom-qobject.h" static GICCapability *gic_cap_new(int version) { @@ -82,3 +88,129 @@ GICCapabilityList *qmp_query_gic_capabilities(Error **errp) return head; } + +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 definition '%s' requires KVM", model->name); + return NULL; + } + + oc = cpu_class_by_name(TYPE_ARM_CPU, model->name); + if (!oc) { + error_setg(errp, "The CPU definition '%s' is unknown.", 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, "The CPU definition '%s' cannot " + "be used 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; + + visitor = qobject_input_visitor_new(model->props); + visit_start_struct(visitor, NULL, NULL, 0, errp); + if (*errp) { + object_unref(obj); + return NULL; + } + + i = 0; + while ((name = cpu_model_advertised_features[i++]) != NULL) { + if (qdict_get(qdict_in, name)) { + object_property_set(obj, visitor, name, errp); + if (*errp) { + break; + } + } + } + + if (!*errp) { + visit_check_struct(visitor, errp); + } + visit_end_struct(visitor, NULL); + visit_free(visitor); + if (*errp) { + object_unref(obj); + 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) { + QObject *value; + + assert(prop->get); + value = object_property_get_qobject(obj, name, errp); + assert(!*errp); + + 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; +}
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. Also, for simplicity, we restrict the list of queryable cpu models to 'max', 'host', or the current type when KVM is in use, even though there may exist KVM hosts where other types would also work. For example on a seattle you could use 'host' for the current type, but then attempt to query 'cortex-a57', which is also a valid CPU type to use with KVM on seattle hosts, but that query will fail with our simplifications. 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. Finally, we only implement expansion type 'full', as Arm does not yet have a "base" CPU type. Below are some example calls and results (to save character clutter they're not in json, but are still json-ish to give the idea) # expand the 'max' CPU model query-cpu-model-expansion: type:full, model:{ name:max } return: model:{ name:max, props:{ 'aarch64': true, 'pmu': true }} # attempt to expand the 'max' CPU model with pmu=off query-cpu-model-expansion: type:full, model:{ name:max, props:{ 'pmu': false }} return: model:{ name:max, props:{ 'aarch64': true, 'pmu': false }} # attempt to expand the 'max' CPU model with aarch64=off query-cpu-model-expansion: type:full, model:{ name:max, props:{ 'aarch64': false }} error: "'aarch64' feature cannot be disabled unless KVM is enabled and 32-bit EL1 is supported" In the last example KVM was not in use so an error was returned. Note1: 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. Note2: Certainly more features may be added to the list of advertised features, e.g. 'vfp' and 'neon'. The only requirement is that their property set accessors fail when invalid configurations are detected. For vfp we would need something like set_vfp() { if (arm_feature(env, ARM_FEATURE_AARCH64) && cpu->has_vfp != cpu->has_neon) error("AArch64 CPUs must have both VFP and Neon or neither") in its set accessor, and the same for neon, rather than doing that check at realize time, which isn't executed at qmp query time. Signed-off-by: Andrew Jones <drjones@redhat.com> --- qapi/target.json | 6 +- target/arm/monitor.c | 132 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-)