[Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Ard Biesheuvel]

The recently introduced feature that allows 32 bit guests to be
executed under KVM on a 64-bit host incorrectly handles the case
where more than 1 cpu is specified using '-smp N'

For instance, this invocation of qemu

  qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off -smp 2

produces the following error

  qemu-system-aarch64: Expected key=value format, found aarch64

which is caused by the destructive parsing performed by
cpu_common_parse_features(), resulting in subsequent attempts
to parse the CPU option string (for each additional CPU) to fail.

So duplicate the string before parsing it, and free it directly
afterwards.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/virt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 69f51ac0da58..f8a6c46323dc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -758,6 +758,7 @@ static void machvirt_init(MachineState *machine)
         CPUClass *cc = CPU_CLASS(oc);
         Object *cpuobj;
         Error *err = NULL;
+        char *cpuopts = g_strdup(cpustr[1]);
 
         if (!oc) {
             fprintf(stderr, "Unable to find CPU definition\n");
@@ -766,7 +767,8 @@ static void machvirt_init(MachineState *machine)
         cpuobj = object_new(object_class_get_name(oc));
 
         /* Handle any CPU options specified by the user */
-        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
+        cc->parse_features(CPU(cpuobj), cpuopts, &err);
+        g_free(cpuopts);
         if (err) {
             error_report("%s", error_get_pretty(err));
             exit(1);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Greg Bellows]

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

On Tue, Mar 3, 2015 at 11:06 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> The recently introduced feature that allows 32 bit guests to be
> executed under KVM on a 64-bit host incorrectly handles the case
> where more than 1 cpu is specified using '-smp N'
>
> For instance, this invocation of qemu
>
>   qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off -smp 2
>
> produces the following error
>
>   qemu-system-aarch64: Expected key=value format, found aarch64
>
> which is caused by the destructive parsing performed by
> cpu_common_parse_features(), resulting in subsequent attempts
> to parse the CPU option string (for each additional CPU) to fail.
>
> So duplicate the string before parsing it, and free it directly
> afterwards.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/virt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 69f51ac0da58..f8a6c46323dc 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -758,6 +758,7 @@ static void machvirt_init(MachineState *machine)
>          CPUClass *cc = CPU_CLASS(oc);
>          Object *cpuobj;
>          Error *err = NULL;
> +        char *cpuopts = g_strdup(cpustr[1]);
>
>          if (!oc) {
>              fprintf(stderr, "Unable to find CPU definition\n");
> @@ -766,7 +767,8 @@ static void machvirt_init(MachineState *machine)
>          cpuobj = object_new(object_class_get_name(oc));
>
>          /* Handle any CPU options specified by the user */
> -        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
> +        cc->parse_features(CPU(cpuobj), cpuopts, &err);
> +        g_free(cpuopts);
>          if (err) {
>              error_report("%s", error_get_pretty(err));
>              exit(1);
> --
> 1.8.3.2
>
> ​​
​Saw your patch after I sent mine out, roughly same fix...

Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

[-- Attachment #2: Type: text/html, Size: 3137 bytes --]

Re: [Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Ard Biesheuvel]

On 3 March 2015 at 19:25, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Tue, Mar 3, 2015 at 11:06 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
>>
>> The recently introduced feature that allows 32 bit guests to be
>> executed under KVM on a 64-bit host incorrectly handles the case
>> where more than 1 cpu is specified using '-smp N'
>>
>> For instance, this invocation of qemu
>>
>>   qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off -smp 2
>>

This is incorrect: it is the command line I used for reproducing the
issue while working on the bug, after removing the KVM check.
So this should read

>>   qemu-system-aarch64 -M virt -enable-kvm -cpu host,aarch64=off -smp 2


>> produces the following error
>>
>>   qemu-system-aarch64: Expected key=value format, found aarch64
>>
>> which is caused by the destructive parsing performed by
>> cpu_common_parse_features(), resulting in subsequent attempts
>> to parse the CPU option string (for each additional CPU) to fail.
>>
>> So duplicate the string before parsing it, and free it directly
>> afterwards.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  hw/arm/virt.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 69f51ac0da58..f8a6c46323dc 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -758,6 +758,7 @@ static void machvirt_init(MachineState *machine)
>>          CPUClass *cc = CPU_CLASS(oc);
>>          Object *cpuobj;
>>          Error *err = NULL;
>> +        char *cpuopts = g_strdup(cpustr[1]);
>>
>>          if (!oc) {
>>              fprintf(stderr, "Unable to find CPU definition\n");
>> @@ -766,7 +767,8 @@ static void machvirt_init(MachineState *machine)
>>          cpuobj = object_new(object_class_get_name(oc));
>>
>>          /* Handle any CPU options specified by the user */
>> -        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
>> +        cc->parse_features(CPU(cpuobj), cpuopts, &err);
>> +        g_free(cpuopts);
>>          if (err) {
>>              error_report("%s", error_get_pretty(err));
>>              exit(1);
>> --
>> 1.8.3.2
>>
> Saw your patch after I sent mine out, roughly same fix...
>
> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>
>

Thanks.

Let's leave it up to Peter which version he is most happy with

-- 
Ard.

Re: [Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Ard Biesheuvel]

On 4 March 2015 at 08:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 3 March 2015 at 19:25, Greg Bellows <greg.bellows@linaro.org> wrote:
>>
>>
>> On Tue, Mar 3, 2015 at 11:06 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> wrote:
>>>
>>> The recently introduced feature that allows 32 bit guests to be
>>> executed under KVM on a 64-bit host incorrectly handles the case
>>> where more than 1 cpu is specified using '-smp N'
>>>
>>> For instance, this invocation of qemu
>>>
>>>   qemu-system-aarch64 -M virt -cpu cortex-a57,aarch64=off -smp 2
>>>
>
> This is incorrect: it is the command line I used for reproducing the
> issue while working on the bug, after removing the KVM check.
> So this should read
>
>>>   qemu-system-aarch64 -M virt -enable-kvm -cpu host,aarch64=off -smp 2
>
>
>>> produces the following error
>>>
>>>   qemu-system-aarch64: Expected key=value format, found aarch64
>>>
>>> which is caused by the destructive parsing performed by
>>> cpu_common_parse_features(), resulting in subsequent attempts
>>> to parse the CPU option string (for each additional CPU) to fail.
>>>
>>> So duplicate the string before parsing it, and free it directly
>>> afterwards.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  hw/arm/virt.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 69f51ac0da58..f8a6c46323dc 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -758,6 +758,7 @@ static void machvirt_init(MachineState *machine)
>>>          CPUClass *cc = CPU_CLASS(oc);
>>>          Object *cpuobj;
>>>          Error *err = NULL;
>>> +        char *cpuopts = g_strdup(cpustr[1]);
>>>
>>>          if (!oc) {
>>>              fprintf(stderr, "Unable to find CPU definition\n");
>>> @@ -766,7 +767,8 @@ static void machvirt_init(MachineState *machine)
>>>          cpuobj = object_new(object_class_get_name(oc));
>>>
>>>          /* Handle any CPU options specified by the user */
>>> -        cc->parse_features(CPU(cpuobj), cpustr[1], &err);
>>> +        cc->parse_features(CPU(cpuobj), cpuopts, &err);
>>> +        g_free(cpuopts);
>>>          if (err) {
>>>              error_report("%s", error_get_pretty(err));
>>>              exit(1);
>>> --
>>> 1.8.3.2
>>>
>> Saw your patch after I sent mine out, roughly same fix...
>>
>> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>
>>
>
> Thanks.
>
> Let's leave it up to Peter which version he is most happy with
>

Ping?

Re: [Qemu-devel] [PATCH] hw/arm/virt: fix cmdline parsing bug with CPU options and smp > 1

[Peter Maydell]

On 10 March 2015 at 09:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Ping?

Applied this one to target-arm.next, thanks.

(I've been on holiday and not doing patch review or target-arm
subtree handling, but am back at work now...)

-- PMM