target-openrisc hw/openrisc: Some OpenRISC fix.

PATCH Superseded Jia Liu 4 participants 25 messages Jul 22, 2013

Share & tools

Jia Liu

author

2013-07-22 08:56 UTC #0 lore raw

v3 cover target-openrisc hw/openrisc: Some OpenRISC fix.

Fix OpenRISC CPU and sim broad, we should free typename and check cpu models by name in cpu.c, and we should use stderr output in openrisc_sim.c.

Updates from V3:
* Replace a typo fix using a stderr fix.

Updates from V2:
* Combine typo patchset and typename free.
* Add cpu_model by name fix, make any and or1200 works both OK.

Jia Liu (4):
hw/openrisc: Indent typo
hw/openrisc: Use stderr output instead of qemu_log
target-openrisc: Free typename
target-openrisc: Fix cpu_model by name

 hw/openrisc/openrisc_sim.c |  6 +++---
 target-openrisc/cpu.c      | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 7 deletions(-)

Signature · 1 line

1.7.12.4 (Apple Git-37)

Jia Liu

author

2013-07-22 08:56 UTC #1 lore raw

PATCH v3 1/4 hw/openrisc: Indent typo

Indent typo.

Signed-off-by Jia Liu <[email protected]>

---
 hw/openrisc/openrisc_sim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/openrisc/openrisc_sim.c +1 -1

index 924438b..250f5b5 100644
@@ -96,7 +96,7 @@ static void openrisc_sim_init(QEMUMachineInitArgs *args)
     ram_addr_t ram_size = args->ram_size;
     const char *cpu_model = args->cpu_model;
     const char *kernel_filename = args->kernel_filename;
-   OpenRISCCPU *cpu = NULL;
+    OpenRISCCPU *cpu = NULL;
     MemoryRegion *ram;
     int n;

Signature · 1 line

1.7.12.4 (Apple Git-37)

Peter Maydell

Reviewed-by on v3 1/4

2013-07-22 13:07 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v3 1/4] hw/openrisc: Indent typo

Quoted 3 lines · Jia Liu

Indent typo.
Signed-off-by Jia Liu <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

-- PMM

Jia Liu

author

2013-07-22 08:56 UTC #3 lore raw

PATCH v3 2/4 hw/openrisc: Use stderr output instead of qemu_log

In reply to Jia Liu

We should use stderr output instead of qemu_log in order to output ErrMsg onto the screen.

Signed-off-by Jia Liu <[email protected]>

---
 hw/openrisc/openrisc_sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

hw/openrisc/openrisc_sim.c +2 -2

