Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 7:50 PM, Fedorov Sergey <address@hidden> wrote:
>
> On 12/03/2013 04:15 PM, Peter Crosthwaite wrote:
>>
>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <address@hidden>
>> wrote:
>>>
>>> TTBCR has additional fields PD0 and PD1 when using Short-descriptor
>>> translation table format on a CPU with TrustZone feature support.
>>>
>>> Signed-off-by: Sergey Fedorov <address@hidden>
>>> ---
>>>   target-arm/helper.c |    4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index a247ca0..6642e53 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env,
>>> const ARMCPRegInfo *ri,
>>>   {
>>>       int maskshift = extract32(value, 0, 3);
>>>
>>> -    if (arm_feature(env, ARM_FEATURE_LPAE)) {
>>> +    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
>>
>> This appears to be changing more than just trustzone dependent
>> behavior. That is, if we take just this hunk and ignore the one below
>> you see a change in the non-tz behaviour. Is the hunk legitimate
>> irrespective of trustzone support?
>
>
> Yes, current implementation is not accurate according to ARMv7-AR reference
> manual. See "B4.1.153 TTBCR, Translation Table Base Control Register, VMSA |
> TTBCR format when using the Long-descriptor translation table format". When
> LPAE feature is supported, EAE, bit[31] selects translation descriptor
> format and, therefore, TTBCR format.
>

Ok,

You should probably split it off as a separate patch. And you could
send probably send it immediately. What you just wrote is a nice
commit message.

>
>>
>>>           value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>>> +    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
>>> +        value &= 0x37;
>>>       } else {
>>>           value &= 7;
>>>       }
>>
>> There are a few magic numbers in the patch probably worth macrofiying.
>
>
> As I can see, magic numbers are widely used through all of this file to
> represent CP register fields and other things. Maybe the macrofying should
> be done separately from this patch series?
>

So target-arm is riddled with legacy style. Converting all of
target-arm to self-doccing macros is a big job, but if we keep
expanding without doing it that job only gets bigger. I'll defer to
PMM on what we want to policy to be going forward. - any thoughts?

Regards,
Peter

>>
>> Regards,
>> Peter
>>
>>> --
>>> 1.7.9.5
>>>
>>>
>>
>
> Best regards,
> Sergey Fedorov
>