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

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

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~