[Qemu-devel] [RFC v1 0/3] Add Microblaze configuration options

[Qemu-devel] [RFC v1 0/3] Add Microblaze configuration options

[Alistair Francis]

This patch series adds the "xlnx.use-stack-protection" property
to the Microblaze CPU, which allows stack protection to be
disabled.

It also converts the previously hard coded method of enabling
the FPU to use standard QEMU properties. This simplifies the
logic in the target-Microblaze translate.c. It also simplifies
the Microblaze reset functions.

Alistair Francis (3):
  target-microblaze: Fix up indentation
  target-microblaze: Allow the stack protection to be disabled
  target-microblaze: Convert use-fpu to a CPU property

 hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
 target-microblaze/cpu-qom.h         |    6 ++++++
 target-microblaze/cpu.c             |   15 +++++++++++++--
 target-microblaze/op_helper.c       |   14 ++++++++------
 target-microblaze/translate.c       |    6 +++---
 5 files changed, 32 insertions(+), 13 deletions(-)

[Qemu-devel] [RFC v1 1/3] target-microblaze: Fix up indentation

[Alistair Francis]

Fix up the incorrect indentation level in the helper_stackprot() function.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-microblaze/op_helper.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
index a4c8f04..d2b3624 100644
--- a/target-microblaze/op_helper.c
+++ b/target-microblaze/op_helper.c
@@ -468,11 +468,11 @@ void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr,
 void helper_stackprot(CPUMBState *env, uint32_t addr)
 {
     if (addr < env->slr || addr > env->shr) {
-            qemu_log("Stack protector violation at %x %x %x\n",
-                     addr, env->slr, env->shr);
-            env->sregs[SR_EAR] = addr;
-            env->sregs[SR_ESR] = ESR_EC_STACKPROT;
-            helper_raise_exception(env, EXCP_HW_EXCP);
+        qemu_log("Stack protector violation at %x %x %x\n",
+                 addr, env->slr, env->shr);
+        env->sregs[SR_EAR] = addr;
+        env->sregs[SR_ESR] = ESR_EC_STACKPROT;
+        helper_raise_exception(env, EXCP_HW_EXCP);
     }
 }
 
-- 
1.7.1

[Qemu-devel] [RFC v1 2/3] target-microblaze: Allow the stack protection to be disabled

[Alistair Francis]

Microblaze stack protection is configurable and isn't always enabled.
This patch allows the stack protection to be disabled from the CPU
properties.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-microblaze/cpu-qom.h   |    5 +++++
 target-microblaze/cpu.c       |    2 ++
 target-microblaze/op_helper.c |    4 +++-
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
index e3e0701..7bc5b81 100644
--- a/target-microblaze/cpu-qom.h
+++ b/target-microblaze/cpu-qom.h
@@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
     uint32_t base_vectors;
     /*< public >*/
 
