[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model |
Date: |
Mon, 17 Jun 2019 07:09:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
Hi Igor, Eduardo,
On 2/6/18 3:43 PM, Igor Mammedov wrote:
> On Mon, 5 Feb 2018 20:42:05 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model,
Philippe Mathieu-Daudé <=
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Eduardo Habkost, 2019/06/17
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Igor Mammedov, 2019/06/17
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Philippe Mathieu-Daudé, 2019/06/17
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Igor Mammedov, 2019/06/17
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Eduardo Habkost, 2019/06/17
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Igor Mammedov, 2019/06/18
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Eduardo Habkost, 2019/06/18
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Igor Mammedov, 2019/06/20
- Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model, Eduardo Habkost, 2019/06/20