diff mbox

[v8,26/27] target-arm: make MAIR0/1 banked

Message ID 1414704538-17103-27-git-send-email-greg.bellows@linaro.org
State New
Headers show

Commit Message

Greg Bellows Oct. 30, 2014, 9:28 p.m. UTC 
Added CP register info entries for the ARMv7 MAIR0/1 secure banks.

Signed-off-by: Greg Bellows <greg.bellows@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(-)

Comments

Peter Maydell Oct. 31, 2014, 5:31 p.m. UTC | #1 
On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote:
> Added CP register info entries for the ARMv7 MAIR0/1 secure banks.
>
> Signed-off-by: Greg Bellows <greg.bellows@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?

(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.)

> +            };
> +            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
Greg Bellows Nov. 3, 2014, 11 p.m. UTC | #2 
On 31 October 2014 12:31, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Added CP register info entries for the ARMv7 MAIR0/1 secure banks.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@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.


>
> > +            };
> > +            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
>
Greg Bellows Nov. 4, 2014, 2:13 p.m. UTC | #3 
On 3 November 2014 17:00, Greg Bellows <greg.bellows@linaro.org> wrote:

>
>
> On 31 October 2014 12:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote:
>> > Added CP register info entries for the ARMv7 MAIR0/1 secure banks.
>> >
>> > Signed-off-by: Greg Bellows <greg.bellows@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
>>
>
>
diff mbox

Patch

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;
+            };
+            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,