[Qemu-devel] [PATCH v1 0/5] target-microblaze: Misc bug fixes

[Qemu-devel] [PATCH v1 0/5] target-microblaze: Misc bug fixes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

This is a series of fixes for bugs I ran into when adding support
for extended addressing. It would be nice to get these into 2.12
if it's not too late.

Cheers,
Edgar

Edgar E. Iglesias (5):
  target-microblaze: Respect MSR.PVR as read-only
  target-microblaze: Fix trap checks for FPU insns
  target-microblaze: Don't clobber the IMM reg for ld/st reversed
  target-microblaze: mmu: Make TLBSX write-only
  target-microblaze: mmu: Make the TLBX MISS bit read-only

 target/microblaze/cpu.h       |  4 +++-
 target/microblaze/mmu.c       |  9 ++++++++-
 target/microblaze/translate.c | 12 ++----------
 3 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Respect MSR.PVR as read-only. We were wrongly overwriting the PVR bit.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/cpu.h       | 4 +++-
 target/microblaze/translate.c | 8 +-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
index 5be71bc320..0eb9e2b8e2 100644
--- a/target/microblaze/cpu.h
+++ b/target/microblaze/cpu.h
@@ -59,6 +59,8 @@ typedef struct CPUMBState CPUMBState;
 #define SR_EDR   0xd
 
 /* MSR flags.  */
+#define MSR_PVR_SHIFT 10
+
 #define MSR_BE  (1<<0) /* 0x001 */
 #define MSR_IE  (1<<1) /* 0x002 */
 #define MSR_C   (1<<2) /* 0x004 */
@@ -69,7 +71,7 @@ typedef struct CPUMBState CPUMBState;
 #define MSR_DCE (1<<7) /* 0x080 */
 #define MSR_EE  (1<<8) /* 0x100 */
 #define MSR_EIP (1<<9) /* 0x200 */
-#define MSR_PVR (1<<10) /* 0x400 */
+#define MSR_PVR (1 << MSR_PVR_SHIFT)
 #define MSR_CC  (1<<31)
 
 /* Machine State Register (MSR) Fields */
diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 7628b0e25b..df62563815 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -417,15 +417,9 @@ static inline void msr_read(DisasContext *dc, TCGv d)
 
 static inline void msr_write(DisasContext *dc, TCGv v)
 {
-    TCGv t;
-
-    t = tcg_temp_new();
     dc->cpustate_changed = 1;
     /* PVR bit is not writable.  */
-    tcg_gen_andi_tl(t, v, ~MSR_PVR);
-    tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
-    tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
-    tcg_temp_free(t);
+    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
 }
 
 static void dec_msr(DisasContext *dc)
-- 
2.14.1

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Richard Henderson]

On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
>  static inline void msr_write(DisasContext *dc, TCGv v)
>  {
> -    TCGv t;
> -
> -    t = tcg_temp_new();
>      dc->cpustate_changed = 1;
>      /* PVR bit is not writable.  */
> -    tcg_gen_andi_tl(t, v, ~MSR_PVR);
> -    tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
> -    tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
> -    tcg_temp_free(t);
> +    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
>  }

Um... the old code was correct, but the new code isn't.

The new code sets msr to v, with bit 10 set to the old msr bit 0.

Why do you believe the old code to be wrong?


r~

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Edgar E. Iglesias]

On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
> >  static inline void msr_write(DisasContext *dc, TCGv v)
> >  {
> > -    TCGv t;
> > -
> > -    t = tcg_temp_new();
> >      dc->cpustate_changed = 1;
> >      /* PVR bit is not writable.  */
> > -    tcg_gen_andi_tl(t, v, ~MSR_PVR);
> > -    tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
> > -    tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
> > -    tcg_temp_free(t);
> > +    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
> >  }
> 
> Um... the old code was correct, but the new code isn't.
> 
> The new code sets msr to v, with bit 10 set to the old msr bit 0.
> 
> Why do you believe the old code to be wrong?

The old code was incorrectly ORing v instead of t...

What about the following?
    tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Richard Henderson]

