Re: [Qemu-devel] [PATCH v2 10/14] target-arm: Honour NS bits in page tables

[Peter Maydell]

On 21 April 2015 at 10:24, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>
>> Honour the NS bit in ARM page tables:
>>  * when adding entries to the TLB, include the Secure/NonSecure
>>    transaction attribute
>>  * set the NS bit in the PAR when doing ATS operations
>>
>> Note that we don't yet correctly use the NSTable bit to
>> cause the page table walk itself to use the right attributes.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  include/exec/memattrs.h |  2 ++
>>  target-arm/helper.c     | 79 
>> +++++++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 69 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index 1cb3fc0..68a9c76 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -29,6 +29,8 @@ typedef struct MemTxAttrs {
>>       * "didn't specify" if necessary.
>>       */
>>      unsigned int unspecified:1;
>> +    /* ARM/AMBA TrustZone Secure access */
>> +    unsigned int secure:1;
>>  } MemTxAttrs;
>>
>>  /* Bus masters which don't specify any attributes will get this,
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index d77c6de..a568299 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -14,7 +14,7 @@
>>  #ifndef CONFIG_USER_ONLY
>>  static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>>                                  int access_type, ARMMMUIdx mmu_idx,
>> -                                hwaddr *phys_ptr, int *prot,
>> +                                hwaddr *phys_ptr, MemTxAttrs *attrs, int 
>> *prot,
>>                                  target_ulong *page_size);
>>
>>  /* Definitions for the PMCCNTR and PMCR registers */
>> @@ -1466,9 +1466,10 @@ static uint64_t do_ats_write(CPUARMState *env, 
>> uint64_t value,
>>      int prot;
>>      int ret;
>>      uint64_t par64;
>> +    MemTxAttrs attrs = {};
>>
>>      ret = get_phys_addr(env, value, access_type, mmu_idx,
>> -                        &phys_addr, &prot, &page_size);
>> +                        &phys_addr, &attrs, &prot, &page_size);
>>      if (extended_addresses_enabled(env)) {
>>          /* ret is a DFSR/IFSR value for the long descriptor
>>           * translation table format, but with WnR always clear.
>> @@ -1477,6 +1478,9 @@ static uint64_t do_ats_write(CPUARMState *env, 
>> uint64_t value,
>>          par64 = (1 << 11); /* LPAE bit always set */
>>          if (ret == 0) {
>>              par64 |= phys_addr & ~0xfffULL;
>> +            if (!attrs.secure) {
>> +                par64 |= (1 << 9); /* NS */
>> +            }
>
> I know this is fitting in with the rest of the code but it does seem
> some of these magic numbers should be defined somewhere or at least a
> reference to the bitfield format added to the comments.

Mmm, maybe. I tend to assume that if you need to review this
code in detail you're looking at the relevant register
description documentation anyway.

If you want to submit patches fixing up some of our magic
numbers in older code I'm happy to take them :-)

thanks
-- PMM