Patchwork [RFC,1/2] ARM: cpu: add "hivecs" property (high vectors on reset)

login
register
mail settings
Submitter Antony Pavlov
Date Dec. 7, 2013, 12:55 a.m.
Message ID <1386377704-1350-2-git-send-email-antonynpavlov@gmail.com>
Download mbox | patch
Permalink /patch/298593/
State New
Headers show

Comments

Antony Pavlov - Dec. 7, 2013, 12:55 a.m.
Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 target-arm/cpu-qom.h |  1 +
 target-arm/cpu.c     | 13 +++++++++++++
 2 files changed, 14 insertions(+)
Peter Crosthwaite - Dec. 7, 2013, 1 a.m.
On Sat, Dec 7, 2013 at 10:55 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  target-arm/cpu-qom.h |  1 +
>  target-arm/cpu.c     | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index b55306a..116320c 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -128,6 +128,7 @@ typedef struct ARMCPU {
>      uint32_t ccsidr[16];
>      uint32_t reset_cbar;
>      uint32_t reset_auxcr;
> +    bool reset_hivecs;
>  } ARMCPU;
>
>  #define TYPE_AARCH64_CPU "aarch64-cpu"
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index d40f2a7..f838499 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -20,6 +20,7 @@
>
>  #include "cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev-properties.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
>  #endif
> @@ -119,6 +120,12 @@ static void arm_cpu_reset(CPUState *s)
>              env->regs[15] = pc & ~1;
>          }
>      }
> +
> +    if (cpu->reset_hivecs) {
> +            uint32_t c1_sys = env->cp15.c1_sys;
> +            env->cp15.c1_sys = c1_sys | (1 << 13);

Why the read modify write? Cant you do it as a one-liner |= ?

env->cp15.c1_sys |= (1 << 13);

> +    }
> +
>      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
>  #endif
>      set_flush_to_zero(1, &env->vfp.standard_fp_status);
> @@ -854,6 +861,11 @@ typedef struct ARMCPUInfo {
>      void (*class_init)(ObjectClass *oc, void *data);
>  } ARMCPUInfo;
>
> +static Property arm_cpu_properties[] = {
> +    DEFINE_PROP_BOOL("hivecs", ARMCPU, reset_hivecs, false),

As this is indicating a reset value, "reset" should feature in the
property name.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static const ARMCPUInfo arm_cpus[] = {
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>      { .name = "arm926",      .initfn = arm926_initfn },
> @@ -902,6 +914,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>
>      acc->parent_realize = dc->realize;
>      dc->realize = arm_cpu_realizefn;
> +    dc->props = arm_cpu_properties;

This unconditionally adds the property to all ARM CPUs, wheras its
only valid for a subset of ARM CPUs (!M). Check the earlier patches in
the CBAR/highbank/zynq series on list for the change pattern for
adding it as a conditional dynamic property instead. I'll take it into
that series if you want to rebase onto my patch set (will save you
having to conflict by adding post-init fn which is still absent from
mainline).

Regards,
Peter

>
>      acc->parent_reset = cc->reset;
>      cc->reset = arm_cpu_reset;
> --
> 1.8.5
>
>
Peter Maydell - Dec. 7, 2013, 1:14 a.m.
On 7 December 2013 00:55, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  target-arm/cpu-qom.h |  1 +
>  target-arm/cpu.c     | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index b55306a..116320c 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -128,6 +128,7 @@ typedef struct ARMCPU {
>      uint32_t ccsidr[16];
>      uint32_t reset_cbar;
>      uint32_t reset_auxcr;
> +    bool reset_hivecs;
>  } ARMCPU;
>
>  #define TYPE_AARCH64_CPU "aarch64-cpu"
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index d40f2a7..f838499 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -20,6 +20,7 @@
>
>  #include "cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev-properties.h"
>  #if !defined(CONFIG_USER_ONLY)
>  #include "hw/loader.h"
>  #endif
> @@ -119,6 +120,12 @@ static void arm_cpu_reset(CPUState *s)
>              env->regs[15] = pc & ~1;
>          }
>      }
> +
> +    if (cpu->reset_hivecs) {
> +            uint32_t c1_sys = env->cp15.c1_sys;
> +            env->cp15.c1_sys = c1_sys | (1 << 13);
> +    }

You don't need to do this on every reset. You can just
set the bit in cpu->reset_sctlr at realize if reset_hivecs
is true, and then reset will just reset to the correct
value.

