[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/14] target/arm/monitor: Introdu
From: |
Auger Eric |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/14] target/arm/monitor: Introduce qmp_query_cpu_model_expansion |
Date: |
Wed, 24 Jul 2019 14:51:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
>>> ---
>>> 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
>>
- Re: [Qemu-arm] [Qemu-devel] [PATCH v2 03/14] target/arm/monitor: Introduce qmp_query_cpu_model_expansion,
Auger Eric <=