target/arm: don't bother with id_aa64pfr0_read for USER_ONLY

Message ID 20191206122247.7507-1-alex.bennee@linaro.org
State Superseded
Headers show
Series
  • target/arm: don't bother with id_aa64pfr0_read for USER_ONLY
Related show

Commit Message

Alex Bennée Dec. 6, 2019, 12:22 p.m.
For system emulation we need to check the state of the GIC before we
report the value. However this isn't relevant to exporting of the
value to linux-user and indeed breaks the exported value as set by
modify_arm_cp_regs.

[AJB: the other option would be just to set reset value anyway and not
ifdef out the readfn as the register will become const anyway]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 target/arm/helper.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.20.1

Comments

Peter Maydell Dec. 6, 2019, 3:29 p.m. | #1
On Fri, 6 Dec 2019 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:
>

> For system emulation we need to check the state of the GIC before we

> report the value. However this isn't relevant to exporting of the

> value to linux-user and indeed breaks the exported value as set by

> modify_arm_cp_regs.

>

> [AJB: the other option would be just to set reset value anyway and not

> ifdef out the readfn as the register will become const anyway]


If you want it to be const it would be clearer to define it
with ARM_CP_CONST... I'm not sure what an ARM_CP_NO_RAW without
a readfn or a fieldoffset will do on reads.

thanks
-- PMM
Richard Henderson Dec. 6, 2019, 5:52 p.m. | #2
On 12/6/19 7:29 AM, Peter Maydell wrote:
> On Fri, 6 Dec 2019 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:

>>

>> For system emulation we need to check the state of the GIC before we

>> report the value. However this isn't relevant to exporting of the

>> value to linux-user and indeed breaks the exported value as set by

>> modify_arm_cp_regs.

>>

>> [AJB: the other option would be just to set reset value anyway and not

>> ifdef out the readfn as the register will become const anyway]

> 

> If you want it to be const it would be clearer to define it

> with ARM_CP_CONST... I'm not sure what an ARM_CP_NO_RAW without

> a readfn or a fieldoffset will do on reads.


Yep, and the accessfn should be ifdefed away as well.


r~
Alex Bennée Dec. 6, 2019, 6:25 p.m. | #3
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 6 Dec 2019 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:

>>

>> For system emulation we need to check the state of the GIC before we

>> report the value. However this isn't relevant to exporting of the

>> value to linux-user and indeed breaks the exported value as set by

>> modify_arm_cp_regs.

>>

>> [AJB: the other option would be just to set reset value anyway and not

>> ifdef out the readfn as the register will become const anyway]

>

> If you want it to be const it would be clearer to define it

> with ARM_CP_CONST... I'm not sure what an ARM_CP_NO_RAW without

> a readfn or a fieldoffset will do on reads.


Well the modify_arm_cp_regs ensures it is ARM_CP_CONST when it changes
the definition. It's just ensuring the reset value is set so it can be
masked/fixed.

However the ifdef approach does reduce the amount of unused stuff in the
linux-user build.

>

> thanks

> -- PMM



-- 
Alex Bennée

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f78dd3b5fe3..489c31504a6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5867,6 +5867,7 @@  static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return pfr1;
 }
 
+#ifndef CONFIG_USER_ONLY
 static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -5877,6 +5878,7 @@  static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
     }
     return pfr0;
 }
+#endif
 
 /* Shared logic between LORID and the rest of the LOR* registers.
  * Secure state has already been delt with.
@@ -6297,16 +6299,22 @@  void register_cp_regs_for_features(ARMCPU *cpu)
          * define new registers here.
          */
         ARMCPRegInfo v8_idregs[] = {
-            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
-             * know the right value for the GIC field until after we
-             * define these regs.
+            /*
+             * ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST in system
+             * emulation because we don't know the right value for the
+             * GIC field until after we define these regs.
              */
             { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_NO_RAW,
               .accessfn = access_aa64_tid3,
+#ifdef CONFIG_USER_ONLY
+              .resetvalue = cpu->isar.id_aa64pfr0
+#else
               .readfn = id_aa64pfr0_read,
-              .writefn = arm_cp_write_ignore },
+              .writefn = arm_cp_write_ignore
+#endif
+            },
             { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,