[Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

Igor Mammedov posted 3 patches 7 years, 1 month ago
Failed in applying to current master (apply log)
include/hw/boards.h  |  5 +++++
include/hw/ppc/ppc.h |  2 --
hw/arm/virt.c        | 46 ++++++++++---------------------------------
hw/core/machine.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/i386/pc.c         | 42 +++++++++++++--------------------------
hw/ppc/ppc.c         | 25 ------------------------
hw/ppc/spapr.c       | 14 +++++++------
vl.c                 |  2 +-
8 files changed, 92 insertions(+), 99 deletions(-)
[Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Igor Mammedov 7 years, 1 month ago
Some callers call CPUClass->parse_features manually to convert
'-cpu cpufoo,featurestr' string to cpu type and featurestr
into a set of global properties. And theni do controlled
cpu creation with setting properties and completing it with realize.
That's a lot of code duplication as they are practically
reimplement the same parsing logic.

Some don't and use cpu_generic_init() instead which does
the same parsing along with creation/realizing cpu within one
wrapper.

And some trying to switch to controlled cpu creation,
implement object_new()/set properties/realize steps
but forget feature parsing logic witch lieads to 'bugs'
commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)


This series moves -cpu option parsing to generic machine code
that removes a little of code duplication and makes cpus creation
process more unified.

PS:
As I don't have time to rewrite this part QEMU, being busy
rewritting yet another part of QEMU,
SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)

It compiles and pc machine even starts but otherwise is untested.

CC: Eduardo Habkost <ehabkost@redhat.com>
CC: patches@linaro.org
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: "Michael S. Tsirkin" <mst@redhat.com>

Igor Mammedov (3):
  machine: call machine init from wrapper
  machine: generalize handling of default cpu_model
  machine: generilize cpu_model parsing

 include/hw/boards.h  |  5 +++++
 include/hw/ppc/ppc.h |  2 --
 hw/arm/virt.c        | 46 ++++++++++---------------------------------
 hw/core/machine.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c         | 42 +++++++++++++--------------------------
 hw/ppc/ppc.c         | 25 ------------------------
 hw/ppc/spapr.c       | 14 +++++++------
 vl.c                 |  2 +-
 8 files changed, 92 insertions(+), 99 deletions(-)

-- 
2.7.4


Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Peter Maydell 7 years, 1 month ago
On 17 February 2017 at 18:56, Igor Mammedov <imammedo@redhat.com> wrote:
> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
>
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
>
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
>
>
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.

This API seems a little awkward for the SoC case, where
the board model doesn't actually know what the default
CPU model or the valid CPU models are, because it just
wants to say "create me a BCM2836 SoC" and let the SoC
object deal with determining whether it's always a cortex-a15
or if it might allow some user configurability either in
cpu choices or in optional flags.
Any thoughts about that use case?

