Re: [PATCH v4 27/40] target/arm: Add VHE system register redirection and aliasing

[Peter Maydell]

On Tue, 3 Dec 2019 at 02:30, Richard Henderson
<address@hidden> wrote:
>
> Several of the EL1/0 registers are redirected to the EL2 version when in
> EL2 and HCR_EL2.E2H is set.  Many of these registers have side effects.
> Link together the two ARMCPRegInfo structures after they have been
> properly instantiated.  Install common dispatch routines to all of the
> relevant registers.
>
> The same set of registers that are redirected also have additional
> EL12/EL02 aliases created to access the original register that was
> redirected.
>
> Omit the generic timer registers from redirection here, because we'll
> need multiple kinds of redirection from both EL0 and EL2.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/cpu.h    |  44 ++++++++----
>  target/arm/helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 193 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 4bd1bf915c..bb5a72520e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2488,19 +2488,6 @@ struct ARMCPRegInfo {
>       */
>      ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
>
> -    /* Offsets of the secure and non-secure fields in CPUARMState for the
> -     * register if it is banked.  These fields are only used during the 
> static
> -     * registration of a register.  During hashing the bank associated
> -     * with a given security state is copied to fieldoffset which is used 
> from
> -     * there on out.
> -     *
> -     * It is expected that register definitions use either fieldoffset or
> -     * bank_fieldoffsets in the definition but not both.  It is also expected
> -     * that both bank offsets are set when defining a banked register.  This
> -     * use indicates that a register is banked.
> -     */
> -    ptrdiff_t bank_fieldoffsets[2];
> -
>      /* Function for making any access checks for this register in addition to
>       * those specified by the 'access' permissions bits. If NULL, no extra
>       * checks required. The access check is performed at runtime, not at
> @@ -2535,6 +2522,37 @@ struct ARMCPRegInfo {
>       * fieldoffset is 0 then no reset will be done.
>       */
>      CPResetFn *resetfn;
> +
> +    union {
> +        /*
> +         * Offsets of the secure and non-secure fields in CPUARMState for
> +         * the register if it is banked.  These fields are only used during
> +         * the static registration of a register.  During hashing the bank
> +         * associated with a given security state is copied to fieldoffset
> +         * which is used from there on out.
> +         *
> +         * It is expected that register definitions use either fieldoffset
> +         * or bank_fieldoffsets in the definition but not both.  It is also
> +         * expected that both bank offsets are set when defining a banked
> +         * register.  This use indicates that a register is banked.
> +         */
> +        ptrdiff_t bank_fieldoffsets[2];
> +
> +        /*
> +         * "Original" writefn and readfn.
> +         * For ARMv8.1-VHE register aliases, we overwrite the read/write
> +         * accessor functions of various EL1/EL0 to perform the runtime
> +         * check for which sysreg should actually be modified, and then
> +         * forwards the operation.  Before overwriting the accessors,
> +         * the original function is copied here, so that accesses that
> +         * really do go to the EL1/EL0 version proceed normally.
> +         * (The corresponding EL2 register is linked via opaque.)
> +         */
> +        struct {
> +            CPReadFn *orig_readfn;
> +            CPWriteFn *orig_writefn;
> +        };

Does this really need to be a union ? It's not clear to me
why we know the two halves of it are never used at the same time.

> +    };
>  };
>
>  /* Macros which are lvalues for the field in CPUARMState for the
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 1812588fa1..0baf188078 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5306,6 +5306,158 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> +#ifndef CONFIG_USER_ONLY
> +/* Test if system register redirection is to occur in the current state.  */
> +static bool redirect_for_e2h(CPUARMState *env)
> +{
> +    return arm_current_el(env) == 2 && (arm_hcr_el2_eff(env) & HCR_E2H);
> +}
> +
> +static uint64_t el2_e2h_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    CPReadFn *readfn;
> +
> +    if (redirect_for_e2h(env)) {
> +        /* Switch to the saved EL2 version of the register.  */
> +        ri = ri->opaque;
> +        readfn = ri->readfn;
> +    } else {
> +        readfn = ri->orig_readfn;
> +    }
> +    if (readfn == NULL) {
> +        readfn = raw_read;
> +    }
> +    return readfn(env, ri);
> +}
> +
> +static void el2_e2h_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                          uint64_t value)
> +{
> +    CPWriteFn *writefn;
> +
> +    if (redirect_for_e2h(env)) {
> +        /* Switch to the saved EL2 version of the register.  */
> +        ri = ri->opaque;
> +        writefn = ri->writefn;
> +    } else {
> +        writefn = ri->orig_writefn;
> +    }
> +    if (writefn == NULL) {
> +        writefn = raw_write;
> +    }
> +    writefn(env, ri, value);
> +}

I see how this works when we have a readfn or writefn,
but how does the redirection work where the access
goes directly via .fieldoffset ?

thanks
-- PMM