On 6/17/19 5:01 PM, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 07:09:59 +0200
> Philippe Mathieu-Daudé <phi...@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 <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.
>>
>> 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.
