Re: [Qemu-devel] [PATCH v4 03/13] target-arm: Add support for AArch32 S2 negative t0sz

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 04:29:35PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <address@hidden> wrote:
> > From: "Edgar E. Iglesias" <address@hidden>
> >
> > Add support for AArch32 S2 negative t0sz. In preparation for
> > using 40bit IPAs on AArch32.
> >
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> >  target-arm/helper.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 4e19838..a8a46db 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6475,6 +6475,17 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> > target_ulong address,
> >      if (va_size == 64) {
> >          t0sz = MIN(t0sz, 39);
> >          t0sz = MAX(t0sz, 16);
> > +    } else {
> > +        bool sext = extract32(t0sz, 4, 1);
> > +        bool sign = extract32(t0sz, 3, 1);
> > +        t0sz = sextract32(t0sz, 0, 4);
> > +
> > +        /* If the sign-extend bit is not the same as t0sz[3], the result
> > +         * is unpredictable. Flag this as a guest error.  */
> > +        if (sign != sext) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
> > +        }
> 
> Shouldn't this be guarded by a check on whether this is an s2
> translation, since the 4-bit signed T0SZ and the S bit are only for
> the VTCR, not for the normal TTBCRs ?

Yes, sounds good. I've changed the patch to the following:

@@ -6521,8 +6521,24 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
      */
     int32_t t0sz = extract32(tcr->raw_tcr, 0, 6);
     if (va_size == 64) {
+        /* AArch64 translation.  */
         t0sz = MIN(t0sz, 39);
         t0sz = MAX(t0sz, 16);
+    } else if (mmu_idx != ARMMMUIdx_S2NS) {
+        /* AArch32 stage 1 translation.  */
+        t0sz = extract32(t0sz, 0, 3);
+    } else {
+        /* AArch32 stage 2 translation.  */
+        bool sext = extract32(t0sz, 4, 1);
+        bool sign = extract32(t0sz, 3, 1);
+        t0sz = sextract32(t0sz, 0, 4);
+
+        /* If the sign-extend bit is not the same as t0sz[3], the result
+         * is unpredictable. Flag this as a guest error.  */
+        if (sign != sext) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "AArch32: VTCR.S / VTCR.T0SZ[3] missmatch\n");
+        }
     }


We can also remove the error log and add more complete checks in future
patches if you prefer...

Cheers,
Edgar




> 
> That is, we have 3 cases here for determining t0sz:
>  * AArch64 6-bit unsigned field
>  * AArch32 stage 1 3-bit unsigned field
>  * AArch32 stage 2 4-bit signed field
> so we need more than just a single if/else.
> 
> It's true that bits 3 and 4 are RES0 for TTBCR, but if we're
> going to actually start logging guest errors here maybe we
> should actually report the real problem (RES0 bits being set)
> for that case.
> 
> thanks
> -- PMM