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(-)
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
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
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
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
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 >
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
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
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
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
© 2016 - 2024 Red Hat, Inc.