Re: [Qemu-devel] [PATCH v2 05/10] target/arm: Introduce arm_hcr_el2_eff

[Peter Maydell]

On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<address@hidden> wrote:
>
> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
> account, as documented for the plethora of bits in HCR_EL2.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/cpu.h          | 67 +++++++++------------------------------
>  hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
>  target/arm/helper.c       | 65 +++++++++++++++++++++++++++++++------
>  3 files changed, 82 insertions(+), 71 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 20d97b66de..e871b946c8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
>  }
>  #endif
>
> +/**
> + * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
> + * E.g. when in secure state, fields in HCR_EL2 are suppressed,
> + * "for all purposes other than a direct read or write access of HCR_EL2."
> + * Not included here are RW, VI, VF.
> + */
> +uint64_t arm_hcr_el2_eff(CPUARMState *env);

This comment says the not-included bits are RW, VI, VF...

> +/*
> + * Return the effective value of HCR_EL2.
> + * Bits that are not included here:
> + * RW       (read from SCR_EL3.RW as needed)
> + */

...but this comment only lists RW. Which is correct ?

Also, if VI, VF are in the list shouldn't VSE be too ?

> +uint64_t arm_hcr_el2_eff(CPUARMState *env)
> +{
> +    uint64_t ret = env->cp15.hcr_el2;
> +
> +    if (arm_is_secure_below_el3(env)) {
> +        /*
> +         * "This register has no effect if EL2 is not enabled in the
> +         * current Security state".  This is ARMv8.4-SecEL2 speak for
> +         * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
> +         *
> +         * Prior to that, the language was "In an implementation that
> +         * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
> +         * as if this field is 0 for all purposes other than a direct
> +         * read or write access of HCR_EL2".  With lots of enumeration
> +         * on a per-field basis.  In current QEMU, this is condition
> +         * is arm_is_secure_below_el3.
> +         *
> +         * Since the v8.4 language applies to the entire register, and
> +         * appears to be backward compatible, use that.
> +         */
> +        ret = 0;
> +    } else if (ret & HCR_TGE) {
> +        if (ret & HCR_E2H) {
> +            ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
> +                     HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
> +                     HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
> +                     HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
> +                     HCR_IMO | HCR_FMO | HCR_VM);


This list should also in theory include HCR_MIOCNCE, but the
QEMU implementation of that bit is going to be RAZ/WI anyway.

HCR_TID1 is in the "ignore if TGE==1" group, not the "ignore if
{E2H, TGE} == {1,1}" group.

Maybe we should be also setting HCR_ATA here ? We could perhaps
leave that til we implement ARMv8.5-MemTag and understand it better.

> +        } else {
> +            ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
> +                     HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
> +                     HCR_TRVM | HCR_TLOR);

Shouldn't these bits be being cleared regardless of E2H, rather
than only in the TGE==1 E2H==0 case ?

Missing HCR_SWIO ?

The list of bits cleared if TGE && E2H is highest-first, but this
list is lowest-first...

> +            ret |= HCR_FMO | HCR_IMO | HCR_AMO;
> +        }
> +    }
> +
> +    return ret;
> +}

Patch looks good otherwise.

thanks
-- PMM