index 250f5b5..a08f27c 100644
@@ -82,7 +82,7 @@ static void cpu_openrisc_load_kernel(ram_addr_t ram_size,
         }
 
         if (kernel_size < 0) {
-            qemu_log("QEMU: couldn't load the kernel '%s'\n",
+            fprintf(stderr, "QEMU: couldn't load the kernel '%s'\n",
                     kernel_filename);
             exit(1);
         }

@@ -107,7 +107,7 @@ static void openrisc_sim_init(QEMUMachineInitArgs *args)
     for (n = 0; n < smp_cpus; n++) {
         cpu = cpu_openrisc_init(cpu_model);
         if (cpu == NULL) {
-            qemu_log("Unable to find CPU definition!\n");
+            fprintf(stderr, "Unable to find CPU definition!\n");
             exit(1);
         }
         qemu_register_reset(main_cpu_reset, cpu);

Signature · 1 line

1.7.12.4 (Apple Git-37)

Peter Maydell

Reviewed-by on v3 2/4

2013-07-22 09:37 UTC #4 lore raw

Re: [Qemu-devel] [PATCH v3 2/4] hw/openrisc: Use stderr output instead of qemu_log

Quoted 4 lines · Jia Liu

We should use stderr output instead of qemu_log in order to output ErrMsg onto the screen.
Signed-off-by Jia Liu <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

-- PMM

Jia Liu

author

2013-07-22 08:56 UTC #5 lore raw

PATCH v3 3/4 target-openrisc: Free typename

In reply to Jia Liu

We should free typename here.

Signed-off-by Jia Liu <[email protected]>

---
 target-openrisc/cpu.c | 1 +
 1 file changed, 1 insertion(+)

target-openrisc/cpu.c +1 -0

index 6d40f1b..e348df0 100644
@@ -99,6 +99,7 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
 
     typename = g_strdup_printf("%s-" TYPE_OPENRISC_CPU, cpu_model);
     oc = object_class_by_name(typename);
+    g_free(typename);
     if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
                        object_class_is_abstract(oc))) {
         return NULL;

Signature · 1 line

1.7.12.4 (Apple Git-37)

Jia Liu

author

2013-07-22 09:37 UTC #6 lore raw

Re: [Qemu-devel] [PATCH v3 3/4] target-openrisc: Free typename

Hi Andreas,

Quoted 3 lines · Jia Liu

We should free typename here.
Signed-off-by Jia Liu <[email protected]>

Sorry I didn't know I need add Signed-off-by: Andreas Färber <your@mail> here.

Quoted 17 lines

---
 target-openrisc/cpu.c | 1 +
 1 file changed, 1 insertion(+)

target-openrisc/cpu.c +1 -1

index 6d40f1b..e348df0 100644
@@ -99,6 +99,7 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
 
     typename = g_strdup_printf("%s-" TYPE_OPENRISC_CPU, cpu_model);
     oc = object_class_by_name(typename);
+    g_free(typename);
     if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
                        object_class_is_abstract(oc))) {
         return NULL;
--

Quoted 2 lines

1.7.12.4 (Apple Git-37)

Regards,
Jia.

Peter Maydell

2013-07-22 09:42 UTC #7 lore raw

Re: [Qemu-devel] [PATCH v3 3/4] target-openrisc: Free typename

Quoted 8 lines · Jia Liu

Hi Andreas,
Quoted 3 lines · Jia Liu
We should free typename here.
Signed-off-by Jia Liu <[email protected]>
Sorry I didn't know I need add Signed-off-by: Andreas Färber <your@mail> here.

Not Signed-off-by, Reviewed-by. The distinction is important.

thanks
-- PMM

Jia Liu

author

2013-07-22 09:43 UTC #8 lore raw

Re: [Qemu-devel] [PATCH v3 3/4] target-openrisc: Free typename

Quoted 4 lines · Peter Maydell

Quoted 3 lines · Jia Liu
Hi Andreas,
On Mon, Jul 22, 2013 at 4:56 PM, Jia Liu <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
We should free typename here.
Signed-off-by Jia Liu <[email protected]>
Sorry I didn't know I need add Signed-off-by: Andreas Färber <your@mail> here.
Not Signed-off-by, Reviewed-by. The distinction is important.

Thanks Peter, I got.

Quoted 3 lines

thanks
-- PMM

Regards,
Jia

Jia Liu

author

2013-07-22 08:56 UTC #9 lore raw

PATCH v3 4/4 target-openrisc: Fix cpu_model by name

In reply to Jia Liu

Fix cpu_model by name, make any and or1200 works both OK.

Signed-off-by Jia Liu <[email protected]>

---
 target-openrisc/cpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

target-openrisc/cpu.c +12 -4

index e348df0..6637166 100644
@@ -100,11 +100,19 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf("%s-" TYPE_OPENRISC_CPU, cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
-                       object_class_is_abstract(oc))) {
-        return NULL;
+
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;
     }
-    return oc;
+
+    oc = object_class_by_name(cpu_model);
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;
+    }
+
+    return NULL;
 }
 
 static void or1200_initfn(Object *obj)

Signature · 1 line

1.7.12.4 (Apple Git-37)

Peter Maydell

2013-07-22 09:30 UTC #10 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 23 lines · Jia Liu

Fix cpu_model by name, make any and or1200 works both OK.

Signed-off-by Jia Liu <[email protected]>

---
 target-openrisc/cpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

target-openrisc/cpu.c +4 -3

index e348df0..6637166 100644
@@ -100,11 +100,19 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf("%s-" TYPE_OPENRISC_CPU, cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
-                       object_class_is_abstract(oc))) {
-        return NULL;
+
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;
     }

target-openrisc/cpu.c static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model) · line 116

Quoted 10 lines

target-openrisc/cpu.c +8 -1

-    return oc;
+
+    oc = object_class_by_name(cpu_model);
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;
+    }
+
+    return NULL;
 }

This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?

thanks
-- PMM

Jia Liu

author

2013-07-22 09:42 UTC #11 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Hi Andreas and Peter,

Quoted 30 lines · Peter Maydell

Quoted 29 lines · Jia Liu

Fix cpu_model by name, make any and or1200 works both OK.

Signed-off-by Jia Liu <[email protected]>