+    /* Microblaze Configuration Settings */
+    struct {
+        bool stackproc;
+    } cfg;
+
     CPUMBState env;
 } MicroBlazeCPU;
 
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index 67e3182..c08da19 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -152,6 +152,8 @@ static const VMStateDescription vmstate_mb_cpu = {
 
 static Property mb_properties[] = {
     DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
+    DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
+                     true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
index d2b3624..24df538 100644
--- a/target-microblaze/op_helper.c
+++ b/target-microblaze/op_helper.c
@@ -467,7 +467,9 @@ void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr,
 
 void helper_stackprot(CPUMBState *env, uint32_t addr)
 {
-    if (addr < env->slr || addr > env->shr) {
+    MicroBlazeCPU *cpu = mb_env_get_cpu(env);
+
+    if (cpu->cfg.stackproc && (addr < env->slr || addr > env->shr)) {
         qemu_log("Stack protector violation at %x %x %x\n",
                  addr, env->slr, env->shr);
         env->sregs[SR_EAR] = addr;
-- 
1.7.1

[Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

Originally the use-fpu PVR bits were manually set for each machine. This
is a hassle and difficult to read, instead set them based on the CPU
properties.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
 target-microblaze/cpu-qom.h         |    1 +
 target-microblaze/cpu.c             |   13 +++++++++++--
 target-microblaze/translate.c       |    6 +++---
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 48c264b..f4e1cc5 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
     env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
     /* setup pvr to match kernel setting */
     env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
-    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
+    env->pvr.regs[0] |= PVR0_ENDI;
     env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
-    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
     env->pvr.regs[4] = 0xc56b8000;
     env->pvr.regs[5] = 0xc56be000;
 }
@@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
 
     /* init CPUs */
     cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
+    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
index 7bc5b81..ce3a574 100644
--- a/target-microblaze/cpu-qom.h
+++ b/target-microblaze/cpu-qom.h
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
 
     CPUMBState env;
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index c08da19..c3a9a5d 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
                         | PVR2_USE_DIV_MASK \
                         | PVR2_USE_HW_MUL_MASK \
                         | PVR2_USE_MUL64_MASK \
-                        | PVR2_USE_FPU_MASK \
-                        | PVR2_USE_FPU2_MASK \
                         | PVR2_FPU_EXC_MASK \
                         | 0;
+
+    if (cpu->cfg.usefpu) {
+        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
+        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
+
+        if (cpu->cfg.usefpu > 1) {
+            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
+        }
+    }
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
 
@@ -154,6 +162,7 @@ static Property mb_properties[] = {
     DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
     DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
                      true),
+    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index 4068946..36cfc5c 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
 
     r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
 
-    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
+    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
         tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
         t_gen_raise_exception(dc, EXCP_HW_EXCP);
     }
-    return r;
+    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
 }
 
 static void dec_fpu(DisasContext *dc)
@@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
 
     if ((dc->tb_flags & MSR_EE_FLAG)
           && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
-          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
+          && (dc->cpu->cfg.usefpu != 1)) {
         tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
         t_gen_raise_exception(dc, EXCP_HW_EXCP);
         return;
-- 
1.7.1

Re: [Qemu-devel] [RFC v1 2/3] target-microblaze: Allow the stack protection to be disabled

[Richard Henderson]

On 05/13/2015 11:08 PM, Alistair Francis wrote:
>  void helper_stackprot(CPUMBState *env, uint32_t addr)
>  {
> -    if (addr < env->slr || addr > env->shr) {
> +    MicroBlazeCPU *cpu = mb_env_get_cpu(env);
> +
> +    if (cpu->cfg.stackproc && (addr < env->slr || addr > env->shr)) {

This check should be done in translate.c to avoid calling this function in the
first place.


r~

Re: [Qemu-devel] [RFC v1 2/3] target-microblaze: Allow the stack protection to be disabled

[Alistair Francis]

On Fri, May 15, 2015 at 4:36 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 05/13/2015 11:08 PM, Alistair Francis wrote:
>>  void helper_stackprot(CPUMBState *env, uint32_t addr)
>>  {
>> -    if (addr < env->slr || addr > env->shr) {
>> +    MicroBlazeCPU *cpu = mb_env_get_cpu(env);
>> +
>> +    if (cpu->cfg.stackproc && (addr < env->slr || addr > env->shr)) {
>
> This check should be done in translate.c to avoid calling this function in the
> first place.

Looking at it again I agree, I will move it.

Thanks,

Alistair

>
>
> r~
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Peter Crosthwaite]

On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Originally the use-fpu PVR bits were manually set for each machine. This
> is a hassle and difficult to read, instead set them based on the CPU
> properties.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>  target-microblaze/cpu-qom.h         |    1 +
>  target-microblaze/cpu.c             |   13 +++++++++++--
>  target-microblaze/translate.c       |    6 +++---
>  4 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
> index 48c264b..f4e1cc5 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>      /* setup pvr to match kernel setting */
>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
> +    env->pvr.regs[0] |= PVR0_ENDI;
>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;

So I think this here will clear PVR2_FPU2 ...

>      env->pvr.regs[4] = 0xc56b8000;
>      env->pvr.regs[5] = 0xc56be000;
>  }
> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>
>      /* init CPUs */
>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);

But here you are setting use-fpu to 2. Is this a functional change for
ML605 board?

>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>
>      /* Attach emulated BRAM through the LMB.  */
> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
> index 7bc5b81..ce3a574 100644
> --- a/target-microblaze/cpu-qom.h
> +++ b/target-microblaze/cpu-qom.h
> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>      /* Microblaze Configuration Settings */
>      struct {
>          bool stackproc;
> +        uint8_t usefpu;
>      } cfg;
>
>      CPUMBState env;
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index c08da19..c3a9a5d 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>                          | PVR2_USE_DIV_MASK \
>                          | PVR2_USE_HW_MUL_MASK \
>                          | PVR2_USE_MUL64_MASK \
> -                        | PVR2_USE_FPU_MASK \
> -                        | PVR2_USE_FPU2_MASK \

If I have this straight, this suggests the property default value
should be 2, and the ml605 board setting should be 1.

>                          | PVR2_FPU_EXC_MASK \
>                          | 0;
> +
> +    if (cpu->cfg.usefpu) {
> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
> +
> +        if (cpu->cfg.usefpu > 1) {
> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
> +        }
> +    }

This should be handled at realize time. See pl330_realize for example
of realize-time PVR ("cfg" in that case) population.

To avoid churn, it might be easiest to convert the PVRs in chunks one
by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
can be moved into realize for settings not worth propertyifying.

> +
>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>
> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>                       true),
> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),

No need for xlnx prefix (FDT generic should trim the prefix).

Regards,
Peter

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
> index 4068946..36cfc5c 100644
> --- a/target-microblaze/translate.c
> +++ b/target-microblaze/translate.c
> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>
>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>
> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>      }
> -    return r;
> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>  }
>
>  static void dec_fpu(DisasContext *dc)
> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>
>      if ((dc->tb_flags & MSR_EE_FLAG)
>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
> +          && (dc->cpu->cfg.usefpu != 1)) {
>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>          return;
> --
> 1.7.1
>
>

Re: [Qemu-devel] [RFC v1 2/3] target-microblaze: Allow the stack protection to be disabled

[Peter Crosthwaite]

On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Microblaze stack protection is configurable and isn't always enabled.
> This patch allows the stack protection to be disabled from the CPU
> properties.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-microblaze/cpu-qom.h   |    5 +++++
>  target-microblaze/cpu.c       |    2 ++
>  target-microblaze/op_helper.c |    4 +++-
>  3 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
> index e3e0701..7bc5b81 100644
> --- a/target-microblaze/cpu-qom.h
> +++ b/target-microblaze/cpu-qom.h
> @@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
>      uint32_t base_vectors;
>      /*< public >*/
>
> +    /* Microblaze Configuration Settings */
> +    struct {
> +        bool stackproc;
> +    } cfg;
> +
>      CPUMBState env;
>  } MicroBlazeCPU;
>
> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
> index 67e3182..c08da19 100644
> --- a/target-microblaze/cpu.c
> +++ b/target-microblaze/cpu.c
> @@ -152,6 +152,8 @@ static const VMStateDescription vmstate_mb_cpu = {
>
>  static Property mb_properties[] = {
>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
> +    DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
> +                     true),

This should deposit (at realize time) into pvr[0].SPROT bit.

Regards,
Peter

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
> index d2b3624..24df538 100644
> --- a/target-microblaze/op_helper.c
> +++ b/target-microblaze/op_helper.c
> @@ -467,7 +467,9 @@ void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr,
>
>  void helper_stackprot(CPUMBState *env, uint32_t addr)
>  {
> -    if (addr < env->slr || addr > env->shr) {
> +    MicroBlazeCPU *cpu = mb_env_get_cpu(env);
> +
> +    if (cpu->cfg.stackproc && (addr < env->slr || addr > env->shr)) {
>          qemu_log("Stack protector violation at %x %x %x\n",
>                   addr, env->slr, env->shr);
>          env->sregs[SR_EAR] = addr;
> --
> 1.7.1
>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Originally the use-fpu PVR bits were manually set for each machine. This
>> is a hassle and difficult to read, instead set them based on the CPU
>> properties.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>  target-microblaze/cpu-qom.h         |    1 +
>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>  target-microblaze/translate.c       |    6 +++---
>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>> index 48c264b..f4e1cc5 100644
>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>      /* setup pvr to match kernel setting */
>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>
> So I think this here will clear PVR2_FPU2 ...
>
>>      env->pvr.regs[4] = 0xc56b8000;
>>      env->pvr.regs[5] = 0xc56be000;
>>  }
>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>
>>      /* init CPUs */
>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>
> But here you are setting use-fpu to 2. Is this a functional change for
> ML605 board?
>
>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>
>>      /* Attach emulated BRAM through the LMB.  */
>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>> index 7bc5b81..ce3a574 100644
>> --- a/target-microblaze/cpu-qom.h
>> +++ b/target-microblaze/cpu-qom.h
>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>      /* Microblaze Configuration Settings */
>>      struct {
>>          bool stackproc;
>> +        uint8_t usefpu;
>>      } cfg;
>>
>>      CPUMBState env;
>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>> index c08da19..c3a9a5d 100644
>> --- a/target-microblaze/cpu.c
>> +++ b/target-microblaze/cpu.c
>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>                          | PVR2_USE_DIV_MASK \
>>                          | PVR2_USE_HW_MUL_MASK \
>>                          | PVR2_USE_MUL64_MASK \
>> -                        | PVR2_USE_FPU_MASK \
>> -                        | PVR2_USE_FPU2_MASK \
>
> If I have this straight, this suggests the property default value
> should be 2, and the ml605 board setting should be 1.

You are correct, I will fix this.

>
>>                          | PVR2_FPU_EXC_MASK \
>>                          | 0;
>> +
>> +    if (cpu->cfg.usefpu) {
>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>> +
>> +        if (cpu->cfg.usefpu > 1) {
>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>> +        }
>> +    }
>
> This should be handled at realize time. See pl330_realize for example
> of realize-time PVR ("cfg" in that case) population.

But the env state gets wiped out at reset, so the information will be lost.

I can tidy up the if logic though.

>
> To avoid churn, it might be easiest to convert the PVRs in chunks one
> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
> can be moved into realize for settings not worth propertyifying.
>
>> +
>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>
>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>                       true),
>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>
> No need for xlnx prefix (FDT generic should trim the prefix).

I agree, but the base-vectors already had it. If the xlnx is unneeded
I'll remove
it from the base-vector as well.

Thanks,

Alistair

>
> Regards,
> Peter
>
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>> index 4068946..36cfc5c 100644
>> --- a/target-microblaze/translate.c
>> +++ b/target-microblaze/translate.c
>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>
>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>
>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>      }
>> -    return r;
>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>  }
>>
>>  static void dec_fpu(DisasContext *dc)
>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>
>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>          return;
>> --
>> 1.7.1
>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Peter Crosthwaite]

On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>> is a hassle and difficult to read, instead set them based on the CPU
>>> properties.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>  target-microblaze/cpu-qom.h         |    1 +
>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>  target-microblaze/translate.c       |    6 +++---
>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>>> index 48c264b..f4e1cc5 100644
>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>      /* setup pvr to match kernel setting */
>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>
>> So I think this here will clear PVR2_FPU2 ...
>>
>>>      env->pvr.regs[4] = 0xc56b8000;
>>>      env->pvr.regs[5] = 0xc56be000;
>>>  }
>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>
>>>      /* init CPUs */
>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>>
>> But here you are setting use-fpu to 2. Is this a functional change for
>> ML605 board?
>>
>>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>>
>>>      /* Attach emulated BRAM through the LMB.  */
>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>> index 7bc5b81..ce3a574 100644
>>> --- a/target-microblaze/cpu-qom.h
>>> +++ b/target-microblaze/cpu-qom.h
>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>      /* Microblaze Configuration Settings */
>>>      struct {
>>>          bool stackproc;
>>> +        uint8_t usefpu;
>>>      } cfg;
>>>
>>>      CPUMBState env;
>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>> index c08da19..c3a9a5d 100644
>>> --- a/target-microblaze/cpu.c
>>> +++ b/target-microblaze/cpu.c
>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>                          | PVR2_USE_DIV_MASK \
>>>                          | PVR2_USE_HW_MUL_MASK \
>>>                          | PVR2_USE_MUL64_MASK \
>>> -                        | PVR2_USE_FPU_MASK \
>>> -                        | PVR2_USE_FPU2_MASK \
>>
>> If I have this straight, this suggests the property default value
>> should be 2, and the ml605 board setting should be 1.
>
> You are correct, I will fix this.
>
>>
>>>                          | PVR2_FPU_EXC_MASK \
>>>                          | 0;
>>> +
>>> +    if (cpu->cfg.usefpu) {
>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>> +
>>> +        if (cpu->cfg.usefpu > 1) {
>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>> +        }
>>> +    }
>>
>> This should be handled at realize time. See pl330_realize for example
>> of realize-time PVR ("cfg" in that case) population.
>
> But the env state gets wiped out at reset, so the information will be lost.
>

Ahh, so the solution there is to do what ARM does and have a section
at the back of the env which survives reset. Check the memset in
arm_cpu_reset.

Regards,
Peter

> I can tidy up the if logic though.
>
>>
>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>> can be moved into realize for settings not worth propertyifying.
>>
>>> +
>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>
>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>>                       true),
>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>
>> No need for xlnx prefix (FDT generic should trim the prefix).
>
> I agree, but the base-vectors already had it. If the xlnx is unneeded
> I'll remove
> it from the base-vector as well.
>
> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>>> index 4068946..36cfc5c 100644
>>> --- a/target-microblaze/translate.c
>>> +++ b/target-microblaze/translate.c
>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>
>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>
>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>      }
>>> -    return r;
>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>  }
>>>
>>>  static void dec_fpu(DisasContext *dc)
>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>
>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>          return;
>>> --
>>> 1.7.1
>>>
>>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>>> is a hassle and difficult to read, instead set them based on the CPU
>>>> properties.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>>  target-microblaze/cpu-qom.h         |    1 +
>>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>>  target-microblaze/translate.c       |    6 +++---
>>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>>>> index 48c264b..f4e1cc5 100644
>>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>>      /* setup pvr to match kernel setting */
>>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>>
>>> So I think this here will clear PVR2_FPU2 ...
>>>
>>>>      env->pvr.regs[4] = 0xc56b8000;
>>>>      env->pvr.regs[5] = 0xc56be000;
>>>>  }
>>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>>
>>>>      /* init CPUs */
>>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>>>
>>> But here you are setting use-fpu to 2. Is this a functional change for
>>> ML605 board?
>>>
>>>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>>>
>>>>      /* Attach emulated BRAM through the LMB.  */
>>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>>> index 7bc5b81..ce3a574 100644
>>>> --- a/target-microblaze/cpu-qom.h
>>>> +++ b/target-microblaze/cpu-qom.h
>>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>>      /* Microblaze Configuration Settings */
>>>>      struct {
>>>>          bool stackproc;
>>>> +        uint8_t usefpu;
>>>>      } cfg;
>>>>
>>>>      CPUMBState env;
>>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>>> index c08da19..c3a9a5d 100644
>>>> --- a/target-microblaze/cpu.c
>>>> +++ b/target-microblaze/cpu.c
>>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>>                          | PVR2_USE_DIV_MASK \
>>>>                          | PVR2_USE_HW_MUL_MASK \
>>>>                          | PVR2_USE_MUL64_MASK \
>>>> -                        | PVR2_USE_FPU_MASK \
>>>> -                        | PVR2_USE_FPU2_MASK \
>>>
>>> If I have this straight, this suggests the property default value
>>> should be 2, and the ml605 board setting should be 1.
>>
>> You are correct, I will fix this.
>>
>>>
>>>>                          | PVR2_FPU_EXC_MASK \
>>>>                          | 0;
>>>> +
>>>> +    if (cpu->cfg.usefpu) {
>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>> +
>>>> +        if (cpu->cfg.usefpu > 1) {
>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>> +        }
>>>> +    }
>>>
>>> This should be handled at realize time. See pl330_realize for example
>>> of realize-time PVR ("cfg" in that case) population.
>>
>> But the env state gets wiped out at reset, so the information will be lost.
>>
>
> Ahh, so the solution there is to do what ARM does and have a section
> at the back of the env which survives reset. Check the memset in
> arm_cpu_reset.

