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

[Richard Henderson <richard.henderson@linaro.org>]

On 12/6/19 9:24 AM, Peter Maydell wrote:
>> +    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.

We don't really need to use a union.  I should probably change that.  I think
AJB had the same question vs one of the previous revisions.

We know they're not used at the same time because bank_fieldoffsets is only
used *before* the structure is duplicated and added into the hash table
(overwriting .fieldoffset depending on the bank as they are duplicated), and
orig_{read,write}fn are only used *after* the structure has been added to the
hash table.

>> +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 ?

When there is no .writefn, we use raw_write, which uses fieldoffset.


r~