thanks
-- PMM
Antony Pavlov - Dec. 7, 2013, 8:44 p.m.
On Sat, 7 Dec 2013 11:00:05 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Sat, Dec 7, 2013 at 10:55 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > ---
> >  target-arm/cpu-qom.h |  1 +
> >  target-arm/cpu.c     | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> > index b55306a..116320c 100644
> > --- a/target-arm/cpu-qom.h
> > +++ b/target-arm/cpu-qom.h
> > @@ -128,6 +128,7 @@ typedef struct ARMCPU {
> >      uint32_t ccsidr[16];
> >      uint32_t reset_cbar;
> >      uint32_t reset_auxcr;
> > +    bool reset_hivecs;
> >  } ARMCPU;
> >
> >  #define TYPE_AARCH64_CPU "aarch64-cpu"
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index d40f2a7..f838499 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -20,6 +20,7 @@
> >
> >  #include "cpu.h"
> >  #include "qemu-common.h"
> > +#include "hw/qdev-properties.h"
> >  #if !defined(CONFIG_USER_ONLY)
> >  #include "hw/loader.h"
> >  #endif
> > @@ -119,6 +120,12 @@ static void arm_cpu_reset(CPUState *s)
> >              env->regs[15] = pc & ~1;
> >          }
> >      }
> > +
> > +    if (cpu->reset_hivecs) {
> > +            uint32_t c1_sys = env->cp15.c1_sys;
> > +            env->cp15.c1_sys = c1_sys | (1 << 13);
> 
> Why the read modify write? Cant you do it as a one-liner |= ?
> 
> env->cp15.c1_sys |= (1 << 13);
> 
> > +    }
> > +
> >      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> >  #endif
> >      set_flush_to_zero(1, &env->vfp.standard_fp_status);
> > @@ -854,6 +861,11 @@ typedef struct ARMCPUInfo {
> >      void (*class_init)(ObjectClass *oc, void *data);
> >  } ARMCPUInfo;
> >
> > +static Property arm_cpu_properties[] = {
> > +    DEFINE_PROP_BOOL("hivecs", ARMCPU, reset_hivecs, false),
> 
> As this is indicating a reset value, "reset" should feature in the
> property name.
> 
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >  static const ARMCPUInfo arm_cpus[] = {
> >  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> >      { .name = "arm926",      .initfn = arm926_initfn },
> > @@ -902,6 +914,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
> >
> >      acc->parent_realize = dc->realize;
> >      dc->realize = arm_cpu_realizefn;
> > +    dc->props = arm_cpu_properties;
> 
> This unconditionally adds the property to all ARM CPUs, wheras its
> only valid for a subset of ARM CPUs (!M). Check the earlier patches in
> the CBAR/highbank/zynq series on list for the change pattern for
> adding it as a conditional dynamic property instead. I'll take it into

Is there any public repo with this changes?

> that series if you want to rebase onto my patch set (will save you
> having to conflict by adding post-init fn which is still absent from
> mainline).
> 
> Regards,
> Peter
> 
> >
> >      acc->parent_reset = cc->reset;
> >      cc->reset = arm_cpu_reset;
> > --
> > 1.8.5
> >
> >
Peter Crosthwaite - Dec. 7, 2013, 10:14 p.m.
On Sun, Dec 8, 2013 at 6:44 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
> On Sat, 7 Dec 2013 11:00:05 +1000
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>
>> On Sat, Dec 7, 2013 at 10:55 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote:
>> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
>> > ---
>> >  target-arm/cpu-qom.h |  1 +
>> >  target-arm/cpu.c     | 13 +++++++++++++
>> >  2 files changed, 14 insertions(+)
>> >
>> > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> > index b55306a..116320c 100644
>> > --- a/target-arm/cpu-qom.h
>> > +++ b/target-arm/cpu-qom.h
>> > @@ -128,6 +128,7 @@ typedef struct ARMCPU {
>> >      uint32_t ccsidr[16];
>> >      uint32_t reset_cbar;
>> >      uint32_t reset_auxcr;
>> > +    bool reset_hivecs;
>> >  } ARMCPU;
>> >
>> >  #define TYPE_AARCH64_CPU "aarch64-cpu"
>> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> > index d40f2a7..f838499 100644
>> > --- a/target-arm/cpu.c
>> > +++ b/target-arm/cpu.c
>> > @@ -20,6 +20,7 @@
>> >
>> >  #include "cpu.h"
>> >  #include "qemu-common.h"
>> > +#include "hw/qdev-properties.h"
>> >  #if !defined(CONFIG_USER_ONLY)
>> >  #include "hw/loader.h"
>> >  #endif
>> > @@ -119,6 +120,12 @@ static void arm_cpu_reset(CPUState *s)
>> >              env->regs[15] = pc & ~1;
>> >          }
>> >      }
>> > +
>> > +    if (cpu->reset_hivecs) {
>> > +            uint32_t c1_sys = env->cp15.c1_sys;
>> > +            env->cp15.c1_sys = c1_sys | (1 << 13);
>>
>> Why the read modify write? Cant you do it as a one-liner |= ?
>>
>> env->cp15.c1_sys |= (1 << 13);
>>
>> > +    }
>> > +
>> >      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
>> >  #endif
>> >      set_flush_to_zero(1, &env->vfp.standard_fp_status);
>> > @@ -854,6 +861,11 @@ typedef struct ARMCPUInfo {
>> >      void (*class_init)(ObjectClass *oc, void *data);
>> >  } ARMCPUInfo;
>> >
>> > +static Property arm_cpu_properties[] = {
>> > +    DEFINE_PROP_BOOL("hivecs", ARMCPU, reset_hivecs, false),
>>
>> As this is indicating a reset value, "reset" should feature in the
>> property name.
>>
>> > +    DEFINE_PROP_END_OF_LIST(),
>> > +};
>> > +
>> >  static const ARMCPUInfo arm_cpus[] = {
>> >  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
>> >      { .name = "arm926",      .initfn = arm926_initfn },
>> > @@ -902,6 +914,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>> >
>> >      acc->parent_realize = dc->realize;
>> >      dc->realize = arm_cpu_realizefn;
>> > +    dc->props = arm_cpu_properties;
>>
>> This unconditionally adds the property to all ARM CPUs, wheras its
>> only valid for a subset of ARM CPUs (!M). Check the earlier patches in
>> the CBAR/highbank/zynq series on list for the change pattern for
>> adding it as a conditional dynamic property instead. I'll take it into
>
> Is there any public repo with this changes?
>

Not as of this moment (and I will do something about that soon), but
generally speaking, this tool makes very light work of getting
someones series as a branch just using the list:

https://github.com/aliguori/patches

I highly recommend it over using personal trees and branches.

Regards,
Peter

>> that series if you want to rebase onto my patch set (will save you
>> having to conflict by adding post-init fn which is still absent from
>> mainline).
>>
>> Regards,
>> Peter
>>
>> >
>> >      acc->parent_reset = cc->reset;
>> >      cc->reset = arm_cpu_reset;
>> > --
>> > 1.8.5
>> >
>> >
>
>
> --
> --
> Best regards,
>   Antony Pavlov
>

Patch

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index b55306a..116320c 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -128,6 +128,7 @@  typedef struct ARMCPU {
     uint32_t ccsidr[16];
     uint32_t reset_cbar;
     uint32_t reset_auxcr;
+    bool reset_hivecs;
 } ARMCPU;
 
 #define TYPE_AARCH64_CPU "aarch64-cpu"
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d40f2a7..f838499 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -20,6 +20,7 @@ 
 
 #include "cpu.h"
 #include "qemu-common.h"
+#include "hw/qdev-properties.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
 #endif
@@ -119,6 +120,12 @@  static void arm_cpu_reset(CPUState *s)
             env->regs[15] = pc & ~1;
         }
     }
+
+    if (cpu->reset_hivecs) {
+            uint32_t c1_sys = env->cp15.c1_sys;
+            env->cp15.c1_sys = c1_sys | (1 << 13);
+    }
+
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
 #endif
     set_flush_to_zero(1, &env->vfp.standard_fp_status);
@@ -854,6 +861,11 @@  typedef struct ARMCPUInfo {
     void (*class_init)(ObjectClass *oc, void *data);
 } ARMCPUInfo;
 
+static Property arm_cpu_properties[] = {
+    DEFINE_PROP_BOOL("hivecs", ARMCPU, reset_hivecs, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static const ARMCPUInfo arm_cpus[] = {
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
     { .name = "arm926",      .initfn = arm926_initfn },
@@ -902,6 +914,7 @@  static void arm_cpu_class_init(ObjectClass *oc, void *data)
 
     acc->parent_realize = dc->realize;
     dc->realize = arm_cpu_realizefn;
+    dc->props = arm_cpu_properties;
 
     acc->parent_reset = cc->reset;
     cc->reset = arm_cpu_reset;