Re: [Qemu-arm] [Qemu-devel] [PATCH v2 5/5] arm: drop intermediate cpu_model -> cpu type parsing and use cpu type directly

[Alistair Francis]

On Thu, Sep 14, 2017 at 12:50 AM, Igor Mammedov <address@hidden> wrote:
> On Thu, 14 Sep 2017 00:47:20 -0300
> Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>> > Reviewed-by: Eduardo Habkost <address@hidden>
>> > ---
>> > 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.

Thanks, I have a few comments I need to look at for this. I'm going to
hold off until this series lands though.

Thanks,
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.
>
>
> [...]