| Message ID | 1414704538-17103-25-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote: > When EL3 is running in Aarch32 (or ARMv7 with Security Extensions) > VBAR has a secure and a non-secure instance, which are mapped to > VBAR_EL1 and VBAR_EL3. > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > v5 -> v6 > - Changed _el field variants to be array based > - Merged VBAR and VBAR_EL1 reginfo entries > > v3 -> v4 > - Fix vbar union/structure definition > - Revert back to array-based vbar definition combined with v7 naming > --- > target-arm/cpu.h | 10 +++++++++- > target-arm/helper.c | 8 ++++---- > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 3c6ff4a..e0954c7 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -306,7 +306,15 @@ typedef struct CPUARMState { > uint32_t c9_pmuserenr; /* perf monitor user enable */ > uint32_t c9_pminten; /* perf monitor interrupt enables */ > uint64_t mair_el1; > - uint64_t vbar_el[4]; /* vector base address register */ > + union { /* vector base address register */ > + struct { > + uint64_t _unused_vbar; > + uint64_t vbar_ns; > + uint64_t hvbar; > + uint64_t vbar_s; > + }; > + uint64_t vbar_el[4]; > + }; > uint32_t mvbar; /* (monitor) vector base address register */ > uint32_t c13_fcse; /* FCSE PID. */ > uint64_t contextidr_el1; /* Context ID. */ > diff --git a/target-arm/helper.c b/target-arm/helper.c > index ec957fb..fb040d4 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -905,9 +905,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > .resetvalue = 0, .writefn = pmintenclr_write, }, > { .name = "VBAR", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, > - .access = PL1_RW, .writefn = vbar_write, > - .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[1]), > - .resetvalue = 0 }, > + .access = PL1_RW, .writefn = vbar_write, .resetvalue = 0, > + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s), > + offsetof(CPUARMState, cp15.vbar_ns) } }, This is unnecessarily moving the .resetvalue setting around again. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
You seem to not be a fan of the fields setting moved around and Fabian appears to have done this in a number of places. I left them as is because I found the changes to add consistency and compactness. Would you like me to undo any such changes? Greg On 31 October 2014 12:22, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote: > > When EL3 is running in Aarch32 (or ARMv7 with Security Extensions) > > VBAR has a secure and a non-secure instance, which are mapped to > > VBAR_EL1 and VBAR_EL3. > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > --- > > > > v5 -> v6 > > - Changed _el field variants to be array based > > - Merged VBAR and VBAR_EL1 reginfo entries > > > > v3 -> v4 > > - Fix vbar union/structure definition > > - Revert back to array-based vbar definition combined with v7 naming > > --- > > target-arm/cpu.h | 10 +++++++++- > > target-arm/helper.c | 8 ++++---- > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 3c6ff4a..e0954c7 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -306,7 +306,15 @@ typedef struct CPUARMState { > > uint32_t c9_pmuserenr; /* perf monitor user enable */ > > uint32_t c9_pminten; /* perf monitor interrupt enables */ > > uint64_t mair_el1; > > - uint64_t vbar_el[4]; /* vector base address register */ > > + union { /* vector base address register */ > > + struct { > > + uint64_t _unused_vbar; > > + uint64_t vbar_ns; > > + uint64_t hvbar; > > + uint64_t vbar_s; > > + }; > > + uint64_t vbar_el[4]; > > + }; > > uint32_t mvbar; /* (monitor) vector base address register */ > > uint32_t c13_fcse; /* FCSE PID. */ > > uint64_t contextidr_el1; /* Context ID. */ > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index ec957fb..fb040d4 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -905,9 +905,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > > .resetvalue = 0, .writefn = pmintenclr_write, }, > > { .name = "VBAR", .state = ARM_CP_STATE_BOTH, > > .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, > > - .access = PL1_RW, .writefn = vbar_write, > > - .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[1]), > > - .resetvalue = 0 }, > > + .access = PL1_RW, .writefn = vbar_write, .resetvalue = 0, > > + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s), > > + offsetof(CPUARMState, cp15.vbar_ns) } }, > > This is unnecessarily moving the .resetvalue setting around again. > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM >
On 3 November 2014 22:06, Greg Bellows <greg.bellows@linaro.org> wrote: > You seem to not be a fan of the fields setting moved around and Fabian > appears to have done this in a number of places. I left them as is because > I found the changes to add consistency and compactness. I particularly dislike it where it makes the diffs harder to read (as in this instance where it disguises the fact that there's only actually been one field that's changed). > Would you like me to undo any such changes? I wouldn't bother except for the patches where I specifically mentioned it. -- PMM
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 3c6ff4a..e0954c7 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -306,7 +306,15 @@ typedef struct CPUARMState { uint32_t c9_pmuserenr; /* perf monitor user enable */ uint32_t c9_pminten; /* perf monitor interrupt enables */ uint64_t mair_el1; - uint64_t vbar_el[4]; /* vector base address register */ + union { /* vector base address register */ + struct { + uint64_t _unused_vbar; + uint64_t vbar_ns; + uint64_t hvbar; + uint64_t vbar_s; + }; + uint64_t vbar_el[4]; + }; uint32_t mvbar; /* (monitor) vector base address register */ uint32_t c13_fcse; /* FCSE PID. */ uint64_t contextidr_el1; /* Context ID. */ diff --git a/target-arm/helper.c b/target-arm/helper.c index ec957fb..fb040d4 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -905,9 +905,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .resetvalue = 0, .writefn = pmintenclr_write, }, { .name = "VBAR", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0, - .access = PL1_RW, .writefn = vbar_write, - .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[1]), - .resetvalue = 0 }, + .access = PL1_RW, .writefn = vbar_write, .resetvalue = 0, + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s), + offsetof(CPUARMState, cp15.vbar_ns) } }, { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE }, @@ -4371,7 +4371,7 @@ void arm_cpu_do_interrupt(CPUState *cs) * This register is only followed in non-monitor mode, and is banked. * Note: only bits 31:5 are valid. */ - addr += env->cp15.vbar_el[1]; + addr += A32_BANKED_CURRENT_REG_GET(env, vbar); } if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) {
When EL3 is running in Aarch32 (or ARMv7 with Security Extensions) VBAR has a secure and a non-secure instance, which are mapped to VBAR_EL1 and VBAR_EL3. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- v5 -> v6 - Changed _el field variants to be array based - Merged VBAR and VBAR_EL1 reginfo entries v3 -> v4 - Fix vbar union/structure definition - Revert back to array-based vbar definition combined with v7 naming --- target-arm/cpu.h | 10 +++++++++- target-arm/helper.c | 8 ++++---- 2 files changed, 13 insertions(+), 5 deletions(-)