[Qemu-devel] [PATCH] target-arm: Prepare cpreg writefns/readfns for EL3/SecExt

[Qemu-devel] [PATCH] target-arm: Prepare cpreg writefns/readfns for EL3/SecExt

[Fabian Aggeler]

This patch changes some readfns/writefns to use raw_write
and raw_read functions, wich use the fieldoffset specified
in ARMCPRegInfo instead of directly accessing the field.
This will simplify patches for EL3 & Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
This patch was previously part of the Security Extension patchset
but is not really Sec-Ext specific.

 target-arm/helper.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 417161e..6302d67 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -319,7 +319,7 @@ static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
 
-    env->cp15.c3 = value;
+    raw_write(env, ri, value);
     tlb_flush(CPU(cpu), 1); /* Flush TLB as domain not tracked in TLB */
 }
 
@@ -327,12 +327,12 @@ static void fcse_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
 
-    if (env->cp15.c13_fcse != value) {
+    if (raw_read(env, ri) != value) {
         /* Unlike real hardware the qemu TLB uses virtual addresses,
          * not modified virtual addresses, so this causes a TLB flush.
          */
         tlb_flush(CPU(cpu), 1);
-        env->cp15.c13_fcse = value;
+        raw_write(env, ri, value);
     }
 }
 
@@ -341,7 +341,7 @@ static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
 
-    if (env->cp15.contextidr_el1 != value && !arm_feature(env, ARM_FEATURE_MPU)
+    if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_MPU)
         && !extended_addresses_enabled(env)) {
         /* For VMSA (when not using the LPAE long descriptor page table
          * format) this register includes the ASID, so do a TLB flush.
@@ -349,7 +349,7 @@ static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
          */
         tlb_flush(CPU(cpu), 1);
     }
-    env->cp15.contextidr_el1 = value;
+    raw_write(env, ri, value);
 }
 
 static void tlbiall_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -657,7 +657,7 @@ static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
      * contexts. (ARMv8 would permit us to do no masking at all, but ARMv7
      * requires the bottom five bits to be RAZ/WI because they're UNK/SBZP.)
      */
-    env->cp15.c12_vbar = value & ~0x1FULL;
+    raw_write(env, ri, value & ~0x1Ful);
 }
 
 static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -669,7 +669,7 @@ static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                          uint64_t value)
 {
-    env->cp15.c0_cssel = value & 0xf;
+    raw_write(env, ri, value & 0xf);
 }
 
 static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -1192,11 +1192,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
 static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     if (arm_feature(env, ARM_FEATURE_LPAE)) {
-        env->cp15.par_el1 = value;
+        raw_write(env, ri, value);
     } else if (arm_feature(env, ARM_FEATURE_V7)) {
-        env->cp15.par_el1 = value & 0xfffff6ff;
+        raw_write(env, ri, value & 0xfffff6ff);
     } else {
-        env->cp15.par_el1 = value & 0xfffff1ff;
+        raw_write(env, ri, value & 0xfffff1ff);
     }
 }
 
@@ -1399,7 +1399,7 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
      * for long-descriptor tables the TTBCR fields are used differently
      * and the c2_mask and c2_base_mask values are meaningless.
      */
-    env->cp15.c2_control = value;
+    raw_write(env, ri, value);
     env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
     env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
 }
@@ -1421,7 +1421,7 @@ static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     env->cp15.c2_base_mask = 0xffffc000u;
-    env->cp15.c2_control = 0;
+    raw_write(env, ri, 0);
     env->cp15.c2_mask = 0;
 }
 
@@ -1432,7 +1432,7 @@ static void vmsa_tcr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
     /* For AArch64 the A1 bit could result in a change of ASID, so TLB flush. */
     tlb_flush(CPU(cpu), 1);
-    env->cp15.c2_control = value;
+    raw_write(env, ri, value);
 }
 
 static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -2081,14 +2081,14 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
 
-    if (env->cp15.c1_sys == value) {
+    if (raw_read(env, ri) == value) {
         /* Skip the TLB flush if nothing actually changed; Linux likes
          * to do a lot of pointless SCTLR writes.
          */
         return;
     }
 
-    env->cp15.c1_sys = value;
+    raw_write(env, ri, value);
     /* ??? Lots of these bits are not implemented.  */
     /* This may enable/disable the MMU, so do a TLB flush.  */
     tlb_flush(CPU(cpu), 1);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] target-arm: Prepare cpreg writefns/readfns for EL3/SecExt

[Aggeler Fabian]

