1. Patchew
  2. QEMU
  3. [RFC][PATCH 0/3] ARMv8.4-TTST extension
  4. View patch

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

Rémi Denis-Courmont posted 3 patches 4 years, 11 months ago
[PATCH 1/3] target/arm: keep translation start level unsigned
Posted by remi.denis.courmont@huawei.com 4 years, 11 months ago 
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
Re: [PATCH 1/3] target/arm: keep translation start level unsigned
Posted by Richard Henderson 4 years, 10 months ago 
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 1/3] target/arm: keep translation start level unsigned
Posted by Rémi Denis-Courmont 4 years, 10 months ago 
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
Posted by Richard Henderson 4 years, 10 months ago 
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
Posted by Richard Henderson 4 years, 10 months ago 
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
Posted by Rémi Denis-Courmont 4 years, 10 months ago 
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
Posted by Richard Henderson 4 years, 10 months ago 
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
Posted by Peter Maydell 4 years, 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.