Re: [Qemu-devel] [PATCH v2 17/27] target/arm: Create ARMVAParameters and helpers

[Peter Maydell]

On Fri, 14 Dec 2018 at 05:24, 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>
> ----
> v2: Incorporate feedback wrt VTCR, HTCR, and more.
> ---
>  target/arm/internals.h |  16 +++
>  target/arm/helper.c    | 286 +++++++++++++++++++++++------------------
>  2 files changed, 174 insertions(+), 128 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1d0d0392c9..9ef9d01ee2 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -935,4 +935,20 @@ static inline ARMMMUIdx arm_stage1_mmu_idx(CPUARMState 
> *env)
>  ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env);
>  #endif
>
> +/*
> + * Parameters of a given virtual address, as extracted from a the

"a" or "the" but not both :-)

> + * translation control register (TCR) for a given regime.
> + */
> +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;
> +
>  #endif
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b1c0ff923f..3422fa5943 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9744,6 +9744,133 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, 
> uint8_t s2attrs)
>      return (hiattr << 6) | (hihint << 4) | (loattr << 2) | lohint;
>  }
>
> +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);
> +        if (mmu_idx == ARMMMUIdx_S2NS) {
> +            /* VTCR_EL2 */
> +            tbi = tbid = hpd = false;
> +        } else {
> +            tbi = extract32(tcr, 20, 1);
> +            hpd = extract32(tcr, 24, 1);
> +            tbid = extract32(tcr, 29, 1);
> +        }
> +        ha = extract32(tcr, 21, 1);
> +        hd = extract32(tcr, 22, 1);
> +        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),

This check on TBID is a behaviour change, isn't it ?
Can we please keep the "no change refactoring" separate from
"add new behaviour"? This patch is tricky enough to review
already. (I like the way the code ends up, but it has taken
me a fair amount of time to try to confirm that it does indeed
do the same thing as the old code, and I'm still not completely
sure about the changes in the ttbr select logic, though I
think it ends up doing the same thing.)

Similarly, .ha and .hd are new, I think, so could be added
in a later patch (presumably with whatever uses them).

thanks
-- PMM