diff mbox series

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

Message ID 20180419112131.16932-2-edgar.iglesias@gmail.com
State New
Headers show
Series target-microblaze: Misc bug fixes | expand

Commit Message

Edgar E. Iglesias April 19, 2018, 11:21 a.m. UTC 
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(-)

Comments

Richard Henderson April 19, 2018, 7:56 p.m. UTC | #1 
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~
Edgar E. Iglesias April 19, 2018, 8:33 p.m. UTC | #2 
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
Richard Henderson April 19, 2018, 9:17 p.m. UTC | #3 
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~
Edgar E. Iglesias April 19, 2018, 9:21 p.m. UTC | #4 
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
diff mbox series

Patch

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)