Re: [PATCH v5 28/41] target/arm: Add VHE system register redirection and aliasing

[Peter Maydell]

On Wed, 29 Jan 2020 at 23:56, 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>
> ---
> v5: Drop unioning in ARMCPRegInfo with bank_fieldoffsets[].
> ---


> +    for (i = 0; i < ARRAY_SIZE(aliases); i++) {
> +        const struct E2HAlias *a = &aliases[i];
> +        ARMCPRegInfo *src_reg, *dst_reg;
> +
> +        if (a->feature && !a->feature(&cpu->isar)) {
> +            continue;
> +        }
> +
> +        src_reg = g_hash_table_lookup(cpu->cp_regs, &a->src_key);
> +        dst_reg = g_hash_table_lookup(cpu->cp_regs, &a->dst_key);
> +        g_assert(src_reg != NULL);
> +        g_assert(dst_reg != NULL);
> +
> +        /* Cross-compare names to detect typos in the keys.  */
> +        g_assert(strcmp(src_reg->name, a->src_name) == 0);
> +        g_assert(strcmp(dst_reg->name, a->dst_name) == 0);
> +
> +        /* None of the core system registers use opaque; we will.  */
> +        g_assert(src_reg->opaque == NULL);
> +
> +        /* Create alias before redirection so we dup the right data. */
> +        if (a->new_key) {
> +            ARMCPRegInfo *new_reg = g_memdup(src_reg, sizeof(ARMCPRegInfo));
> +            uint32_t *new_key = g_memdup(&a->new_key, sizeof(uint32_t));
> +            bool ok;
> +
> +            new_reg->name = a->new_name;
> +            new_reg->type |= ARM_CP_ALIAS;
> +            /* Remove PL1/PL0 access, leaving PL2/PL3 R/W in place.  */
> +            new_reg->access &= 0xf0;

That seems like it would be more clear written as
   new_reg->access &= (PL2_RW | PL3_RW);

(strictly there the PL3_RW is useless as PL2_RW implies it but
it captures the intent better I think)

> +
> +            ok = g_hash_table_insert(cpu->cp_regs, new_key, new_reg);
> +            g_assert(ok);
> +        }
> +
> +        src_reg->opaque = dst_reg;
> +        src_reg->orig_readfn = src_reg->readfn ?: raw_read;
> +        src_reg->orig_writefn = src_reg->writefn ?: raw_write;
> +        if (!src_reg->raw_readfn) {
> +            src_reg->raw_readfn = raw_read;
> +        }
> +        if (!src_reg->raw_writefn) {
> +            src_reg->raw_writefn = raw_write;
> +        }
> +        src_reg->readfn = el2_e2h_read;
> +        src_reg->writefn = el2_e2h_write;
> +    }
> +}
> +#endif

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM