Re: [PATCH v3 32/60] target/arm: Update sysreg fields when redirecting for E2H

[Richard Henderson]

On 4/22/22 03:39, Peter Maydell wrote:
> On Sun, 17 Apr 2022 at 19:07, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > The new_key is always non-zero during redirection,
> > so remove the if.  Update opc0 et al from the new key.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/arm/helper.c | 35 +++++++++++++++++++++++------------
> >   1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 7c569a569a..aee195400b 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -5915,7 +5915,9 @@ static void define_arm_vh_e2h_redirects_aliases(ARMCPU 
> > *cpu)
> >
> >       for (i = 0; i < ARRAY_SIZE(aliases); i++) {
> >           const struct E2HAlias *a = &aliases[i];
> > -        ARMCPRegInfo *src_reg, *dst_reg;
> > +        ARMCPRegInfo *src_reg, *dst_reg, *new_reg;
> > +        uint32_t *new_key;
> > +        bool ok;
> >
> >           if (a->feature && !a->feature(&cpu->isar)) {
> >               continue;
> > @@ -5934,19 +5936,28 @@ static void define_arm_vh_e2h_redirects_aliases(ARMCPU 
> > *cpu)
> >           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 = g_memdup(src_reg, sizeof(ARMCPRegInfo));
> > +        new_key = g_memdup(&a->new_key, sizeof(uint32_t));
> >
> > -            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 &= PL2_RW | PL3_RW;
> > +        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 &= PL2_RW;
> >
> > -            ok = g_hash_table_insert(cpu->cp_regs, new_key, new_reg);
> > -            g_assert(ok);
> > -        }
> > +#define E(X, N) \
> > +    ((X & CP_REG_ARM64_SYSREG_##N##_MASK) >> CP_REG_ARM64_SYSREG_##N##_SHIFT)
> > +
> > +        /* Update the sysreg fields */
> > +        new_reg->opc0 = E(a->new_key, OP0);
> > +        new_reg->opc1 = E(a->new_key, OP1);
> > +        new_reg->crn = E(a->new_key, CRN);
> > +        new_reg->crm = E(a->new_key, CRM);
> > +        new_reg->opc2 = E(a->new_key, OP2);
> > +
> > +#undef E
> > +
> > +        ok = g_hash_table_insert(cpu->cp_regs, new_key, new_reg);
> > +        g_assert(ok);
>
> So is setting the new_reg opc etc fields here fixing a bug
> (or otherwise changing behaviour)?
>
> The effect is that read/write callbacks now get an ARMCPRegInfo*
> that has the opc &c for the alias, rather than for the thing being
> aliased. That's good if the read/write callbacks have a need to
> distinguish the alias access from a normal one (do they anywhere?).
> On the other hand it's bad if we have existing code that thinks it
> can distinguish FOO_EL1 from FOO_EL2 by looking at the opc &c
> values and now might get confused.
>
> Overall, unless we have a definite reason why we want the
> callback functions to be able to tell the alias from the normal
> access, I'm inclined to say we should just comment that we
> deliberately leave the sysreg fields alone. (Put another way,
> I don't really want to have to work through all the aliased
> registers here checking whether they have read/write functions
> that look at the opc fields and whether any of them would
> end up doing the wrong thing when handed the alias reginfo.)

I don't recall what I was thinking here, it's definitely wrong.

> The "remove the if()" part is fine if you wanted to do that
> as its own patch.

Yeah, I'll do that, since it's always true.


r~