On 15 December 2014 at 11:21, Peter Maydell <peter.mayd...@linaro.org>
wrote:
>
> On 15 December 2014 at 17:18, Greg Bellows <greg.bell...@linaro.org>
> wrote:
> >
> >
> > On 15 December 2014 at 11:01, Peter Maydell <peter.mayd...@linaro.org>
> > wrote:
> >>
> >> On 11 December 2014 at 23:29, Greg Bellows <greg.bell...@linaro.org>
> >> wrote:
> >> > +static Property arm_cpu_has_el3_property =
> >> > + DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, false);
> >>
> >> I think the default value here should be "true": we want
> >> CPUs to default to having all their features turned on
> >> unless the board specifically disables it.
> >>
> >> (This won't affect behaviour til we get to patch 15 because
> >> before then ARM_FEATURE_EL3 isn't set and we never add
> >> the property here.)
> >
> >
> > I went with false in the initialization because it matches how features
> are
> > initialized (disabled unless otherwise enabled). In the case of EL3, it
> is
> > disabled on CPUs by default unless specifically enabled. This is also
> the
> > case of the has_el3 property, disabled by default unless we see that the
> EL3
> > feature is enabled.
>
> This is correct...
>
> > As-is, the tunable is set to true once we discover that
> > the EL3 feature is set
>
> ...but this isn't. When you call qdev_property_add_static()
> it will call object_property_set_bool() to set the flag to
> the default value as specified in the Property struct, so
> we'll end up with has_el3 set to false by default on CPUs
> which can support EL3, which is not what we want.
>
> In fact you're manually setting cpu->has_el3 = true in
> the arm_cpu_post_init(). You should just set the property
> struct's default value correctly in the first place...
>
>
Ah... I see what you are saying. If I init it to true I can skip manual
setting of it in the step that follows. I'll fix it in the next version.
> thanks
> -- PMM
>
