All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alistair Francis <alistair23@gmail.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH v2 5/5] arm: drop intermediate cpu_model -> cpu type parsing and use cpu type directly
Date: Thu, 14 Sep 2017 15:09:37 -0700	[thread overview]
Message-ID: <CAKmqyKNENp+E+VFS8-GHL=WdgSwKQHh1xVDYDJ-D+_2mUi-cFw@mail.gmail.com> (raw)
In-Reply-To: <20170914095011.2fba8aab@nial.brq.redhat.com>

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

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

  reply	other threads:[~2017-09-14 22:10 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-13 16:04 [Qemu-devel] [PATCH v2 0/5] generalize parsing of cpu_model (x86/arm) Igor Mammedov
2017-09-13 16:04 ` [Qemu-devel] [PATCH v2 1/5] qom: cpus: split cpu_generic_init() on feature parsing and cpu creation parts Igor Mammedov
2017-09-13 16:04 ` [Qemu-devel] [PATCH v2 2/5] cpu: make cpu_generic_init() abort QEMU on error Igor Mammedov
2017-09-13 16:04 ` [Qemu-devel] [PATCH v2 3/5] vl.c: convert cpu_model to cpu type and set of global properties before machine_init() Igor Mammedov
2017-09-14 18:19   ` Philippe Mathieu-Daudé
2017-09-13 16:04 ` [Qemu-devel] [PATCH v2 4/5] pc: use generic cpu_model parsing Igor Mammedov
2017-09-13 16:04 ` [Qemu-devel] [PATCH v2 5/5] arm: drop intermediate cpu_model -> cpu type parsing and use cpu type directly Igor Mammedov
2017-09-14  3:47   ` Philippe Mathieu-Daudé
2017-09-14  7:50     ` Igor Mammedov
2017-09-14 22:09       ` Alistair Francis [this message]
2017-09-14 17:57   ` Alistair Francis
2017-09-18 21:53   ` [Qemu-devel] [PATCH] !fixup arm: missed comment update Philippe Mathieu-Daudé
2017-09-15 15:03 ` [Qemu-devel] [PATCH v2 0/5] generalize parsing of cpu_model (x86/arm) Igor Mammedov
2017-09-18 16:42   ` Igor Mammedov
2017-09-19 12:11     ` Eduardo Habkost
2017-09-19 12:46       ` Igor Mammedov
2017-09-19 12:56       ` Philippe Mathieu-Daudé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKmqyKNENp+E+VFS8-GHL=WdgSwKQHh1xVDYDJ-D+_2mUi-cFw@mail.gmail.com' \
    --to=alistair23@gmail.com \
    --cc=f4bug@amsat.org \
    --cc=imammedo@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.