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

["Rémi Denis-Courmont" <remi@remlab.net>]

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