[v5,1/6] machine: Convert the valid cpu types to use cpu_model

As cpu_type is not a user visible string let's convert the
valid_cpu_types to compare against cpu_model instead. This way we have a
user friendly string to report back.

Once we have a cpu_type to cpu_model conversion this patch should be
reverted and we should use cpu_type instead.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/core/machine.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> As cpu_type is not a user visible string let's convert the
> valid_cpu_types to compare against cpu_model instead. This way we have a
> user friendly string to report back.
> 
> Once we have a cpu_type to cpu_model conversion this patch should be
> reverted and we should use cpu_type instead.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  hw/core/machine.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cdc1163dc6..de5bac1c84 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
>      /* If the machine supports the valid_cpu_types check and the user
>       * specified a CPU with -cpu check here that the user CPU is supported.
>       */
> -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> +    if (machine_class->valid_cpu_types && machine->cpu_model) {
>          int i;
>  
>          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> -            if (object_class_dynamic_cast(class,
> -                                          machine_class->valid_cpu_types[i])) {
> +            if (!strcmp(machine->cpu_model,
> +                        machine_class->valid_cpu_types[i])) {

I would rename valid_cpu_types to valid_cpu_models to make the
new semantics clearer.

Anyway, I have bad and good news:

The bad news is Igor already sent patches last week that remove
MachineState::cpu_model, so this conflicts with his series.  Now
parse_cpu_model() will be the only place where the original CPU model name is
available, but the function needs to work on *-user too.  See:
"[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".

The good news is that I think we can fix this very easily if
validation is done at the same place where parse_cpu_model() is
called.  e.g.:

    current_machine->cpu_type = machine_class->default_cpu_type;
    if (cpu_model) {
        current_machine->cpu_type = parse_cpu_model(cpu_model);

        if (machine_class->valid_cpu_models) {
            ObjectClass *class = object_class_by_name(machine->cpu_type);
            int i;

            for (i = 0; machine_class->valid_cpu_models[i]; i++) {
                const char *valid_model = machine_class->valid_cpu_models[i];
                ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
                if (object_class_dynamic_cast(class,
                                              object_class_get_name(valid_class))) {
                     /* Valid CPU type, we're good to go */
                     break;
                }
            }
            if (!machine_class->valid_cpu_models[i]) {
                error_report("Invalid CPU model: %s", cpu_model);
                error_printf("The valid CPU models are: %s",
                             machine_class->valid_cpu_models[0]);
                for (i = 1; machine_class->valid_cpu_models[i]; i++) {
                    error_printf(", %s", machine_class->valid_cpu_models[i]);
                }
                error_printf("\n");
                exit(1);
            }
        }
    }

This can be done inside main(), or moved inside
machine_run_board_init() if main() pass cpu_model as argument to
the function.

On either case, I think it's a good idea to do validation and
printing of error messages closer to the code that parses the
command-line options.  This way we separate parsing/validation
from initialization.

>                  /* The user specificed CPU is in the valid field, we are
>                   * good to go.
>                   */
> @@ -792,8 +791,8 @@ void machine_run_board_init(MachineState *machine)
>  
>          if (!machine_class->valid_cpu_types[i]) {
>              /* The user specified CPU is not valid */
> -            error_report("Invalid CPU type: %s", machine->cpu_type);
> -            error_printf("The valid types are: %s",
> +            error_report("Invalid CPU model: %s", machine->cpu_model);
> +            error_printf("The valid models are: %s",
>                           machine_class->valid_cpu_types[0]);
>              for (i = 1; machine_class->valid_cpu_types[i]; i++) {
>                  error_printf(", %s", machine_class->valid_cpu_types[i]);
> -- 
> 2.14.1
> 
>

On Fri, 2 Feb 2018 16:23:26 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > As cpu_type is not a user visible string let's convert the
> > valid_cpu_types to compare against cpu_model instead. This way we have a
> > user friendly string to report back.
> > 
> > Once we have a cpu_type to cpu_model conversion this patch should be
> > reverted and we should use cpu_type instead.
> > 
> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > ---
> > 
> >  hw/core/machine.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index cdc1163dc6..de5bac1c84 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> >      /* If the machine supports the valid_cpu_types check and the user
> >       * specified a CPU with -cpu check here that the user CPU is supported.
> >       */
> > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> >          int i;
> >  
> >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > -            if (object_class_dynamic_cast(class,
> > -                                          machine_class->valid_cpu_types[i])) {
> > +            if (!strcmp(machine->cpu_model,
> > +                        machine_class->valid_cpu_types[i])) {  
> 
> I would rename valid_cpu_types to valid_cpu_models to make the
> new semantics clearer.
> 
> Anyway, I have bad and good news:
> 
> The bad news is Igor already sent patches last week that remove
> MachineState::cpu_model, so this conflicts with his series.  Now
> parse_cpu_model() will be the only place where the original CPU model name is
> available, but the function needs to work on *-user too.  See:
> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> 
> The good news is that I think we can fix this very easily if
> validation is done at the same place where parse_cpu_model() is
> called.  e.g.:
> 
>     current_machine->cpu_type = machine_class->default_cpu_type;
>     if (cpu_model) {
>         current_machine->cpu_type = parse_cpu_model(cpu_model);
> 
>         if (machine_class->valid_cpu_models) {
>             ObjectClass *class = object_class_by_name(machine->cpu_type);
>             int i;
> 
>             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
>                 const char *valid_model = machine_class->valid_cpu_models[i];
>                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
>                 if (object_class_dynamic_cast(class,
>                                               object_class_get_name(valid_class))) {
>                      /* Valid CPU type, we're good to go */
>                      break;
>                 }
>             }
>             if (!machine_class->valid_cpu_models[i]) {
>                 error_report("Invalid CPU model: %s", cpu_model);
>                 error_printf("The valid CPU models are: %s",
>                              machine_class->valid_cpu_models[0]);
>                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
>                     error_printf(", %s", machine_class->valid_cpu_models[i]);
>                 }
>                 error_printf("\n");
>                 exit(1);
>             }
>         }
>     }
> 
> This can be done inside main(), or moved inside
> machine_run_board_init() if main() pass cpu_model as argument to
> the function.
> 
> On either case, I think it's a good idea to do validation and
> printing of error messages closer to the code that parses the
> command-line options.  This way we separate parsing/validation
> from initialization.
I agree it's better like you suggest as at least it prevents
ms->cpu_model creeping back into boards code.

But I still dislike (hate) an idea of new code adding non
canonized cpu_model strings back in the boards code.
It's just a matter of time when someone would use them
and cpu_model parsing will creep back into boards.

It would be much better to if we add 
   char *MachineClass::cpu_name_by_type_name(char *cpu_type)
callback and let machines in this patchset to set it,
something along following lines which is not much of
refactoring and allows for gradual conversion:

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9631670..85cca84 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
     return cpu->el_change_hook_opaque;
 }
 
+char *arm_cpu_name_by_type_name(const char *typename);
+
 #endif
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f936017..ae6adb7 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
     mc->desc = "Netduino 2 Machine";
     mc->init = netduino2_init;
     mc->ignore_memory_transaction_failures = true;
+    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
 }
 
 DEFINE_MACHINE("netduino2", netduino2_machine_init)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index cc1856c..dacc3cc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -928,6 +928,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     acc->parent_realize(dev, errp);
 }
 
+char *arm_cpu_name_by_type_name(const char *typename)
+{
+    return g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU));
+}
+
 static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c83c901..e27b7f0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5373,7 +5373,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data)
     char *name;
 
     typename = object_class_get_name(oc);
-    name = g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU));
+    name = arm_cpu_name_by_type_name(typename);
     (*s->cpu_fprintf)(s->file, "  %s\n",
                       name);
     g_free(name);
@@ -5410,8 +5410,7 @@ static void arm_cpu_add_definition(gpointer data, gpointer user_data)
 
     typename = object_class_get_name(oc);
     info = g_malloc0(sizeof(*info));
-    info->name = g_strndup(typename,
-                           strlen(typename) - strlen("-" TYPE_ARM_CPU));
+    info->name = arm_cpu_name_by_type_name(typename);
     info->q_typename = g_strdup(typename);
 
     entry = g_malloc0(sizeof(*entry));
diff --git a/vl.c b/vl.c
index c10b0f4..fda1cb2 100644
--- a/vl.c
+++ b/vl.c
@@ -4646,6 +4646,11 @@ int main(int argc, char **argv, char **envp)
         if (cpu_model) {
             current_machine->cpu_type =
                 cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model);
+
+            if !is_valid_cpu(current_machine->cpu_type)
+                print("valid cpu types: ")
+                for_each_valid_type
+                    print(machine_class->cpu_name_by_type_name())
         }
     }

On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> On Fri, 2 Feb 2018 16:23:26 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > > As cpu_type is not a user visible string let's convert the
> > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > user friendly string to report back.
> > > 
> > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > reverted and we should use cpu_type instead.
> > > 
> > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > ---
> > > 
> > >  hw/core/machine.c | 11 +++++------
> > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index cdc1163dc6..de5bac1c84 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > >      /* If the machine supports the valid_cpu_types check and the user
> > >       * specified a CPU with -cpu check here that the user CPU is supported.
> > >       */
> > > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> > >          int i;
> > >  
> > >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > -            if (object_class_dynamic_cast(class,
> > > -                                          machine_class->valid_cpu_types[i])) {
> > > +            if (!strcmp(machine->cpu_model,
> > > +                        machine_class->valid_cpu_types[i])) {  
> > 
> > I would rename valid_cpu_types to valid_cpu_models to make the
> > new semantics clearer.
> > 
> > Anyway, I have bad and good news:
> > 
> > The bad news is Igor already sent patches last week that remove
> > MachineState::cpu_model, so this conflicts with his series.  Now
> > parse_cpu_model() will be the only place where the original CPU model name is
> > available, but the function needs to work on *-user too.  See:
> > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > 
> > The good news is that I think we can fix this very easily if
> > validation is done at the same place where parse_cpu_model() is
> > called.  e.g.:
> > 
> >     current_machine->cpu_type = machine_class->default_cpu_type;
> >     if (cpu_model) {
> >         current_machine->cpu_type = parse_cpu_model(cpu_model);
> > 
> >         if (machine_class->valid_cpu_models) {
> >             ObjectClass *class = object_class_by_name(machine->cpu_type);
> >             int i;
> > 
> >             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> >                 const char *valid_model = machine_class->valid_cpu_models[i];
> >                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> >                 if (object_class_dynamic_cast(class,
> >                                               object_class_get_name(valid_class))) {
> >                      /* Valid CPU type, we're good to go */
> >                      break;
> >                 }
> >             }
> >             if (!machine_class->valid_cpu_models[i]) {
> >                 error_report("Invalid CPU model: %s", cpu_model);
> >                 error_printf("The valid CPU models are: %s",
> >                              machine_class->valid_cpu_models[0]);
> >                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> >                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> >                 }
> >                 error_printf("\n");
> >                 exit(1);
> >             }
> >         }
> >     }
> > 
> > This can be done inside main(), or moved inside
> > machine_run_board_init() if main() pass cpu_model as argument to
> > the function.
> > 
> > On either case, I think it's a good idea to do validation and
> > printing of error messages closer to the code that parses the
> > command-line options.  This way we separate parsing/validation
> > from initialization.
> I agree it's better like you suggest as at least it prevents
> ms->cpu_model creeping back into boards code.
> 
> But I still dislike (hate) an idea of new code adding non
> canonized cpu_model strings back in the boards code.
> It's just a matter of time when someone would use them
> and cpu_model parsing will creep back into boards.
> 
> It would be much better to if we add 
>    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> callback and let machines in this patchset to set it,
> something along following lines which is not much of
> refactoring and allows for gradual conversion:
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9631670..85cca84 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
>      return cpu->el_change_hook_opaque;
>  }
>  
> +char *arm_cpu_name_by_type_name(const char *typename);
> +
>  #endif
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f936017..ae6adb7 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
>      mc->desc = "Netduino 2 Machine";
>      mc->init = netduino2_init;
>      mc->ignore_memory_transaction_failures = true;
> +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:

I really don't want to introduce a new arch-specific hook just
for that.  We should move CPU type lookup logic to common code
and make it unnecessary to write new hooks.

