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

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

Posted by Liang Yan 5 years, 9 months ago

Commit e19afd56 mentioned that target-arm only supports queryable
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 <lyan@suse.com>
---
 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);
+            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, "We cannot guarantee the CPU type '%s' works "
-- 
2.25.0

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

Posted by Andrew Jones 5 years, 9 months ago

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 <lyan@suse.com>
> ---
>  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 <drjones@redhat.com>
Tested-by: Andrew Jones <drjones@redhat.com>


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

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

Posted by Liang Yan 5 years, 9 months ago


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 <lyan@suse.com>
>> ---
>>  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 <drjones@redhat.com>
> Tested-by: Andrew Jones <drjones@redhat.com>
> 

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
>