(The stm32f205 SoC object has a "cpu-model" QOM property
that the board sets, but I think that's as much because
we somewhat awkwardly need to pass it into armv7m_init()
as a deliberate design choice.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Igor Mammedov 7 years, 1 month ago
On Fri, 17 Feb 2017 19:05:20 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 17 February 2017 at 18:56, Igor Mammedov <imammedo@redhat.com> wrote:
> > Some callers call CPUClass->parse_features manually to convert
> > '-cpu cpufoo,featurestr' string to cpu type and featurestr
> > into a set of global properties. And theni do controlled
> > cpu creation with setting properties and completing it with realize.
> > That's a lot of code duplication as they are practically
> > reimplement the same parsing logic.
> >
> > Some don't and use cpu_generic_init() instead which does
> > the same parsing along with creation/realizing cpu within one
> > wrapper.
> >
> > And some trying to switch to controlled cpu creation,
> > implement object_new()/set properties/realize steps
> > but forget feature parsing logic witch lieads to 'bugs'
> > commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> >
> >
> > This series moves -cpu option parsing to generic machine code
> > that removes a little of code duplication and makes cpus creation
> > process more unified.  
> 
> This API seems a little awkward for the SoC case, where
> the board model doesn't actually know what the default
> CPU model or the valid CPU models are, because it just
> wants to say "create me a BCM2836 SoC" and let the SoC
> object deal with determining whether it's always a cortex-a15
> or if it might allow some user configurability either in
> cpu choices or in optional flags.
> Any thoughts about that use case?
I've probably been too aggressive trying to force
conversion of all boards, assuming that all boards support
"-cpu CLI" option, currently option could be specified for
any board but it is ignored if board doesn't care
about it explicitly.

 "-cpu cpu_name,feat_str" handling is always based on
 availability of base CPU type supported by target as
 it's needed both for
   - translating cpu_name to QOM CPU type
       cpu_class_by_name(typename, name)
   - converting feat_str into set of global properties
     for matching QOM CPU type
       cc->parse_features(object_class_get_name(oc), featurestr, &err);

Boards that don't care/need '-cpu' support won't need
to do anything as we can skip cpu_name,feat_str parsing
if MachineClass::base_cpu_type isn't set (NULL by default),
that way we won't break ignored CLI if we care.
And currently we don't have convenient way to disable
feat_str parsing, but machine could be extended
with flag to [en|dis]able it explicitly.

The goal of this series is generalizing/consolidating
parsing of '-cpu' for boards that use it, removing
code duplication scattered around codebase and trying
normalize default cpu_model initialization.

For SoC with fixed CPU type it would be better to use
directly CPU type names directly but that hits legacy
cpu_init()/cpu_foo_init() wall, which is currently
cpu_model based and would be a lot of re-factoring.
I would convert cpu_init(cpu_model) to cpu_init(cpu_type)
and call '-cpu' handling logic from generic
machine/user_[bsd|linux] code (3 places in total).

> (The stm32f205 SoC object has a "cpu-model" QOM property
> that the board sets, but I think that's as much because
> we somewhat awkwardly need to pass it into armv7m_init()
> as a deliberate design choice.)
its sole user netduino2 board has cpu_model hardcodded
at board level which could be left alone or converted
to this API. Using API would make code more consistent
at the cost of more code for default/valid callbacks.

> 
> thanks
> -- PMM


Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Peter Maydell 7 years, 1 month ago
On 20 February 2017 at 18:55, Igor Mammedov <imammedo@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> This API seems a little awkward for the SoC case, where
>> the board model doesn't actually know what the default
>> CPU model or the valid CPU models are, because it just
>> wants to say "create me a BCM2836 SoC" and let the SoC
>> object deal with determining whether it's always a cortex-a15
>> or if it might allow some user configurability either in
>> cpu choices or in optional flags.
>> Any thoughts about that use case?
>
> I've probably been too aggressive trying to force
> conversion of all boards, assuming that all boards support
> "-cpu CLI" option, currently option could be specified for
> any board but it is ignored if board doesn't care
> about it explicitly.

I think the conversion in this patchset is OK for
the boards you've touched. It's the ones that it
doesn't touch that I need to think a little more about.

>  "-cpu cpu_name,feat_str" handling is always based on
>  availability of base CPU type supported by target as
>  it's needed both for
>    - translating cpu_name to QOM CPU type
>        cpu_class_by_name(typename, name)
>    - converting feat_str into set of global properties
>      for matching QOM CPU type
>        cc->parse_features(object_class_get_name(oc), featurestr, &err);

> Boards that don't care/need '-cpu' support won't need
> to do anything as we can skip cpu_name,feat_str parsing
> if MachineClass::base_cpu_type isn't set (NULL by default),
> that way we won't break ignored CLI if we care.
> And currently we don't have convenient way to disable
> feat_str parsing, but machine could be extended
> with flag to [en|dis]able it explicitly.
>
> The goal of this series is generalizing/consolidating
> parsing of '-cpu' for boards that use it, removing
> code duplication scattered around codebase and trying
> normalize default cpu_model initialization.

Yeah, I definitely like the cleanup to avoid all the
duplicate parse calls.

> For SoC with fixed CPU type it would be better to use
> directly CPU type names directly but that hits legacy
> cpu_init()/cpu_foo_init() wall, which is currently
> cpu_model based and would be a lot of re-factoring.
> I would convert cpu_init(cpu_model) to cpu_init(cpu_type)
> and call '-cpu' handling logic from generic
> machine/user_[bsd|linux] code (3 places in total).
>
>> (The stm32f205 SoC object has a "cpu-model" QOM property
>> that the board sets, but I think that's as much because
>> we somewhat awkwardly need to pass it into armv7m_init()
>> as a deliberate design choice.)

> its sole user netduino2 board has cpu_model hardcodded
> at board level which could be left alone or converted
> to this API.

I think this may be for the benefit of the not-yet-in-tree
netduinoplus2 board, which is a very similar SoC but
with a cortex-m4. There are likely other ways we could
model that, though.

> Using API would make code more consistent
> at the cost of more code for default/valid callbacks.

Mmm, we could have the board code just forward the
default/valid callbacks through to the SoC (which
would then either implement them or forward them again).
But I think we should definitely see if we can make
the code for handling "the CPU is actually inside some
other object" as clean as we can, because I think
that's by far the most common case. Machines like
the x86 pc and the ARM virt board where the CPU is
instantiated directly in the board code are the
non-standard cases I think. We have a lot of boards
where we do instantiate the CPU in the board code
just for legacy reasons where we don't properly
model the SoC the CPU is inside. But hardware that
really has the CPU as a top level pluggable object
is rare.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Igor Mammedov 7 years, 1 month ago
On Mon, 20 Feb 2017 19:11:00 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 20 February 2017 at 18:55, Igor Mammedov <imammedo@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
[...]
> 
> > its sole user netduino2 board has cpu_model hardcodded
> > at board level which could be left alone or converted
> > to this API.  
> 
> I think this may be for the benefit of the not-yet-in-tree
> netduinoplus2 board, which is a very similar SoC but
> with a cortex-m4. There are likely other ways we could
> model that, though.
> 
> > Using API would make code more consistent
> > at the cost of more code for default/valid callbacks.  
> 
> Mmm, we could have the board code just forward the
> default/valid callbacks through to the SoC (which
> would then either implement them or forward them again).
> But I think we should definitely see if we can make
> the code for handling "the CPU is actually inside some
> other object" as clean as we can, because I think
> that's by far the most common case. Machines like
> the x86 pc and the ARM virt board where the CPU is
> instantiated directly in the board code are the
> non-standard cases I think. We have a lot of boards
> where we do instantiate the CPU in the board code
> just for legacy reasons where we don't properly
> model the SoC the CPU is inside. But hardware that
> really has the CPU as a top level pluggable object
> is rare.

If we are to ditch legacy approach,
It would be cleaner for SoC to have fixed/unconfigurable
CPU type and each SoC being modeled as a separate
QOM object/type that would instantiate CPU directly
with QOM style, using type name, like:

    cpu = object_new(TYPE_FOO_CPU)
    set props if necessary               
    object_property_set_bool(cpu, true, "realized", &err)
    object_unref(cpu)

or similar with extra check/logic on top of plain object_new()

    prepare cpu_opts for cpu type TYPE_FOO_CPU
    cpu = qdev_device_add(cpu_opts)
    object_unref(cpu)

instead of using cpu_init/cpu_foo_init/cpu_generic_init()
with cpu_model parsing logic which is not really needed
there as concrete SoC model knows exact CPU type.

It should work for CPUs that are converted to QOM types,
i.e. a cpu_model maps to a distinct QOM type but we
still have legacy cpu_model handling where we have
not completely QOMifed single base CPU types, ex.

  cpu_mips_init(const char *cpu_model) 
      cpudef = get_feature_set_by_name(cpu+model)
      cpu = object_new(TYPE_MIPS_CPU)
      cpu->cpudef = cdef

which rely on cpu_model parsing like x86 used to be
before conversion from a single CPU type to a set
of concrete classes. QOMification of remaining
CPUs that rely on legacy cpu_model could be idea
for gsoc/outreachy project so students could learn
about QEMU internals (CCing Stefan). 

> 
> thanks
> -- PMM
> 


Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Peter Maydell 7 years, 1 month ago
On 21 February 2017 at 12:44, Igor Mammedov <imammedo@redhat.com> wrote:
> If we are to ditch legacy approach,
> It would be cleaner for SoC to have fixed/unconfigurable
> CPU type and each SoC being modeled as a separate
> QOM object/type that would instantiate CPU directly
> with QOM style, using type name, like:
>
>     cpu = object_new(TYPE_FOO_CPU)
>     set props if necessary
>     object_property_set_bool(cpu, true, "realized", &err)
>     object_unref(cpu)
>
> or similar with extra check/logic on top of plain object_new()
>
>     prepare cpu_opts for cpu type TYPE_FOO_CPU
>     cpu = qdev_device_add(cpu_opts)
>     object_unref(cpu)
>
> instead of using cpu_init/cpu_foo_init/cpu_generic_init()
> with cpu_model parsing logic which is not really needed
> there as concrete SoC model knows exact CPU type.

Mmm, I think that's probably best. Would the end-user
still be able to set CPU properties if necessary via
a -global command line switch (since -cpu type,foo=on
wouldn't work)?

(I just worry slightly that we might have users doing
weird things which we'd break.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Eduardo Habkost 7 years, 1 month ago
On Fri, Feb 17, 2017 at 07:56:32PM +0100, Igor Mammedov wrote:
> 
> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
> 
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
> 
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> 
> 
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.
> 
> PS:
> As I don't have time to rewrite this part QEMU, being busy
> rewritting yet another part of QEMU,
> SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
> BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
> TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)
> 
> It compiles and pc machine even starts but otherwise is untested.

The approach makes sense to me, but I haven't reviewed all the
code yet.

That said, I don't think this excludes the possiblity of adding a
temporary cpu_generic_new() wrapper in case it is useful in the
meantime. After this series is applied, cpu_generic_new() could
be automatically replaced by object_new(machine->cpu_typename).

Peter, do you think a wrapper for parse_features + object_new()
would still be necessary to avoid too much code duplication on
arm boards in 2.9, or can we live without it until we apply
Igor's series (probaly after 2.9)?

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Peter Maydell 7 years, 1 month ago
On 21 February 2017 at 18:28, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Peter, do you think a wrapper for parse_features + object_new()
> would still be necessary to avoid too much code duplication on
> arm boards in 2.9, or can we live without it until we apply
> Igor's series (probaly after 2.9)?

2.9 is very close now. I don't think we need to change any
of the existing board code. Though there are quite a lot that
don't accept -cpu foo,feat=on syntax because they use
object_new() to create the CPU but don't call parse_features,
they've been that way for years (eg since commit 223a72f1179dc0b
from 2014 for versatilepb). We should figure out how we want
to handle this and change all the boards just once, for 2.10.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model
Posted by Igor Mammedov 7 years, 1 month ago
On Fri, 17 Feb 2017 19:56:32 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
> 
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
> 
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> 
> 
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.
> 
> PS:
> As I don't have time to rewrite this part QEMU, being busy
> rewritting yet another part of QEMU,
> SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
> BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
> TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)

series has been written on top my numa tree which apply it to
upstream master. So here goes rebased on top of current master variant:

https://github.com/imammedo/qemu/commits/generilize_cpu_model_parsing_rfc