Re: [PATCH 12/22] target/arm: NSTable is RES0 for the RME EL3 regime

[Richard Henderson <richard.henderson@linaro.org>]

On 2/10/23 01:36, Peter Maydell wrote:
> On Tue, 24 Jan 2023 at 00:01, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Test in_space instead of in_secure so that we don't switch
>> out of Root space.  Handle the output space change immediately,
>> rather than try and combine the NSTable and NS bits later.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/ptw.c | 31 ++++++++++++++-----------------
>>   1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
>> index c1b0b8e610..ddafb1f329 100644
>> --- a/target/arm/ptw.c
>> +++ b/target/arm/ptw.c
>> @@ -1240,7 +1240,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>>   {
>>       ARMCPU *cpu = env_archcpu(env);
>>       ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
>> -    bool is_secure = ptw->in_secure;
>>       int32_t level;
>>       ARMVAParameters param;
>>       uint64_t ttbr;
>> @@ -1256,7 +1255,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>>       uint64_t descaddrmask;
>>       bool aarch64 = arm_el_is_aa64(env, el);
>>       uint64_t descriptor, new_descriptor;
>> -    bool nstable;
>>
>>       /* TODO: This code does not support shareability levels. */
>>       if (aarch64) {
>> @@ -1417,29 +1415,29 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
>>           descaddrmask = MAKE_64BIT_MASK(0, 40);
>>       }
>>       descaddrmask &= ~indexmask_grainsize;
>> -
>> -    /*
>> -     * Secure accesses start with the page table in secure memory and
>> -     * can be downgraded to non-secure at any step. Non-secure accesses
>> -     * remain non-secure. We implement this by just ORing in the NSTable/NS
>> -     * bits at each step.
>> -     */
>> -    tableattrs = is_secure ? 0 : (1 << 4);
>> +    tableattrs = 0;
>>
>>    next_level:
>>       descaddr |= (address >> (stride * (4 - level))) & indexmask;
>>       descaddr &= ~7ULL;
>> -    nstable = extract32(tableattrs, 4, 1);
>> -    if (nstable && ptw->in_secure) {
>> -        /*
>> -         * Stage2_S -> Stage2 or Phys_S -> Phys_NS
>> -         * Assert that the non-secure idx are even, and relative order.
>> -         */
>> +
>> +    /*
>> +     * Process the NSTable bit from the previous level.  This changes
>> +     * the table address space and the output space from Secure to
>> +     * NonSecure.  With RME, the EL3 translation regime does not change
>> +     * from Root to NonSecure.
>> +     */
> 
> To check my understanding, this means that the bit that the spec
> describes as FEAT_RME changing the behaviour of NSTable in the EL3
> stage 1 translation regime is implemented by us by having the
> in_space for EL3 be different for FEAT_RME and not-FEAT_RME ?

Correct -- space is Secure for non-RME EL3, and Root for RME EL3.

>>       attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50, 14));
>>       if (!regime_is_stage2(mmu_idx)) {
>> -        attrs |= nstable << 5; /* NS */
> 
> This removes the code where we copy the NSTable bit across to attrs,
> but there's still code below here that assumes it can get the combined
> NS bit from bit 5 of attrs, isn't there? (It passes it to get_S1prot().)

Oops.  This gets fixed in patch 14.  Some reordering needed...


r~