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 12, 2017 at 3:53 AM, Igor Mammedov <address@hidden> wrote:
> On Tue, 5 Sep 2017 19:12:26 -0300
> 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.
>>
>>
>> > 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.
> Agreed, it would be useful, however goal of the patch to drop
> cpu_generic_init() preferably without changing behavior
>
> so I'd leave extra stuff you mention upto board maintainers
> to fix up on top.
>
>
>> 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.
> so far I've met 3 use cases for valid_cpu_types
>  * no check - just try to use whatever user provided
>  * check for concrete cpu models (leaf classes)
>  * check for super-class based in partial cpu_model match
>
> it is nice to have valid_cpu_types[] /I recall even trying out something 
> similar/
> but then series turns into mess where one tries to fix several things
> and on top of it in all targets, hence I've decided first to get rid of
> all cpu_model handling in boards and only then think about valid_cpus using 
> cpu types.
>
> I'd gladly give up 'valid cpus' to someone else more interested in it,
> there are other users beside of ARM boards for it.
> /seems Alistair is interested in it, at least in ARM part/

Yeah, I'm interested in getting a generic framework to make this
possible in. Still just an RFC, I need to get back to that this week
and tidy it up.

Thanks,
Alistair

>
>
>> > 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.
> in ARM case there are boards that use
>  * '-cpu' provided model
>  * '-cpu' provided model with valid cpu checks
>  * 'hardcoded' cpu model ignoring '-cpu'/-global
>
> I've thought commit message sufficiently described current situation and 
> changes.