Re: [Qemu-arm] [Qemu-devel] [PATCH 6/6] arm: drop intermadiate cpu_model -> cpu type parsing and use cpu type directly

[Alistair Francis]

On Tue, Sep 5, 2017 at 3:12 PM, Eduardo Habkost <address@hidden> wrote:
> On Tue, Sep 05, 2017 at 02:47:52PM -0700, Alistair Francis wrote:
>> On Tue, Sep 5, 2017 at 2:31 PM, Eduardo Habkost <address@hidden> wrote:
> [...]
>> >> diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
>> >> index f61e735..1cd6374 100644
>> >> --- a/hw/arm/stm32f205_soc.c
>> >> +++ b/hw/arm/stm32f205_soc.c
>> >> @@ -112,7 +112,7 @@ static void stm32f205_soc_realize(DeviceState 
>> >> *dev_soc, Error **errp)
>> >>
>> >>      armv7m = DEVICE(&s->armv7m);
>> >>      qdev_prop_set_uint32(armv7m, "num-irq", 96);
>> >> -    qdev_prop_set_string(armv7m, "cpu-model", s->cpu_model);
>> >> +    qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
>> >>      object_property_set_link(OBJECT(&s->armv7m), 
>> >> OBJECT(get_system_memory()),
>> >>                                       "memory", &error_abort);
>> >>      object_property_set_bool(OBJECT(&s->armv7m), true, "realized", &err);
>> >> @@ -200,7 +200,7 @@ static void stm32f205_soc_realize(DeviceState 
>> >> *dev_soc, Error **errp)
>> >>  }
>> >>
>> >>  static Property stm32f205_soc_properties[] = {
>> >> -    DEFINE_PROP_STRING("cpu-model", STM32F205State, cpu_model),
>> >> +    DEFINE_PROP_STRING("cpu-type", STM32F205State, cpu_type),
>> >
>> > Same as armv7m: are we 100% sure users are not setting this
>> > manually?
>>
>> In an embedded board like this it really doesn't make sense to let the
>> user overwrite the CPU. The SoC will take it as an option, but the
>> board (which creates the SoC) just blindly always uses the same CPU.
>> That feature is more for QOMificatoion then any real reason though.
>>
>
> I'm not talking about -cpu (no user-visible change in the
> handling of -cpu should result from this patch), but about
> possible cases where the user set the "cpu-model" property using
> another mechanism, like -global.  Probably it's impossible for an
> user to override the property successfully, but I would like to
> be sure.

Ah, that is trickier.

I guess that is possible to do, but the object setting logic should
handle the error gracefully and inform the user of the error.

>
>
>> In saying that I think a warning if the user tries to set the CPU
>> would make sense. I know that this issues comes up in other ARM boards
>> (Zynq-7000 has the same issue as well) so maybe a machine property
>> saying that the board doesn't accept custom CPUs would be a good idea.
>
> Yeah, there are multiple cases in this patch where boards are
> validating the CPU model, but not all boards do that.  A generic
> MachineClass::valid_cpu_types[] field would be useful.
>
>>
>> Overall I think this patch is moving in the right direction though and
>> this CPU option being ignored existed before this series.
>
> I agree this is going on the right direction.  However, I don't
> see any board that ignore the CPU option: all of them seem to use
> cpu_model when creating the CPUs, already.

The Netduino2 will ignore any CPU options and always use a Cortex-m3.
I was wrong about Zynq-7000 though, it does respect the -cpu option.

Thanks,
Alistair

>
> --
> Eduardo