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

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 8:52 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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;

So if we let the magic numbers slide, can we at least add a comment
about what fields these are?

/* PD0, PD1, N */

You would also be a little more self documenting with:

1 << 5 | 1 << 4 | 0x7

Regards,
Peter

>>>>       } 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
>>