---
 target-openrisc/cpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

target-openrisc/cpu.c +9 -4

index e348df0..6637166 100644
@@ -100,11 +100,19 @@ static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
     typename = g_strdup_printf("%s-" TYPE_OPENRISC_CPU, cpu_model);
     oc = object_class_by_name(typename);
     g_free(typename);
-    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
-                       object_class_is_abstract(oc))) {
-        return NULL;
+
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;
     }
-    return oc;
+
+    oc = object_class_by_name(cpu_model);
+    if (oc != NULL && object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) != NULL
+        && !object_class_is_abstract(oc)) {
+        return oc;

Quoted 8 lines

target-openrisc/cpu.c static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model) · line 116
Quoted 4 lines
target-openrisc/cpu.c +3 -0
+    }
+
+    return NULL;
 }
This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?

commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.

Quoted 3 lines

thanks
-- PMM

Regards,
Jia

Peter Maydell

2013-07-22 09:43 UTC #12 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 9 lines · Jia Liu

Hi Andreas and Peter,
Quoted 3 lines · Peter Maydell
This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.

The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".

-- PMM

Jia Liu

author

2013-07-22 09:55 UTC #13 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 8 lines · Peter Maydell

Quoted 7 lines · Jia Liu
Hi Andreas and Peter,
Quoted 3 lines · Peter Maydell
This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?

Quoted 5 lines

Quoted 2 lines
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.
The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".

So, I need make only "-cpu any" and "-cpu or1200" accepted, thanks.

Quoted 2 lines

-- PMM

Regards,
Jia

Jia Liu

author

2013-07-22 10:12 UTC #14 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

In reply to Peter Maydell

Quoted 7 lines · Peter Maydell

Quoted 6 lines · Jia Liu
Hi Andreas and Peter,
Quoted 3 lines · Peter Maydell
This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?

Yes! you are right, it is not needed. It was my mistake, I didn't understand Andreas.
Thanks!

It seems I need resend a v4 with Reviewed-by Andreas&Peter.

Quoted 8 lines

Quoted 3 lines
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.
The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".
-- PMM

Regards,
Jia.

Andreas Färber

2013-07-22 10:38 UTC #15 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

In reply to Peter Maydell

Am 22.07.2013 11:43, schrieb Peter Maydell:

Quoted 8 lines · Jia Liu

Quoted 7 lines
Hi Andreas and Peter,
Quoted 3 lines · Peter Maydell
This looks a bit odd. The commit message suggests it's an attempt to fix the bug that was fixed by commit 071b3364e -- is it really needed? Andreas?

Quoted 5 lines

Quoted 2 lines
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.
The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".

Peter, the story is that due to an oversight on my part, it was possible to use -cpu or1200-or32-cpu only for some weeks or months. This has been fixed so that only -cpu or1200 works.

It is up to Jia whether he sees a need to have both work or whether -cpu or1200 would be sufficient. In any case, I was not CC'ed on this series, so I only came across one patch by accident during a search for something else. ;)

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Maydell

2013-07-22 10:41 UTC #16 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 10 lines · Andreas Färber

Am 22.07.2013 11:43, schrieb Peter Maydell:
Quoted 5 lines
Quoted 2 lines
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.
The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".
Peter, the story is that due to an oversight on my part, it was possible to use -cpu or1200-or32-cpu only for some weeks or months. This has been fixed so that only -cpu or1200 works.

Yep; my view is that the acceptance of the -or32-cpu varieties was a bug which we have now fixed. In any case we should be consistent across target architectures about what we allow. I'm happy with the approach Jia has taken of just dropping this patch in v4 of this series.

thanks
-- PMM

Andreas Färber

2013-07-22 11:17 UTC #17 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Am 22.07.2013 12:40, schrieb Peter Maydell:

Quoted 12 lines · Andreas Färber

Quoted 10 lines
Am 22.07.2013 11:43, schrieb Peter Maydell:
Quoted 5 lines
Quoted 2 lines
commit 071b3364e fix the typename only, but this one make "-cpu any" "-cpu or1200" "-cpu any-or32-cpu" "-cpu or1200-or32-cpu" working fine.
The last two of these would be wrong: -cpu should only accept CPU names, ie "any" or "or1200".
Peter, the story is that due to an oversight on my part, it was possible to use -cpu or1200-or32-cpu only for some weeks or months. This has been fixed so that only -cpu or1200 works.

Quoted 5 lines

