On Mon, 5 Feb 2018 20:42:05 -0200
Eduardo Habkost <ehabk...@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 <ehabk...@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 <ehabk...@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.fran...@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.