Ok, just to clarify we want all of the pvr registers to be preserved on reset?

Thanks,

Alistair

>
> Regards,
> Peter
>
>> I can tidy up the if logic though.
>>
>>>
>>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>>> can be moved into realize for settings not worth propertyifying.
>>>
>>>> +
>>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>>
>>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>>>                       true),
>>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>>
>>> No need for xlnx prefix (FDT generic should trim the prefix).
>>
>> I agree, but the base-vectors already had it. If the xlnx is unneeded
>> I'll remove
>> it from the base-vector as well.
>>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>>
>>>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>>>> index 4068946..36cfc5c 100644
>>>> --- a/target-microblaze/translate.c
>>>> +++ b/target-microblaze/translate.c
>>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>>
>>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>>
>>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>      }
>>>> -    return r;
>>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>>  }
>>>>
>>>>  static void dec_fpu(DisasContext *dc)
>>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>>
>>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>          return;
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Peter Crosthwaite]

On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>>>> is a hassle and difficult to read, instead set them based on the CPU
>>>>> properties.
>>>>>
>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>> ---
>>>>>
>>>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>>>  target-microblaze/cpu-qom.h         |    1 +
>>>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>>>  target-microblaze/translate.c       |    6 +++---
>>>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>>>>> index 48c264b..f4e1cc5 100644
>>>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>>>      /* setup pvr to match kernel setting */
>>>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>>>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>>>
>>>> So I think this here will clear PVR2_FPU2 ...
>>>>
>>>>>      env->pvr.regs[4] = 0xc56b8000;
>>>>>      env->pvr.regs[5] = 0xc56be000;
>>>>>  }
>>>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>>>
>>>>>      /* init CPUs */
>>>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>>>>
>>>> But here you are setting use-fpu to 2. Is this a functional change for
>>>> ML605 board?
>>>>
>>>>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>>>>
>>>>>      /* Attach emulated BRAM through the LMB.  */
>>>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>>>> index 7bc5b81..ce3a574 100644
>>>>> --- a/target-microblaze/cpu-qom.h
>>>>> +++ b/target-microblaze/cpu-qom.h
>>>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>>>      /* Microblaze Configuration Settings */
>>>>>      struct {
>>>>>          bool stackproc;
>>>>> +        uint8_t usefpu;
>>>>>      } cfg;
>>>>>
>>>>>      CPUMBState env;
>>>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>>>> index c08da19..c3a9a5d 100644
>>>>> --- a/target-microblaze/cpu.c
>>>>> +++ b/target-microblaze/cpu.c
>>>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>>>                          | PVR2_USE_DIV_MASK \
>>>>>                          | PVR2_USE_HW_MUL_MASK \
>>>>>                          | PVR2_USE_MUL64_MASK \
>>>>> -                        | PVR2_USE_FPU_MASK \
>>>>> -                        | PVR2_USE_FPU2_MASK \
>>>>
>>>> If I have this straight, this suggests the property default value
>>>> should be 2, and the ml605 board setting should be 1.
>>>
>>> You are correct, I will fix this.
>>>
>>>>
>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>                          | 0;
>>>>> +
>>>>> +    if (cpu->cfg.usefpu) {
>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>> +
>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>> +        }
>>>>> +    }
>>>>
>>>> This should be handled at realize time. See pl330_realize for example
>>>> of realize-time PVR ("cfg" in that case) population.
>>>
>>> But the env state gets wiped out at reset, so the information will be lost.
>>>
>>
>> Ahh, so the solution there is to do what ARM does and have a section
>> at the back of the env which survives reset. Check the memset in
>> arm_cpu_reset.
>
> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>

