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...
thanks
-- PMM
