Re: [Qemu-arm] [PATCH V3] target/arm: change arch timer registers access permission

[gengdongjiu]

Add "Reviewed-by" from Richard because Richard has reviewed it in patch v2[1], 
thanks.

Reviewed-by: Richard Henderson <address@hidden>

[1]: https://www.mail-archive.com/address@hidden/msg604509.html

On 2019/3/12 20:52, Dongjiu Geng wrote:
> Some generic arch timer registers are Config-RW in the EL0,
> which means the EL0 exception level can have write permission
> if it is appropriately configured.
> 
> When VM access registers, QEMU firstly checks whether they have RW
> permission, then check whether it is appropriately configured.
> If they are defined to read only in EL0, even though they have been
> appropriately configured, they still do not have write permission.
> So need to add the write permission according to ARMV8 spec when
> define it.
> 
> Signed-off-by: Dongjiu Geng <address@hidden>
> ---
> Change since V2:
> 1. Change 'Ready only' to 'read only' in the comments
> 
> Change since V1:
> 1. Change 'PL1_RW | RL0_RW' to PL0_RW because PL0_RW implied that the higher 
> PLx all have RW permission
> 
> When VM kernel or Hypervisor configures the timer registers to RW in EL0
> user space, it will still have below panic when EL0 user space access
> the timer registers.
> 
> [INFO ]@(el0_sync:60): UNIMPLEMENTED, esr=2000000
> [INFO ]@(unimpl_exception:88): KERNEL UNIMPLEMENTED EXCEPTION
> [INFO ]@(unimpl_exception:98): FAR=0000000000000000, ESR=02000000 (EC=0x0, 
> IL=0x1, ISS=0x0)
> [INFO ]@(dump_registers:64): KERNEL REGISTERS
> [INFO ]@(dump_registers:68): X0=00000000f52b7d50 X1=00000000040d5040
> [INFO ]@(dump_registers:68): X2=0000004000033e10 X3=0000000000000000
> [INFO ]@(dump_registers:68): X4=000000007fffffff X5=0000000000000020
> [INFO ]@(dump_registers:68): X6=0000000000000020 X7=000000000c00b030
> ---
>  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 2607d39..c8d3c21 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2665,7 +2665,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      /* per-timer control */
>      { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 
> 1,
>        .secure = ARM_CP_SECSTATE_NS,
> -      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL0_RW,
>        .accessfn = gt_ptimer_access,
>        .fieldoffset = offsetoflow32(CPUARMState,
>                                     cp15.c14_timer[GTIMER_PHYS].ctl),
> @@ -2674,7 +2674,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      { .name = "CNTP_CTL_S",
>        .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1,
>        .secure = ARM_CP_SECSTATE_S,
> -      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL0_RW,
>        .accessfn = gt_ptimer_access,
>        .fieldoffset = offsetoflow32(CPUARMState,
>                                     cp15.c14_timer[GTIMER_SEC].ctl),
> @@ -2682,14 +2682,14 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] 
> = {
>      },
>      { .name = "CNTP_CTL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 1,
> -      .type = ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_ptimer_access,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].ctl),
>        .resetvalue = 0,
>        .writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
>      },
>      { .name = "CNTV_CTL", .cp = 15, .crn = 14, .crm = 3, .opc1 = 0, .opc2 = 
> 1,
> -      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL0_RW,
>        .accessfn = gt_vtimer_access,
>        .fieldoffset = offsetoflow32(CPUARMState,
>                                     cp15.c14_timer[GTIMER_VIRT].ctl),
> @@ -2697,7 +2697,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      },
>      { .name = "CNTV_CTL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 1,
> -      .type = ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_vtimer_access,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].ctl),
>        .resetvalue = 0,
> @@ -2706,31 +2706,31 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] 
> = {
>      /* TimerValue views: a 32 bit downcounting view of the underlying state 
> */
>      { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 
> 0,
>        .secure = ARM_CP_SECSTATE_NS,
> -      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_ptimer_access,
>        .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
>      },
>      { .name = "CNTP_TVAL_S",
>        .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
>        .secure = ARM_CP_SECSTATE_S,
> -      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_ptimer_access,
>        .readfn = gt_sec_tval_read, .writefn = gt_sec_tval_write,
>      },
>      { .name = "CNTP_TVAL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 0,
> -      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_ptimer_access, .resetfn = gt_phys_timer_reset,
>        .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
>      },
>      { .name = "CNTV_TVAL", .cp = 15, .crn = 14, .crm = 3, .opc1 = 0, .opc2 = 
> 0,
> -      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_vtimer_access,
>        .readfn = gt_virt_tval_read, .writefn = gt_virt_tval_write,
>      },
>      { .name = "CNTV_TVAL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 0,
> -      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL0_RW,
>        .accessfn = gt_vtimer_access, .resetfn = gt_virt_timer_reset,
>        .readfn = gt_virt_tval_read, .writefn = gt_virt_tval_write,
>      },
> @@ -2758,7 +2758,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      /* Comparison value, indicating when the timer goes off */
>      { .name = "CNTP_CVAL", .cp = 15, .crm = 14, .opc1 = 2,
>        .secure = ARM_CP_SECSTATE_NS,
> -      .access = PL1_RW | PL0_R,
> +      .access = PL0_RW,
>        .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].cval),
>        .accessfn = gt_ptimer_access,
> @@ -2766,7 +2766,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      },
>      { .name = "CNTP_CVAL_S", .cp = 15, .crm = 14, .opc1 = 2,
>        .secure = ARM_CP_SECSTATE_S,
> -      .access = PL1_RW | PL0_R,
> +      .access = PL0_RW,
>        .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
>        .accessfn = gt_ptimer_access,
> @@ -2774,14 +2774,14 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] 
> = {
>      },
>      { .name = "CNTP_CVAL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 2,
> -      .access = PL1_RW | PL0_R,
> +      .access = PL0_RW,
>        .type = ARM_CP_IO,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].cval),
>        .resetvalue = 0, .accessfn = gt_ptimer_access,
>        .writefn = gt_phys_cval_write, .raw_writefn = raw_write,
>      },
>      { .name = "CNTV_CVAL", .cp = 15, .crm = 14, .opc1 = 3,
> -      .access = PL1_RW | PL0_R,
> +      .access = PL0_RW,
>        .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].cval),
>        .accessfn = gt_vtimer_access,
> @@ -2789,7 +2789,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      },
>      { .name = "CNTV_CVAL_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 2,
> -      .access = PL1_RW | PL0_R,
> +      .access = PL0_RW,
>        .type = ARM_CP_IO,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].cval),
>        .resetvalue = 0, .accessfn = gt_vtimer_access,
>