yes. But something that just occured to me, does it make sense to move
it outside the env? into the CPUState? Andreas mentioned that fields
in the CPU state before the env can be used with negative env* offsets
by translated code. This means the PVR could just be pushed up to
CPUState.

Regards,
Peter

> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>> I can tidy up the if logic though.
>>>
>>>>
>>>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>>>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>>>> can be moved into realize for settings not worth propertyifying.
>>>>
>>>>> +
>>>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>>>
>>>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>>>>                       true),
>>>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>>>
>>>> No need for xlnx prefix (FDT generic should trim the prefix).
>>>
>>> I agree, but the base-vectors already had it. If the xlnx is unneeded
>>> I'll remove
>>> it from the base-vector as well.
>>>
>>> Thanks,
>>>
>>> Alistair
>>>
>>>>
>>>> Regards,
>>>> Peter
>>>>
>>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>>  };
>>>>>
>>>>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>>>>> index 4068946..36cfc5c 100644
>>>>> --- a/target-microblaze/translate.c
>>>>> +++ b/target-microblaze/translate.c
>>>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>>>
>>>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>>>
>>>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>      }
>>>>> -    return r;
>>>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>>>  }
>>>>>
>>>>>  static void dec_fpu(DisasContext *dc)
>>>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>>>
>>>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>          return;
>>>>> --
>>>>> 1.7.1
>>>>>
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>>>>> is a hassle and difficult to read, instead set them based on the CPU
>>>>>> properties.
>>>>>>
>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>> ---
>>>>>>
>>>>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>>>>  target-microblaze/cpu-qom.h         |    1 +
>>>>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>>>>  target-microblaze/translate.c       |    6 +++---
>>>>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> index 48c264b..f4e1cc5 100644
>>>>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>>>>      /* setup pvr to match kernel setting */
>>>>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>>>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>>>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>>>>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>>>>
>>>>> So I think this here will clear PVR2_FPU2 ...
>>>>>
>>>>>>      env->pvr.regs[4] = 0xc56b8000;
>>>>>>      env->pvr.regs[5] = 0xc56be000;
>>>>>>  }
>>>>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>>>>
>>>>>>      /* init CPUs */
>>>>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>>>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>>>>>
>>>>> But here you are setting use-fpu to 2. Is this a functional change for
>>>>> ML605 board?
>>>>>
>>>>>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>>>>>
>>>>>>      /* Attach emulated BRAM through the LMB.  */
>>>>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>>>>> index 7bc5b81..ce3a574 100644
>>>>>> --- a/target-microblaze/cpu-qom.h
>>>>>> +++ b/target-microblaze/cpu-qom.h
>>>>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>>>>      /* Microblaze Configuration Settings */
>>>>>>      struct {
>>>>>>          bool stackproc;
>>>>>> +        uint8_t usefpu;
>>>>>>      } cfg;
>>>>>>
>>>>>>      CPUMBState env;
>>>>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>>>>> index c08da19..c3a9a5d 100644
>>>>>> --- a/target-microblaze/cpu.c
>>>>>> +++ b/target-microblaze/cpu.c
>>>>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>>>>                          | PVR2_USE_DIV_MASK \
>>>>>>                          | PVR2_USE_HW_MUL_MASK \
>>>>>>                          | PVR2_USE_MUL64_MASK \
>>>>>> -                        | PVR2_USE_FPU_MASK \
>>>>>> -                        | PVR2_USE_FPU2_MASK \
>>>>>
>>>>> If I have this straight, this suggests the property default value
>>>>> should be 2, and the ml605 board setting should be 1.
>>>>
>>>> You are correct, I will fix this.
>>>>
>>>>>
>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>                          | 0;
>>>>>> +
>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>> +
>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> This should be handled at realize time. See pl330_realize for example
>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>
>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>
>>>
>>> Ahh, so the solution there is to do what ARM does and have a section
>>> at the back of the env which survives reset. Check the memset in
>>> arm_cpu_reset.
>>
>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>
>
> yes. But something that just occured to me, does it make sense to move
> it outside the env? into the CPUState? Andreas mentioned that fields
> in the CPU state before the env can be used with negative env* offsets
> by translated code. This means the PVR could just be pushed up to
> CPUState.

