Re: [PATCH v3] target/arm/monitor: query-cpu-model-expansion crashed qemu when using machine type none

[Andrew Jones]

On Fri, Jan 31, 2020 at 10:46:49PM -0500, Liang Yan wrote:
> Commit e19afd56 mentioned that target-arm only supports queryable

Please use more hexdigits. I'm not sure QEMU has a policy for that,
but I'd go with 12.

> cpu models 'max', 'host', and the current type when KVM is in use.
> The logic works well until using machine type none.
> 
> For machine type none, cpu_type will be null if cpu option is not
> set by command line, strlen(cpu_type) will terminate process.
> So We add a check above it.
> 
> This won't affect i386 and s390x since they do not use current_cpu.
> 
> Signed-off-by: Liang Yan <address@hidden>
> ---
>  v3: change git commit message
>  v2: fix code style issue
> ---
>  target/arm/monitor.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index 9725dfff16..3350cd65d0 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -137,17 +137,19 @@ CpuModelExpansionInfo 
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      }
>  
>      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;
> +        } else if (current_machine->cpu_type) {
> +            const char *cpu_type = current_machine->cpu_type;
> +            int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);

Need a blank line here.

> +            if (strlen(model->name) == len &&
> +                    !strncmp(model->name, cpu_type, len)) {

Four spaces of indent too many on the line above.

> +                /* KVM is enabled and we're using this type, so it works. */
> +                supported = true;
> +            }
>          }
>          if (!supported) {
>              error_setg(errp, "We cannot guarantee the CPU type '%s' works "
> -- 
> 2.25.0
> 
>

With the three changes above


Reviewed-by: Andrew Jones <address@hidden>
Tested-by: Andrew Jones <address@hidden>


It'd be nice to extend tests/qtest/arm-cpu-features.c to also do
some checks with machine='none' with and without KVM.

Thanks,
drew