I agree it would be better if we had a cpu_name_by_type_name()
function, but I would like to have it implemented cleanly.

On Mon, 5 Feb 2018 11:54:01 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> > On Fri, 2 Feb 2018 16:23:26 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:  
> > > > As cpu_type is not a user visible string let's convert the
> > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > user friendly string to report back.
> > > > 
> > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > reverted and we should use cpu_type instead.
> > > > 
> > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > ---
> > > > 
> > > >  hw/core/machine.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index cdc1163dc6..de5bac1c84 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > >      /* If the machine supports the valid_cpu_types check and the user
> > > >       * specified a CPU with -cpu check here that the user CPU is supported.
> > > >       */
> > > > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > >          int i;
> > > >  
> > > >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > -            if (object_class_dynamic_cast(class,
> > > > -                                          machine_class->valid_cpu_types[i])) {
> > > > +            if (!strcmp(machine->cpu_model,
> > > > +                        machine_class->valid_cpu_types[i])) {    
> > > 
> > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > new semantics clearer.
> > > 
> > > Anyway, I have bad and good news:
> > > 
> > > The bad news is Igor already sent patches last week that remove
> > > MachineState::cpu_model, so this conflicts with his series.  Now
> > > parse_cpu_model() will be the only place where the original CPU model name is
> > > available, but the function needs to work on *-user too.  See:
> > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > > 
> > > The good news is that I think we can fix this very easily if
> > > validation is done at the same place where parse_cpu_model() is
> > > called.  e.g.:
> > > 
> > >     current_machine->cpu_type = machine_class->default_cpu_type;
> > >     if (cpu_model) {
> > >         current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > 
> > >         if (machine_class->valid_cpu_models) {
> > >             ObjectClass *class = object_class_by_name(machine->cpu_type);
> > >             int i;
> > > 
> > >             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > >                 const char *valid_model = machine_class->valid_cpu_models[i];
> > >                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > >                 if (object_class_dynamic_cast(class,
> > >                                               object_class_get_name(valid_class))) {
> > >                      /* Valid CPU type, we're good to go */
> > >                      break;
> > >                 }
> > >             }
> > >             if (!machine_class->valid_cpu_models[i]) {
> > >                 error_report("Invalid CPU model: %s", cpu_model);
> > >                 error_printf("The valid CPU models are: %s",
> > >                              machine_class->valid_cpu_models[0]);
> > >                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > >                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> > >                 }
> > >                 error_printf("\n");
> > >                 exit(1);
> > >             }
> > >         }
> > >     }
> > > 
> > > This can be done inside main(), or moved inside
> > > machine_run_board_init() if main() pass cpu_model as argument to
> > > the function.
> > > 
> > > On either case, I think it's a good idea to do validation and
> > > printing of error messages closer to the code that parses the
> > > command-line options.  This way we separate parsing/validation
> > > from initialization.  
> > I agree it's better like you suggest as at least it prevents
> > ms->cpu_model creeping back into boards code.
> > 
> > But I still dislike (hate) an idea of new code adding non
> > canonized cpu_model strings back in the boards code.
> > It's just a matter of time when someone would use them
> > and cpu_model parsing will creep back into boards.
> > 
> > It would be much better to if we add 
> >    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > callback and let machines in this patchset to set it,
> > something along following lines which is not much of
> > refactoring and allows for gradual conversion:
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9631670..85cca84 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> >      return cpu->el_change_hook_opaque;
> >  }
> >  
> > +char *arm_cpu_name_by_type_name(const char *typename);
> > +
> >  #endif
> > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > index f936017..ae6adb7 100644
> > --- a/hw/arm/netduino2.c
> > +++ b/hw/arm/netduino2.c
> > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> >      mc->desc = "Netduino 2 Machine";
> >      mc->init = netduino2_init;
> >      mc->ignore_memory_transaction_failures = true;
> > +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:  
> 
> I really don't want to introduce a new arch-specific hook just
> for that.  We should move CPU type lookup logic to common code
> and make it unnecessary to write new hooks.
unfortunately cpu_model (cpu name part) is target specific
and it's translation to type and back is target specific mayhem.

So I'd prefer having both back and forth functions together in
one place. And common code to call them when necessary.

We could do global cpu_name_by_type_name() instead of hook,
which I'd prefer even more but then conversion can't be done
only for one target but rather for all targets at once.
 
> I agree it would be better if we had a cpu_name_by_type_name()
> function, but I would like to have it implemented cleanly.
In some cases(targets) it can be common helper, but in other
cases it's not so.
My suggestion though allows to do gradual conversion and
avoid putting cpu_model names back in board's code (which I just manged to remove).
Once all targets converted and relevant code is isolated
we can attempt to generalize it if it's possible or at least
make of it global per target helper to get rid of
temporary machine hook.

(seeing this series reposted with cpu_model names in boards code,
it doesn't looks like author would like to implement tree-wide
generalization first)

On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> On Mon, 5 Feb 2018 11:54:01 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> > > On Fri, 2 Feb 2018 16:23:26 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:  
> > > > > As cpu_type is not a user visible string let's convert the
> > > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > > user friendly string to report back.
> > > > > 
> > > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > > reverted and we should use cpu_type instead.
> > > > > 
> > > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > > ---
> > > > > 
> > > > >  hw/core/machine.c | 11 +++++------
> > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index cdc1163dc6..de5bac1c84 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > > >      /* If the machine supports the valid_cpu_types check and the user
> > > > >       * specified a CPU with -cpu check here that the user CPU is supported.
> > > > >       */
> > > > > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > > -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > >          int i;
> > > > >  
> > > > >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > > -            if (object_class_dynamic_cast(class,
> > > > > -                                          machine_class->valid_cpu_types[i])) {
> > > > > +            if (!strcmp(machine->cpu_model,
> > > > > +                        machine_class->valid_cpu_types[i])) {    
> > > > 
> > > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > > new semantics clearer.
> > > > 
> > > > Anyway, I have bad and good news:
> > > > 
> > > > The bad news is Igor already sent patches last week that remove
> > > > MachineState::cpu_model, so this conflicts with his series.  Now
> > > > parse_cpu_model() will be the only place where the original CPU model name is
> > > > available, but the function needs to work on *-user too.  See:
> > > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > > > 
> > > > The good news is that I think we can fix this very easily if
> > > > validation is done at the same place where parse_cpu_model() is
> > > > called.  e.g.:
> > > > 
> > > >     current_machine->cpu_type = machine_class->default_cpu_type;
> > > >     if (cpu_model) {
> > > >         current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > > 
> > > >         if (machine_class->valid_cpu_models) {
> > > >             ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > >             int i;
> > > > 
> > > >             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > >                 const char *valid_model = machine_class->valid_cpu_models[i];
> > > >                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > > >                 if (object_class_dynamic_cast(class,
> > > >                                               object_class_get_name(valid_class))) {
> > > >                      /* Valid CPU type, we're good to go */
> > > >                      break;
> > > >                 }
> > > >             }
> > > >             if (!machine_class->valid_cpu_models[i]) {
> > > >                 error_report("Invalid CPU model: %s", cpu_model);
> > > >                 error_printf("The valid CPU models are: %s",
> > > >                              machine_class->valid_cpu_models[0]);
> > > >                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > >                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> > > >                 }
> > > >                 error_printf("\n");
> > > >                 exit(1);
> > > >             }
> > > >         }
> > > >     }
> > > > 
> > > > This can be done inside main(), or moved inside
> > > > machine_run_board_init() if main() pass cpu_model as argument to
> > > > the function.
> > > > 
> > > > On either case, I think it's a good idea to do validation and
> > > > printing of error messages closer to the code that parses the
> > > > command-line options.  This way we separate parsing/validation
> > > > from initialization.  
> > > I agree it's better like you suggest as at least it prevents
> > > ms->cpu_model creeping back into boards code.
> > > 
> > > But I still dislike (hate) an idea of new code adding non
> > > canonized cpu_model strings back in the boards code.
> > > It's just a matter of time when someone would use them
> > > and cpu_model parsing will creep back into boards.
> > > 
> > > It would be much better to if we add 
> > >    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > > callback and let machines in this patchset to set it,
> > > something along following lines which is not much of
> > > refactoring and allows for gradual conversion:
> > > 
> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > index 9631670..85cca84 100644
> > > --- a/target/arm/cpu.h
> > > +++ b/target/arm/cpu.h
> > > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > >      return cpu->el_change_hook_opaque;
> > >  }
> > >  
> > > +char *arm_cpu_name_by_type_name(const char *typename);
> > > +
> > >  #endif
> > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > > index f936017..ae6adb7 100644
> > > --- a/hw/arm/netduino2.c
> > > +++ b/hw/arm/netduino2.c
> > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > >      mc->desc = "Netduino 2 Machine";
> > >      mc->init = netduino2_init;
> > >      mc->ignore_memory_transaction_failures = true;
> > > +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:  
> > 
> > I really don't want to introduce a new arch-specific hook just
> > for that.  We should move CPU type lookup logic to common code
> > and make it unnecessary to write new hooks.
> unfortunately cpu_model (cpu name part) is target specific
> and it's translation to type and back is target specific mayhem.

Why can't the model<->type translation be represented as data?
We could have simple cpu_type_name_suffix + an alias table.

We have at least 4 arches that return a constant at
class_by_name.  We have at least 10 arches that simply add a
suffix to the CPU model name.  We must make them use common code
instead of requiring them to implement yet another hook[1].

In addition to the ones above, we have 3 that seem to just need
an alias table (cris, superh, alpha).  ppc can probably also use
an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
needs whitespaces translated to '-' (sparc), which can be done
using an alias table.

In the end I couldn't find any example that can't be represented
by a suffix + alias table.


> 
> So I'd prefer having both back and forth functions together in
> one place. And common code to call them when necessary.
> 
> We could do global cpu_name_by_type_name() instead of hook,
> which I'd prefer even more but then conversion can't be done
> only for one target but rather for all targets at once.

I don't mind letting a few targets override default behavior with
a hook if really necessary, but I have a problem with requiring
all targets to implement what's basically the same boilerplate
code to add/remove a string suffix and translating aliases.


>  
> > I agree it would be better if we had a cpu_name_by_type_name()
> > function, but I would like to have it implemented cleanly.
> In some cases(targets) it can be common helper, but in other
> cases it's not so.
> My suggestion though allows to do gradual conversion and
> avoid putting cpu_model names back in board's code (which I just manged to remove).
> Once all targets converted and relevant code is isolated
> we can attempt to generalize it if it's possible or at least
> make of it global per target helper to get rid of
> temporary machine hook.
> 
> (seeing this series reposted with cpu_model names in boards code,
> it doesn't looks like author would like to implement tree-wide
> generalization first)

Well, if nobody is willing to generalize all target-specific code
right now, I don't see the harm in having cpu_model-based tables
in a few boards in the meantime (as this patch series does).  But
I do see harm in requiring all our 20 targets to implement yet
another hook and increasing the costs of cleaning up the mess
later.

---
[1] Really, this amount of duplication is insane:

static char *mips_cpu_type_name(const char *cpu_model)
{
    return g_strdup_printf(MIPS_CPU_TYPE_NAME("%s"), cpu_model);
}

static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = mips_cpu_type_name(cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    return oc;
}

static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU) ||
        object_class_is_abstract(oc)) {
        return NULL;
    }
    return oc;
}

static ObjectClass *uc32_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(UNICORE32_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_UNICORE32_CPU) ||
                       object_class_is_abstract(oc))) {
        oc = NULL;
    }
    return oc;
}

static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU) ||
        object_class_is_abstract(oc)) {
        return NULL;
    }
    return oc;
}

static char *x86_cpu_type_name(const char *model_name)
{
    return g_strdup_printf(X86_CPU_TYPE_NAME("%s"), model_name);
}

static ObjectClass *x86_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    if (cpu_model == NULL) {
        return NULL;
    }

    typename = x86_cpu_type_name(cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    return oc;
}

static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;
    char **cpuname;

    cpuname = g_strsplit(cpu_model, ",", 1);
    typename = g_strdup_printf(ARM_CPU_TYPE_NAME("%s"), cpuname[0]);
    oc = object_class_by_name(typename);
    g_strfreev(cpuname);
    g_free(typename);
    if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
        object_class_is_abstract(oc)) {
        return NULL;
    }
    return oc;
}

