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

[Eduardo Habkost]

On Tue, Sep 12, 2017 at 12:22:14PM +0200, Igor Mammedov wrote:
> On Sat, 9 Sep 2017 17:30:14 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Tue, Sep 05, 2017 at 03:46:07PM -0700, Alistair Francis wrote:
> > > 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.  
> > 
> > After looking at the code more closely, I think we can be 100%
> > sure the user doesn't rely on the property, because:
> > 
> > * TYPE_ARMV7M and TYPE_STM32F205_SOC are both sysbus devices
> >   with user_creatable=false, so the user can't instantiate them
> >   directly;
> > * The only places where those objects are realized inside the
> >   code are:
> >   * mps2_common_init()
> >   * netduino2_init()
> >   * stm32f205_soc_realize()
> >   * armv7m_init()
> >   Those functions always set the "cpu-model" property immediately
> >   before realize.
> > 
> > This means any value set by the user (e.g. using -global) would
> > be always overwritten before realize.
> > 
> > However, I have a suggestion for Igor: making a separate patch
> > that renames the existing property to "x-cpu-model", and using
> > "x-cpu-type" in this series.  This way we will explicitly
> > document the fact that the property is not a stable
> > user/management interface.
> There is no much point in renaming to "x-cpu-model" as it will be deleted
> right afterwards, I'd just delete "cpu-model" and use "x-cpu-type"
> in this patch. I'm not a fun of 'x-' prefix and would prefer a flag
> in property to mark it as internal. But it's out of scope of this series,
> so I don't care much about naming at the moment and will use "x-cpu-type"
> as you suggest.

Peter pointed out that this is the case with lots of properties
on non-user-creatable devices.  So we can keep "cpu-type" for
consistency by now.

-- 
Eduardo