Do any other machines do that?

I'm happy with leaving it in the env (partly because I just did it)
and it also matches the way that ARM does it.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> I can tidy up the if logic though.
>>>>
>>>>>
>>>>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>>>>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>>>>> can be moved into realize for settings not worth propertyifying.
>>>>>
>>>>>> +
>>>>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>>>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>>>>
>>>>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>>>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>>>>>                       true),
>>>>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>>>>
>>>>> No need for xlnx prefix (FDT generic should trim the prefix).
>>>>
>>>> I agree, but the base-vectors already had it. If the xlnx is unneeded
>>>> I'll remove
>>>> it from the base-vector as well.
>>>>
>>>> Thanks,
>>>>
>>>> Alistair
>>>>
>>>>>
>>>>> Regards,
>>>>> Peter
>>>>>
>>>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>>>  };
>>>>>>
>>>>>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>>>>>> index 4068946..36cfc5c 100644
>>>>>> --- a/target-microblaze/translate.c
>>>>>> +++ b/target-microblaze/translate.c
>>>>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>>>>
>>>>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>>>>
>>>>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>      }
>>>>>> -    return r;
>>>>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>>>>  }
>>>>>>
>>>>>>  static void dec_fpu(DisasContext *dc)
>>>>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>>>>
>>>>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>>>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>>>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>          return;
>>>>>> --
>>>>>> 1.7.1
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Peter Crosthwaite]

