[RFC][PATCH 0/3] ARMv8.4-TTST extension

[RFC][PATCH 0/3] ARMv8.4-TTST extension

[Rémi Denis-Courmont]

	Hello,

The following changes since commit 6d3fd902ab9c4d6762532d4b5cce2275e458aa32:

  target/arm: refactor vae1_tlbmask() (2020-12-18 12:27:56 +0200)

follow. They add support for small translation tables, allowing for 
translation regimes as small as 16-bits. This extension is nominally a 
required dependency of Secure EL2.

----------------------------------------------------------------
Rémi Denis-Courmont (3):
      target/arm: keep translation start level unsigned
      target/arm: ARMv8.4-TTST extension
      target/arm: enable Small Translation tables in max CPU

 target/arm/cpu.h    |  5 +++++
 target/arm/cpu64.c  |  1 +
 target/arm/helper.c | 25 +++++++++++++++----------
 3 files changed, 21 insertions(+), 10 deletions(-)

-- 
Rémi Denis-Courmont

[PATCH 1/3] target/arm: keep translation start level unsigned

[remi.denis.courmont]

From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>

Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
---
 target/arm/helper.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index df195c314c..b927e53ab0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10821,17 +10821,12 @@ do_fault:
  * Returns true if the suggested S2 translation parameters are OK and
  * false otherwise.
  */