static ObjectClass *lm32_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(LM32_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_LM32_CPU) ||
                       object_class_is_abstract(oc))) {
        oc = NULL;
    }
    return oc;
}

static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc != NULL && (object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL ||
                       object_class_is_abstract(oc))) {
        return NULL;
    }
    return oc;
}

static ObjectClass *moxie_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(MOXIE_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_MOXIE_CPU) ||
                       object_class_is_abstract(oc))) {
        return NULL;
    }
    return oc;
}


static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
{
    ObjectClass *oc;
    char *typename;

    typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model);
    oc = object_class_by_name(typename);
    g_free(typename);
    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
                       object_class_is_abstract(oc))) {
        return NULL;
    }
    return oc;
}

On Mon, 5 Feb 2018 20:42:05 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> > On Mon, 5 Feb 2018 11:54:01 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 2 Feb 2018 16:23:26 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:    
> > > > > > As cpu_type is not a user visible string let's convert the
> > > > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > > > user friendly string to report back.
> > > > > > 
> > > > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > > > reverted and we should use cpu_type instead.
> > > > > > 
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > > > ---
> > > > > > 
> > > > > >  hw/core/machine.c | 11 +++++------
> > > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index cdc1163dc6..de5bac1c84 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > > > >      /* If the machine supports the valid_cpu_types check and the user
> > > > > >       * specified a CPU with -cpu check here that the user CPU is supported.
> > > > > >       */
> > > > > > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > > > -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > > > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > > >          int i;
> > > > > >  
> > > > > >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > > > -            if (object_class_dynamic_cast(class,
> > > > > > -                                          machine_class->valid_cpu_types[i])) {
> > > > > > +            if (!strcmp(machine->cpu_model,
> > > > > > +                        machine_class->valid_cpu_types[i])) {      
> > > > > 
> > > > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > > > new semantics clearer.
> > > > > 
> > > > > Anyway, I have bad and good news:
> > > > > 
> > > > > The bad news is Igor already sent patches last week that remove
> > > > > MachineState::cpu_model, so this conflicts with his series.  Now
> > > > > parse_cpu_model() will be the only place where the original CPU model name is
> > > > > available, but the function needs to work on *-user too.  See:
> > > > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > > > > 
> > > > > The good news is that I think we can fix this very easily if
> > > > > validation is done at the same place where parse_cpu_model() is
> > > > > called.  e.g.:
> > > > > 
> > > > >     current_machine->cpu_type = machine_class->default_cpu_type;
> > > > >     if (cpu_model) {
> > > > >         current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > > > 
> > > > >         if (machine_class->valid_cpu_models) {
> > > > >             ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > >             int i;
> > > > > 
> > > > >             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > > >                 const char *valid_model = machine_class->valid_cpu_models[i];
> > > > >                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > > > >                 if (object_class_dynamic_cast(class,
> > > > >                                               object_class_get_name(valid_class))) {
> > > > >                      /* Valid CPU type, we're good to go */
> > > > >                      break;
> > > > >                 }
> > > > >             }
> > > > >             if (!machine_class->valid_cpu_models[i]) {
> > > > >                 error_report("Invalid CPU model: %s", cpu_model);
> > > > >                 error_printf("The valid CPU models are: %s",
> > > > >                              machine_class->valid_cpu_models[0]);
> > > > >                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > > >                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> > > > >                 }
> > > > >                 error_printf("\n");
> > > > >                 exit(1);
> > > > >             }
> > > > >         }
> > > > >     }
> > > > > 
> > > > > This can be done inside main(), or moved inside
> > > > > machine_run_board_init() if main() pass cpu_model as argument to
> > > > > the function.
> > > > > 
> > > > > On either case, I think it's a good idea to do validation and
> > > > > printing of error messages closer to the code that parses the
> > > > > command-line options.  This way we separate parsing/validation
> > > > > from initialization.    
> > > > I agree it's better like you suggest as at least it prevents
> > > > ms->cpu_model creeping back into boards code.
> > > > 
> > > > But I still dislike (hate) an idea of new code adding non
> > > > canonized cpu_model strings back in the boards code.
> > > > It's just a matter of time when someone would use them
> > > > and cpu_model parsing will creep back into boards.
> > > > 
> > > > It would be much better to if we add 
> > > >    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > > > callback and let machines in this patchset to set it,
> > > > something along following lines which is not much of
> > > > refactoring and allows for gradual conversion:
> > > > 
> > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > index 9631670..85cca84 100644
> > > > --- a/target/arm/cpu.h
> > > > +++ b/target/arm/cpu.h
> > > > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > > >      return cpu->el_change_hook_opaque;
> > > >  }
> > > >  
> > > > +char *arm_cpu_name_by_type_name(const char *typename);
> > > > +
> > > >  #endif
> > > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > > > index f936017..ae6adb7 100644
> > > > --- a/hw/arm/netduino2.c
> > > > +++ b/hw/arm/netduino2.c
> > > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > > >      mc->desc = "Netduino 2 Machine";
> > > >      mc->init = netduino2_init;
> > > >      mc->ignore_memory_transaction_failures = true;
> > > > +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:    
> > > 
> > > I really don't want to introduce a new arch-specific hook just
> > > for that.  We should move CPU type lookup logic to common code
> > > and make it unnecessary to write new hooks.  
> > unfortunately cpu_model (cpu name part) is target specific
> > and it's translation to type and back is target specific mayhem.  
> 
> Why can't the model<->type translation be represented as data?
> We could have simple cpu_type_name_suffix + an alias table.
> 
> We have at least 4 arches that return a constant at
> class_by_name.  We have at least 10 arches that simply add a
> suffix to the CPU model name.  We must make them use common code
> instead of requiring them to implement yet another hook[1].
True, some of them could use generic hook and reduce
code duplication greatly, we should do it regardless of whether
table or target specific func approach is used.

> In addition to the ones above, we have 3 that seem to just need
> an alias table (cris, superh, alpha).  ppc can probably also use
> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
> needs whitespaces translated to '-' (sparc), which can be done
> using an alias table.
> 
> In the end I couldn't find any example that can't be represented
> by a suffix + alias table.

Table based approach is possible but it won't be as simple
as you've just pictured it.

From what I recall from cpu_class_by_name cleanups table should be able
to describe cases like (sometimes combination of them):
   * 1:1 mapping - where cpu_model == cpu_type
   * cpu_model <==> cpu_model + suffix  - most common usecase
   * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add such cpu types
   * NULL => some_fixed type
   * case (in) sensitive flag
   * garbage => some_fixed type
   * substitutions
   * aliases (sometimes dynamic depending on --enable-kvm (PPC))
Maybe something else.

We can think about it at leisure but I can't say if new approach
complexity it's worth of the effort.
 
It would be nice see impl, but it's a lot of refactoring that's
clearly out of scope of this series.
I'd prefer small incremental refactoring (if possible) that
won't scare people of and easy to review vs a huge one.

> > So I'd prefer having both back and forth functions together in
> > one place. And common code to call them when necessary.
> > 
> > We could do global cpu_name_by_type_name() instead of hook,
> > which I'd prefer even more but then conversion can't be done
> > only for one target but rather for all targets at once.  
> 
> I don't mind letting a few targets override default behavior with
> a hook if really necessary, but I have a problem with requiring
> all targets to implement what's basically the same boilerplate
> code to add/remove a string suffix and translating aliases.
it could be generic helper if target does the same plus
not mandatory at that (in case target/board doesn't care
about valid cpus).

> > > I agree it would be better if we had a cpu_name_by_type_name()
> > > function, but I would like to have it implemented cleanly.  
> > In some cases(targets) it can be common helper, but in other
> > cases it's not so.
> > My suggestion though allows to do gradual conversion and
> > avoid putting cpu_model names back in board's code (which I just manged to remove).
> > Once all targets converted and relevant code is isolated
> > we can attempt to generalize it if it's possible or at least
> > make of it global per target helper to get rid of
> > temporary machine hook.
> > 
> > (seeing this series reposted with cpu_model names in boards code,
> > it doesn't looks like author would like to implement tree-wide
> > generalization first)  
> 
> Well, if nobody is willing to generalize all target-specific code
> right now, I don't see the harm in having cpu_model-based tables
> in a few boards in the meantime (as this patch series does).  But
> I do see harm in requiring all our 20 targets to implement yet
> another hook and increasing the costs of cleaning up the mess
> later.
If we use MachineClass hook then it might be done per target
on demand, so no one would require that every target should
implement it.
Also there could be a generic helper for targets that do the same.
Machine which needs to enable valid_cpus, will have to use generic
hook impl or provide target specific if it's special case.

Though I do see harm in adding cpu_model tables in boards code
vs target specific hooks on demand as that will be copy-pasted
in other boards afterwards (number of which is bigger compared
to targets count) and ultimately it would duplicate cpu_name
strings in every board vs hook approach where cpu_model could
be calculated from cpu_type by a function (generic or
target specific).

Good thing about hook is that it's non intrusive and
isolates(consolidates) existing cpu_type -> cpu_model
conversion in multiple places into one place.
Then later it would be easier to generalize if someone
decides to do it.

Hi Igor, Eduardo,