On Thu, May 14, 2015 at 11:36 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>>>>>> is a hassle and difficult to read, instead set them based on the CPU
>>>>>>> properties.
>>>>>>>
>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>>>>>  target-microblaze/cpu-qom.h         |    1 +
>>>>>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>>>>>  target-microblaze/translate.c       |    6 +++---
>>>>>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>>> index 48c264b..f4e1cc5 100644
>>>>>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>>>>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>>>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>>>>>      /* setup pvr to match kernel setting */
>>>>>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>>>>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>>>>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>>>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 << 8);
>>>>>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>>>>>
>>>>>> So I think this here will clear PVR2_FPU2 ...
>>>>>>
>>>>>>>      env->pvr.regs[4] = 0xc56b8000;
>>>>>>>      env->pvr.regs[5] = 0xc56be000;
>>>>>>>  }
>>>>>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>>>>>
>>>>>>>      /* init CPUs */
>>>>>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>>>>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", &error_abort);
>>>>>>
>>>>>> But here you are setting use-fpu to 2. Is this a functional change for
>>>>>> ML605 board?
>>>>>>
>>>>>>>      object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
>>>>>>>
>>>>>>>      /* Attach emulated BRAM through the LMB.  */
>>>>>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>>>>>> index 7bc5b81..ce3a574 100644
>>>>>>> --- a/target-microblaze/cpu-qom.h
>>>>>>> +++ b/target-microblaze/cpu-qom.h
>>>>>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>>>>>      /* Microblaze Configuration Settings */
>>>>>>>      struct {
>>>>>>>          bool stackproc;
>>>>>>> +        uint8_t usefpu;
>>>>>>>      } cfg;
>>>>>>>
>>>>>>>      CPUMBState env;
>>>>>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>>>>>> index c08da19..c3a9a5d 100644
>>>>>>> --- a/target-microblaze/cpu.c
>>>>>>> +++ b/target-microblaze/cpu.c
>>>>>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>>>>>                          | PVR2_USE_DIV_MASK \
>>>>>>>                          | PVR2_USE_HW_MUL_MASK \
>>>>>>>                          | PVR2_USE_MUL64_MASK \
>>>>>>> -                        | PVR2_USE_FPU_MASK \
>>>>>>> -                        | PVR2_USE_FPU2_MASK \
>>>>>>
>>>>>> If I have this straight, this suggests the property default value
>>>>>> should be 2, and the ml605 board setting should be 1.
>>>>>
>>>>> You are correct, I will fix this.
>>>>>
>>>>>>
>>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>>                          | 0;
>>>>>>> +
>>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>>> +
>>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>
>>>>>> This should be handled at realize time. See pl330_realize for example
>>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>>
>>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>>
>>>>
>>>> Ahh, so the solution there is to do what ARM does and have a section
>>>> at the back of the env which survives reset. Check the memset in
>>>> arm_cpu_reset.
>>>
>>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>>
>>
>> yes. But something that just occured to me, does it make sense to move
>> it outside the env? into the CPUState? Andreas mentioned that fields
>> in the CPU state before the env can be used with negative env* offsets
>> by translated code. This means the PVR could just be pushed up to
>> CPUState.
>
> Do any other machines do that?
>
> I'm happy with leaving it in the env (partly because I just did it)
> and it also matches the way that ARM does it.
>

Yeh ARM style is fine with me too. We can figure out the env vs CPU thing later.

Regards,
Peter