On 04/19/2018 10:33 AM, Edgar E. Iglesias wrote:
> On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
>> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
>>>  static inline void msr_write(DisasContext *dc, TCGv v)
>>>  {
>>> -    TCGv t;
>>> -
>>> -    t = tcg_temp_new();
>>>      dc->cpustate_changed = 1;
>>>      /* PVR bit is not writable.  */
>>> -    tcg_gen_andi_tl(t, v, ~MSR_PVR);
>>> -    tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
>>> -    tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
>>> -    tcg_temp_free(t);
>>> +    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
>>>  }
>>
>> Um... the old code was correct, but the new code isn't.
>>
>> The new code sets msr to v, with bit 10 set to the old msr bit 0.
>>
>> Why do you believe the old code to be wrong?
> 
> The old code was incorrectly ORing v instead of t...

Ah, yes.

> What about the following?
>     tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
>     tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);

*shrug* While that would be functional, I don't think it's any better than just
fixing the typo in the OR.


r~

Re: [Qemu-devel] [PATCH v1 1/5] target-microblaze: Respect MSR.PVR as read-only

[Edgar E. Iglesias]

On Thu, Apr 19, 2018 at 11:17:58AM -1000, Richard Henderson wrote:
> On 04/19/2018 10:33 AM, Edgar E. Iglesias wrote:
> > On Thu, Apr 19, 2018 at 09:56:40AM -1000, Richard Henderson wrote:
> >> On 04/19/2018 01:21 AM, Edgar E. Iglesias wrote:
> >>>  static inline void msr_write(DisasContext *dc, TCGv v)
> >>>  {
> >>> -    TCGv t;
> >>> -
> >>> -    t = tcg_temp_new();
> >>>      dc->cpustate_changed = 1;
> >>>      /* PVR bit is not writable.  */
> >>> -    tcg_gen_andi_tl(t, v, ~MSR_PVR);
> >>> -    tcg_gen_andi_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR);
> >>> -    tcg_gen_or_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], v);
> >>> -    tcg_temp_free(t);
> >>> +    tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
> >>>  }
> >>
> >> Um... the old code was correct, but the new code isn't.
> >>
> >> The new code sets msr to v, with bit 10 set to the old msr bit 0.
> >>
> >> Why do you believe the old code to be wrong?
> > 
> > The old code was incorrectly ORing v instead of t...
> 
> Ah, yes.
> 
> > What about the following?
> >     tcg_gen_shri_tl(cpu_SR[SR_MSR], cpu_SR[SR_MSR], MSR_PVR_SHIFT);
> >     tcg_gen_deposit_tl(cpu_SR[SR_MSR], v, cpu_SR[SR_MSR], MSR_PVR_SHIFT, 1);
> 
> *shrug* While that would be functional, I don't think it's any better than just
> fixing the typo in the OR.

Allright, I'll fix the typo and send out a v2.

Thanks for catching this.

Cheers,
Edgar

[Qemu-devel] [PATCH v1 2/5] target-microblaze: Fix trap checks for FPU insns

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Fix trap checks for FPU insns when extended FPU insns are enabled.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index df62563815..5f9efcdd11 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1406,7 +1406,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->cfg.use_fpu != 1)) {
+          && !dc->cpu->cfg.use_fpu) {
         tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
         t_gen_raise_exception(dc, EXCP_HW_EXCP);
         return;
-- 
2.14.1

Re: [Qemu-devel] [PATCH v1 2/5] target-microblaze: Fix trap checks for FPU insns

[Alistair Francis]

On Thu, Apr 19, 2018 at 4:21 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Fix trap checks for FPU insns when extended FPU insns are enabled.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/microblaze/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index df62563815..5f9efcdd11 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1406,7 +1406,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->cfg.use_fpu != 1)) {
> +          && !dc->cpu->cfg.use_fpu) {
>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>          return;
> --
> 2.14.1
>
>

[Qemu-devel] [PATCH v1 3/5] target-microblaze: Don't clobber the IMM reg for ld/st reversed

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Do not clobber the IMM register on reversed load/stores.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/translate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index 5f9efcdd11..9dcbdf5509 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -946,7 +946,6 @@ static void dec_load(DisasContext *dc)
                 tcg_gen_sub_tl(low, tcg_const_tl(3), low);
                 tcg_gen_andi_tl(t, t, ~3);
                 tcg_gen_or_tl(t, t, low);
-                tcg_gen_mov_tl(env_imm, t);
                 tcg_temp_free(low);
                 break;
             }