On 2/6/18 3:43 PM, Igor Mammedov wrote:
> On Mon, 5 Feb 2018 20:42:05 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
>>> On Mon, 5 Feb 2018 11:54:01 -0200
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>   
>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:  
>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>     
>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:    
>>>>>>> As cpu_type is not a user visible string let's convert the
>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
>>>>>>> user friendly string to report back.
>>>>>>>
>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
>>>>>>> reverted and we should use cpu_type instead.
>>>>>>>
>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  hw/core/machine.c | 11 +++++------
>>>>>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>>>> index cdc1163dc6..de5bac1c84 100644
>>>>>>> --- a/hw/core/machine.c
>>>>>>> +++ b/hw/core/machine.c
>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
>>>>>>>      /* If the machine supports the valid_cpu_types check and the user
>>>>>>>       * specified a CPU with -cpu check here that the user CPU is supported.
>>>>>>>       */
>>>>>>> -    if (machine_class->valid_cpu_types && machine->cpu_type) {
>>>>>>> -        ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>> +    if (machine_class->valid_cpu_types && machine->cpu_model) {
>>>>>>>          int i;
>>>>>>>  
>>>>>>>          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>>>>>>> -            if (object_class_dynamic_cast(class,
>>>>>>> -                                          machine_class->valid_cpu_types[i])) {
>>>>>>> +            if (!strcmp(machine->cpu_model,
>>>>>>> +                        machine_class->valid_cpu_types[i])) {      
>>>>>>
>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
>>>>>> new semantics clearer.
>>>>>>
>>>>>> Anyway, I have bad and good news:
>>>>>>
>>>>>> The bad news is Igor already sent patches last week that remove
>>>>>> MachineState::cpu_model, so this conflicts with his series.  Now
>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
>>>>>> available, but the function needs to work on *-user too.  See:
>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
>>>>>>
>>>>>> The good news is that I think we can fix this very easily if
>>>>>> validation is done at the same place where parse_cpu_model() is
>>>>>> called.  e.g.:
>>>>>>
>>>>>>     current_machine->cpu_type = machine_class->default_cpu_type;
>>>>>>     if (cpu_model) {
>>>>>>         current_machine->cpu_type = parse_cpu_model(cpu_model);
>>>>>>
>>>>>>         if (machine_class->valid_cpu_models) {
>>>>>>             ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>             int i;
>>>>>>
>>>>>>             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
>>>>>>                 const char *valid_model = machine_class->valid_cpu_models[i];
>>>>>>                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
>>>>>>                 if (object_class_dynamic_cast(class,
>>>>>>                                               object_class_get_name(valid_class))) {
>>>>>>                      /* Valid CPU type, we're good to go */
>>>>>>                      break;
>>>>>>                 }
>>>>>>             }
>>>>>>             if (!machine_class->valid_cpu_models[i]) {
>>>>>>                 error_report("Invalid CPU model: %s", cpu_model);
>>>>>>                 error_printf("The valid CPU models are: %s",
>>>>>>                              machine_class->valid_cpu_models[0]);
>>>>>>                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
>>>>>>                     error_printf(", %s", machine_class->valid_cpu_models[i]);
>>>>>>                 }
>>>>>>                 error_printf("\n");
>>>>>>                 exit(1);
>>>>>>             }
>>>>>>         }
>>>>>>     }
>>>>>>
>>>>>> This can be done inside main(), or moved inside
>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
>>>>>> the function.
>>>>>>
>>>>>> On either case, I think it's a good idea to do validation and
>>>>>> printing of error messages closer to the code that parses the
>>>>>> command-line options.  This way we separate parsing/validation
>>>>>> from initialization.    
>>>>> I agree it's better like you suggest as at least it prevents
>>>>> ms->cpu_model creeping back into boards code.
>>>>>
>>>>> But I still dislike (hate) an idea of new code adding non
>>>>> canonized cpu_model strings back in the boards code.
>>>>> It's just a matter of time when someone would use them
>>>>> and cpu_model parsing will creep back into boards.
>>>>>
>>>>> It would be much better to if we add 
>>>>>    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
>>>>> callback and let machines in this patchset to set it,
>>>>> something along following lines which is not much of
>>>>> refactoring and allows for gradual conversion:
>>>>>
>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>> index 9631670..85cca84 100644
>>>>> --- a/target/arm/cpu.h
>>>>> +++ b/target/arm/cpu.h
>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
>>>>>      return cpu->el_change_hook_opaque;
>>>>>  }
>>>>>  
>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
>>>>> +
>>>>>  #endif
>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>>>>> index f936017..ae6adb7 100644
>>>>> --- a/hw/arm/netduino2.c
>>>>> +++ b/hw/arm/netduino2.c
>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
>>>>>      mc->desc = "Netduino 2 Machine";
>>>>>      mc->init = netduino2_init;
>>>>>      mc->ignore_memory_transaction_failures = true;
>>>>> +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:    
>>>>
>>>> I really don't want to introduce a new arch-specific hook just
>>>> for that.  We should move CPU type lookup logic to common code
>>>> and make it unnecessary to write new hooks.  
>>> unfortunately cpu_model (cpu name part) is target specific
>>> and it's translation to type and back is target specific mayhem.  
>>
>> Why can't the model<->type translation be represented as data?
>> We could have simple cpu_type_name_suffix + an alias table.
>>
>> We have at least 4 arches that return a constant at
>> class_by_name.  We have at least 10 arches that simply add a
>> suffix to the CPU model name.  We must make them use common code
>> instead of requiring them to implement yet another hook[1].
> True, some of them could use generic hook and reduce
> code duplication greatly, we should do it regardless of whether
> table or target specific func approach is used.
> 
>> In addition to the ones above, we have 3 that seem to just need
>> an alias table (cris, superh, alpha).  ppc can probably also use
>> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
>> needs whitespaces translated to '-' (sparc), which can be done
>> using an alias table.
>>
>> In the end I couldn't find any example that can't be represented
>> by a suffix + alias table.
> 
> Table based approach is possible but it won't be as simple
> as you've just pictured it.
> 
> From what I recall from cpu_class_by_name cleanups table should be able
> to describe cases like (sometimes combination of them):
>    * 1:1 mapping - where cpu_model == cpu_type
>    * cpu_model <==> cpu_model + suffix  - most common usecase
>    * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add such cpu types
>    * NULL => some_fixed type
>    * case (in) sensitive flag
>    * garbage => some_fixed type
>    * substitutions
>    * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> Maybe something else.
> 
> We can think about it at leisure but I can't say if new approach
> complexity it's worth of the effort.
>  
> It would be nice see impl, but it's a lot of refactoring that's
> clearly out of scope of this series.
> I'd prefer small incremental refactoring (if possible) that
> won't scare people of and easy to review vs a huge one.
> 
>>> So I'd prefer having both back and forth functions together in
>>> one place. And common code to call them when necessary.
>>>
>>> We could do global cpu_name_by_type_name() instead of hook,
>>> which I'd prefer even more but then conversion can't be done
>>> only for one target but rather for all targets at once.  
>>
>> I don't mind letting a few targets override default behavior with
>> a hook if really necessary, but I have a problem with requiring
>> all targets to implement what's basically the same boilerplate
>> code to add/remove a string suffix and translating aliases.
> it could be generic helper if target does the same plus
> not mandatory at that (in case target/board doesn't care
> about valid cpus).
> 
>>>> I agree it would be better if we had a cpu_name_by_type_name()
>>>> function, but I would like to have it implemented cleanly.  
>>> In some cases(targets) it can be common helper, but in other
>>> cases it's not so.
>>> My suggestion though allows to do gradual conversion and
>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
>>> Once all targets converted and relevant code is isolated
>>> we can attempt to generalize it if it's possible or at least
>>> make of it global per target helper to get rid of
>>> temporary machine hook.
>>>
>>> (seeing this series reposted with cpu_model names in boards code,
>>> it doesn't looks like author would like to implement tree-wide
>>> generalization first)  
>>
>> Well, if nobody is willing to generalize all target-specific code
>> right now, I don't see the harm in having cpu_model-based tables
>> in a few boards in the meantime (as this patch series does).  But
>> I do see harm in requiring all our 20 targets to implement yet
>> another hook and increasing the costs of cleaning up the mess
>> later.
> If we use MachineClass hook then it might be done per target
> on demand, so no one would require that every target should
> implement it.
> Also there could be a generic helper for targets that do the same.
> Machine which needs to enable valid_cpus, will have to use generic
> hook impl or provide target specific if it's special case.
> 
> Though I do see harm in adding cpu_model tables in boards code
> vs target specific hooks on demand as that will be copy-pasted
> in other boards afterwards (number of which is bigger compared
> to targets count) and ultimately it would duplicate cpu_name
> strings in every board vs hook approach where cpu_model could
> be calculated from cpu_type by a function (generic or
> target specific).
> 
> Good thing about hook is that it's non intrusive and
> isolates(consolidates) existing cpu_type -> cpu_model
> conversion in multiple places into one place.
> Then later it would be easier to generalize if someone
> decides to do it.

I wonder how you want to proceed with this series, the first patch got
merged (c9cf636d48f) but after your "CPU model name" rework, this commit
seems now not very complete/usable.

Rebasing this series, i.e. with this snippet:

-- >8 --
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f57fc38f92..cca4ec6648 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
     DeviceState *dev;

     dev = qdev_create(NULL, TYPE_STM32F205_SOC);
-    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
+    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
     object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);

     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
@@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)

 static void netduino2_machine_init(MachineClass *mc)
 {
+    static const char *valid_cpus[] = {
+        ARM_CPU_TYPE_NAME("cortex-m3"),
+        ARM_CPU_TYPE_NAME("cortex-m4"),
+        NULL
+    };
     mc->desc = "Netduino 2 Machine";
     mc->init = netduino2_init;
+    mc->valid_cpu_types = valid_cpus;
     mc->ignore_memory_transaction_failures = true;
 }
---

We get cpu names with suffix:

  $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
  qemu-system-arm: Invalid CPU type: arm926-arm-cpu
  The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu

I understand you won't want a global cpu_name_by_type_name, how do you
want to do then?

Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
Then we could have generic machine code to parse the names.

Thanks,

Phil.

On Mon, Jun 17, 2019 at 07:09:59AM +0200, Philippe Mathieu-Daudé wrote:
[...]
> 
> We get cpu names with suffix:
> 
>   $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
>   qemu-system-arm: Invalid CPU type: arm926-arm-cpu
>   The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
> 
> I understand you won't want a global cpu_name_by_type_name, how do you
> want to do then?

I would like having a global cpu_name_by_type_name() function.
I wouldn't want to force each architecture to provide yet another
string conversion/parsing function, though.

> 
> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> Then we could have generic machine code to parse the names.

I think we could do that, but with a CPUClass field instead of a
preprocessor macro.

For a reference on existing corner cases of CPU model name
parsing that might get in the way, see this series:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg611813.html

If you are interested, I have work in progress for a generic CPU
model listing function at:
https://github.com/ehabkost/qemu-hacks/commits/work/cpu-generic-list
https://github.com/ehabkost/qemu-hacks/commit/df122e7e47476d7e2b7b809a4b4120537f50c137

