On 3 November 2014 17:00, Greg Bellows <greg.bell...@linaro.org> wrote:
>
>
> On 31 October 2014 12:31, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
>> On 30 October 2014 21:28, Greg Bellows <greg.bell...@linaro.org> wrote:
>> > Added CP register info entries for the ARMv7 MAIR0/1 secure banks.
>> >
>> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
>> >
>> > ---
>> >
>> > v5 -> v6
>> > - Changed _el field variants to be array based
>> > ---
>> > target-arm/cpu.h | 12 +++++++++++-
>> > target-arm/helper.c | 8 +++++---
>> > 2 files changed, 16 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> > index 348ce73..1a76fc6 100644
>> > --- a/target-arm/cpu.h
>> > +++ b/target-arm/cpu.h
>> > @@ -305,7 +305,17 @@ typedef struct CPUARMState {
>> > uint32_t c9_pmxevtyper; /* perf monitor event type */
>> > uint32_t c9_pmuserenr; /* perf monitor user enable */
>> > uint32_t c9_pminten; /* perf monitor interrupt enables */
>> > - uint64_t mair_el1;
>> > + union { /* Memory attribute redirection */
>> > + struct {
>> > + uint64_t _unused_mair_0;
>> > + uint32_t mair0_ns;
>> > + uint32_t mair1_ns;
>> > + uint64_t _unused_mair_1;
>> > + uint32_t mair0_s;
>> > + uint32_t mair1_s;
>>
>> Surely these need the big-endian ifdefs too if we're unioning
>> uint32_ts with a uint64_t array?
>>
>
> Fixed in v9.
>
>
>>
>> (Can you check the other patches for this too, please? I might
>> have missed it when reviewing some of them, but a quick scan through
>> the cpu.h file in your tree ought to catch any other cases.)
>>
>
> Will do, but I think there were only two places where I had to mix uint32
> and uint64.
>
> Looking closer, I am thinking that this breaks with the ifdef in
> add_cpreg_to_hash as it seems it will reverse the structure logic. The
> function ifdef is really only designed to handle a single uint32 overlaying
> a uint64. I have to look into this more before we move forward with it.
>
>
Looked further into this and all is good. The special registers where two
32-bit registers map to separate halves simply need to be defined
separately along with the endian ifdef and all should work out fine.
>
>> > + };
>> > + uint64_t mair_el[4];
>> > + };
>> > union { /* vector base address register */
>> > struct {
>> > uint64_t _unused_vbar;
>> > diff --git a/target-arm/helper.c b/target-arm/helper.c
>> > index d782897..fd5f074 100644
>> > --- a/target-arm/helper.c
>> > +++ b/target-arm/helper.c
>> > @@ -939,7 +939,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>> > */
>> > { .name = "MAIR_EL1", .state = ARM_CP_STATE_AA64,
>> > .opc0 = 3, .opc1 = 0, .crn = 10, .crm = 2, .opc2 = 0,
>> > - .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
>> cp15.mair_el1),
>> > + .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
>> cp15.mair_el[1]),
>> > .resetvalue = 0 },
>> > /* For non-long-descriptor page tables these are PRRR and NMRR;
>> > * regardless they still act as reads-as-written for QEMU.
>> > @@ -948,11 +948,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>> > */
>> > { .name = "MAIR0", .state = ARM_CP_STATE_AA32, .type =
>> ARM_CP_OVERRIDE,
>> > .cp = 15, .opc1 = 0, .crn = 10, .crm = 2, .opc2 = 0, .access =
>> PL1_RW,
>> > - .fieldoffset = offsetoflow32(CPUARMState, cp15.mair_el1),
>> > + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.mair0_s),
>> > + offsetof(CPUARMState, cp15.mair0_ns) },
>> > .resetfn = arm_cp_reset_ignore },
>> > { .name = "MAIR1", .state = ARM_CP_STATE_AA32, .type =
>> ARM_CP_OVERRIDE,
>> > .cp = 15, .opc1 = 0, .crn = 10, .crm = 2, .opc2 = 1, .access =
>> PL1_RW,
>> > - .fieldoffset = offsetofhigh32(CPUARMState, cp15.mair_el1),
>> > + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.mair1_s),
>> > + offsetof(CPUARMState, cp15.mair1_ns) },
>> > .resetfn = arm_cp_reset_ignore },
>> > { .name = "ISR_EL1", .state = ARM_CP_STATE_BOTH,
>> > .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 1, .opc2 = 0,
>> > --
>> > 1.8.3.2
>> >
>>
>> Otherwise you can add my reviewed-by tag.
>>
>> -- PMM
>>
>
>
