Re: [Qemu-devel] [PATCH 4/4] x86: Support feature=force on the command-line

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On 05/02/2017 03:31 PM, Eduardo Habkost wrote:
> Introduce a new CPUFeatureSetting QAPI data type, and use it to support
> feature=force on -cpu.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qapi-schema.json              | 32 +++++++++++++++++++++++++
>  target/i386/cpu.h             |  2 ++
>  target/i386/cpu.c             | 55 +++++++++++++++++++++++++++++++++----------
>  tests/test-x86-cpuid-compat.c | 14 ++++++++++-
>  4 files changed, 90 insertions(+), 13 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 01b087fa16..d716409114 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4250,6 +4250,38 @@
>  { 'command': 'query-machines', 'returns': ['MachineInfo'] }
>  
>  ##
> +# @CPUFeatureSettingEnum:
> +#
> +# Additional valid values for a CPUFeatureSetting property.
> +#
> +# @force: Force feature to be enabled, even if the accelerator
> +#         reports the feature as unavailable. Should be used only
> +#         for testing or debugging purposes.
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'CPUFeatureSettingEnum',
> +  'data': ['force'] }
> +
> +##
> +# @CPUFeatureSetting:
> +#
> +# Values for a CPU feature property.
> +#
> +# @bool: If false, the feature is forcibly disabled.
> +#        If true, QEMU will try to enable the feature. QEMU will
> +#        refuse to start if the feature is unavailable and
> +#        'enforce' mode is enabled in the CPU.
> +#
> +# @enum: See @CPUFeatureSettingEnum.
> +#
> +# Since: 2.10
> +##
> +{ 'alternate': 'CPUFeatureSetting',
> +  'data': { 'bool': 'bool',
> +            'enum': 'CPUFeatureSettingEnum' } }
> +

Looks reasonable; I'm glad the suggestion for using an alternate worked.


> -    if (value) {
> -        cpu->env.features[fp->w] |= fp->mask;
> -    } else {
> -        cpu->env.features[fp->w] &= ~fp->mask;
> +    switch (value->type) {
> +    case QTYPE_QBOOL:
> +        if (value->u.q_bool) {
> +            cpu->env.features[fp->w] |= fp->mask;
> +        } else {
> +            cpu->env.features[fp->w] &= ~fp->mask;
> +        }
> +        cpu->env.forced_features[fp->w] &= ~fp->mask;
> +        cpu->env.user_features[fp->w] |= fp->mask;
> +    break;

Isn't the break supposed to be indented four more spaces?

> +    case QTYPE_QSTRING:
> +        switch (value->u.q_enum) {
> +        case CPU_FEATURE_SETTING_ENUM_FORCE:
> +            cpu->env.features[fp->w] |= fp->mask;
> +            cpu->env.forced_features[fp->w] |= fp->mask;
> +        break;

and again

> +        default:
> +            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name,
> +                       "CPUFeatureSetting");
> +        }
> +    break;

and again

> +    default:
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name,
> +                   "CPUFeatureSetting");
>      }
> -    cpu->env.user_features[fp->w] |= fp->mask;
> +
> +    qapi_free_CPUFeatureSetting(value);
>  }
>  

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]