On Mon, 17 Jun 2019 07:09:59 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor, Eduardo,
> 
> On 2/6/18 3:43 PM, Igor Mammedov wrote:
> > On Mon, 5 Feb 2018 20:42:05 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> >> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:  
> >>> On Mon, 5 Feb 2018 11:54:01 -0200
> >>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>     
> >>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:    
> >>>>> On Fri, 2 Feb 2018 16:23:26 -0200
> >>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>       
> >>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:      
> >>>>>>> As cpu_type is not a user visible string let's convert the
> >>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
> >>>>>>> user friendly string to report back.
> >>>>>>>
> >>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
> >>>>>>> reverted and we should use cpu_type instead.
> >>>>>>>
> >>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>>  hw/core/machine.c | 11 +++++------
> >>>>>>>  1 file changed, 5 insertions(+), 6 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >>>>>>> index cdc1163dc6..de5bac1c84 100644
> >>>>>>> --- a/hw/core/machine.c
> >>>>>>> +++ b/hw/core/machine.c
> >>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> >>>>>>>      /* If the machine supports the valid_cpu_types check and the user
> >>>>>>>       * specified a CPU with -cpu check here that the user CPU is supported.
> >>>>>>>       */
> >>>>>>> -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> >>>>>>> -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>> +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> >>>>>>>          int i;
> >>>>>>>  
> >>>>>>>          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >>>>>>> -            if (object_class_dynamic_cast(class,
> >>>>>>> -                                          machine_class->valid_cpu_types[i])) {
> >>>>>>> +            if (!strcmp(machine->cpu_model,
> >>>>>>> +                        machine_class->valid_cpu_types[i])) {        
> >>>>>>
> >>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
> >>>>>> new semantics clearer.
> >>>>>>
> >>>>>> Anyway, I have bad and good news:
> >>>>>>
> >>>>>> The bad news is Igor already sent patches last week that remove
> >>>>>> MachineState::cpu_model, so this conflicts with his series.  Now
> >>>>>> parse_cpu_model() will be the only place where the original CPU model name is
> >>>>>> available, but the function needs to work on *-user too.  See:
> >>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> >>>>>>
> >>>>>> The good news is that I think we can fix this very easily if
> >>>>>> validation is done at the same place where parse_cpu_model() is
> >>>>>> called.  e.g.:
> >>>>>>
> >>>>>>     current_machine->cpu_type = machine_class->default_cpu_type;
> >>>>>>     if (cpu_model) {
> >>>>>>         current_machine->cpu_type = parse_cpu_model(cpu_model);
> >>>>>>
> >>>>>>         if (machine_class->valid_cpu_models) {
> >>>>>>             ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>             int i;
> >>>>>>
> >>>>>>             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>                 const char *valid_model = machine_class->valid_cpu_models[i];
> >>>>>>                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> >>>>>>                 if (object_class_dynamic_cast(class,
> >>>>>>                                               object_class_get_name(valid_class))) {
> >>>>>>                      /* Valid CPU type, we're good to go */
> >>>>>>                      break;
> >>>>>>                 }
> >>>>>>             }
> >>>>>>             if (!machine_class->valid_cpu_models[i]) {
> >>>>>>                 error_report("Invalid CPU model: %s", cpu_model);
> >>>>>>                 error_printf("The valid CPU models are: %s",
> >>>>>>                              machine_class->valid_cpu_models[0]);
> >>>>>>                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> >>>>>>                 }
> >>>>>>                 error_printf("\n");
> >>>>>>                 exit(1);
> >>>>>>             }
> >>>>>>         }
> >>>>>>     }
> >>>>>>
> >>>>>> This can be done inside main(), or moved inside
> >>>>>> machine_run_board_init() if main() pass cpu_model as argument to
> >>>>>> the function.
> >>>>>>
> >>>>>> On either case, I think it's a good idea to do validation and
> >>>>>> printing of error messages closer to the code that parses the
> >>>>>> command-line options.  This way we separate parsing/validation
> >>>>>> from initialization.      
> >>>>> I agree it's better like you suggest as at least it prevents
> >>>>> ms->cpu_model creeping back into boards code.
> >>>>>
> >>>>> But I still dislike (hate) an idea of new code adding non
> >>>>> canonized cpu_model strings back in the boards code.
> >>>>> It's just a matter of time when someone would use them
> >>>>> and cpu_model parsing will creep back into boards.
> >>>>>
> >>>>> It would be much better to if we add 
> >>>>>    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> >>>>> callback and let machines in this patchset to set it,
> >>>>> something along following lines which is not much of
> >>>>> refactoring and allows for gradual conversion:
> >>>>>
> >>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >>>>> index 9631670..85cca84 100644
> >>>>> --- a/target/arm/cpu.h
> >>>>> +++ b/target/arm/cpu.h
> >>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> >>>>>      return cpu->el_change_hook_opaque;
> >>>>>  }
> >>>>>  
> >>>>> +char *arm_cpu_name_by_type_name(const char *typename);
> >>>>> +
> >>>>>  #endif
> >>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >>>>> index f936017..ae6adb7 100644
> >>>>> --- a/hw/arm/netduino2.c
> >>>>> +++ b/hw/arm/netduino2.c
> >>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> >>>>>      mc->desc = "Netduino 2 Machine";
> >>>>>      mc->init = netduino2_init;
> >>>>>      mc->ignore_memory_transaction_failures = true;
> >>>>> +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:      
> >>>>
> >>>> I really don't want to introduce a new arch-specific hook just
> >>>> for that.  We should move CPU type lookup logic to common code
> >>>> and make it unnecessary to write new hooks.    
> >>> unfortunately cpu_model (cpu name part) is target specific
> >>> and it's translation to type and back is target specific mayhem.    
> >>
> >> Why can't the model<->type translation be represented as data?
> >> We could have simple cpu_type_name_suffix + an alias table.
> >>
> >> We have at least 4 arches that return a constant at
> >> class_by_name.  We have at least 10 arches that simply add a
> >> suffix to the CPU model name.  We must make them use common code
> >> instead of requiring them to implement yet another hook[1].  
> > True, some of them could use generic hook and reduce
> > code duplication greatly, we should do it regardless of whether
> > table or target specific func approach is used.
> >   
> >> In addition to the ones above, we have 3 that seem to just need
> >> an alias table (cris, superh, alpha).  ppc can probably also use
> >> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
> >> needs whitespaces translated to '-' (sparc), which can be done
> >> using an alias table.
> >>
> >> In the end I couldn't find any example that can't be represented
> >> by a suffix + alias table.  
> > 
> > Table based approach is possible but it won't be as simple
> > as you've just pictured it.
> > 
> > From what I recall from cpu_class_by_name cleanups table should be able
> > to describe cases like (sometimes combination of them):
> >    * 1:1 mapping - where cpu_model == cpu_type
> >    * cpu_model <==> cpu_model + suffix  - most common usecase
> >    * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add such cpu types
> >    * NULL => some_fixed type
> >    * case (in) sensitive flag
> >    * garbage => some_fixed type
> >    * substitutions
> >    * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> > Maybe something else.
> > 
> > We can think about it at leisure but I can't say if new approach
> > complexity it's worth of the effort.
> >  
> > It would be nice see impl, but it's a lot of refactoring that's
> > clearly out of scope of this series.
> > I'd prefer small incremental refactoring (if possible) that
> > won't scare people of and easy to review vs a huge one.
> >   
> >>> So I'd prefer having both back and forth functions together in
> >>> one place. And common code to call them when necessary.
> >>>
> >>> We could do global cpu_name_by_type_name() instead of hook,
> >>> which I'd prefer even more but then conversion can't be done
> >>> only for one target but rather for all targets at once.    
> >>
> >> I don't mind letting a few targets override default behavior with
> >> a hook if really necessary, but I have a problem with requiring
> >> all targets to implement what's basically the same boilerplate
> >> code to add/remove a string suffix and translating aliases.  
> > it could be generic helper if target does the same plus
> > not mandatory at that (in case target/board doesn't care
> > about valid cpus).
> >   
> >>>> I agree it would be better if we had a cpu_name_by_type_name()
> >>>> function, but I would like to have it implemented cleanly.    
> >>> In some cases(targets) it can be common helper, but in other
> >>> cases it's not so.
> >>> My suggestion though allows to do gradual conversion and
> >>> avoid putting cpu_model names back in board's code (which I just manged to remove).
> >>> Once all targets converted and relevant code is isolated
> >>> we can attempt to generalize it if it's possible or at least
> >>> make of it global per target helper to get rid of
> >>> temporary machine hook.
> >>>
> >>> (seeing this series reposted with cpu_model names in boards code,
> >>> it doesn't looks like author would like to implement tree-wide
> >>> generalization first)    
> >>
> >> Well, if nobody is willing to generalize all target-specific code
> >> right now, I don't see the harm in having cpu_model-based tables
> >> in a few boards in the meantime (as this patch series does).  But
> >> I do see harm in requiring all our 20 targets to implement yet
> >> another hook and increasing the costs of cleaning up the mess
> >> later.  
> > If we use MachineClass hook then it might be done per target
> > on demand, so no one would require that every target should
> > implement it.
> > Also there could be a generic helper for targets that do the same.
> > Machine which needs to enable valid_cpus, will have to use generic
> > hook impl or provide target specific if it's special case.
> > 
> > Though I do see harm in adding cpu_model tables in boards code
> > vs target specific hooks on demand as that will be copy-pasted
> > in other boards afterwards (number of which is bigger compared
> > to targets count) and ultimately it would duplicate cpu_name
> > strings in every board vs hook approach where cpu_model could
> > be calculated from cpu_type by a function (generic or
> > target specific).
> > 
> > Good thing about hook is that it's non intrusive and
> > isolates(consolidates) existing cpu_type -> cpu_model
> > conversion in multiple places into one place.
> > Then later it would be easier to generalize if someone
> > decides to do it.  
> 
> I wonder how you want to proceed with this series, the first patch got
> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
> seems now not very complete/usable.
> 
> Rebasing this series, i.e. with this snippet:
> 
> -- >8 --  
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f57fc38f92..cca4ec6648 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
>      DeviceState *dev;
> 
>      dev = qdev_create(NULL, TYPE_STM32F205_SOC);
> -    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
> +    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
>      object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
> 
>      armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
> 
>  static void netduino2_machine_init(MachineClass *mc)
>  {
> +    static const char *valid_cpus[] = {
> +        ARM_CPU_TYPE_NAME("cortex-m3"),
> +        ARM_CPU_TYPE_NAME("cortex-m4"),
> +        NULL
> +    };
>      mc->desc = "Netduino 2 Machine";
>      mc->init = netduino2_init;
> +    mc->valid_cpu_types = valid_cpus;
>      mc->ignore_memory_transaction_failures = true;
>  }
> ---
> 
> We get cpu names with suffix:
> 
>   $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
>   qemu-system-arm: Invalid CPU type: arm926-arm-cpu
>   The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
> 
> I understand you won't want a global cpu_name_by_type_name, how do you
> want to do then?
> 
> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> Then we could have generic machine code to parse the names.
It would work only for some cases,
problem is that we have a zoo of naming schemes.
Considering that cpus models are used widely we probably can't
deprecate it outright (for versioned machine types).  

Instead of wasting resources on translating cpu-type => 'cpu-name',
(hook or lookup tables) how about simplifying code and making all
boards accept full typenames?

It could be handled in generic way and then printing error with
full type names would be acceptable since user would be able to feed it
to -cpu.

'-cpu help' - would need some work to display types as well (also could be generic)

Perhaps with it we could deprecate cpu_models for non versioned
machines/targets, which in most cases would allow us to drop special
suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
is already existing and keep exiting translation routines (hooks) only
for versioned machine types as necessary evil.


> Thanks,
> 
> Phil.

