Re: [PATCH 2/4] target/arm: Correctly honour HCR.TID3 for v7A cores

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> The HCR.TID3 bit defines that we should trap to the hypervisor for
> reads to a collection of ID registers. Different architecture versions
> have defined this differently:
>
>  * v7A has a set of ID regs that definitely must trap:
>     - ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3},
>       ID_ISAR{0,1,2,3,4,5}, MVFR{0,1}
>    and somewhat vaguely says that "there is no requirement"
>    to trap for registers that are reserved in the ID reg space
>    (i.e. which RAZ and might be used for new ID regs in future)
>  * v8A adds to this list:
>     - ID_PFR2 and MVFR2 must trap
>     - ID_MMFR4, ID_MMFR5, ID_ISAR6, ID_DFR1 and reserved registers
>       in the ID reg space must trap if FEAT_FGT is implemented,
>       and it is IMPDEF if they trap if FEAT_FGT is not implemented
>
> In QEMU we seem to have attempted to implement this distinction
> (taking the "we do trap" IMPDEF choice if no FEAT_FGT), with
> access_aa64_tid3() always trapping on TID3 and access_aa32_tid3()
> trapping only if ARM_FEATURE_V8 is set.  However, we didn't apply
> these to the right set of registers: we use access_aa32_tid3() on all
> the 32-bit ID registers *except* ID_PFR2, ID_DFR1, ID_MMFR5 and the
> RES0 space, which means that for a v7 CPU we don't trap on a lot of
> registers that we should trap on, and we do trap on various things
> that the v7A Arm ARM says there is "no requirement" to trap on.
>
> Straighten this out by naming the access functions more clearly for
> their purpose, and documenting this: access_v7_tid3() is only for the
> fixed set of ID registers that v7A traps on HCR.TID3, and
> access_tid3() is for any others, including the reserved encoding
> spaces and any new registers we add in future.

I'm confused by the naming - especially as searching the Arm doc site
with the Armv7-A filter didn't show up an HCR register (although it does
show up in the PDF).

I guess what you are saying is these registers trap from v7a onwards?
v8/v9 don't change the trapping on this subset of registers.

>
> AArch32 MVFR2 access is handled differently, in check_hcr_el2_trap;
> there we already do not trap on TID3 on v7A cores (where MVFR2
> doesn't exist), because we in the code-generation function we UNDEF
> if ARM_FEATURE_V8 is not set, without generating code to call
> check_hcr_el2_trap.
>
> This bug was causing a problem for Xen which (after a recent change
> to Xen) expects to be able to trap ID_PFR0 on a Cortex-A15.
>
> The result of these changes is that our v8A behaviour remains
> the same, and on v7A we now trap the registers the Arm ARM definitely
> requires us to trap, and don't trap the reserved space that "there is
> no requirement" to trap.
>
> Cc: qemu-stable@nongnu.org
> Fixes: 6a4ef4e5d1084c ("target/arm: Honor HCR_EL2.TID3 trapping requirements")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 146 ++++++++++++++++++++++++--------------------
>  1 file changed, 81 insertions(+), 65 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ec82ea6203..c4f73eb3f3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5835,9 +5835,18 @@ static const ARMCPRegInfo ccsidr2_reginfo[] = {
>        .readfn = ccsidr2_read, .type = ARM_CP_NO_RAW },
>  };
>  
> -static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> -                                       bool isread)
> +static CPAccessResult access_v7_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
> +    /*
> +     * Trap on TID3 always. This should be used only for the fixed set of
> +     * registers which are defined to trap on HCR.TID3 in v7A, which is:
> +     *   ID_PFR{0,1}, ID_DFR0, ID_AFR0, ID_MMFR{0,1,2,3}, ID_ISAR{0,1,2,3,4,5}
> +     * (MVFR0 and MVFR1 also trap in v7A, but this is not handled via
> +     * this accessfn but in check_hcr_el2_trap.)
> +     * Any other registers in the TID3 trap space should use access_tid3(),
> +     * so that they trap on v8 and above, but not on v7.
> +     */
>      if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
>          return CP_ACCESS_TRAP_EL2;
>      }
> @@ -5845,11 +5854,18 @@ static CPAccessResult access_aa64_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult access_aa32_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> -                                       bool isread)
> +static CPAccessResult access_tid3(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
>  {
> +    /*
> +     * Trap on TID3, if we implement at least v8. For v8 and above
> +     * the ID register space is at least IMPDEF permitted to trap,
> +     * and must trap if FEAT_FGT is implemented. We choose to trap
> +     * always. Use this function for any new registers that should
> +     * trap on TID3.
> +     */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
> -        return access_aa64_tid3(env, ri, isread);
> +        return access_v7_tid3(env, ri, isread);

This seems even more incongruous - we test for v8 but use the v7 helper. 

<snip>

I think I understand whats happening but it is confusing to follow.
Naming things is hard.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro