On Thu, 14 Sep 2017 00:47:20 -0300 Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> Hi Igor, > > awesome clean refactor! Thanks, there is more patches on this topic for other targets to post but it's waiting on 1-3/5 to land in master so it would be easier for maintainers to verify/test them without fishing out dependencies from mail list. hopefully everything will land in 2.11 so we won't have to deal with cpu_model anywhere except of one place vl.c. > just 1 comment inlined. > > On 09/13/2017 01:04 PM, Igor Mammedov wrote: > > there are 2 use cases to deal with: > > 1: fixed CPU models per board/soc > > 2: boards with user configurable cpu_model and fallback to > > default cpu_model if user hasn't specified one explicitly > > > > For the 1st > > drop intermediate cpu_model parsing and use const cpu type > > directly, which replaces: > > typename = object_class_get_name( > > cpu_class_by_name(TYPE_ARM_CPU, cpu_model)) > > object_new(typename) > > with > > object_new(FOO_CPU_TYPE_NAME) > > or > > cpu_generic_init(BASE_CPU_TYPE, "my cpu model") > > with > > cpu_create(FOO_CPU_TYPE_NAME) > > > > as result 1st use case doesn't have to invoke not necessary > > translation and not needed code is removed. > > > > For the 2nd > > 1: set default cpu type with MachineClass::default_cpu_type and > > 2: use generic cpu_model parsing that done before machine_init() > > is run and: > > 2.1: drop custom cpu_model parsing where pattern is: > > typename = object_class_get_name( > > cpu_class_by_name(TYPE_ARM_CPU, cpu_model)) > > [parse_features(typename, cpu_model, &err) ] > > > > 2.2: or replace cpu_generic_init() which does what > > 2.1 does + create_cpu(typename) with just > > create_cpu(machine->cpu_type) > > as result cpu_name -> cpu_type translation is done using > > generic machine code one including parsing optional features > > if supported/present (removes a bunch of duplicated cpu_model > > parsing code) and default cpu type is defined in an uniform way > > within machine_class_init callbacks instead of adhoc places > > in boadr's machine_init code. > > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> > > --- > > v2: > > - fix merge conflicts with ignore_memory_transaction_failures > > - fix couple merge conflicts where SoC type string where replaced by type > > macro > > - keep plain prefix string in: strncmp(cpu_type, "pxa27", 5) > > - s/"%s" ARM_CPU_TYPE_SUFFIX/ARM_CPU_TYPE_NAME("%s")/ > > --- [...] > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index fe96557..fe26e99 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -163,13 +163,13 @@ static const int a15irqmap[] = { > > }; > > > > static const char *valid_cpus[] = { > > - "cortex-a15", > > - "cortex-a53", > > - "cortex-a57", > > - "host", > > + ARM_CPU_TYPE_NAME("cortex-a15"), > > + ARM_CPU_TYPE_NAME("cortex-a53"), > > + ARM_CPU_TYPE_NAME("cortex-a57"), > > + ARM_CPU_TYPE_NAME("host"), > > }; > > > > -static bool cpuname_valid(const char *cpu) > > +static bool cpu_type_valid(const char *cpu) > > { > > int i; > > I'd just change this by: > > static bool cpuname_valid(const char *cpu) > { > static const char *valid_cpus[] = { > ARM_CPU_TYPE_NAME("cortex-a15"), > ARM_CPU_TYPE_NAME("cortex-a53"), > ARM_CPU_TYPE_NAME("cortex-a57"), > }; > int i; > > for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) { > if (strcmp(cpu, valid_cpus[i]) == 0) { > return true; > } > } > return kvm_enabled() && !strcmp(cpu, ARM_CPU_TYPE_NAME("host"); here is one more case to consider for valid_cpus refactoring, CCing Alistair. > } > > Anyway this can be a later patch. this check might be removed or superseded by generic valid_cpus work that Alistair is working on, anyways it should be part of that work as change is not directly related to this series. [...]