-static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
+static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
                                int inputsize, int stride)
 {
     const int grainsize = stride + 3;
     int startsizecheck;
 
-    /* Negative levels are never allowed.  */
-    if (level < 0) {
-        return false;
-    }
-
     startsizecheck = inputsize - ((3 - level) * stride + grainsize);
     if (startsizecheck < 1 || startsizecheck > stride + 4) {
         return false;
@@ -10856,6 +10851,9 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
             if (level == 0 && pamax <= 42) {
                 return false;
             }
+            if (level == 3) {
+                return false;
+            }
             break;
         default:
             g_assert_not_reached();
@@ -10871,7 +10869,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
         /* AArch32 only supports 4KB pages. Assert on that.  */
         assert(stride == 9);
 
-        if (level == 0) {
+        if (level == 0 || level >= 3) {
             return false;
         }
     }
@@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
 
         if (!aarch64 || stride == 9) {
             /* AArch32 or 4KB pages */
-            startlevel = 2 - sl0;
+            startlevel = (2 - sl0) & 3;
         } else {
             /* 16KB or 64KB pages */
             startlevel = 3 - sl0;
-- 
2.29.2

[PATCH 2/3] target/arm: ARMv8.4-TTST extension

[remi.denis.courmont]

From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>

This adds for the Small Translation tables extension in AArch64 state.

Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
---
 target/arm/cpu.h    |  5 +++++
 target/arm/helper.c | 13 ++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 39abb2a36b..604b9cdd0e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3991,6 +3991,11 @@ static inline bool isar_feature_aa64_uao(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0;
 }
 
+static inline bool isar_feature_aa64_st(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, ST) != 0;
+}
+
 static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b927e53ab0..c3a186db35 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10851,7 +10851,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
             if (level == 0 && pamax <= 42) {
                 return false;
             }
-            if (level == 3) {
+            if (level == 3 && !cpu_isar_feature(aa64_st, cpu)) {
                 return false;
             }
             break;
@@ -10946,7 +10946,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 {
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
     bool epd, hpd, using16k, using64k;
-    int select, tsz, tbi;
+    int select, tsz, tbi, max_tsz;
 
     if (!regime_has_2_ranges(mmu_idx)) {
         select = 0;
@@ -10981,7 +10981,14 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
             hpd = extract64(tcr, 42, 1);
         }
     }
-    tsz = MIN(tsz, 39);  /* TODO: ARMv8.4-TTST */
+
+    if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
+        max_tsz = 48 - using64k;
+    } else {
+        max_tsz = 39;
+    }
+
+    tsz = MIN(tsz, max_tsz);
     tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
 
     /* Present TBI as a composite with TBID.  */
-- 
2.29.2

[PATCH 3/3] target/arm: enable Small Translation tables in max CPU

[remi.denis.courmont]

From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>

Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
---
 target/arm/cpu64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 3dc2f5da6c..8d3473db3e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -670,6 +670,7 @@ static void aarch64_max_initfn(Object *obj)
         t = cpu->isar.id_aa64mmfr2;
         t = FIELD_DP64(t, ID_AA64MMFR2, UAO, 1);
         t = FIELD_DP64(t, ID_AA64MMFR2, CNP, 1); /* TTCNP */
+        t = FIELD_DP64(t, ID_AA64MMFR2, ST, 1); /* TTST */
         cpu->isar.id_aa64mmfr2 = t;
 
         /* Replicate the same data to the 32-bit id registers.  */
-- 
2.29.2

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Richard Henderson]

On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> ---
>  target/arm/helper.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

The patch does more than what is described above.

> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index df195c314c..b927e53ab0 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10821,17 +10821,12 @@ do_fault:
>   * Returns true if the suggested S2 translation parameters are OK and
>   * false otherwise.
>   */
> -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
>                                 int inputsize, int stride)
>  {
>      const int grainsize = stride + 3;
>      int startsizecheck;
>  
> -    /* Negative levels are never allowed.  */
> -    if (level < 0) {
> -        return false;
> -    }
> -

I would expect this to be the only hunk from the patch description.  Probably
changing this negative check to a >= 3 check.


r~

>      startsizecheck = inputsize - ((3 - level) * stride + grainsize);
>      if (startsizecheck < 1 || startsizecheck > stride + 4) {
>          return false;
> @@ -10856,6 +10851,9 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>              if (level == 0 && pamax <= 42) {
>                  return false;
>              }
> +            if (level == 3) {
> +                return false;
> +            }
>              break;
>          default:
>              g_assert_not_reached();
> @@ -10871,7 +10869,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>          /* AArch32 only supports 4KB pages. Assert on that.  */
>          assert(stride == 9);
>  
> -        if (level == 0) {
> +        if (level == 0 || level >= 3) {
>              return false;
>          }
>      }
> @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>  
>          if (!aarch64 || stride == 9) {
>              /* AArch32 or 4KB pages */
> -            startlevel = 2 - sl0;
> +            startlevel = (2 - sl0) & 3;
>          } else {
>              /* 16KB or 64KB pages */
>              startlevel = 3 - sl0;
> 

Re: [PATCH 2/3] target/arm: ARMv8.4-TTST extension

[Richard Henderson]

On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> 
> This adds for the Small Translation tables extension in AArch64 state.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> ---
>  target/arm/cpu.h    |  5 +++++
>  target/arm/helper.c | 13 ++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 39abb2a36b..604b9cdd0e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3991,6 +3991,11 @@ static inline bool isar_feature_aa64_uao(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, UAO) != 0;
>  }
>  
> +static inline bool isar_feature_aa64_st(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64mmfr2, ID_AA64MMFR2, ST) != 0;
> +}
> +
>  static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b927e53ab0..c3a186db35 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -10851,7 +10851,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
>              if (level == 0 && pamax <= 42) {
>                  return false;
>              }
> -            if (level == 3) {
> +            if (level == 3 && !cpu_isar_feature(aa64_st, cpu)) {
>                  return false;
>              }
>              break;

As mentioned vs patch 1, I think this hunk should be handled differently.

> @@ -10946,7 +10946,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>  {
>      uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
>      bool epd, hpd, using16k, using64k;
> -    int select, tsz, tbi;
> +    int select, tsz, tbi, max_tsz;
>  
>      if (!regime_has_2_ranges(mmu_idx)) {
>          select = 0;
> @@ -10981,7 +10981,14 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>              hpd = extract64(tcr, 42, 1);
>          }
>      }
> -    tsz = MIN(tsz, 39);  /* TODO: ARMv8.4-TTST */
> +
> +    if (cpu_isar_feature(aa64_st, env_archcpu(env))) {
> +        max_tsz = 48 - using64k;
> +    } else {
> +        max_tsz = 39;
> +    }
> +
> +    tsz = MIN(tsz, max_tsz);
>      tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
>  
>      /* Present TBI as a composite with TBID.  */
> 

But the rest of the patch looks good.


r~

Re: [PATCH 3/3] target/arm: enable Small Translation tables in max CPU

[Richard Henderson]

On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> ---
>  target/arm/cpu64.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Richard Henderson]

On 12/30/20 2:10 PM, Richard Henderson wrote:
> On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
>> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>>
>> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>> ---
>>  target/arm/helper.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> The patch does more than what is described above.
> 
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index df195c314c..b927e53ab0 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -10821,17 +10821,12 @@ do_fault:
>>   * Returns true if the suggested S2 translation parameters are OK and
>>   * false otherwise.
>>   */
>> -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>> +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
>>                                 int inputsize, int stride)
>>  {
>>      const int grainsize = stride + 3;
>>      int startsizecheck;
>>  
>> -    /* Negative levels are never allowed.  */
>> -    if (level < 0) {
>> -        return false;
>> -    }
>> -
> 
> I would expect this to be the only hunk from the patch description.  Probably
> changing this negative check to a >= 3 check.

Having read the next patch, I think you should drop this type change.

>> @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address,
>>  
>>          if (!aarch64 || stride == 9) {
>>              /* AArch32 or 4KB pages */
>> -            startlevel = 2 - sl0;
>> +            startlevel = (2 - sl0) & 3;

This hunk belongs with the next patch, implementing TTST, and should be
conditional.  I.e.

    if (stride == 9) {
        startlevel = 2 - sl0;
        if (aarch64 &&
            cpu_isar_feature(aa64_st, env_archcpu(env)) {
            startlevel &= 3;
        }
    ...


r~

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Rémi Denis-Courmont]

Le jeudi 31 décembre 2020, 00:10:09 EET Richard Henderson a écrit :
> On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> > From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> > 
> > Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> > ---
> > 
> >  target/arm/helper.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> The patch does more than what is described above.

No? It removes generating negative values, and handling them, for translation 
levels.

> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index df195c314c..b927e53ab0 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > 
> > @@ -10821,17 +10821,12 @@ do_fault:
> >   * Returns true if the suggested S2 translation parameters are OK and
> >   * false otherwise.
> >   */
> > 
> > -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> > +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t level,
> > 
> >                                 int inputsize, int stride)
> >  
> >  {
> >  
> >      const int grainsize = stride + 3;
> >      int startsizecheck;
> > 
> > -    /* Negative levels are never allowed.  */
> > -    if (level < 0) {
> > -        return false;
> > -    }
> > -
> 
> I would expect this to be the only hunk from the patch description. 
> Probably changing this negative check to a >= 3 check.

You could do that but you'd end up relying on implicity conversion from signed 
to unsigned negative. That seems needlessly confusing to me in this case, 
considering that (positive) values larger than 3 cannot actually happen.

> 
> r~
> 
> >      startsizecheck = inputsize - ((3 - level) * stride + grainsize);
> >      if (startsizecheck < 1 || startsizecheck > stride + 4) {
> >      
> >          return false;
> > 
> > @@ -10856,6 +10851,9 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool
> > is_aa64, int level,> 
> >              if (level == 0 && pamax <= 42) {
> >              
> >                  return false;
> >              
> >              }
> > 
> > +            if (level == 3) {
> > +                return false;
> > +            }
> > 
> >              break;
> >          
> >          default:
> >              g_assert_not_reached();
> > 
> > @@ -10871,7 +10869,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool
> > is_aa64, int level,> 
> >          /* AArch32 only supports 4KB pages. Assert on that.  */
> >          assert(stride == 9);
> > 
> > -        if (level == 0) {
> > +        if (level == 0 || level >= 3) {
> > 
> >              return false;
> >          
> >          }
> >      
> >      }
> > 
> > @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> > uint64_t address,> 
> >          if (!aarch64 || stride == 9) {
> >          
> >              /* AArch32 or 4KB pages */
> > 
> > -            startlevel = 2 - sl0;
> > +            startlevel = (2 - sl0) & 3;
> > 
> >          } else {
> >          
> >              /* 16KB or 64KB pages */
> >              startlevel = 3 - sl0;


-- 
Rémi Denis-Courmont

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Rémi Denis-Courmont]

Le jeudi 31 décembre 2020, 00:38:14 EET Richard Henderson a écrit :
> On 12/30/20 2:10 PM, Richard Henderson wrote:
> > On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> >> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> >> 
> >> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> >> ---
> >> 
> >>  target/arm/helper.c | 14 ++++++--------
> >>  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > The patch does more than what is described above.
> > 
> >> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >> index df195c314c..b927e53ab0 100644
> >> --- a/target/arm/helper.c
> >> +++ b/target/arm/helper.c
> >> 
> >> @@ -10821,17 +10821,12 @@ do_fault:
> >>   * Returns true if the suggested S2 translation parameters are OK and
> >>   * false otherwise.
> >>   */
> >> 
> >> -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> >> +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t
> >> level,
> >> 
> >>                                 int inputsize, int stride)
> >>  
> >>  {
> >>  
> >>      const int grainsize = stride + 3;
> >>      int startsizecheck;
> >> 
> >> -    /* Negative levels are never allowed.  */
> >> -    if (level < 0) {
> >> -        return false;
> >> -    }
> >> -
> > 
> > I would expect this to be the only hunk from the patch description. 
> > Probably changing this negative check to a >= 3 check.
> 
> Having read the next patch, I think you should drop this type change.
> 
> >> @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> >> uint64_t address,>> 
> >>          if (!aarch64 || stride == 9) {
> >>          
> >>              /* AArch32 or 4KB pages */
> >> 
> >> -            startlevel = 2 - sl0;
> >> +            startlevel = (2 - sl0) & 3;
> 
> This hunk belongs with the next patch, implementing TTST, and should be
> conditional.  I.e.
> 
>     if (stride == 9) {
>         startlevel = 2 - sl0;
>         if (aarch64 &&
>             cpu_isar_feature(aa64_st, env_archcpu(env)) {
>             startlevel &= 3;
>         }

You can do that but:
1) Nothing in the spec says that SL0 == b11 without ST means start level -1. 
It's undefined, and I don't see any reasons to treat it differently than with 
ST.
2) Functionally, checking for ST seems to belong naturally within 
check_s2_mmu_setup() in this particular case.

>     ...
> 
> 
> r~


-- 
Rémi Denis-Courmont

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Richard Henderson]

On 12/31/20 1:59 AM, Rémi Denis-Courmont wrote:
> Le jeudi 31 décembre 2020, 00:38:14 EET Richard Henderson a écrit :
>> On 12/30/20 2:10 PM, Richard Henderson wrote:
>>> On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
>>>> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>>>>
>>>> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>>>> ---
>>>>
>>>>  target/arm/helper.c | 14 ++++++--------
>>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>>
>>> The patch does more than what is described above.
>>>
>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>> index df195c314c..b927e53ab0 100644
>>>> --- a/target/arm/helper.c
>>>> +++ b/target/arm/helper.c
>>>>
>>>> @@ -10821,17 +10821,12 @@ do_fault:
>>>>   * Returns true if the suggested S2 translation parameters are OK and
>>>>   * false otherwise.
>>>>   */
>>>>
>>>> -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>>>> +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t
>>>> level,
>>>>
>>>>                                 int inputsize, int stride)
>>>>  
>>>>  {
>>>>  
>>>>      const int grainsize = stride + 3;
>>>>      int startsizecheck;
>>>>
>>>> -    /* Negative levels are never allowed.  */
>>>> -    if (level < 0) {
>>>> -        return false;
>>>> -    }
>>>> -
>>>
>>> I would expect this to be the only hunk from the patch description. 
>>> Probably changing this negative check to a >= 3 check.
>>
>> Having read the next patch, I think you should drop this type change.
>>
>>>> @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
>>>> uint64_t address,>> 
>>>>          if (!aarch64 || stride == 9) {
>>>>          
>>>>              /* AArch32 or 4KB pages */
>>>>
>>>> -            startlevel = 2 - sl0;
>>>> +            startlevel = (2 - sl0) & 3;
>>
>> This hunk belongs with the next patch, implementing TTST, and should be
>> conditional.  I.e.
>>
>>     if (stride == 9) {
>>         startlevel = 2 - sl0;
>>         if (aarch64 &&
>>             cpu_isar_feature(aa64_st, env_archcpu(env)) {
>>             startlevel &= 3;
>>         }
> 
> You can do that but:
> 1) Nothing in the spec says that SL0 == b11 without ST means start level -1. 
> It's undefined, and I don't see any reasons to treat it differently than with 
> ST.

By that logic there's no reason to treat it differently at all; you could drop
the feature check entirely.  In lieu, -1 makes a decent error indicator.

> 2) Functionally, checking for ST seems to belong naturally within 
> check_s2_mmu_setup() in this particular case.

I guess.  I'll leave it to Peter's preference.


r~

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Richard Henderson]

On 12/31/20 1:55 AM, Rémi Denis-Courmont wrote:
> Le jeudi 31 décembre 2020, 00:10:09 EET Richard Henderson a écrit :
>> On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
>>> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>>>
>>> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
>>> ---
>>>
>>>  target/arm/helper.c | 14 ++++++--------
>>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> The patch does more than what is described above.
> 
> No? It removes generating negative values, and handling them, for translation 
> levels.

But the removal of generating negative values, i.e. the mask with 3, is clearly
tied to TTST and makes no sense by itself.

Maybe the whole thing is clearer all squashed together...


r~

Re: [PATCH 1/3] target/arm: keep translation start level unsigned

[Peter Maydell]

On Thu, 31 Dec 2020 at 16:43, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/31/20 1:59 AM, Rémi Denis-Courmont wrote:
> > Le jeudi 31 décembre 2020, 00:38:14 EET Richard Henderson a écrit :
> >> On 12/30/20 2:10 PM, Richard Henderson wrote:
> >>> On 12/18/20 6:33 AM, remi.denis.courmont@huawei.com wrote:
> >>>> From: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> >>>>
> >>>> Signed-off-by: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
> >>>> ---
> >>>>
> >>>>  target/arm/helper.c | 14 ++++++--------
> >>>>  1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>> The patch does more than what is described above.
> >>>
> >>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >>>> index df195c314c..b927e53ab0 100644
> >>>> --- a/target/arm/helper.c
> >>>> +++ b/target/arm/helper.c
> >>>>
> >>>> @@ -10821,17 +10821,12 @@ do_fault:
> >>>>   * Returns true if the suggested S2 translation parameters are OK and
> >>>>   * false otherwise.
> >>>>   */
> >>>>
> >>>> -static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
> >>>> +static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, uint32_t
> >>>> level,
> >>>>
> >>>>                                 int inputsize, int stride)
> >>>>
> >>>>  {
> >>>>
> >>>>      const int grainsize = stride + 3;
> >>>>      int startsizecheck;
> >>>>
> >>>> -    /* Negative levels are never allowed.  */
> >>>> -    if (level < 0) {
> >>>> -        return false;
> >>>> -    }
> >>>> -
> >>>
> >>> I would expect this to be the only hunk from the patch description.
> >>> Probably changing this negative check to a >= 3 check.
> >>
> >> Having read the next patch, I think you should drop this type change.
> >>
> >>>> @@ -11203,7 +11201,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> >>>> uint64_t address,>>
> >>>>          if (!aarch64 || stride == 9) {
> >>>>
> >>>>              /* AArch32 or 4KB pages */
> >>>>
> >>>> -            startlevel = 2 - sl0;
> >>>> +            startlevel = (2 - sl0) & 3;
> >>
> >> This hunk belongs with the next patch, implementing TTST, and should be
> >> conditional.  I.e.
> >>
> >>     if (stride == 9) {
> >>         startlevel = 2 - sl0;
> >>         if (aarch64 &&
> >>             cpu_isar_feature(aa64_st, env_archcpu(env)) {
> >>             startlevel &= 3;
> >>         }
> >
> > You can do that but:
> > 1) Nothing in the spec says that SL0 == b11 without ST means start level -1.
> > It's undefined, and I don't see any reasons to treat it differently than with
> > ST.
>
> By that logic there's no reason to treat it differently at all; you could drop
> the feature check entirely.  In lieu, -1 makes a decent error indicator.
>
> > 2) Functionally, checking for ST seems to belong naturally within
> > check_s2_mmu_setup() in this particular case.
>
> I guess.  I'll leave it to Peter's preference.

You've reviewed the patchset, I'm happy to go with whatever your
preference is (because I don't want to have to duplicate the
review work to form an opinion).

thanks
-- PMM