> Thanks,
>
> Alistair
>
>>
>> Regards,
>> Peter
>>
>>> Thanks,
>>>
>>> Alistair
>>>
>>>>
>>>> Regards,
>>>> Peter
>>>>
>>>>> I can tidy up the if logic though.
>>>>>
>>>>>>
>>>>>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>>>>>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>>>>>> can be moved into realize for settings not worth propertyifying.
>>>>>>
>>>>>>> +
>>>>>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>>>>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>>>>>
>>>>>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>>>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>>>>>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>>>>>>>                       true),
>>>>>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>>>>>
>>>>>> No need for xlnx prefix (FDT generic should trim the prefix).
>>>>>
>>>>> I agree, but the base-vectors already had it. If the xlnx is unneeded
>>>>> I'll remove
>>>>> it from the base-vector as well.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Alistair
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Peter
>>>>>>
>>>>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>>>>  };
>>>>>>>
>>>>>>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>>>>>>> index 4068946..36cfc5c 100644
>>>>>>> --- a/target-microblaze/translate.c
>>>>>>> +++ b/target-microblaze/translate.c
>>>>>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>>>>>
>>>>>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>>>>>
>>>>>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>>      }
>>>>>>> -    return r;
>>>>>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>>>>>  }
>>>>>>>
>>>>>>>  static void dec_fpu(DisasContext *dc)
>>>>>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>>>>>
>>>>>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>>>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>>>>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>>>>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>>          return;
>>>>>>> --
>>>>>>> 1.7.1
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [RFC v1 2/3] target-microblaze: Allow the stack protection to be disabled

[Alistair Francis]