On 31 May 2014, at 02:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Fri, May 16, 2014 at 10:43 PM, Fabian Aggeler <aggelerf@ethz.ch> wrote:
>> This patch changes some readfns/writefns to use raw_write
>> and raw_read functions, wich use the fieldoffset specified
> 
> "which"
> 
>> in ARMCPRegInfo instead of directly accessing the field.
>> This will simplify patches for EL3 & Security Extensions.
>> 
> 
> Yes I like this idea is generally and universally. It makes the code
> more self documenting as these raw_write/raw_read sites clearly
> indicate that "this is the actual register state value", which
> everything else in the fn is then side effects. It does also mean any
> renaming of variables in the env now only have to be changed twice (in
> the env and in the .fieldoffset of CPRegInfo) rather than three/four
> times (in read/write handlers as well).
> 
>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> ---
>> This patch was previously part of the Security Extension patchset
>> but is not really Sec-Ext specific.
>> 
>> target-arm/helper.c | 30 +++++++++++++++---------------
>> 1 file changed, 15 insertions(+), 15 deletions(-)
>> 
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 417161e..6302d67 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -319,7 +319,7 @@ static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> {
>>     ARMCPU *cpu = arm_env_get_cpu(env);
>> 
>> -    env->cp15.c3 = value;
>> +    raw_write(env, ri, value);
>>     tlb_flush(CPU(cpu), 1); /* Flush TLB as domain not tracked in TLB */
>> }
>> 
>> @@ -327,12 +327,12 @@ static void fcse_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> {
>>     ARMCPU *cpu = arm_env_get_cpu(env);
>> 
>> -    if (env->cp15.c13_fcse != value) {
>> +    if (raw_read(env, ri) != value) {
>>         /* Unlike real hardware the qemu TLB uses virtual addresses,
>>          * not modified virtual addresses, so this causes a TLB flush.
>>          */
>>         tlb_flush(CPU(cpu), 1);
>> -        env->cp15.c13_fcse = value;
>> +        raw_write(env, ri, value);
>>     }
>> }
>> 
>> @@ -341,7 +341,7 @@ static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> {
>>     ARMCPU *cpu = arm_env_get_cpu(env);
>> 
>> -    if (env->cp15.contextidr_el1 != value && !arm_feature(env, ARM_FEATURE_MPU)
>> +    if (raw_read(env, ri) != value && !arm_feature(env, ARM_FEATURE_MPU)
>>         && !extended_addresses_enabled(env)) {
>>         /* For VMSA (when not using the LPAE long descriptor page table
>>          * format) this register includes the ASID, so do a TLB flush.
>> @@ -349,7 +349,7 @@ static void contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>          */
>>         tlb_flush(CPU(cpu), 1);
>>     }
>> -    env->cp15.contextidr_el1 = value;
>> +    raw_write(env, ri, value);
>> }
>> 
>> static void tlbiall_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> @@ -657,7 +657,7 @@ static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      * contexts. (ARMv8 would permit us to do no masking at all, but ARMv7
>>      * requires the bottom five bits to be RAZ/WI because they're UNK/SBZP.)
>>      */
>> -    env->cp15.c12_vbar = value & ~0x1FULL;
>> +    raw_write(env, ri, value & ~0x1Ful);
> 
> This one was already done in Edgar's series (now merged) so best to
> rebase to catch any other conflicts.
> 
> But otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks, I will rebase and send v2 to avoid conflicts.

Fabian

> 
>> }
>> 
>> static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> @@ -669,7 +669,7 @@ static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                          uint64_t value)
>> {
>> -    env->cp15.c0_cssel = value & 0xf;
>> +    raw_write(env, ri, value & 0xf);
>> }
>> 
>> static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> @@ -1192,11 +1192,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>> static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> {
>>     if (arm_feature(env, ARM_FEATURE_LPAE)) {
>> -        env->cp15.par_el1 = value;
>> +        raw_write(env, ri, value);
>>     } else if (arm_feature(env, ARM_FEATURE_V7)) {
>> -        env->cp15.par_el1 = value & 0xfffff6ff;
>> +        raw_write(env, ri, value & 0xfffff6ff);
>>     } else {
>> -        env->cp15.par_el1 = value & 0xfffff1ff;
>> +        raw_write(env, ri, value & 0xfffff1ff);
>>     }
>> }
>> 
>> @@ -1399,7 +1399,7 @@ static void vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      * for long-descriptor tables the TTBCR fields are used differently
>>      * and the c2_mask and c2_base_mask values are meaningless.
>>      */
>> -    env->cp15.c2_control = value;
>> +    raw_write(env, ri, value);
>>     env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
>>     env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
>> }
>> @@ -1421,7 +1421,7 @@ static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>> {
>>     env->cp15.c2_base_mask = 0xffffc000u;
>> -    env->cp15.c2_control = 0;
>> +    raw_write(env, ri, 0);
>>     env->cp15.c2_mask = 0;
>> }
>> 
>> @@ -1432,7 +1432,7 @@ static void vmsa_tcr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> 
>>     /* For AArch64 the A1 bit could result in a change of ASID, so TLB flush. */
>>     tlb_flush(CPU(cpu), 1);
>> -    env->cp15.c2_control = value;
>> +    raw_write(env, ri, value);
>> }
>> 
>> static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> @@ -2081,14 +2081,14 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> {
>>     ARMCPU *cpu = arm_env_get_cpu(env);
>> 
>> -    if (env->cp15.c1_sys == value) {
>> +    if (raw_read(env, ri) == value) {
>>         /* Skip the TLB flush if nothing actually changed; Linux likes
>>          * to do a lot of pointless SCTLR writes.
>>          */
>>         return;
>>     }
>> 
>> -    env->cp15.c1_sys = value;
>> +    raw_write(env, ri, value);
>>     /* ??? Lots of these bits are not implemented.  */
>>     /* This may enable/disable the MMU, so do a TLB flush.  */
>>     tlb_flush(CPU(cpu), 1);
>> --
>> 1.8.3.2