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

[Liang Yan]

On 2/3/20 8:08 AM, Andrew Jones wrote:
> 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>
> 

Thanks for the review, I will update soon.

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

Will do it later, thanks for the suggestion.

Best,
Liang

> Thanks,
> drew
>