Skip to contentSkip to conversation

Add Microblaze configuration options

RFC In review Alistair Francis 4 participants 19 messages May 14, 2015 – May 18, 2015

lore
Share & tools
lore

Alistair Francis

author
#0 lore raw

cover Add Microblaze configuration options

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(-)
author
#1 lore raw

RFC 1/3 target-microblaze: Fix up indentation

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

Signed-off-by Alistair Francis <[email protected]>

---

 target-microblaze/op_helper.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
target-microblaze/op_helper.c +5 -5
index a4c8f04..d2b3624 100644
@@ -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);
     }
 }
Signature · 1 line
1.7.1
author
#2 lore raw

RFC 2/3 target-microblaze: Allow the stack protection to be disabled

In reply to 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 <[email protected]>

---

 target-microblaze/cpu-qom.h   |    5 +++++
 target-microblaze/cpu.c       |    2 ++
 target-microblaze/op_helper.c |    4 +++-
 3 files changed, 10 insertions(+), 1 deletions(-)
target-microblaze/cpu-qom.h +5 -0
index e3e0701..7bc5b81 100644
@@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
     uint32_t base_vectors;
     /*< public >*/
 
+    /* Microblaze Configuration Settings */
+    struct {
+        bool stackproc;
+    } cfg;
+
     CPUMBState env;
 } MicroBlazeCPU;
target-microblaze/cpu.c +2 -0
index 67e3182..c08da19 100644
@@ -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(),
 };
target-microblaze/op_helper.c +3 -1
index d2b3624..24df538 100644
@@ -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;
Signature · 1 line
1.7.1
#3 lore raw

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

target-microblaze/op_helper.c void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr, · line 472

Quoted 6 lines · Alistair Francis

