| Message ID | 1441311266-8644-5-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 3 September 2015 at 21:14, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target-arm/cpu.h | 1 + > target-arm/helper.c | 28 ++++++++++++++++++++++++++-- > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 31825d3..ba22e12 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -223,6 +223,7 @@ typedef struct CPUARMState { > }; > /* MMU translation table base control. */ > TCR tcr_el[4]; > + TCR vtcr_el2; /* Virtualization Translation Control. */ > uint32_t c2_data; /* MPU data cachable bits. */ > uint32_t c2_insn; /* MPU instruction cachable bits. */ > union { /* MMU domain access control register > diff --git a/target-arm/helper.c b/target-arm/helper.c > index a057a70..c82aa1d 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -325,6 +325,21 @@ void init_cpreg_list(ARMCPU *cpu) > g_list_free(keys); > } > > +/* > + * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but > + * they are accesible when EL3 is using AArch64 regardless of EL3.NS. > + */ > +static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, > + const ARMCPRegInfo *ri) > +{ > + bool secure = arm_is_secure_below_el3(env); > + > + if (secure && !arm_el_is_aa64(env, 3)) { > + return CP_ACCESS_TRAP_UNCATEGORIZED; > + } > + return CP_ACCESS_OK; > +} This access function will always return OK for the AArch64 register, so probably better to split the regdef rather than using STATE_BOTH, and then avoid the accessfn on the 64-bit register. > + > static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > { > ARMCPU *cpu = arm_env_get_cpu(env); > @@ -3112,6 +3127,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = { > { .name = "TCR_EL2", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 2, > .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, > + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, > + .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore }, RAZ/WI register should use CP_CONST/resetvalue=0. (Access functions apply even for const registers.) > { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, > .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > @@ -3246,6 +3265,12 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { > .access = PL2_RW, .writefn = vmsa_tcr_el1_write, > .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, > .fieldoffset = offsetof(CPUARMState, cp15.tcr_el[2]) }, > + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, > + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, > + .writefn = vmsa_tcr_el1_write, There's no AS bit in the VTCR_EL2, so you could avoid an unnecessary TLB flush by not using the writefn we use for TCR_EL1. (I think that if you don't provide a writefn or raw_writefn it should just work, but check that...) > + .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, > + .fieldoffset = offsetof(CPUARMState, cp15.vtcr_el2) }, > { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, > .access = PL2_RW, .raw_writefn = raw_write, .writefn = sctlr_write, > @@ -5735,8 +5760,7 @@ static inline bool regime_translation_disabled(CPUARMState *env, > static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx) > { > if (mmu_idx == ARMMMUIdx_S2NS) { > - /* TODO: return VTCR_EL2 */ > - g_assert_not_reached(); > + return &env->cp15.vtcr_el2; > } > return &env->cp15.tcr_el[regime_el(env, mmu_idx)]; > } thanks -- PMM
On Tue, Sep 08, 2015 at 03:19:37PM +0100, Peter Maydell wrote: > On 3 September 2015 at 21:14, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/cpu.h | 1 + > > target-arm/helper.c | 28 ++++++++++++++++++++++++++-- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 31825d3..ba22e12 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -223,6 +223,7 @@ typedef struct CPUARMState { > > }; > > /* MMU translation table base control. */ > > TCR tcr_el[4]; > > + TCR vtcr_el2; /* Virtualization Translation Control. */ > > uint32_t c2_data; /* MPU data cachable bits. */ > > uint32_t c2_insn; /* MPU instruction cachable bits. */ > > union { /* MMU domain access control register > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index a057a70..c82aa1d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -325,6 +325,21 @@ void init_cpreg_list(ARMCPU *cpu) > > g_list_free(keys); > > } > > > > +/* > > + * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but > > + * they are accesible when EL3 is using AArch64 regardless of EL3.NS. > > + */ > > +static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, > > + const ARMCPRegInfo *ri) > > +{ > > + bool secure = arm_is_secure_below_el3(env); > > + > > + if (secure && !arm_el_is_aa64(env, 3)) { > > + return CP_ACCESS_TRAP_UNCATEGORIZED; > > + } > > + return CP_ACCESS_OK; > > +} > > This access function will always return OK for the AArch64 register, > so probably better to split the regdef rather than using STATE_BOTH, > and then avoid the accessfn on the 64-bit register. > > > + > > static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > > { > > ARMCPU *cpu = arm_env_get_cpu(env); > > @@ -3112,6 +3127,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = { > > { .name = "TCR_EL2", .state = ARM_CP_STATE_BOTH, > > .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 2, > > .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > > + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, > > + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, > > + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, > > + .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore }, > > RAZ/WI register should use CP_CONST/resetvalue=0. (Access functions > apply even for const registers.) > > > { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, > > .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, > > .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > > @@ -3246,6 +3265,12 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { > > .access = PL2_RW, .writefn = vmsa_tcr_el1_write, > > .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, > > .fieldoffset = offsetof(CPUARMState, cp15.tcr_el[2]) }, > > + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, > > + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, > > + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, > > + .writefn = vmsa_tcr_el1_write, > > There's no AS bit in the VTCR_EL2, so you could avoid an unnecessary > TLB flush by not using the writefn we use for TCR_EL1. (I think > that if you don't provide a writefn or raw_writefn it should just > work, but check that...) I think you are right, nice catch. I'll fix all of these up. Cheers, Edgar > > > + .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, > > + .fieldoffset = offsetof(CPUARMState, cp15.vtcr_el2) }, > > { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, > > .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, > > .access = PL2_RW, .raw_writefn = raw_write, .writefn = sctlr_write, > > @@ -5735,8 +5760,7 @@ static inline bool regime_translation_disabled(CPUARMState *env, > > static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx) > > { > > if (mmu_idx == ARMMMUIdx_S2NS) { > > - /* TODO: return VTCR_EL2 */ > > - g_assert_not_reached(); > > + return &env->cp15.vtcr_el2; > > } > > return &env->cp15.tcr_el[regime_el(env, mmu_idx)]; > > } > > thanks > -- PMM
On Tue, Sep 08, 2015 at 03:19:37PM +0100, Peter Maydell wrote: > On 3 September 2015 at 21:14, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > target-arm/cpu.h | 1 + > > target-arm/helper.c | 28 ++++++++++++++++++++++++++-- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 31825d3..ba22e12 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -223,6 +223,7 @@ typedef struct CPUARMState { > > }; > > /* MMU translation table base control. */ > > TCR tcr_el[4]; > > + TCR vtcr_el2; /* Virtualization Translation Control. */ > > uint32_t c2_data; /* MPU data cachable bits. */ > > uint32_t c2_insn; /* MPU instruction cachable bits. */ > > union { /* MMU domain access control register > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index a057a70..c82aa1d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -325,6 +325,21 @@ void init_cpreg_list(ARMCPU *cpu) > > g_list_free(keys); > > } > > > > +/* > > + * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but > > + * they are accesible when EL3 is using AArch64 regardless of EL3.NS. > > + */ > > +static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, > > + const ARMCPRegInfo *ri) > > +{ > > + bool secure = arm_is_secure_below_el3(env); > > + > > + if (secure && !arm_el_is_aa64(env, 3)) { > > + return CP_ACCESS_TRAP_UNCATEGORIZED; > > + } > > + return CP_ACCESS_OK; > > +} > > This access function will always return OK for the AArch64 register, > so probably better to split the regdef rather than using STATE_BOTH, > and then avoid the accessfn on the 64-bit register. Hi Peter, In the interest avoiding duplication, do you think the following makes sense for regs with the el3_aa32ns_aa64any access checks? 1. Use STATE_BOTH for "low-activity" registers (e.g the EL3 view when EL2 does not exist). 2. Use STATE_BOTH for regs that anyway have a read/write function 3. Split AA64 and AA32 reg entries for regs without read/write helper call for spead (e.g VTCR_EL2). Cheers, Edgar
On 11 September 2015 at 15:40, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote: > In the interest avoiding duplication, do you think the following makes > sense for regs with the el3_aa32ns_aa64any access checks? > > 1. Use STATE_BOTH for "low-activity" registers (e.g the EL3 view when EL2 does not exist). > 2. Use STATE_BOTH for regs that anyway have a read/write function > 3. Split AA64 and AA32 reg entries for regs without read/write helper call for spead (e.g VTCR_EL2). Sounds plausible. -- PMM
On Fri, Sep 11, 2015 at 03:43:48PM +0100, Peter Maydell wrote: > On 11 September 2015 at 15:40, Edgar E. Iglesias > <edgar.iglesias@xilinx.com> wrote: > > In the interest avoiding duplication, do you think the following makes > > sense for regs with the el3_aa32ns_aa64any access checks? > > > > 1. Use STATE_BOTH for "low-activity" registers (e.g the EL3 view when EL2 does not exist). > > 2. Use STATE_BOTH for regs that anyway have a read/write function > > 3. Split AA64 and AA32 reg entries for regs without read/write helper call for spead (e.g VTCR_EL2). > > Sounds plausible. > OK, thanks!
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 31825d3..ba22e12 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -223,6 +223,7 @@ typedef struct CPUARMState { }; /* MMU translation table base control. */ TCR tcr_el[4]; + TCR vtcr_el2; /* Virtualization Translation Control. */ uint32_t c2_data; /* MPU data cachable bits. */ uint32_t c2_insn; /* MPU instruction cachable bits. */ union { /* MMU domain access control register diff --git a/target-arm/helper.c b/target-arm/helper.c index a057a70..c82aa1d 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -325,6 +325,21 @@ void init_cpreg_list(ARMCPU *cpu) g_list_free(keys); } +/* + * Some registers are not accessible if EL3.NS=0 and EL3 is using AArch32 but + * they are accesible when EL3 is using AArch64 regardless of EL3.NS. + */ +static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env, + const ARMCPRegInfo *ri) +{ + bool secure = arm_is_secure_below_el3(env); + + if (secure && !arm_el_is_aa64(env, 3)) { + return CP_ACCESS_TRAP_UNCATEGORIZED; + } + return CP_ACCESS_OK; +} + static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { ARMCPU *cpu = arm_env_get_cpu(env); @@ -3112,6 +3127,10 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = { { .name = "TCR_EL2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 0, .opc2 = 2, .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, + .readfn = arm_cp_read_zero, .writefn = arm_cp_write_ignore }, { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, @@ -3246,6 +3265,12 @@ static const ARMCPRegInfo el2_cp_reginfo[] = { .access = PL2_RW, .writefn = vmsa_tcr_el1_write, .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, .fieldoffset = offsetof(CPUARMState, cp15.tcr_el[2]) }, + { .name = "VTCR_EL2", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .opc1 = 4, .crn = 2, .crm = 1, .opc2 = 2, + .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any, + .writefn = vmsa_tcr_el1_write, + .resetfn = vmsa_ttbcr_reset, .raw_writefn = raw_write, + .fieldoffset = offsetof(CPUARMState, cp15.vtcr_el2) }, { .name = "SCTLR_EL2", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 0, .access = PL2_RW, .raw_writefn = raw_write, .writefn = sctlr_write, @@ -5735,8 +5760,7 @@ static inline bool regime_translation_disabled(CPUARMState *env, static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx) { if (mmu_idx == ARMMMUIdx_S2NS) { - /* TODO: return VTCR_EL2 */ - g_assert_not_reached(); + return &env->cp15.vtcr_el2; } return &env->cp15.tcr_el[regime_el(env, mmu_idx)]; }