[v4,03/13] target-arm: Add support for AArch32 S2 negative t0sz

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for AArch32 S2 negative t0sz. In preparation for
using 40bit IPAs on AArch32.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for AArch32 S2 negative t0sz. In preparation for
> using 40bit IPAs on AArch32.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  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 ?

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

On Fri, Oct 23, 2015 at 04:29:35PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add support for AArch32 S2 negative t0sz. In preparation for
> > using 40bit IPAs on AArch32.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  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

On 26 October 2015 at 09:20, Edgar E. Iglesias
<edgar.iglesias@xilinx.com> wrote:
> 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");
> +        }
>      }
>

Looks good, but maybe we should just do all the extracts
on tcr->raw_tcr, rather than extracting 6 bits of it and
then re-extracting some subset of bits from that extract
(for the 32-bit stage 1 case in particular it would be
simpler).

-- PMM

On Mon, Oct 26, 2015 at 09:52:12AM +0000, Peter Maydell wrote:
> On 26 October 2015 at 09:20, Edgar E. Iglesias
> <edgar.iglesias@xilinx.com> wrote:
> > 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");
> > +        }
> >      }
> >
> 
> Looks good, but maybe we should just do all the extracts
> on tcr->raw_tcr, rather than extracting 6 bits of it and
> then re-extracting some subset of bits from that extract
> (for the 32-bit stage 1 case in particular it would be
> simpler).

OK, I've rearranged the code a bit to use raw_tcr.

Thanks,
Edgar