Yep; my view is that the acceptance of the -or32-cpu varieties was a bug which we have now fixed. In any case we should be consistent across target architectures about what we allow. I'm happy with the approach Jia has taken of just dropping this patch in v4 of this series.

alpha allows both, but -cpu ? just gave me a segfault. :/
It didn't have -cpu ? before QOM, so we decided to print the type names there. Will need to investigate what's going wrong currently.

Stripping -alpha-cpu off typenames would surely be possible.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Maydell

2013-07-22 11:35 UTC #18 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 8 lines · Andreas Färber

Am 22.07.2013 12:40, schrieb Peter Maydell:
Quoted 5 lines
Yep; my view is that the acceptance of the -or32-cpu varieties was a bug which we have now fixed. In any case we should be consistent across target architectures about what we allow. I'm happy with the approach Jia has taken of just dropping this patch in v4 of this series.
alpha allows both, but -cpu ? just gave me a segfault. :/

Odd; that works for me:
cam-vm-266:precise:qemu$ ./build/x86-all/alpha-softmmu/qemu-system-alpha -cpu ? Available CPUs:
ev4-alpha-cpu
ev5-alpha-cpu
ev56-alpha-cpu
ev6-alpha-cpu
ev67-alpha-cpu
ev68-alpha-cpu
pca56-alpha-cpu

Quoted 2 lines

It didn't have -cpu ? before QOM, so we decided to print the type names there.

Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.

Quoted 1 line

Stripping -alpha-cpu off typenames would surely be possible.

I think that that would be better in the name of consistency.

Also regarding consistency, not all targets react very well to being asked for a nonexistent cpu via "-cpu xyzzy":
alpha and s390x just plough on without an error lm32 and unicore32 segfault

(some of this may be default board model bugs rather than target-* bugs).

-- PMM

Andreas Färber

2013-07-22 12:27 UTC #19 lore raw

Re: [Qemu-devel] -cpu behavior (was: [PATCH v3 4/4] target-openrisc: Fix cpu_model by name)

Am 22.07.2013 13:34, schrieb Peter Maydell:

Quoted 7 lines · Andreas Färber

Quoted 5 lines
Am 22.07.2013 12:40, schrieb Peter Maydell:
Quoted 2 lines
In any case we should be
consistent across target architectures about what we allow.
alpha allows both, but -cpu ? just gave me a segfault. :/

Quoted 10 lines

Odd; that works for me:
cam-vm-266:precise:qemu$ ./build/x86-all/alpha-softmmu/qemu-system-alpha -cpu ? Available CPUs:
ev4-alpha-cpu
ev5-alpha-cpu
ev56-alpha-cpu
ev6-alpha-cpu
ev67-alpha-cpu
ev68-alpha-cpu
pca56-alpha-cpu

For -cpu ? I got

Program received signal SIGSEGV, Segmentation fault. 0x00005555557323fa in object_class_dynamic_cast (class=0x55555606df20,

    typename=typename@entry=0x55555582b82e "virtio-scsi-common")
    at /home/andreas/QEMU/qemu/qom/object.c:496

496 if (type->class->interfaces &&

but that might be related to local QOM patches... :/ confirmed, qom-next is working fine.

For -cpu ev67-alpha-cpu I got:

0x00005555557af890 in access_with_adjusted_size (addr=addr@entry=1021,

    value=value@entry=0x7fffe73bb5e0, size=size@entry=1, access_size_min=1,
    access_size_max=4, access=access@entry=
    0x5555557afb80 <memory_region_read_accessor>, opaque=opaque@entry=
    0x5555560bd3b0) at /home/andreas/QEMU/qemu/memory.c:429

429 access_size = MAX(MIN(size, access_size_max), access_size_min);

Seems unrelated and is no longer reproducible either. Hmm.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Andreas Färber

2013-07-22 12:34 UTC #20 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

In reply to Peter Maydell

Am 22.07.2013 13:34, schrieb Peter Maydell:

Quoted 6 lines · Andreas Färber

Quoted 5 lines
Am 22.07.2013 12:40, schrieb Peter Maydell:
Quoted 2 lines
In any case we should be
consistent across target architectures about what we allow.
alpha allows both

[...]

Quoted 6 lines

Quoted 2 lines
It didn't have -cpu ? before QOM, so we decided to print the type names there.
Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.

Right, because all others had implemented -cpu ? before we introduced that naming scheme and I tried to keep output compatibility for them. Focus for alpha was therefore on -cpu foo compatibility only.

Anthony had clearly stated on a KVM call that using full type names for future CPU hot-add was the right thing to do and possibly even composite convenience types like 4core-xeonblabla-x86_64-cpu; how that relates to -cpu and new targets was never clearly defined though. ;)