On 6/17/19 5:01 PM, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 07:09:59 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Hi Igor, Eduardo,
>>
>> On 2/6/18 3:43 PM, Igor Mammedov wrote:
>>> On Mon, 5 Feb 2018 20:42:05 -0200
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>   
>>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:  
>>>>> On Mon, 5 Feb 2018 11:54:01 -0200
>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>     
>>>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:    
>>>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
>>>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>>       
>>>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:      
>>>>>>>>> As cpu_type is not a user visible string let's convert the
>>>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
>>>>>>>>> user friendly string to report back.
>>>>>>>>>
>>>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
>>>>>>>>> reverted and we should use cpu_type instead.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  hw/core/machine.c | 11 +++++------
>>>>>>>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>>>>>> index cdc1163dc6..de5bac1c84 100644
>>>>>>>>> --- a/hw/core/machine.c
>>>>>>>>> +++ b/hw/core/machine.c
>>>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
>>>>>>>>>      /* If the machine supports the valid_cpu_types check and the user
>>>>>>>>>       * specified a CPU with -cpu check here that the user CPU is supported.
>>>>>>>>>       */
>>>>>>>>> -    if (machine_class->valid_cpu_types && machine->cpu_type) {
>>>>>>>>> -        ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>>>> +    if (machine_class->valid_cpu_types && machine->cpu_model) {
>>>>>>>>>          int i;
>>>>>>>>>  
>>>>>>>>>          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>>>>>>>>> -            if (object_class_dynamic_cast(class,
>>>>>>>>> -                                          machine_class->valid_cpu_types[i])) {
>>>>>>>>> +            if (!strcmp(machine->cpu_model,
>>>>>>>>> +                        machine_class->valid_cpu_types[i])) {        
>>>>>>>>
>>>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
>>>>>>>> new semantics clearer.
>>>>>>>>
>>>>>>>> Anyway, I have bad and good news:
>>>>>>>>
>>>>>>>> The bad news is Igor already sent patches last week that remove
>>>>>>>> MachineState::cpu_model, so this conflicts with his series.  Now
>>>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
>>>>>>>> available, but the function needs to work on *-user too.  See:
>>>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
>>>>>>>>
>>>>>>>> The good news is that I think we can fix this very easily if
>>>>>>>> validation is done at the same place where parse_cpu_model() is
>>>>>>>> called.  e.g.:
>>>>>>>>
>>>>>>>>     current_machine->cpu_type = machine_class->default_cpu_type;
>>>>>>>>     if (cpu_model) {
>>>>>>>>         current_machine->cpu_type = parse_cpu_model(cpu_model);
>>>>>>>>
>>>>>>>>         if (machine_class->valid_cpu_models) {
>>>>>>>>             ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>>>             int i;
>>>>>>>>
>>>>>>>>             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
>>>>>>>>                 const char *valid_model = machine_class->valid_cpu_models[i];
>>>>>>>>                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
>>>>>>>>                 if (object_class_dynamic_cast(class,
>>>>>>>>                                               object_class_get_name(valid_class))) {
>>>>>>>>                      /* Valid CPU type, we're good to go */
>>>>>>>>                      break;
>>>>>>>>                 }
>>>>>>>>             }
>>>>>>>>             if (!machine_class->valid_cpu_models[i]) {
>>>>>>>>                 error_report("Invalid CPU model: %s", cpu_model);
>>>>>>>>                 error_printf("The valid CPU models are: %s",
>>>>>>>>                              machine_class->valid_cpu_models[0]);
>>>>>>>>                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
>>>>>>>>                     error_printf(", %s", machine_class->valid_cpu_models[i]);
>>>>>>>>                 }
>>>>>>>>                 error_printf("\n");
>>>>>>>>                 exit(1);
>>>>>>>>             }
>>>>>>>>         }
>>>>>>>>     }
>>>>>>>>
>>>>>>>> This can be done inside main(), or moved inside
>>>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
>>>>>>>> the function.
>>>>>>>>
>>>>>>>> On either case, I think it's a good idea to do validation and
>>>>>>>> printing of error messages closer to the code that parses the
>>>>>>>> command-line options.  This way we separate parsing/validation
>>>>>>>> from initialization.      
>>>>>>> I agree it's better like you suggest as at least it prevents
>>>>>>> ms->cpu_model creeping back into boards code.
>>>>>>>
>>>>>>> But I still dislike (hate) an idea of new code adding non
>>>>>>> canonized cpu_model strings back in the boards code.
>>>>>>> It's just a matter of time when someone would use them
>>>>>>> and cpu_model parsing will creep back into boards.
>>>>>>>
>>>>>>> It would be much better to if we add 
>>>>>>>    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
>>>>>>> callback and let machines in this patchset to set it,
>>>>>>> something along following lines which is not much of
>>>>>>> refactoring and allows for gradual conversion:
>>>>>>>
>>>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>>>> index 9631670..85cca84 100644
>>>>>>> --- a/target/arm/cpu.h
>>>>>>> +++ b/target/arm/cpu.h
>>>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
>>>>>>>      return cpu->el_change_hook_opaque;
>>>>>>>  }
>>>>>>>  
>>>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
>>>>>>> +
>>>>>>>  #endif
>>>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>>>>>>> index f936017..ae6adb7 100644
>>>>>>> --- a/hw/arm/netduino2.c
>>>>>>> +++ b/hw/arm/netduino2.c
>>>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
>>>>>>>      mc->desc = "Netduino 2 Machine";
>>>>>>>      mc->init = netduino2_init;
>>>>>>>      mc->ignore_memory_transaction_failures = true;
>>>>>>> +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:      
>>>>>>
>>>>>> I really don't want to introduce a new arch-specific hook just
>>>>>> for that.  We should move CPU type lookup logic to common code
>>>>>> and make it unnecessary to write new hooks.    
>>>>> unfortunately cpu_model (cpu name part) is target specific
>>>>> and it's translation to type and back is target specific mayhem.    
>>>>
>>>> Why can't the model<->type translation be represented as data?
>>>> We could have simple cpu_type_name_suffix + an alias table.
>>>>
>>>> We have at least 4 arches that return a constant at
>>>> class_by_name.  We have at least 10 arches that simply add a
>>>> suffix to the CPU model name.  We must make them use common code
>>>> instead of requiring them to implement yet another hook[1].  
>>> True, some of them could use generic hook and reduce
>>> code duplication greatly, we should do it regardless of whether
>>> table or target specific func approach is used.
>>>   
>>>> In addition to the ones above, we have 3 that seem to just need
>>>> an alias table (cris, superh, alpha).  ppc can probably also use
>>>> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
>>>> needs whitespaces translated to '-' (sparc), which can be done
>>>> using an alias table.
>>>>
>>>> In the end I couldn't find any example that can't be represented
>>>> by a suffix + alias table.  
>>>
>>> Table based approach is possible but it won't be as simple
>>> as you've just pictured it.
>>>
>>> From what I recall from cpu_class_by_name cleanups table should be able
>>> to describe cases like (sometimes combination of them):
>>>    * 1:1 mapping - where cpu_model == cpu_type
>>>    * cpu_model <==> cpu_model + suffix  - most common usecase
>>>    * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add such cpu types
>>>    * NULL => some_fixed type
>>>    * case (in) sensitive flag
>>>    * garbage => some_fixed type
>>>    * substitutions
>>>    * aliases (sometimes dynamic depending on --enable-kvm (PPC))
>>> Maybe something else.
>>>
>>> We can think about it at leisure but I can't say if new approach
>>> complexity it's worth of the effort.
>>>  
>>> It would be nice see impl, but it's a lot of refactoring that's
>>> clearly out of scope of this series.
>>> I'd prefer small incremental refactoring (if possible) that
>>> won't scare people of and easy to review vs a huge one.
>>>   
>>>>> So I'd prefer having both back and forth functions together in
>>>>> one place. And common code to call them when necessary.
>>>>>
>>>>> We could do global cpu_name_by_type_name() instead of hook,
>>>>> which I'd prefer even more but then conversion can't be done
>>>>> only for one target but rather for all targets at once.    
>>>>
>>>> I don't mind letting a few targets override default behavior with
>>>> a hook if really necessary, but I have a problem with requiring
>>>> all targets to implement what's basically the same boilerplate
>>>> code to add/remove a string suffix and translating aliases.  
>>> it could be generic helper if target does the same plus
>>> not mandatory at that (in case target/board doesn't care
>>> about valid cpus).
>>>   
>>>>>> I agree it would be better if we had a cpu_name_by_type_name()
>>>>>> function, but I would like to have it implemented cleanly.    
>>>>> In some cases(targets) it can be common helper, but in other
>>>>> cases it's not so.
>>>>> My suggestion though allows to do gradual conversion and
>>>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
>>>>> Once all targets converted and relevant code is isolated
>>>>> we can attempt to generalize it if it's possible or at least
>>>>> make of it global per target helper to get rid of
>>>>> temporary machine hook.
>>>>>
>>>>> (seeing this series reposted with cpu_model names in boards code,
>>>>> it doesn't looks like author would like to implement tree-wide
>>>>> generalization first)    
>>>>
>>>> Well, if nobody is willing to generalize all target-specific code
>>>> right now, I don't see the harm in having cpu_model-based tables
>>>> in a few boards in the meantime (as this patch series does).  But
>>>> I do see harm in requiring all our 20 targets to implement yet
>>>> another hook and increasing the costs of cleaning up the mess
>>>> later.  
>>> If we use MachineClass hook then it might be done per target
>>> on demand, so no one would require that every target should
>>> implement it.
>>> Also there could be a generic helper for targets that do the same.
>>> Machine which needs to enable valid_cpus, will have to use generic
>>> hook impl or provide target specific if it's special case.
>>>
>>> Though I do see harm in adding cpu_model tables in boards code
>>> vs target specific hooks on demand as that will be copy-pasted
>>> in other boards afterwards (number of which is bigger compared
>>> to targets count) and ultimately it would duplicate cpu_name
>>> strings in every board vs hook approach where cpu_model could
>>> be calculated from cpu_type by a function (generic or
>>> target specific).
>>>
>>> Good thing about hook is that it's non intrusive and
>>> isolates(consolidates) existing cpu_type -> cpu_model
>>> conversion in multiple places into one place.
>>> Then later it would be easier to generalize if someone
>>> decides to do it.  
>>
>> I wonder how you want to proceed with this series, the first patch got
>> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
>> seems now not very complete/usable.
>>
>> Rebasing this series, i.e. with this snippet:
>>
>> -- >8 --  
>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>> index f57fc38f92..cca4ec6648 100644
>> --- a/hw/arm/netduino2.c
>> +++ b/hw/arm/netduino2.c
>> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
>>      DeviceState *dev;
>>
>>      dev = qdev_create(NULL, TYPE_STM32F205_SOC);
>> -    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
>> +    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
>>      object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
>>
>>      armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
>> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
>>
>>  static void netduino2_machine_init(MachineClass *mc)
>>  {
>> +    static const char *valid_cpus[] = {
>> +        ARM_CPU_TYPE_NAME("cortex-m3"),
>> +        ARM_CPU_TYPE_NAME("cortex-m4"),
>> +        NULL
>> +    };
>>      mc->desc = "Netduino 2 Machine";
>>      mc->init = netduino2_init;
>> +    mc->valid_cpu_types = valid_cpus;
>>      mc->ignore_memory_transaction_failures = true;
>>  }
>> ---
>>
>> We get cpu names with suffix:
>>
>>   $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
>>   qemu-system-arm: Invalid CPU type: arm926-arm-cpu
>>   The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>>
>> I understand you won't want a global cpu_name_by_type_name, how do you
>> want to do then?
>>
>> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
>> Then we could have generic machine code to parse the names.
> It would work only for some cases,
> problem is that we have a zoo of naming schemes.
> Considering that cpus models are used widely we probably can't
> deprecate it outright (for versioned machine types).  
> 
> Instead of wasting resources on translating cpu-type => 'cpu-name',
> (hook or lookup tables) how about simplifying code and making all
> boards accept full typenames?
> 
> It could be handled in generic way and then printing error with
> full type names would be acceptable since user would be able to feed it
> to -cpu.
> 
> '-cpu help' - would need some work to display types as well (also could be generic)
> 
> Perhaps with it we could deprecate cpu_models for non versioned
> machines/targets, which in most cases would allow us to drop special
> suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
> is already existing and keep exiting translation routines (hooks) only
> for versioned machine types as necessary evil.

Yes. Eduardo and you should write some lines to explain this, and then
we will follow :)

I feel concerned because:
1/ Alistair series is very helpful to new users, and
2/ As the RX architecture series showed, today it is not very clear how
to correctly use cpu_models.

Eduardo is working on a series, I'll wait for his work.

Regards,

Phil.