void helper_stackprot(CPUMBState *env, uint32_t addr) {

target-microblaze/op_helper.c +3 -1
-    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~

author
#4 lore raw

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

Quoted 10 lines · Richard Henderson

target-microblaze/op_helper.c void helper_memalign(CPUMBState *env, uint32_t addr, uint32_t dr, uint32_t wr, · line 472

Quoted 6 lines · Alistair Francis

void helper_stackprot(CPUMBState *env, uint32_t addr) {

target-microblaze/op_helper.c +3 -1
-    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

Quoted 4 lines

r~

#5 lore raw

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

In reply to Alistair Francis
Quoted 33 lines · 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 <[email protected]>

---

 target-microblaze/cpu-qom.h   |    5 +++++
 target-microblaze/cpu.c       |    2 ++
 target-microblaze/op_helper.c |    4 +++-
 3 files changed, 10 insertions(+), 1 deletions(-)
target-microblaze/cpu-qom.h +5 -0
index e3e0701..7bc5b81 100644
@@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
     uint32_t base_vectors;
     /*< public >*/
 
+    /* Microblaze Configuration Settings */
+    struct {
+        bool stackproc;
+    } cfg;
+
     CPUMBState env;
 } MicroBlazeCPU;
target-microblaze/cpu.c +0 -0
index 67e3182..c08da19 100644

target-microblaze/cpu.c static const VMStateDescription vmstate_mb_cpu = { · line 156

Quoted 5 lines
target-microblaze/cpu.c +2 -0
@@ -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

Quoted 22 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/op_helper.c +3 -2
index d2b3624..24df538 100644
@@ -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

author
#6 lore raw

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

Quoted 36 lines · Peter Crosthwaite
Quoted 34 lines · 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 <[email protected]>

---

 target-microblaze/cpu-qom.h   |    5 +++++
 target-microblaze/cpu.c       |    2 ++
 target-microblaze/op_helper.c |    4 +++-
 3 files changed, 10 insertions(+), 1 deletions(-)
target-microblaze/cpu-qom.h +5 -0
index e3e0701..7bc5b81 100644
@@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
     uint32_t base_vectors;
     /*< public >*/
 
+    /* Microblaze Configuration Settings */
+    struct {
+        bool stackproc;
+    } cfg;
+
     CPUMBState env;
 } MicroBlazeCPU;
target-microblaze/cpu.c +0 -0
index 67e3182..c08da19 100644
@@ -152,6 +152,8 @@ static const VMStateDescription vmstate_mb_cpu = {
Quoted 6 lines

target-microblaze/cpu.c static const VMStateDescription vmstate_mb_cpu = { · line 156

Quoted 4 lines
target-microblaze/cpu.c +2 -0
 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

Quoted 27 lines

Regards,
Peter

Quoted 22 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/op_helper.c +3 -2
index d2b3624..24df538 100644
@@ -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

author
#7 lore raw

RFC 3/3 target-microblaze: Convert use-fpu to a CPU property

In reply to 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +2 -2
index 48c264b..f4e1cc5 100644
@@ -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.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
 
     CPUMBState env;
target-microblaze/cpu.c +11 -2
index c08da19..c3a9a5d 100644
@@ -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(),
 };
target-microblaze/translate.c +3 -3
index 4068946..36cfc5c 100644
@@ -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;
Signature · 1 line
1.7.1
#8 lore raw

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

Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 20 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
 
     CPUMBState env;
target-microblaze/cpu.c +0 -0
index c08da19..c3a9a5d 100644

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 5 lines
target-microblaze/cpu.c +0 -2
@@ -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.

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 11 lines
target-microblaze/cpu.c +9 -0
                         | 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.

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 9 lines
target-microblaze/cpu.c +2 -0
+
     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

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

author
#9 lore raw

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

Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

#10 lore raw

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

Quoted 91 lines · Alistair Francis
Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 2 lines

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

Quoted 67 lines

I can tidy up the if logic though.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

author
#11 lore raw

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

Quoted 93 lines · Peter Crosthwaite
Quoted 91 lines · Alistair Francis
Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 72 lines

Regards,
Peter

Quoted 67 lines

I can tidy up the if logic though.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

#12 lore raw

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

Quoted 95 lines · Alistair Francis
Quoted 93 lines · Peter Crosthwaite
Quoted 91 lines · Alistair Francis
Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

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

Quoted 77 lines

Thanks,

Alistair

Quoted 72 lines

Regards,
Peter

Quoted 67 lines

I can tidy up the if logic though.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

author
#13 lore raw

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

Quoted 104 lines · Peter Crosthwaite
Quoted 102 lines · Alistair Francis
Quoted 99 lines · Peter Crosthwaite
Quoted 91 lines · Alistair Francis
Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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

Quoted 82 lines

Regards,
Peter

Quoted 77 lines

Thanks,

Alistair

Quoted 72 lines

Regards,
Peter

Quoted 67 lines

I can tidy up the if logic though.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

#14 lore raw

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

Quoted 115 lines · Alistair Francis
Quoted 104 lines · Peter Crosthwaite
Quoted 102 lines · Alistair Francis
Quoted 99 lines · Peter Crosthwaite
Quoted 91 lines · Alistair Francis
Quoted 56 lines · Peter Crosthwaite
Quoted 18 lines · 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 <[email protected]>

---

 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(-)
hw/microblaze/petalogix_ml605_mmu.c +0 -0
index 48c264b..f4e1cc5 100644

hw/microblaze/petalogix_ml605_mmu.c static void machine_cpu_reset(MicroBlazeCPU *cpu) · line 76

Quoted 7 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -2
@@ -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 ...

hw/microblaze/petalogix_ml605_mmu.c petalogix_ml605_init(MachineState *machine) · line 97

Quoted 8 lines
hw/microblaze/petalogix_ml605_mmu.c +1 -0
     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?

Quoted 14 lines
hw/microblaze/petalogix_ml605_mmu.c +0 -0
     object_property_set_bool(OBJECT(cpu), true, "realized", &error_abort);
 
     /* Attach emulated BRAM through the LMB.  */
target-microblaze/cpu-qom.h +1 -0
index 7bc5b81..ce3a574 100644
@@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
     /* Microblaze Configuration Settings */
     struct {
         bool stackproc;
+        uint8_t usefpu;
     } cfg;
Quoted 14 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 93

Quoted 11 lines
target-microblaze/cpu-qom.h +0 -0
     CPUMBState env;
target-microblaze/cpu.c +0 -2
index c08da19..c3a9a5d 100644
@@ -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.

Quoted 15 lines

target-microblaze/cpu.c static void mb_cpu_reset(CPUState *s) · line 102

Quoted 12 lines
target-microblaze/cpu.c +9 -0
                         | 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.

Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 5 lines

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

Quoted 87 lines

Thanks,

Alistair

Quoted 82 lines

Regards,
Peter

Quoted 77 lines

Thanks,

Alistair

Quoted 72 lines

Regards,
Peter

Quoted 67 lines

I can tidy up the if logic though.

Quoted 9 lines

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.

Quoted 4 lines
target-microblaze/cpu.c +1 -0
+
     env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
     env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
Quoted 7 lines

target-microblaze/cpu.c static Property mb_properties[] = { · line 165

Quoted 5 lines
target-microblaze/cpu.c +1 -0
@@ -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

Quoted 39 lines

Regards,
Peter

Quoted 34 lines
target-microblaze/cpu.c +0 -0
     DEFINE_PROP_END_OF_LIST(),
 };
target-microblaze/translate.c +3 -4
index 4068946..36cfc5c 100644
@@ -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

#15 lore raw

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

In reply to Alistair Francis

Am 15.05.2015 um 08:36 schrieb Alistair Francis:

Quoted 41 lines
Quoted 31 lines · Peter Crosthwaite
Quoted 29 lines · Alistair Francis
Quoted 26 lines · Peter Crosthwaite
Quoted 19 lines · Alistair Francis
Quoted 16 lines · Peter Crosthwaite

Review

Quoted 11 lines · Alistair Francis
                         | PVR2_FPU_EXC_MASK \
                         | 0;
(quoted diff) +9 -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.

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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.

Quoted 4 lines

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

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
author
#16 lore raw

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

Quoted 44 lines · Andreas Färber

Am 15.05.2015 um 08:36 schrieb Alistair Francis:

Quoted 43 lines
Quoted 31 lines · Peter Crosthwaite
Quoted 29 lines · Alistair Francis
Quoted 26 lines · Peter Crosthwaite
Quoted 19 lines · Alistair Francis
Quoted 16 lines · Peter Crosthwaite

Review

Quoted 11 lines · Alistair Francis
                         | PVR2_FPU_EXC_MASK \
                         | 0;
(quoted diff) +9 -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.

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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?

Quoted 7 lines
Quoted 2 lines

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

Quoted 14 lines

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)

#17 lore raw

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

Am 17.05.2015 um 14:26 schrieb Alistair Francis:

Quoted 53 lines · Andreas Färber
Quoted 44 lines

Am 15.05.2015 um 08:36 schrieb Alistair Francis:

Quoted 43 lines
Quoted 31 lines · Peter Crosthwaite
Quoted 29 lines · Alistair Francis
Quoted 26 lines · Peter Crosthwaite
Quoted 19 lines · Alistair Francis
Quoted 16 lines · Peter Crosthwaite

Review

Quoted 11 lines · Alistair Francis
                         | PVR2_FPU_EXC_MASK \
                         | 0;
(quoted diff) +9 -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.

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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?

Quoted 7 lines
Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 7 lines

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

Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
author
#18 lore raw

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

Quoted 54 lines · Andreas Färber

Am 17.05.2015 um 14:26 schrieb Alistair Francis:

Quoted 53 lines · Andreas Färber
Quoted 51 lines

Am 15.05.2015 um 08:36 schrieb Alistair Francis:

Quoted 41 lines
Quoted 31 lines · Peter Crosthwaite
Quoted 29 lines · Alistair Francis
Quoted 26 lines · Peter Crosthwaite
Quoted 19 lines · Alistair Francis
Quoted 16 lines · Peter Crosthwaite

Review

Quoted 11 lines · Alistair Francis
                         | PVR2_FPU_EXC_MASK \
                         | 0;
(quoted diff) +9 -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.

Quoted 1 line

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.

Quoted 7 lines
Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 7 lines
Quoted 4 lines

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

Quoted 18 lines

But as usual things can be done in multiple steps.

Andreas

Quoted 7 lines

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)