For VMSD we decided to deviate for new migratable targets from legacy CPUs in favor of consistency with devices, for instance.

Quoted 7 lines

Quoted 1 line
Stripping -alpha-cpu off typenames would surely be possible.
I think that that would be better in the name of consistency.
Also regarding consistency, not all targets react very well to being asked for a nonexistent cpu via "-cpu xyzzy":
alpha and s390x just plough on without an error

s390x does not have models yet. This will get fixed once we have agreed on model names and their implementation.

Quoted 4 lines

lm32 and unicore32 segfault
(some of this may be default board model bugs rather than target-* bugs).

Yeah, for one sh4 board where I noticed it while refactoring I already applied an error check. I guess cpu_init() / cpu_*_init() is not NULL-checked in more machines. Not sure if trying invalid arguments would be applicable for a qtest?

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Anthony Liguori

2013-07-22 15:25 UTC #21 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 15 lines · Andreas Färber

Am 22.07.2013 13:34, schrieb Peter Maydell:
Quoted 6 lines · Andreas Färber
Quoted 5 lines
Am 22.07.2013 12:40, schrieb Peter Maydell:
Quoted 2 lines
In any case we should be
consistent across target architectures about what we allow.
alpha allows both
[...]
Quoted 6 lines
Quoted 2 lines
It didn't have -cpu ? before QOM, so we decided to print the type names there.
Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.

Quoted 8 lines

Right, because all others had implemented -cpu ? before we introduced that naming scheme and I tried to keep output compatibility for them. Focus for alpha was therefore on -cpu foo compatibility only.
Anthony had clearly stated on a KVM call that using full type names for future CPU hot-add was the right thing to do and possibly even composite convenience types like 4core-xeonblabla-x86_64-cpu; how that relates to -cpu and new targets was never clearly defined though. ;)

That's pretty gross, but yes, we should have:

qemu -device Xeon-E5-4610,id=sock0 -device Xeon-E5-4610,id=sock1

Which effectively does:

qemu -cpu SandyBridge -smp cores=6,threads=2,sockets=2

By today's standards.

I think this applies equally well to other architecture. Model hardware more closely.

Regards,

Anthony Liguori

Quoted 30 lines

For VMSD we decided to deviate for new migratable targets from legacy CPUs in favor of consistency with devices, for instance.
Quoted 7 lines
Quoted 1 line
Stripping -alpha-cpu off typenames would surely be possible.
I think that that would be better in the name of consistency.
Also regarding consistency, not all targets react very well to being asked for a nonexistent cpu via "-cpu xyzzy":
alpha and s390x just plough on without an error
s390x does not have models yet. This will get fixed once we have agreed on model names and their implementation.
Quoted 4 lines
lm32 and unicore32 segfault
(some of this may be default board model bugs rather than target-* bugs).
Yeah, for one sh4 board where I noticed it while refactoring I already applied an error check. I guess cpu_init() / cpu_*_init() is not NULL-checked in more machines. Not sure if trying invalid arguments would be applicable for a qtest?
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Maydell

2013-07-22 15:39 UTC #22 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

Quoted 17 lines · Anthony Liguori

Quoted 5 lines · Andreas Färber
Am 22.07.2013 13:34, schrieb Peter Maydell:
Quoted 3 lines
Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.
Quoted 8 lines
Right, because all others had implemented -cpu ? before we introduced that naming scheme and I tried to keep output compatibility for them. Focus for alpha was therefore on -cpu foo compatibility only.
Anthony had clearly stated on a KVM call that using full type names for future CPU hot-add was the right thing to do and possibly even composite convenience types like 4core-xeonblabla-x86_64-cpu; how that relates to -cpu and new targets was never clearly defined though. ;)
That's pretty gross, but yes, we should have:

Quoted 7 lines

qemu -device Xeon-E5-4610,id=sock0 -device Xeon-E5-4610,id=sock1
Which effectively does:
qemu -cpu SandyBridge -smp cores=6,threads=2,sockets=2
By today's standards.

That doesn't really answer the question of "should the argument to -cpu be a QOM typename or a human friendly name?" though (though I note none of your -cpu or -device argument examples are QOM type names, since they're missing the -$arch-cpu suffix).