On Mon, 17 Jun 2019 17:15:21 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 6/17/19 5:01 PM, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 07:09:59 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >   
> >> Hi Igor, Eduardo,
> >>
> >> On 2/6/18 3:43 PM, Igor Mammedov wrote:  
> >>> On Mon, 5 Feb 2018 20:42:05 -0200
> >>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>     
> >>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:    
> >>>>> On Mon, 5 Feb 2018 11:54:01 -0200
> >>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>       
> >>>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:      
> >>>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
> >>>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>>>         
> >>>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:        
> >>>>>>>>> As cpu_type is not a user visible string let's convert the
> >>>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
> >>>>>>>>> user friendly string to report back.
> >>>>>>>>>
> >>>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
> >>>>>>>>> reverted and we should use cpu_type instead.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>  hw/core/machine.c | 11 +++++------
> >>>>>>>>>  1 file changed, 5 insertions(+), 6 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >>>>>>>>> index cdc1163dc6..de5bac1c84 100644
> >>>>>>>>> --- a/hw/core/machine.c
> >>>>>>>>> +++ b/hw/core/machine.c
> >>>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> >>>>>>>>>      /* If the machine supports the valid_cpu_types check and the user
> >>>>>>>>>       * specified a CPU with -cpu check here that the user CPU is supported.
> >>>>>>>>>       */
> >>>>>>>>> -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> >>>>>>>>> -        ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>>>> +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> >>>>>>>>>          int i;
> >>>>>>>>>  
> >>>>>>>>>          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >>>>>>>>> -            if (object_class_dynamic_cast(class,
> >>>>>>>>> -                                          machine_class->valid_cpu_types[i])) {
> >>>>>>>>> +            if (!strcmp(machine->cpu_model,
> >>>>>>>>> +                        machine_class->valid_cpu_types[i])) {          
> >>>>>>>>
> >>>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
> >>>>>>>> new semantics clearer.
> >>>>>>>>
> >>>>>>>> Anyway, I have bad and good news:
> >>>>>>>>
> >>>>>>>> The bad news is Igor already sent patches last week that remove
> >>>>>>>> MachineState::cpu_model, so this conflicts with his series.  Now
> >>>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
> >>>>>>>> available, but the function needs to work on *-user too.  See:
> >>>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> >>>>>>>>
> >>>>>>>> The good news is that I think we can fix this very easily if
> >>>>>>>> validation is done at the same place where parse_cpu_model() is
> >>>>>>>> called.  e.g.:
> >>>>>>>>
> >>>>>>>>     current_machine->cpu_type = machine_class->default_cpu_type;
> >>>>>>>>     if (cpu_model) {
> >>>>>>>>         current_machine->cpu_type = parse_cpu_model(cpu_model);
> >>>>>>>>
> >>>>>>>>         if (machine_class->valid_cpu_models) {
> >>>>>>>>             ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>>>             int i;
> >>>>>>>>
> >>>>>>>>             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>>>                 const char *valid_model = machine_class->valid_cpu_models[i];
> >>>>>>>>                 ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> >>>>>>>>                 if (object_class_dynamic_cast(class,
> >>>>>>>>                                               object_class_get_name(valid_class))) {
> >>>>>>>>                      /* Valid CPU type, we're good to go */
> >>>>>>>>                      break;
> >>>>>>>>                 }
> >>>>>>>>             }
> >>>>>>>>             if (!machine_class->valid_cpu_models[i]) {
> >>>>>>>>                 error_report("Invalid CPU model: %s", cpu_model);
> >>>>>>>>                 error_printf("The valid CPU models are: %s",
> >>>>>>>>                              machine_class->valid_cpu_models[0]);
> >>>>>>>>                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>>>                     error_printf(", %s", machine_class->valid_cpu_models[i]);
> >>>>>>>>                 }
> >>>>>>>>                 error_printf("\n");
> >>>>>>>>                 exit(1);
> >>>>>>>>             }
> >>>>>>>>         }
> >>>>>>>>     }
> >>>>>>>>
> >>>>>>>> This can be done inside main(), or moved inside
> >>>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
> >>>>>>>> the function.
> >>>>>>>>
> >>>>>>>> On either case, I think it's a good idea to do validation and
> >>>>>>>> printing of error messages closer to the code that parses the
> >>>>>>>> command-line options.  This way we separate parsing/validation
> >>>>>>>> from initialization.        
> >>>>>>> I agree it's better like you suggest as at least it prevents
> >>>>>>> ms->cpu_model creeping back into boards code.
> >>>>>>>
> >>>>>>> But I still dislike (hate) an idea of new code adding non
> >>>>>>> canonized cpu_model strings back in the boards code.
> >>>>>>> It's just a matter of time when someone would use them
> >>>>>>> and cpu_model parsing will creep back into boards.
> >>>>>>>
> >>>>>>> It would be much better to if we add 
> >>>>>>>    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> >>>>>>> callback and let machines in this patchset to set it,
> >>>>>>> something along following lines which is not much of
> >>>>>>> refactoring and allows for gradual conversion:
> >>>>>>>
> >>>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >>>>>>> index 9631670..85cca84 100644
> >>>>>>> --- a/target/arm/cpu.h
> >>>>>>> +++ b/target/arm/cpu.h
> >>>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> >>>>>>>      return cpu->el_change_hook_opaque;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
> >>>>>>> +
> >>>>>>>  #endif
> >>>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >>>>>>> index f936017..ae6adb7 100644
> >>>>>>> --- a/hw/arm/netduino2.c
> >>>>>>> +++ b/hw/arm/netduino2.c
> >>>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> >>>>>>>      mc->desc = "Netduino 2 Machine";
> >>>>>>>      mc->init = netduino2_init;
> >>>>>>>      mc->ignore_memory_transaction_failures = true;
> >>>>>>> +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:        
> >>>>>>
> >>>>>> I really don't want to introduce a new arch-specific hook just
> >>>>>> for that.  We should move CPU type lookup logic to common code
> >>>>>> and make it unnecessary to write new hooks.      
> >>>>> unfortunately cpu_model (cpu name part) is target specific
> >>>>> and it's translation to type and back is target specific mayhem.      
> >>>>
> >>>> Why can't the model<->type translation be represented as data?
> >>>> We could have simple cpu_type_name_suffix + an alias table.
> >>>>
> >>>> We have at least 4 arches that return a constant at
> >>>> class_by_name.  We have at least 10 arches that simply add a
> >>>> suffix to the CPU model name.  We must make them use common code
> >>>> instead of requiring them to implement yet another hook[1].    
> >>> True, some of them could use generic hook and reduce
> >>> code duplication greatly, we should do it regardless of whether
> >>> table or target specific func approach is used.
> >>>     
> >>>> In addition to the ones above, we have 3 that seem to just need
> >>>> an alias table (cris, superh, alpha).  ppc can probably also use
> >>>> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
> >>>> needs whitespaces translated to '-' (sparc), which can be done
> >>>> using an alias table.
> >>>>
> >>>> In the end I couldn't find any example that can't be represented
> >>>> by a suffix + alias table.    
> >>>
> >>> Table based approach is possible but it won't be as simple
> >>> as you've just pictured it.
> >>>
> >>> From what I recall from cpu_class_by_name cleanups table should be able
> >>> to describe cases like (sometimes combination of them):
> >>>    * 1:1 mapping - where cpu_model == cpu_type
> >>>    * cpu_model <==> cpu_model + suffix  - most common usecase
> >>>    * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add such cpu types
> >>>    * NULL => some_fixed type
> >>>    * case (in) sensitive flag
> >>>    * garbage => some_fixed type
> >>>    * substitutions
> >>>    * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> >>> Maybe something else.
> >>>
> >>> We can think about it at leisure but I can't say if new approach
> >>> complexity it's worth of the effort.
> >>>  
> >>> It would be nice see impl, but it's a lot of refactoring that's
> >>> clearly out of scope of this series.
> >>> I'd prefer small incremental refactoring (if possible) that
> >>> won't scare people of and easy to review vs a huge one.
> >>>     
> >>>>> So I'd prefer having both back and forth functions together in
> >>>>> one place. And common code to call them when necessary.
> >>>>>
> >>>>> We could do global cpu_name_by_type_name() instead of hook,
> >>>>> which I'd prefer even more but then conversion can't be done
> >>>>> only for one target but rather for all targets at once.      
> >>>>
> >>>> I don't mind letting a few targets override default behavior with
> >>>> a hook if really necessary, but I have a problem with requiring
> >>>> all targets to implement what's basically the same boilerplate
> >>>> code to add/remove a string suffix and translating aliases.    
> >>> it could be generic helper if target does the same plus
> >>> not mandatory at that (in case target/board doesn't care
> >>> about valid cpus).
> >>>     
> >>>>>> I agree it would be better if we had a cpu_name_by_type_name()
> >>>>>> function, but I would like to have it implemented cleanly.      
> >>>>> In some cases(targets) it can be common helper, but in other
> >>>>> cases it's not so.
> >>>>> My suggestion though allows to do gradual conversion and
> >>>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
> >>>>> Once all targets converted and relevant code is isolated
> >>>>> we can attempt to generalize it if it's possible or at least
> >>>>> make of it global per target helper to get rid of
> >>>>> temporary machine hook.
> >>>>>
> >>>>> (seeing this series reposted with cpu_model names in boards code,
> >>>>> it doesn't looks like author would like to implement tree-wide
> >>>>> generalization first)      
> >>>>
> >>>> Well, if nobody is willing to generalize all target-specific code
> >>>> right now, I don't see the harm in having cpu_model-based tables
> >>>> in a few boards in the meantime (as this patch series does).  But
> >>>> I do see harm in requiring all our 20 targets to implement yet
> >>>> another hook and increasing the costs of cleaning up the mess
> >>>> later.    
> >>> If we use MachineClass hook then it might be done per target
> >>> on demand, so no one would require that every target should
> >>> implement it.
> >>> Also there could be a generic helper for targets that do the same.
> >>> Machine which needs to enable valid_cpus, will have to use generic
> >>> hook impl or provide target specific if it's special case.
> >>>
> >>> Though I do see harm in adding cpu_model tables in boards code
> >>> vs target specific hooks on demand as that will be copy-pasted
> >>> in other boards afterwards (number of which is bigger compared
> >>> to targets count) and ultimately it would duplicate cpu_name
> >>> strings in every board vs hook approach where cpu_model could
> >>> be calculated from cpu_type by a function (generic or
> >>> target specific).
> >>>
> >>> Good thing about hook is that it's non intrusive and
> >>> isolates(consolidates) existing cpu_type -> cpu_model
> >>> conversion in multiple places into one place.
> >>> Then later it would be easier to generalize if someone
> >>> decides to do it.    
> >>
> >> I wonder how you want to proceed with this series, the first patch got
> >> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
> >> seems now not very complete/usable.
> >>
> >> Rebasing this series, i.e. with this snippet:
> >>  
> >> -- >8 --    
> >> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >> index f57fc38f92..cca4ec6648 100644
> >> --- a/hw/arm/netduino2.c
> >> +++ b/hw/arm/netduino2.c
> >> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
> >>      DeviceState *dev;
> >>
> >>      dev = qdev_create(NULL, TYPE_STM32F205_SOC);
> >> -    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
> >> +    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
> >>      object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
> >>
> >>      armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> >> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
> >>
> >>  static void netduino2_machine_init(MachineClass *mc)
> >>  {
> >> +    static const char *valid_cpus[] = {
> >> +        ARM_CPU_TYPE_NAME("cortex-m3"),
> >> +        ARM_CPU_TYPE_NAME("cortex-m4"),
> >> +        NULL
> >> +    };
> >>      mc->desc = "Netduino 2 Machine";
> >>      mc->init = netduino2_init;
> >> +    mc->valid_cpu_types = valid_cpus;
> >>      mc->ignore_memory_transaction_failures = true;
> >>  }
> >> ---
> >>
> >> We get cpu names with suffix:
> >>
> >>   $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
> >>   qemu-system-arm: Invalid CPU type: arm926-arm-cpu
> >>   The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
> >>
> >> I understand you won't want a global cpu_name_by_type_name, how do you
> >> want to do then?
> >>
> >> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> >> Then we could have generic machine code to parse the names.  
> > It would work only for some cases,
> > problem is that we have a zoo of naming schemes.
> > Considering that cpus models are used widely we probably can't
> > deprecate it outright (for versioned machine types).  
> > 
> > Instead of wasting resources on translating cpu-type => 'cpu-name',
> > (hook or lookup tables) how about simplifying code and making all
> > boards accept full typenames?
> > 
> > It could be handled in generic way and then printing error with
> > full type names would be acceptable since user would be able to feed it
> > to -cpu.
> > 
> > '-cpu help' - would need some work to display types as well (also could be generic)
> > 
> > Perhaps with it we could deprecate cpu_models for non versioned
> > machines/targets, which in most cases would allow us to drop special
> > suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
> > is already existing and keep exiting translation routines (hooks) only
> > for versioned machine types as necessary evil.  
Maybe we don't even need to keep it versioned machine types, considering
change mapping is static and doesn't influence ABI/migration.
Management interface could just translate cpu_model to type name
for new QEMU.

That would allow us to remove quite a bit target specific code
that deals with all permutations of CPU model and replace a bunch of
'-cpu help' target specific handlers with a generic enumeration
of built-in CPU types.


> Yes. Eduardo and you should write some lines to explain this, and then
> we will follow :)
Unfortunately I don't recall details anymore. One could check out all
implementations of class_by_name callbacks to find out current state.


> I feel concerned because:
> 1/ Alistair series is very helpful to new users, and
> 2/ As the RX architecture series showed, today it is not very clear how
> to correctly use cpu_models.
That's why I'm pushing for supporting only type names whenever possible
so there  won't be confusion anymore. Using types is consistent with
-device and some QMP interfaces, enforces strict error checking so
user would get error on nonsense input.


> Eduardo is working on a series, I'll wait for his work.
> 
> Regards,
> 
> Phil.

On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 17:15:21 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
[...]
> > Yes. Eduardo and you should write some lines to explain this, and then
> > we will follow :)
> Unfortunately I don't recall details anymore. One could check out all
> implementations of class_by_name callbacks to find out current state.

See this message for a summary of existing class_by_name quirks:

  https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
  Date: Wed, 08 May 2019 10:34:44 +0200
  Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
  Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions

I'm unsure about Igor's suggestion to get rid of CPU model names
and use only QOM type names in external interfaces.  In either
case, we can still simplify the rules rules and reduce the amount
of arch-specific code.

On Mon, 17 Jun 2019 13:27:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 17:15:21 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:  
> [...]
> > > Yes. Eduardo and you should write some lines to explain this, and then
> > > we will follow :)  
> > Unfortunately I don't recall details anymore. One could check out all
> > implementations of class_by_name callbacks to find out current state.  
> 
> See this message for a summary of existing class_by_name quirks:
> 
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
>   Date: Wed, 08 May 2019 10:34:44 +0200
>   Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
>   Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> 
> I'm unsure about Igor's suggestion to get rid of CPU model names
> and use only QOM type names in external interfaces.  In either
> case, we can still simplify the rules rules and reduce the amount
> of arch-specific code.
as far as we have cpu_class_by_name, we have to watch over that
new patches/targets won't add some custom handling/fallbac/whatnot.

On contrary -device works just with type names for all devices,
applying the same to -cpu which is basically translator
   model->type[,-global type.foo,...]
would be consistent with -device and less confusing for everyone
(not counting significant code reduction).
It would certainly simplify contributing new targets as contributor
won't have to care about cpu model naming and do something about it.

This option wasn't considered before because we didn't have deprecation
back then, but now it opens possibility to simplify qemu and consolidate
naming. (we probably would be able to fold '-cpu help' into '-device help'
as well).

On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 13:27:00 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:  
> > [...]
> > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > we will follow :)  
> > > Unfortunately I don't recall details anymore. One could check out all
> > > implementations of class_by_name callbacks to find out current state.  
> > 
> > See this message for a summary of existing class_by_name quirks:
> > 
> >   https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> >   Date: Wed, 08 May 2019 10:34:44 +0200
> >   Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> >   Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > 
> > I'm unsure about Igor's suggestion to get rid of CPU model names
> > and use only QOM type names in external interfaces.  In either
> > case, we can still simplify the rules rules and reduce the amount
> > of arch-specific code.
> as far as we have cpu_class_by_name, we have to watch over that
> new patches/targets won't add some custom handling/fallbac/whatnot.

