| Message ID | 1425402380-10488-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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>
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
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?
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
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);
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(-)