Re: [Qemu-devel] [PATCH 16/26] target/arm: Create ARMVAParameters and helpers

[Peter Maydell]

On Fri, 7 Dec 2018 at 10:37, Richard Henderson
<address@hidden> wrote:
>
> Split out functions to extract the virtual address parameters.
> Let the functions choose T0 or T1 address space half, if present.
> Extract (most of) the control bits that vary between EL or Tx.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper.c | 274 ++++++++++++++++++++++++--------------------
>  1 file changed, 147 insertions(+), 127 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index be8daefc46..99ceed2cab 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9754,6 +9754,123 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, 
> uint8_t s2attrs)
>      return (hiattr << 6) | (hihint << 4) | (loattr << 2) | lohint;
>  }
>
> +typedef struct ARMVAParameters {
> +    unsigned tsz    : 8;
> +    unsigned select : 1;
> +    bool tbi        : 1;
> +    bool epd        : 1;
> +    bool hpd        : 1;
> +    bool ha         : 1;
> +    bool hd         : 1;
> +    bool using16k   : 1;
> +    bool using64k   : 1;
> +} ARMVAParameters;
> +
> +static ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
> +                                          ARMMMUIdx mmu_idx, bool data)
> +{
> +    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> +    uint32_t el = regime_el(env, mmu_idx);
> +    bool tbi, tbid, epd, hpd, ha, hd, using16k, using64k;
> +    int select, tsz;
> +
> +    /* Bit 55 is always between the two regions, and is canonical for
> +     * determining if address tagging is enabled.
> +     */
> +    select = extract64(va, 55, 1);
> +
> +    if (el > 1) {
> +        tsz = extract32(tcr, 0, 6);
> +        using64k = extract32(tcr, 14, 1);
> +        using16k = extract32(tcr, 15, 1);
> +        tbi = extract32(tcr, 20, 1);

The old code explicitly avoided looking at the TCR bit 20
for the mmu_idx == ARMMMUIdx_S2NS case. At the moment VTCR_EL2
bit 20 is RES0 but it might not be in future.

> +        ha = extract32(tcr, 21, 1);
> +        hd = extract32(tcr, 22, 1);
> +        hpd = extract32(tcr, 24, 1);
> +        tbid = extract32(tcr, 29, 1);

No HPD in VTCR_EL2 either, and bit 29 is NSW there.

> +        epd = false;
> +    } else if (!select) {
> +        tsz = extract32(tcr, 0, 6);
> +        epd = extract32(tcr, 7, 1);
> +        using64k = extract32(tcr, 14, 1);
> +        using16k = extract32(tcr, 15, 1);
> +        tbi = extract64(tcr, 37, 1);
> +        ha = extract64(tcr, 39, 1);
> +        hd = extract64(tcr, 40, 1);
> +        hpd = extract64(tcr, 41, 1);
> +        tbid = extract64(tcr, 51, 1);
> +    } else {
> +        int tg = extract32(tcr, 30, 2);
> +        using16k = tg == 1;
> +        using64k = tg == 3;
> +        tsz = extract32(tcr, 16, 6);
> +        epd = extract32(tcr, 23, 1);
> +        tbi = extract64(tcr, 38, 1);
> +        ha = extract64(tcr, 39, 1);
> +        hd = extract64(tcr, 40, 1);
> +        hpd = extract64(tcr, 42, 1);
> +        tbid = extract64(tcr, 52, 1);
> +    }
> +    tsz = MIN(tsz, 39);  /* TODO: ARMv8.4-TTST */
> +    tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
> +
> +    return (ARMVAParameters) {
> +        .tsz = tsz,
> +        .select = select,
> +        .tbi = tbi & (data | !tbid),
> +        .epd = epd,
> +        .hpd = hpd,
> +        .ha = ha,
> +        .hd = hd,
> +        .using16k = using16k,
> +        .using64k = using64k,
> +    };
> +}
> +
> +static ARMVAParameters aa32_va_parameters(CPUARMState *env, uint32_t va,
> +                                          ARMMMUIdx mmu_idx)
> +{
> +    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> +    int select, tsz;
> +    bool epd, hpd;
> +
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        tsz = sextract32(tcr, 0, 4) + 8;
> +        select = 0;
> +        hpd = false;
> +        epd = false;
> +    } else {
> +        int t0sz = extract32(tcr, 0, 3);
> +        int t1sz = extract32(tcr, 16, 3);

If this is EL2 (ie regime_tcr() is HTCR) then we
shouldn't be looking at bits 16:3, because there is
no T1SZ field there.

> +
> +        if (t1sz == 0) {
> +            select = va > (0xffffffffu >> t0sz);
> +        } else {
> +            /* Note that we will detect errors later.  */
> +            select = va >= ~(0xffffffffu >> t1sz);
> +        }
> +
> +        if (!select) {
> +            tsz = t0sz;
> +            epd = extract32(tcr, 7, 1);
> +            hpd = extract64(tcr, 41, 1);
> +        } else {
> +            tsz = t1sz;
> +            epd = extract32(tcr, 23, 1);
> +            hpd = extract64(tcr, 42, 1);

In HTCR the HPD bit is 24, and there is no EPD bit and no T2E bit.

> +        }
> +        /* For aarch32, hpd0 is not enabled without t2e as well.  */
> +        hpd &= extract32(tcr, 6, 1);
> +    }
> +
> +    return (ARMVAParameters) {
> +        .tsz = tsz,
> +        .select = select,
> +        .epd = epd,
> +        .hpd = hpd,
> +    };
> +}
> +
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
> *prot,
> @@ -9765,26 +9882,20 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>      /* Read an LPAE long-descriptor translation table. */
>      ARMFaultType fault_type = ARMFault_Translation;
>      uint32_t level;
> -    uint32_t epd = 0;
> -    int32_t t0sz, t1sz;
> -    uint32_t tg;
> +    ARMVAParameters param;
>      uint64_t ttbr;
> -    int ttbr_select;
>      hwaddr descaddr, indexmask, indexmask_grainsize;
>      uint32_t tableattrs;
>      target_ulong page_size;
>      uint32_t attrs;
> -    int32_t stride = 9;
> -    int32_t addrsize;
> -    int inputsize;
> -    int32_t tbi = 0;
> +    int32_t stride;
> +    int top_bit, inputsize;
>      TCR *tcr = regime_tcr(env, mmu_idx);
>      int ap, ns, xn, pxn;
>      uint32_t el = regime_el(env, mmu_idx);
> -    bool ttbr1_valid = true;
> +    bool ttbr1_valid;
>      uint64_t descaddrmask;
>      bool aarch64 = arm_el_is_aa64(env, el);
> -    bool hpd = false;
>
>      /* TODO:
>       * This code does not handle the different format TCR for VTCR_EL2.
> @@ -9793,91 +9904,43 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>       * support for those page table walks.
>       */
>      if (aarch64) {
> +        param = aa64_va_parameters(env, address, mmu_idx,
> +                                   access_type != MMU_INST_FETCH);
>          level = 0;
> -        addrsize = 64;
> -        if (el > 1) {
> -            if (mmu_idx != ARMMMUIdx_S2NS) {
> -                tbi = extract64(tcr->raw_tcr, 20, 1);
> -            }
> -        } else {
> -            if (extract64(address, 55, 1)) {
> -                tbi = extract64(tcr->raw_tcr, 38, 1);
> -            } else {
> -                tbi = extract64(tcr->raw_tcr, 37, 1);
> -            }
> -        }
> -        tbi *= 8;
> -
>          /* If we are in 64-bit EL2 or EL3 then there is no TTBR1, so mark it
>           * invalid.
>           */
> -        if (el > 1) {
> -            ttbr1_valid = false;
> -        }
> +        ttbr1_valid = (el < 1);
>      } else {
> +        param = aa32_va_parameters(env, address, mmu_idx);
>          level = 1;
> -        addrsize = 32;
>          /* There is no TTBR1 for EL2 */
> -        if (el == 2) {
> -            ttbr1_valid = false;
> -        }
> +        ttbr1_valid = (el != 2);
>      }
>
> -    /* Determine whether this address is in the region controlled by
> -     * TTBR0 or TTBR1 (or if it is in neither region and should fault).
> -     * This is a Non-secure PL0/1 stage 1 translation, so controlled by
> -     * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
> +    top_bit = (aarch64 ? 64 : 32);

The old code had 3 cases here: 64 for AArch64, 32 for AArch32 stage 1,
and 40 for AArch32 stage 2.

> +    inputsize = top_bit - param.tsz;
> +    top_bit -= 8 * param.tbi;
> +
> +    /* We determined the region when collecting the parameters, but we
> +     * have not yet validated that the address is valid for the region.
>       */
> -    if (aarch64) {
> -        /* AArch64 translation.  */
> -        t0sz = extract32(tcr->raw_tcr, 0, 6);
> -        t0sz = MIN(t0sz, 39);
> -        t0sz = MAX(t0sz, 16);
> -    } else if (mmu_idx != ARMMMUIdx_S2NS) {
> -        /* AArch32 stage 1 translation.  */
> -        t0sz = extract32(tcr->raw_tcr, 0, 3);
> -    } else {
> -        /* AArch32 stage 2 translation.  */
> -        bool sext = extract32(tcr->raw_tcr, 4, 1);
> -        bool sign = extract32(tcr->raw_tcr, 3, 1);
> -        /* Address size is 40-bit for a stage 2 translation,
> -         * and t0sz can be negative (from -8 to 7),
> -         * so we need to adjust it to use the TTBR selecting logic below.
> -         */
> -        addrsize = 40;
> -        t0sz = sextract32(tcr->raw_tcr, 0, 4) + 8;
> -
> -        /* 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] mismatch\n");

This guest-error check seems to have vanished in the refactoring.

> -        }
> -    }
> -    t1sz = extract32(tcr->raw_tcr, 16, 6);
> -    if (aarch64) {
> -        t1sz = MIN(t1sz, 39);
> -        t1sz = MAX(t1sz, 16);
> -    }
> -    if (t0sz && !extract64(address, addrsize - t0sz, t0sz - tbi)) {
> -        /* there is a ttbr0 region and we are in it (high bits all zero) */
> -        ttbr_select = 0;
> -    } else if (ttbr1_valid && t1sz &&
> -               !extract64(~address, addrsize - t1sz, t1sz - tbi)) {
> -        /* there is a ttbr1 region and we are in it (high bits all one) */
> -        ttbr_select = 1;
> -    } else if (!t0sz) {
> -        /* ttbr0 region is "everything not in the ttbr1 region" */
> -        ttbr_select = 0;
> -    } else if (!t1sz && ttbr1_valid) {
> -        /* ttbr1 region is "everything not in the ttbr0 region" */
> -        ttbr_select = 1;
> -    } else {
> -        /* in the gap between the two regions, this is a Translation fault */
> +    if (param.select
> +        ? extract64(~address, inputsize, top_bit - inputsize) || !ttbr1_valid
> +        : extract64(address, inputsize, top_bit - inputsize)) {

I find this expression rather confusing...

> +        /* In the gap between the two regions, this is a Translation fault */
>          fault_type = ARMFault_Translation;
>          goto do_fault;
>      }
>
> +    if (param.using64k) {
> +        stride = 13;
> +    } else if (param.using16k) {
> +        stride = 11;
> +    } else {
> +        stride = 9;
> +    }
> +
>      /* Note that QEMU ignores shareability and cacheability attributes,
>       * so we don't need to do anything with the SH, ORGN, IRGN fields
>       * in the TTBCR.  Similarly, TTBCR:A1 selects whether we get the
> @@ -9885,56 +9948,13 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>       * implement any ASID-like capability so we can ignore it (instead
>       * we will always flush the TLB any time the ASID is changed).
>       */
> -    if (ttbr_select == 0) {
> -        ttbr = regime_ttbr(env, mmu_idx, 0);
> -        if (el < 2) {
> -            epd = extract32(tcr->raw_tcr, 7, 1);
> -        }
> -        inputsize = addrsize - t0sz;
> -
> -        tg = extract32(tcr->raw_tcr, 14, 2);
> -        if (tg == 1) { /* 64KB pages */
> -            stride = 13;
> -        }
> -        if (tg == 2) { /* 16KB pages */
> -            stride = 11;
> -        }
> -        if (aarch64 && el > 1) {
> -            hpd = extract64(tcr->raw_tcr, 24, 1);
> -        } else {
> -            hpd = extract64(tcr->raw_tcr, 41, 1);
> -        }
> -        if (!aarch64) {
> -            /* For aarch32, hpd0 is not enabled without t2e as well.  */
> -            hpd &= extract64(tcr->raw_tcr, 6, 1);
> -        }
> -    } else {
> -        /* We should only be here if TTBR1 is valid */
> -        assert(ttbr1_valid);
> -
> -        ttbr = regime_ttbr(env, mmu_idx, 1);
> -        epd = extract32(tcr->raw_tcr, 23, 1);
> -        inputsize = addrsize - t1sz;
> -
> -        tg = extract32(tcr->raw_tcr, 30, 2);
> -        if (tg == 3)  { /* 64KB pages */
> -            stride = 13;
> -        }
> -        if (tg == 1) { /* 16KB pages */
> -            stride = 11;
> -        }
> -        hpd = extract64(tcr->raw_tcr, 42, 1);
> -        if (!aarch64) {
> -            /* For aarch32, hpd1 is not enabled without t2e as well.  */
> -            hpd &= extract64(tcr->raw_tcr, 6, 1);
> -        }
> -    }
> +    ttbr = regime_ttbr(env, mmu_idx, param.select);
>
>      /* Here we should have set up all the parameters for the translation:
>       * inputsize, ttbr, epd, stride, tbi
>       */
>
> -    if (epd) {
> +    if (param.epd) {
>          /* Translation table walk disabled => Translation fault on TLB miss
>           * Note: This is always 0 on 64-bit EL2 and EL3.
>           */
> @@ -10047,7 +10067,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          }
>          /* Merge in attributes from table descriptors */
>          attrs |= nstable << 3; /* NS */
> -        if (hpd) {
> +        if (param.hpd) {
>              /* HPD disables all the table attributes except NSTable.  */
>              break;
>          }
> --
> 2.17.2
>

thanks
-- PMM