We can get rid of CPUClass::cpu_class_by_name() without changing
the external interfaces provided by QEMU.

I don't have a strong opinion about using only QOM types at -cpu,
yet.  But first we need to get rid of the arch-specific CPU model
name exceptions enumerated at the URL above (which would be very
welcome).

> 
> On contrary -device works just with type names for all devices,
> applying the same to -cpu which is basically translator
>    model->type[,-global type.foo,...]
> would be consistent with -device and less confusing for everyone
> (not counting significant code reduction).
> It would certainly simplify contributing new targets as contributor
> won't have to care about cpu model naming and do something about it.
> 
> This option wasn't considered before because we didn't have deprecation
> back then, but now it opens possibility to simplify qemu and consolidate
> naming. (we probably would be able to fold '-cpu help' into '-device help'
> as well).
>

On Tue, 18 Jun 2019 10:55:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 13:27:00 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:  
> > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:    
> > > [...]  
> > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > we will follow :)    
> > > > Unfortunately I don't recall details anymore. One could check out all
> > > > implementations of class_by_name callbacks to find out current state.    
> > > 
> > > See this message for a summary of existing class_by_name quirks:
> > > 
> > >   https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > >   Date: Wed, 08 May 2019 10:34:44 +0200
> > >   Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > >   Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > > 
> > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > and use only QOM type names in external interfaces.  In either
> > > case, we can still simplify the rules rules and reduce the amount
> > > of arch-specific code.  
> > as far as we have cpu_class_by_name, we have to watch over that
> > new patches/targets won't add some custom handling/fallbac/whatnot.  
> 
> We can get rid of CPUClass::cpu_class_by_name() without changing
> the external interfaces provided by QEMU.
if you mean QMP, than it is possible to keep model there where
it already exists. Based on experiment [1](x86) I did, it's local to
affected target and doesn't pollute other code.

> I don't have a strong opinion about using only QOM types at -cpu,
> yet.  But first we need to get rid of the arch-specific CPU model
> name exceptions enumerated at the URL above (which would be very
> welcome).
One way to get rid of them is to deprecate them in favor of strict
match (no fallback/substitutions/aliases) to typename and when we
drop it make switch type based naming only.

1) I've just took a quick look at how much of duplicated/confusing
code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
It amounts to >800LOC of trivial removal (not counting ppc/s390
that depend on model naming heavily and in need of some non
trivial refactoring)

> 
> > 
> > On contrary -device works just with type names for all devices,
> > applying the same to -cpu which is basically translator
> >    model->type[,-global type.foo,...]
> > would be consistent with -device and less confusing for everyone
> > (not counting significant code reduction).
> > It would certainly simplify contributing new targets as contributor
> > won't have to care about cpu model naming and do something about it.
> > 
> > This option wasn't considered before because we didn't have deprecation
> > back then, but now it opens possibility to simplify qemu and consolidate
> > naming. (we probably would be able to fold '-cpu help' into '-device help'
> > as well).
> >   
>

On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
> On Tue, 18 Jun 2019 10:55:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > > On Mon, 17 Jun 2019 13:27:00 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:  
> > > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:    
> > > > [...]  
> > > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > > we will follow :)    
> > > > > Unfortunately I don't recall details anymore. One could check out all
> > > > > implementations of class_by_name callbacks to find out current state.    
> > > > 
> > > > See this message for a summary of existing class_by_name quirks:
> > > > 
> > > >   https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > > >   Date: Wed, 08 May 2019 10:34:44 +0200
> > > >   Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > > >   Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > > > 
> > > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > > and use only QOM type names in external interfaces.  In either
> > > > case, we can still simplify the rules rules and reduce the amount
> > > > of arch-specific code.  
> > > as far as we have cpu_class_by_name, we have to watch over that
> > > new patches/targets won't add some custom handling/fallbac/whatnot.  
> > 
> > We can get rid of CPUClass::cpu_class_by_name() without changing
> > the external interfaces provided by QEMU.
> if you mean QMP, than it is possible to keep model there where
> it already exists. Based on experiment [1](x86) I did, it's local to
> affected target and doesn't pollute other code.

I mean both command line and QMP.

> 
> > I don't have a strong opinion about using only QOM types at -cpu,
> > yet.  But first we need to get rid of the arch-specific CPU model
> > name exceptions enumerated at the URL above (which would be very
> > welcome).
> One way to get rid of them is to deprecate them in favor of strict
> match (no fallback/substitutions/aliases) to typename and when we
> drop it make switch type based naming only.

This is true, but is it desirable?  Aren't we just moving
complexity elsewhere?  Management software would still need to
translate CPU models from existing VM configurations to QOM type
names.

> 
> 1) I've just took a quick look at how much of duplicated/confusing
> code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
> It amounts to >800LOC of trivial removal (not counting ppc/s390
> that depend on model naming heavily and in need of some non
> trivial refactoring)

Removing the code might be trivial.  Coordinating with the
maintainers of all architectures to confirm this is really
something everybody wants is the difficult part.

If you really want to do it, please make sure all the
architecture maintainers (and libvirt developers) are involved in
the discussion.

> 
> > 
> > > 
> > > On contrary -device works just with type names for all devices,
> > > applying the same to -cpu which is basically translator
> > >    model->type[,-global type.foo,...]
> > > would be consistent with -device and less confusing for everyone
> > > (not counting significant code reduction).
> > > It would certainly simplify contributing new targets as contributor
> > > won't have to care about cpu model naming and do something about it.
> > > 
> > > This option wasn't considered before because we didn't have deprecation
> > > back then, but now it opens possibility to simplify qemu and consolidate
> > > naming. (we probably would be able to fold '-cpu help' into '-device help'
> > > as well).
> > >   
> > 
>

On 6/20/19 4:43 PM, Eduardo Habkost wrote:
> On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
>> On Tue, 18 Jun 2019 10:55:16 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
>>>> On Mon, 17 Jun 2019 13:27:00 -0300
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>    
>>>>> On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
>>>>>> On Mon, 17 Jun 2019 17:15:21 +0200
>>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>> [...]
>>>>>>> Yes. Eduardo and you should write some lines to explain this, and then
>>>>>>> we will follow :)
>>>>>> Unfortunately I don't recall details anymore. One could check out all
>>>>>> implementations of class_by_name callbacks to find out current state.
>>>>>
>>>>> See this message for a summary of existing class_by_name quirks:
>>>>>
>>>>>    https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
>>>>>    Date: Wed, 08 May 2019 10:34:44 +0200
>>>>>    Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
>>>>>    Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
>>>>>
>>>>> I'm unsure about Igor's suggestion to get rid of CPU model names
>>>>> and use only QOM type names in external interfaces.  In either
>>>>> case, we can still simplify the rules rules and reduce the amount
>>>>> of arch-specific code.
>>>> as far as we have cpu_class_by_name, we have to watch over that
>>>> new patches/targets won't add some custom handling/fallbac/whatnot.
>>>
>>> We can get rid of CPUClass::cpu_class_by_name() without changing
>>> the external interfaces provided by QEMU.
>> if you mean QMP, than it is possible to keep model there where
>> it already exists. Based on experiment [1](x86) I did, it's local to
>> affected target and doesn't pollute other code.
> 
> I mean both command line and QMP.
> 
>>
>>> I don't have a strong opinion about using only QOM types at -cpu,
>>> yet.  But first we need to get rid of the arch-specific CPU model
>>> name exceptions enumerated at the URL above (which would be very
>>> welcome).
>> One way to get rid of them is to deprecate them in favor of strict
>> match (no fallback/substitutions/aliases) to typename and when we
>> drop it make switch type based naming only.
> 
> This is true, but is it desirable?  Aren't we just moving
> complexity elsewhere?  Management software would still need to
> translate CPU models from existing VM configurations to QOM type
> names.
> 
>>
>> 1) I've just took a quick look at how much of duplicated/confusing
>> code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
>> It amounts to >800LOC of trivial removal (not counting ppc/s390
>> that depend on model naming heavily and in need of some non
>> trivial refactoring)
> 
> Removing the code might be trivial.  Coordinating with the
> maintainers of all architectures to confirm this is really
> something everybody wants is the difficult part.
> 
> If you really want to do it, please make sure all the
> architecture maintainers (and libvirt developers) are involved in
> the discussion.

 From the previous link (summary of existing class_by_name quirks):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html

   "Deprecating unwanted stuff now is likely to make a
    later cleanup so much easier."

This was 8 months ago :/

IIUC we need to restart this topic addressing the libvirt community 
first, then see with the QEMU one?

> 
>>
>>>
>>>>
>>>> On contrary -device works just with type names for all devices,
>>>> applying the same to -cpu which is basically translator
>>>>     model->type[,-global type.foo,...]
>>>> would be consistent with -device and less confusing for everyone
>>>> (not counting significant code reduction).
>>>> It would certainly simplify contributing new targets as contributor
>>>> won't have to care about cpu model naming and do something about it.
>>>>
>>>> This option wasn't considered before because we didn't have deprecation
>>>> back then, but now it opens possibility to simplify qemu and consolidate
>>>> naming. (we probably would be able to fold '-cpu help' into '-device help'
>>>> as well).
>>>>    
>>>
>>
>

On Thu, Jan 23, 2020 at 07:48:21PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/20/19 4:43 PM, Eduardo Habkost wrote:
> > On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
> > > On Tue, 18 Jun 2019 10:55:16 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > > > > On Mon, 17 Jun 2019 13:27:00 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > > > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > > > > [...]
> > > > > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > > > > we will follow :)
> > > > > > > Unfortunately I don't recall details anymore. One could check out all
> > > > > > > implementations of class_by_name callbacks to find out current state.
> > > > > > 
> > > > > > See this message for a summary of existing class_by_name quirks:
> > > > > > 
> > > > > >    https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > > > > >    Date: Wed, 08 May 2019 10:34:44 +0200
> > > > > >    Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > > > > >    Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > > > > > 
> > > > > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > > > > and use only QOM type names in external interfaces.  In either
> > > > > > case, we can still simplify the rules rules and reduce the amount
> > > > > > of arch-specific code.
> > > > > as far as we have cpu_class_by_name, we have to watch over that
> > > > > new patches/targets won't add some custom handling/fallbac/whatnot.
> > > > 
> > > > We can get rid of CPUClass::cpu_class_by_name() without changing
> > > > the external interfaces provided by QEMU.
> > > if you mean QMP, than it is possible to keep model there where
> > > it already exists. Based on experiment [1](x86) I did, it's local to
> > > affected target and doesn't pollute other code.
> > 
> > I mean both command line and QMP.
> > 
> > > 
> > > > I don't have a strong opinion about using only QOM types at -cpu,
> > > > yet.  But first we need to get rid of the arch-specific CPU model
> > > > name exceptions enumerated at the URL above (which would be very
> > > > welcome).
> > > One way to get rid of them is to deprecate them in favor of strict
> > > match (no fallback/substitutions/aliases) to typename and when we
> > > drop it make switch type based naming only.
> > 
> > This is true, but is it desirable?  Aren't we just moving
> > complexity elsewhere?  Management software would still need to
> > translate CPU models from existing VM configurations to QOM type
> > names.
> > 
> > > 
> > > 1) I've just took a quick look at how much of duplicated/confusing
> > > code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
> > > It amounts to >800LOC of trivial removal (not counting ppc/s390
> > > that depend on model naming heavily and in need of some non
> > > trivial refactoring)
> > 
> > Removing the code might be trivial.  Coordinating with the
> > maintainers of all architectures to confirm this is really
> > something everybody wants is the difficult part.
> > 
> > If you really want to do it, please make sure all the
> > architecture maintainers (and libvirt developers) are involved in
> > the discussion.
> 
> From the previous link (summary of existing class_by_name quirks):
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html
> 
>   "Deprecating unwanted stuff now is likely to make a
>    later cleanup so much easier."
> 
> This was 8 months ago :/
> 
> IIUC we need to restart this topic addressing the libvirt community first,
> then see with the QEMU one?

The next step is to deprecate the weird and pointless exceptions
listed at the URL you mentioned.  Not all exceptions will affect libvirt (maybe most won't).

In either case, we can just use qemu-deprecated.texi to
communicate this to libvirt developers.  MAINTAINERS already has
a "R: libvir-list" line for qemu-deprecated.texi.