Re: [Qemu-devel] [PATCH v3 00/25] generalize parsing of cpu_model (part 4)

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 6 Feb 2018 20:32:13 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> I will try to summarize my comments here:

Thanks for review

> * I suggest squashing patches 2-22 together.  This way we
>   shouldn't have any intermediate commits where "make check"
>   generates warnings, and the series is shorter.
>   * Or, even better: squash the CPU_RESOLVING_TYPE parts of 3-22
>     into one patch, and the tests/machine-none-test.c parts of
>     3-22 into patch 2.
CPU_RESOLVING_TYPE parts we split to simplify review
for target maintainers.
I'll respin squashing them into one commit.

> 
> * The linux-user/main.c hunk of patch 03/25 looks unnecessary.
Indeed, I think it was a leftover slipped in from previous revision
where I was touching default cpus handling in *-user targets,
with the later dropped from series it's not needed.

> * I suggest testing all CPU models in patch 02/25, but this
>   shouldn't block the series.  Can be a follow-up patch.
the goal of the test is that refactoring didn't regress
'none' machine after following change in 23/25:

@@ -24,9 +24,9 @@ static void machine_none_init(MachineState *mch)
 {
     CPUState *cpu = NULL;
 
-    /* Initialize CPU (if a model has been specified) */
-    if (mch->cpu_model) {
-        cpu = cpu_init(mch->cpu_model);
+    /* Initialize CPU (if user asked for it) */
+    if (mch->cpu_type) {
+        cpu = cpu_create(mch->cpu_type);

for that it's sufficient to probe one cpu model.

We can do all cpus probing later if there will be need in it,
but that's not related to this series and would just waste
'make check' time if it won't be doing something meaningful
with probed cpu.

> All the rest looks good to me.
> 
> Thanks!
> 
> 
> On Tue, Jan 23, 2018 at 09:07:59AM +0100, Igor Mammedov wrote:
> > 
> > v3:
> >   - use qtest_startf() instead of qtest_start()
> >   - rename tests/machine-none.c to tests/machine-none-test.c
> >   - introduce first CPU_RESOLVING_TYPE for all targets and
> >     only then use it parse_cpu_model() 
> >   - stop abusing  mc->default_cpu_type as resolving cpu type,
> >     move cpu_parse_cpu_model() in to exec.c and embed in
> >     CPU_RESOLVING_TYPE, so that callers won't have to know
> >     about unnecessary detail
> > 
> > v2:
> >   - implemented new approach only for x86/ARM (will be done for all targets
> >     if approach seems acceptable)
> >   - add test case for '-M none -cpu FOO' case
> >   - redefine TARGET_DEFAULT_CPU_TYPE into CPU_RESOLVING_TYPE
> >   - scrape off default cpu_model refactoring, so it would cause
> >     less conflicts with Laurent's series where he tries to rework
> >     defaults to use ELF hints of executed program
> > 
> > Series is finishing work on generalizing cpu_model parsing
> > and limiting parts that deal with inconsistent cpu_model
> > naming to "-cpu" CLI option in vl.c, bsd|linux-user/main.c
> > CLI and default cpu_model processing and FOO_cpu_class_by_name()
> > callbacks.
> > 
> > It introduces CPU_RESOLVING_TYPE which must be defined
> > by each target and is used by helper parse_cpu_model()
> > (former cpu_parse_cpu_model()) to get access to target
> > specific FOO_cpu_class_by_name() callback.
> > 
> > git tree for testing:
> >    https://github.com/imammedo/qemu.git cpu_init_removal_v3
> > 
> > CC: Laurent Vivier <laurent@vivier.eu>
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > CC: qemu-s390x@nongnu.org
> > CC: qemu-ppc@nongnu.org
> > CC: qemu-arm@nongnu.org
> > 
> > Igor Mammedov (25):
> >   nios2: 10m50_devboard: replace cpu_model with cpu_type
> >   tests: add machine 'none' with -cpu test
> >   arm: cpu: add CPU_RESOLVING_TYPE macro
> >   x86: cpu: add CPU_RESOLVING_TYPE macro
> >   alpha: cpu: add CPU_RESOLVING_TYPE macro
> >   cris: cpu: add CPU_RESOLVING_TYPE macro
> >   lm32: cpu: add CPU_RESOLVING_TYPE macro
> >   m68k: cpu: add CPU_RESOLVING_TYPE macro
> >   microblaze: cpu: add CPU_RESOLVING_TYPE macro
> >   mips: cpu: add CPU_RESOLVING_TYPE macro
> >   moxie: cpu: add CPU_RESOLVING_TYPE macro
> >   nios2: cpu: add CPU_RESOLVING_TYPE macro
> >   openrisc: cpu: add CPU_RESOLVING_TYPE macro
> >   ppc: cpu: add CPU_RESOLVING_TYPE macro
> >   s390x: cpu: add CPU_RESOLVING_TYPE macro
> >   sh4: cpu: add CPU_RESOLVING_TYPE macro
> >   sparc: cpu: add CPU_RESOLVING_TYPE macro
> >   tricore: cpu: add CPU_RESOLVING_TYPE macro
> >   unicore32: cpu: add CPU_RESOLVING_TYPE macro
> >   xtensa: cpu: add CPU_RESOLVING_TYPE macro
> >   hppa: cpu: add CPU_RESOLVING_TYPE macro
> >   tilegx: cpu: add CPU_RESOLVING_TYPE macro
> >   Use cpu_create(type) instead of cpu_init(cpu_model)
> >   cpu: get rid of unused cpu_init() defines
> >   cpu: get rid of cpu_generic_init()
> > 
> >  include/qom/cpu.h         | 16 +-------
> >  target/alpha/cpu.h        |  3 +-
> >  target/arm/cpu.h          |  3 +-
> >  target/cris/cpu.h         |  3 +-
> >  target/hppa/cpu.h         |  2 +-
> >  target/i386/cpu.h         |  3 +-
> >  target/lm32/cpu.h         |  3 +-
> >  target/m68k/cpu.h         |  3 +-
> >  target/microblaze/cpu.h   |  2 +-
> >  target/mips/cpu.h         |  3 +-
> >  target/moxie/cpu.h        |  3 +-
> >  target/nios2/cpu.h        |  2 +-
> >  target/openrisc/cpu.h     |  3 +-
> >  target/ppc/cpu.h          |  3 +-
> >  target/s390x/cpu.h        |  3 +-
> >  target/sh4/cpu.h          |  3 +-
> >  target/sparc/cpu.h        |  5 +--
> >  target/tilegx/cpu.h       |  2 +-
> >  target/tricore/cpu.h      |  3 +-
> >  target/unicore32/cpu.h    |  3 +-
> >  target/xtensa/cpu.h       |  3 +-
> >  bsd-user/main.c           |  4 +-
> >  exec.c                    | 23 +++++++++++
> >  hw/core/null-machine.c    |  6 +--
> >  hw/nios2/10m50_devboard.c |  2 +-
> >  linux-user/main.c         | 10 +++--
> >  qom/cpu.c                 | 48 +----------------------
> >  tests/Makefile.include    |  2 +
> >  tests/machine-none-test.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                      | 10 ++---
> >  30 files changed, 162 insertions(+), 114 deletions(-)
> >  create mode 100644 tests/machine-none-test.c
> > 
> > -- 
> > 2.7.4
> > 
> >   
>