@@ -1098,7 +1097,6 @@ static void dec_store(DisasContext *dc)
                 tcg_gen_sub_tl(low, tcg_const_tl(3), low);
                 tcg_gen_andi_tl(t, t, ~3);
                 tcg_gen_or_tl(t, t, low);
-                tcg_gen_mov_tl(env_imm, t);
                 tcg_temp_free(low);
                 break;
             }
-- 
2.14.1

[Qemu-devel] [PATCH v1 4/5] target-microblaze: mmu: Make TLBSX write-only

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Make TLBSX write-only and guest-error log reads from it.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/mmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
index a0f06758f8..8391811900 100644
--- a/target/microblaze/mmu.c
+++ b/target/microblaze/mmu.c
@@ -182,7 +182,7 @@ done:
 uint32_t mmu_read(CPUMBState *env, uint32_t rn)
 {
     unsigned int i;
-    uint32_t r;
+    uint32_t r = 0;
 
     if (env->mmu.c_mmu < 2 || !env->mmu.c_mmu_tlb_access) {
         qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
@@ -211,6 +211,9 @@ uint32_t mmu_read(CPUMBState *env, uint32_t rn)
             }
             r = env->mmu.regs[rn];
             break;
+        case MMU_R_TLBSX:
+            qemu_log_mask(LOG_GUEST_ERROR, "TLBSX is write-only.\n");
+            break;
         default:
             r = env->mmu.regs[rn];
             break;
-- 
2.14.1

Re: [Qemu-devel] [PATCH v1 4/5] target-microblaze: mmu: Make TLBSX write-only

[Alistair Francis]

On Thu, Apr 19, 2018 at 4:21 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Make TLBSX write-only and guest-error log reads from it.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/microblaze/mmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index a0f06758f8..8391811900 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -182,7 +182,7 @@ done:
>  uint32_t mmu_read(CPUMBState *env, uint32_t rn)
>  {
>      unsigned int i;
> -    uint32_t r;
> +    uint32_t r = 0;
>
>      if (env->mmu.c_mmu < 2 || !env->mmu.c_mmu_tlb_access) {
>          qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
> @@ -211,6 +211,9 @@ uint32_t mmu_read(CPUMBState *env, uint32_t rn)
>              }
>              r = env->mmu.regs[rn];
>              break;
> +        case MMU_R_TLBSX:
> +            qemu_log_mask(LOG_GUEST_ERROR, "TLBSX is write-only.\n");
> +            break;
>          default:
>              r = env->mmu.regs[rn];
>              break;
> --
> 2.14.1
>
>

[Qemu-devel] [PATCH v1 5/5] target-microblaze: mmu: Make the TLBX MISS bit read-only

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Make the TLBX MISS bit read-only.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target/microblaze/mmu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
index 8391811900..9d5e6aa8a5 100644
--- a/target/microblaze/mmu.c
+++ b/target/microblaze/mmu.c
@@ -273,6 +273,10 @@ void mmu_write(CPUMBState *env, uint32_t rn, uint32_t v)
                 env->mmu.regs[rn] = v;
             }
             break;
+        case MMU_R_TLBX:
+            /* Bit 31 is read-only.  */
+            env->mmu.regs[rn] = deposit32(env->mmu.regs[rn], 0, 31, v);
+            break;
         case MMU_R_TLBSX:
         {
             struct microblaze_mmu_lookup lu;
-- 
2.14.1

Re: [Qemu-devel] [PATCH v1 5/5] target-microblaze: mmu: Make the TLBX MISS bit read-only

[Alistair Francis]

On Thu, Apr 19, 2018 at 4:21 AM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Make the TLBX MISS bit read-only.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/microblaze/mmu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 8391811900..9d5e6aa8a5 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -273,6 +273,10 @@ void mmu_write(CPUMBState *env, uint32_t rn, uint32_t v)
>                  env->mmu.regs[rn] = v;
>              }
>              break;
> +        case MMU_R_TLBX:
> +            /* Bit 31 is read-only.  */
> +            env->mmu.regs[rn] = deposit32(env->mmu.regs[rn], 0, 31, v);
> +            break;
>          case MMU_R_TLBSX:
>          {
>              struct microblaze_mmu_lookup lu;
> --
> 2.14.1
>
>