diff mbox series

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

Message ID 20220417174426.711829-33-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Cleanups, new features, new cpus | expand

Commit Message

Richard Henderson April 17, 2022, 5:43 p.m. UTC
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(-)

Comments

Peter Maydell April 22, 2022, 10:39 a.m. UTC | #1
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.)

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

thanks
-- PMM
Richard Henderson May 1, 2022, 1:03 a.m. UTC | #2
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~
diff mbox series

Patch

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);
 
         src_reg->opaque = dst_reg;
         src_reg->orig_readfn = src_reg->readfn ?: raw_read;