| Message ID | 20220417174426.711829-33-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Cleanups, new features, new cpus | expand |
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
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 --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;
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(-)