Quoted 2 lines

I think this applies equally well to other architecture.
Model hardware more closely.

For ARM this would mean "don't support -cpu at all, it is always hardwired by the board model" :-)

-- PMM

Andreas Färber

2013-07-22 16:29 UTC #23 lore raw

Re: [Qemu-devel] -cpu behavior (was: [PATCH v3 4/4] target-openrisc: Fix cpu_model by name)

Am 22.07.2013 17:38, schrieb Peter Maydell:

Quoted 22 lines · Anthony Liguori

Quoted 21 lines
Quoted 5 lines · Andreas Färber
Am 22.07.2013 13:34, schrieb Peter Maydell:
Quoted 3 lines
Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.
Quoted 8 lines
Right, because all others had implemented -cpu ? before we introduced that naming scheme and I tried to keep output compatibility for them. Focus for alpha was therefore on -cpu foo compatibility only.
Anthony had clearly stated on a KVM call that using full type names for future CPU hot-add was the right thing to do and possibly even composite convenience types like 4core-xeonblabla-x86_64-cpu; how that relates to -cpu and new targets was never clearly defined though. ;)
That's pretty gross, but yes, we should have:
qemu -device Xeon-E5-4610,id=sock0 -device Xeon-E5-4610,id=sock1
Which effectively does:

Quoted 8 lines

Quoted 3 lines
qemu -cpu SandyBridge -smp cores=6,threads=2,sockets=2
By today's standards.
That doesn't really answer the question of "should the argument to -cpu be a QOM typename or a human friendly name?" though (though I note none of your -cpu or -device argument examples are QOM type names, since they're missing the -$arch-cpu suffix).

Depending on how we register those types, they don't necessarily need an -$arch-cpu suffix iff they are deemed globally unique. In this case there would be a 32-bit and 64-bit type though, I guess.

But there's no qemu executable either, so we shouldn't take the example literally. :P

Quoted 5 lines

Quoted 2 lines
I think this applies equally well to other architecture.
Model hardware more closely.
For ARM this would mean "don't support -cpu at all, it is always hardwired by the board model" :-)

No, it means the board creates the equivalent of -device tegra2-soc. :) Or exynos4210, highbank-soc or whatever. Less code in machine init, more code in QOM devices reusable with -M none -readconfig foo.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Anthony Liguori

2013-07-22 17:39 UTC #24 lore raw

Re: [Qemu-devel] [PATCH v3 4/4] target-openrisc: Fix cpu_model by name

In reply to Peter Maydell

Quoted 20 lines · Peter Maydell

Quoted 19 lines · Anthony Liguori
Quoted 5 lines · Andreas Färber
Am 22.07.2013 13:34, schrieb Peter Maydell:
Quoted 3 lines
Looking at all of the '-cpu help' output, alpha seems to be the odd one out here: none of the others list valid CPUs with "-$arch-cpu" suffixes.
Quoted 8 lines
Right, because all others had implemented -cpu ? before we introduced that naming scheme and I tried to keep output compatibility for them. Focus for alpha was therefore on -cpu foo compatibility only.
Anthony had clearly stated on a KVM call that using full type names for future CPU hot-add was the right thing to do and possibly even composite convenience types like 4core-xeonblabla-x86_64-cpu; how that relates to -cpu and new targets was never clearly defined though. ;)
That's pretty gross, but yes, we should have:
qemu -device Xeon-E5-4610,id=sock0 -device Xeon-E5-4610,id=sock1

Quoted 8 lines

Quoted 5 lines
Which effectively does:
qemu -cpu SandyBridge -smp cores=6,threads=2,sockets=2
By today's standards.
That doesn't really answer the question of "should the argument to -cpu be a QOM typename or a human friendly name?"

They shouldn't be different things IMHO.

Quoted 3 lines

though
(though I note none of your -cpu or -device argument examples are QOM type names, since they're missing the -$arch-cpu suffix).

I'm not sure the rationale of $arch-cpu but I think having a forced suffix is a bad idea.

Quoted 5 lines

Quoted 2 lines
I think this applies equally well to other architecture.
Model hardware more closely.
For ARM this would mean "don't support -cpu at all, it is always hardwired by the board model" :-)

Is that a bad thing?

I really hate the -cpu option. I hope it dies a horrible bitrotten death over time once -device can be used to replace it.

Regards,

Anthony Liguori

Quoted 2 lines

-- PMM