On Fri, May 15, 2015 at 3:26 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Microblaze stack protection is configurable and isn't always enabled.
>> This patch allows the stack protection to be disabled from the CPU
>> properties.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-microblaze/cpu-qom.h   |    5 +++++
>>  target-microblaze/cpu.c       |    2 ++
>>  target-microblaze/op_helper.c |    4 +++-
>>  3 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>> index e3e0701..7bc5b81 100644
>> --- a/target-microblaze/cpu-qom.h
>> +++ b/target-microblaze/cpu-qom.h
>> @@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
>>      uint32_t base_vectors;
>>      /*< public >*/
>>
>> +    /* Microblaze Configuration Settings */
>> +    struct {
>> +        bool stackproc;
>> +    } cfg;
>> +
>>      CPUMBState env;
>>  } MicroBlazeCPU;
>>
>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>> index 67e3182..c08da19 100644
>> --- a/target-microblaze/cpu.c
>> +++ b/target-microblaze/cpu.c
>> @@ -152,6 +152,8 @@ static const VMStateDescription vmstate_mb_cpu = {
>>
>>  static Property mb_properties[] = {
>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>> +    DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>> +                     true),
>
> This should deposit (at realize time) into pvr[0].SPROT bit.

Thanks Peter. I will add that.

Thanks,

Alistair

>
> Regards,
> Peter
>
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/target-microblaze/op_helper.c b/target-microblaze/op_helper.c
>> index d2b3624..24df538 100644
>> --- a/target-microblaze/op_helper.c
>> +++ b/target-microblaze/op_helper.c
>> @@ -467,7 +467,9 @@ void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr,
>>
>>  void helper_stackprot(CPUMBState *env, uint32_t addr)
>>  {
>> -    if (addr < env->slr || addr > env->shr) {
>> +    MicroBlazeCPU *cpu = mb_env_get_cpu(env);
>> +
>> +    if (cpu->cfg.stackproc && (addr < env->slr || addr > env->shr)) {
>>          qemu_log("Stack protector violation at %x %x %x\n",
>>                   addr, env->slr, env->shr);
>>          env->sregs[SR_EAR] = addr;
>> --
>> 1.7.1
>>
>>
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Andreas Färber]

Am 15.05.2015 um 08:36 schrieb Alistair Francis:
> On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>>                          | 0;
>>>>>>> +
>>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>>> +
>>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>
>>>>>> This should be handled at realize time. See pl330_realize for example
>>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>>
>>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>
>>>> Ahh, so the solution there is to do what ARM does and have a section
>>>> at the back of the env which survives reset. Check the memset in
>>>> arm_cpu_reset.
>>>
>>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>
>> yes. But something that just occured to me, does it make sense to move
>> it outside the env? into the CPUState? Andreas mentioned that fields
>> in the CPU state before the env can be used with negative env* offsets
>> by translated code. This means the PVR could just be pushed up to
>> CPUState.
> 
> Do any other machines do that?
> 
> I'm happy with leaving it in the env (partly because I just did it)
> and it also matches the way that ARM does it.

All targets had everything in env originally, so that's not really an
argument for anything. You need to decide what makes sense for your
target - icount is an example using negative offsets in CPUState now,
whereas TCGv* variables declared via offset from env remained in env.

If moving something to the end of env is an option, then moving it to
MicroBlazeCPU (not CPUState!) would seem better, especially when it's
not supposed to be reset. Going from CPUMBState *env to MicroBlazeCPU
*cpu is a cheap offset operation, whereas CPUState *cs involves a QOM cast.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Fri, May 15, 2015 at 9:15 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 15.05.2015 um 08:36 schrieb Alistair Francis:
>> On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>>>                          | 0;
>>>>>>>> +
>>>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>>>> +
>>>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>
>>>>>>> This should be handled at realize time. See pl330_realize for example
>>>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>>>
>>>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>>
>>>>> Ahh, so the solution there is to do what ARM does and have a section
>>>>> at the back of the env which survives reset. Check the memset in
>>>>> arm_cpu_reset.
>>>>
>>>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>>
>>> yes. But something that just occured to me, does it make sense to move
>>> it outside the env? into the CPUState? Andreas mentioned that fields
>>> in the CPU state before the env can be used with negative env* offsets
>>> by translated code. This means the PVR could just be pushed up to
>>> CPUState.
>>
>> Do any other machines do that?
>>
>> I'm happy with leaving it in the env (partly because I just did it)
>> and it also matches the way that ARM does it.
>
> All targets had everything in env originally, so that's not really an
> argument for anything. You need to decide what makes sense for your
> target - icount is an example using negative offsets in CPUState now,
> whereas TCGv* variables declared via offset from env remained in env.

I think env makes sense in this case, especially as the
DEFINE_PROP_UINT8() macro defaults to setting variables in the env
structure. Admittedly I haven't looked into it, so it might be just as
easy to set variables in the MicroBlazeCPU.

I have another patch set ready to send out tomorrow with all of the
variables in env. We can decide from there if they should be moved to
the MicroBlazeCPU struct.

Thanks,

Alistair

>
> If moving something to the end of env is an option, then moving it to
> MicroBlazeCPU (not CPUState!) would seem better, especially when it's
> not supposed to be reset. Going from CPUMBState *env to MicroBlazeCPU
> *cpu is a cheap offset operation, whereas CPUState *cs involves a QOM cast.
>
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Andreas Färber]

Am 17.05.2015 um 14:26 schrieb Alistair Francis:
> On Fri, May 15, 2015 at 9:15 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 15.05.2015 um 08:36 schrieb Alistair Francis:
>>> On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>>>>                          | 0;
>>>>>>>>> +
>>>>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>>>>> +
>>>>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>
>>>>>>>> This should be handled at realize time. See pl330_realize for example
>>>>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>>>>
>>>>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>>>
>>>>>> Ahh, so the solution there is to do what ARM does and have a section
>>>>>> at the back of the env which survives reset. Check the memset in
>>>>>> arm_cpu_reset.
>>>>>
>>>>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>>>
>>>> yes. But something that just occured to me, does it make sense to move
>>>> it outside the env? into the CPUState? Andreas mentioned that fields
>>>> in the CPU state before the env can be used with negative env* offsets
>>>> by translated code. This means the PVR could just be pushed up to
>>>> CPUState.
>>>
>>> Do any other machines do that?
>>>
>>> I'm happy with leaving it in the env (partly because I just did it)
>>> and it also matches the way that ARM does it.
>>
>> All targets had everything in env originally, so that's not really an
>> argument for anything. You need to decide what makes sense for your
>> target - icount is an example using negative offsets in CPUState now,
>> whereas TCGv* variables declared via offset from env remained in env.
> 
> I think env makes sense in this case, especially as the
> DEFINE_PROP_UINT8() macro defaults to setting variables in the env
> structure. Admittedly I haven't looked into it, so it might be just as
> easy to set variables in the MicroBlazeCPU.

In fact the CPU properties operate on MicroBlazeCPU. The legacy
properties thus need an explicit env.foo as macro argument.

But as usual things can be done in multiple steps.

Andreas

> I have another patch set ready to send out tomorrow with all of the
> variables in env. We can decide from there if they should be moved to
> the MicroBlazeCPU struct.
> 
> Thanks,
> 
> Alistair

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Mon, May 18, 2015 at 1:12 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 17.05.2015 um 14:26 schrieb Alistair Francis:
>> On Fri, May 15, 2015 at 9:15 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 15.05.2015 um 08:36 schrieb Alistair Francis:
>>>> On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>>>>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>>>>>                          | 0;
>>>>>>>>>> +
>>>>>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>>>>>> +
>>>>>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>>>>>> +        }
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> This should be handled at realize time. See pl330_realize for example
>>>>>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>>>>>
>>>>>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>>>>
>>>>>>> Ahh, so the solution there is to do what ARM does and have a section
>>>>>>> at the back of the env which survives reset. Check the memset in
>>>>>>> arm_cpu_reset.
>>>>>>
>>>>>> Ok, just to clarify we want all of the pvr registers to be preserved on reset?
>>>>>
>>>>> yes. But something that just occured to me, does it make sense to move
>>>>> it outside the env? into the CPUState? Andreas mentioned that fields
>>>>> in the CPU state before the env can be used with negative env* offsets
>>>>> by translated code. This means the PVR could just be pushed up to
>>>>> CPUState.
>>>>
>>>> Do any other machines do that?
>>>>
>>>> I'm happy with leaving it in the env (partly because I just did it)
>>>> and it also matches the way that ARM does it.
>>>
>>> All targets had everything in env originally, so that's not really an
>>> argument for anything. You need to decide what makes sense for your
>>> target - icount is an example using negative offsets in CPUState now,
>>> whereas TCGv* variables declared via offset from env remained in env.
>>
>> I think env makes sense in this case, especially as the
>> DEFINE_PROP_UINT8() macro defaults to setting variables in the env
>> structure. Admittedly I haven't looked into it, so it might be just as
>> easy to set variables in the MicroBlazeCPU.
>
> In fact the CPU properties operate on MicroBlazeCPU. The legacy
> properties thus need an explicit env.foo as macro argument.

I confused myself with that last comment and forgot that we were talking
about the pvr regs and not the configuration variables. So it doesn't matter
what the DEFINE_PROP_UINT8() macro does.

I'll send my patches now and we can keep discussing it there.

Thanks,

Alistair

>
> But as usual things can be done in multiple steps.
>
> Andreas
>
>> I have another patch set ready to send out tomorrow with all of the
>> variables in env. We can decide from there if they should be moved to
>> the MicroBlazeCPU struct.
>>
>> Thanks,
>>
>> Alistair
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>