[Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Shin-ichiro KAWASAKI]

This patch adds check for all SH4 instructions which are
executed only in privileged mode.

I have run such instructions in non-privileged mode.
SH-Linux prints 'illegal instruction' message in System Emulation,
and QEMU prints 'Unhandled trap' message in User Mode Emulation.
No regression found. 

The instruction name 'lds Rm, SR' in comments modified to
'ldc Rm, SR' for consistency with SH4 spec document.

I hope this patch will be merged in trunk.
Handling the patch I sent before will be appreciated, too.
http://lists.gnu.org/archive/html/qemu-devel/2008-09/msg00358.html

Any comments on those patches are welcome.

regards,
Shin-ichiro KAWASAKI


Index: trunk/target-sh4/translate.c
===================================================================
--- trunk/target-sh4/translate.c	(revision 5177)
+++ trunk/target-sh4/translate.c	(working copy)
@@ -48,6 +48,12 @@
     int singlestep_enabled;
 } DisasContext;
 
+#if defined(CONFIG_USER_ONLY)
+#define IS_USER(s) 1
+#else
+#define IS_USER(s) (!s->memidx)
+#endif
+
 enum {
     BS_NONE     = 0, /* We go out of the TB without reaching a branch or an
                       * exception condition
@@ -448,6 +454,13 @@
   {tcg_gen_helper_0_0(helper_raise_slot_illegal_instruction); ctx->bstate = BS_EXCP; \
    return;}
 
+#define CHECK_PRIVILEGED                                      \
+  if (IS_USER(ctx)) {                                         \
+      tcg_gen_helper_0_0(helper_raise_illegal_instruction);   \
+      ctx->bstate = BS_EXCP;                                  \
+      return;                                                 \
+  }
+
 void _decode_opc(DisasContext * ctx)
 {
 #if 0
@@ -474,13 +487,11 @@
 	gen_clr_t();
 	return;
     case 0x0038:		/* ldtlb */
-#if defined(CONFIG_USER_ONLY)
-	assert(0);		/* XXXXX */
-#else
+        CHECK_PRIVILEGED
 	tcg_gen_helper_0_0(helper_ldtlb);
-#endif
 	return;
     case 0x002b:		/* rte */
+        CHECK_PRIVILEGED
 	CHECK_NOT_DELAY_SLOT
 	tcg_gen_mov_i32(cpu_sr, cpu_ssr);
 	tcg_gen_mov_i32(cpu_delayed_pc, cpu_spc);
@@ -504,12 +515,8 @@
     case 0x0009:		/* nop */
 	return;
     case 0x001b:		/* sleep */
-	if (ctx->memidx) {
-		tcg_gen_helper_0_0(helper_sleep);
-	} else {
-		tcg_gen_helper_0_0(helper_raise_illegal_instruction);
-		ctx->bstate = BS_EXCP;
-	}
+        CHECK_PRIVILEGED
+	tcg_gen_helper_0_1(helper_sleep, tcg_const_i32(ctx->pc + 2));
 	return;
     }
 
@@ -1350,16 +1357,20 @@
 
     switch (ctx->opcode & 0xf08f) {
     case 0x408e:		/* ldc Rm,Rn_BANK */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(ALTREG(B6_4), REG(B11_8));
 	return;
     case 0x4087:		/* ldc.l @Rm+,Rn_BANK */
+        CHECK_PRIVILEGED
 	tcg_gen_qemu_ld32s(ALTREG(B6_4), REG(B11_8), ctx->memidx);
 	tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
 	return;
     case 0x0082:		/* stc Rm_BANK,Rn */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(REG(B11_8), ALTREG(B6_4));
 	return;
     case 0x4083:		/* stc.l Rm_BANK,@-Rn */
+        CHECK_PRIVILEGED
 	{
 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
@@ -1407,11 +1418,13 @@
 	ctx->flags |= DELAY_SLOT;
 	ctx->delayed_pc = (uint32_t) - 1;
 	return;
-    case 0x400e:		/* lds Rm,SR */
+    case 0x400e:		/* ldc Rm,SR */
+        CHECK_PRIVILEGED
 	tcg_gen_andi_i32(cpu_sr, REG(B11_8), 0x700083f3);
 	ctx->bstate = BS_STOP;
 	return;
-    case 0x4007:		/* lds.l @Rm+,SR */
+    case 0x4007:		/* ldc.l @Rm+,SR */
+        CHECK_PRIVILEGED
 	{
 	    TCGv val = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_qemu_ld32s(val, REG(B11_8), ctx->memidx);
@@ -1421,10 +1434,12 @@
 	    ctx->bstate = BS_STOP;
 	}
 	return;
-    case 0x0002:		/* sts SR,Rn */
+    case 0x0002:		/* stc SR,Rn */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(REG(B11_8), cpu_sr);
 	return;
-    case 0x4003:		/* sts SR,@-Rn */
+    case 0x4003:		/* stc SR,@-Rn */
+        CHECK_PRIVILEGED
 	{
 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
@@ -1433,18 +1448,22 @@
 	    tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);
 	}
 	return;
-#define LDST(reg,ldnum,ldpnum,stnum,stpnum)			\
+#define LDST(reg,ldnum,ldpnum,stnum,stpnum,prechk)		\
   case ldnum:							\
+    prechk    							\
     tcg_gen_mov_i32 (cpu_##reg, REG(B11_8));			\
     return;							\
   case ldpnum:							\
+    prechk    							\
     tcg_gen_qemu_ld32s (cpu_##reg, REG(B11_8), ctx->memidx);	\
     tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);		\
     return;							\
   case stnum:							\
+    prechk    							\
     tcg_gen_mov_i32 (REG(B11_8), cpu_##reg);			\
     return;							\
   case stpnum:							\
+    prechk    							\
     {								\
 	TCGv addr = tcg_temp_new(TCG_TYPE_I32);			\
 	tcg_gen_subi_i32(addr, REG(B11_8), 4);			\
@@ -1453,15 +1472,15 @@
 	tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);		\
     }								\
     return;
-	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013)
-	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023)
-	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033)
-	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043)
-	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2)
-	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002)
-	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012)
-	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022)
-	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052)
+	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013, {})
+	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023, CHECK_PRIVILEGED)
+	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033, CHECK_PRIVILEGED)
+	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043, CHECK_PRIVILEGED)
+	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2, CHECK_PRIVILEGED)
+	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002, {})
+	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012, {})
+	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022, {})
+	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052, {})
     case 0x406a:		/* lds Rm,FPSCR */
 	tcg_gen_helper_0_1(helper_ld_fpscr, REG(B11_8));
 	ctx->bstate = BS_STOP;

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Blue Swirl]

On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> This patch adds check for all SH4 instructions which are
>  executed only in privileged mode.

The checks get the privileged mode status from translation context. In
theory, the same TB code block could be used in unprivileged and
privileged mode, so the status that was true at translation time may
no longer be correct at execution time. Of course normally kernel code
is not visible or executable to user processes.

The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
the SH part correctly, the flags copied from env->flags don't contain
the privileged mode bits, isn't that in env->sr & SR_MD?

Alternatively, the check could be made at execution time, but that's
less efficient.

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Shin-ichiro KAWASAKI]

Thank you for the comment!

Blue Swirl wrote:
> On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>> This patch adds check for all SH4 instructions which are
>>  executed only in privileged mode.
> 
> The checks get the privileged mode status from translation context. In
> theory, the same TB code block could be used in unprivileged and
> privileged mode, so the status that was true at translation time may
> no longer be correct at execution time. Of course normally kernel code
> is not visible or executable to user processes.

As you say, this patch has the restriction that you pointed out : the
generated TB cannot used for both unprivileged and privileged.
I guess the codes generated by tcg_gen_qemu_st/ld() have the same
restriction, because those tcg_gen functions take the argument QEMU memory
index flags, which is decided at translation time.  If it is true, the
restriction might be allowed for privilege check. 

> The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
> the SH part correctly, the flags copied from env->flags don't contain
> the privileged mode bits, isn't that in env->sr & SR_MD?

Right.  In target-sh4/translate.c:get_intermediate_code_internal(),
the value env->sr & SR_MD used to set ctx->memidx.
We can use ctx->memidx to check the privileged mode at translation time,
and can use env->sr to check at execution time.  Both implementation
can be done, I guess.

> Alternatively, the check could be made at execution time, but that's
> less efficient.

If QEMU means *quick* emulator, more efficient way seems proper,
so my current opinion is that privilege check should be done at
translation time.

Anyway I'm not yet sure about QEMU internal.
More comments will be welcome!

Best Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Blue Swirl]

On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> Thank you for the comment!
>
>  Blue Swirl wrote:
>
> > On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> >
> > > This patch adds check for all SH4 instructions which are
> > >  executed only in privileged mode.
> > >
> >
> > The checks get the privileged mode status from translation context. In
> > theory, the same TB code block could be used in unprivileged and
> > privileged mode, so the status that was true at translation time may
> > no longer be correct at execution time. Of course normally kernel code
> > is not visible or executable to user processes.
> >
>
>  As you say, this patch has the restriction that you pointed out : the
>  generated TB cannot used for both unprivileged and privileged.

Qemu will happily use the same TB for both modes, if the TB flags
match (cpu-exec.c):

    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
                 tb->flags != flags)) {
        tb = tb_find_slow(pc, cs_base, flags);
    }

>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>  restriction, because those tcg_gen functions take the argument QEMU memory
>  index flags, which is decided at translation time.  If it is true, the
>  restriction might be allowed for privilege check.

The loads and stores have the same problem, the generated code assumes
that the privilege mode stays the same as what it was during
translation.

> > The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
> > the SH part correctly, the flags copied from env->flags don't contain
> > the privileged mode bits, isn't that in env->sr & SR_MD?
> >
>
>  Right.  In
> target-sh4/translate.c:get_intermediate_code_internal(),
>  the value env->sr & SR_MD used to set ctx->memidx.
>  We can use ctx->memidx to check the privileged mode at translation time,
>  and can use env->sr to check at execution time.  Both implementation
>  can be done, I guess.

But ctx->memidx value will be accurate only if the TB flags contain
the SR_MD bit. Then if the bit is different, a new TB will be
generated using ctx-memidx that reflects the SR_MD bit.

> > Alternatively, the check could be made at execution time, but that's
> > less efficient.
> >
>
>  If QEMU means *quick* emulator, more efficient way seems proper,
>  so my current opinion is that privilege check should be done at
>  translation time.

Right.

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Shin-ichiro KAWASAKI]

Blue Swirl wrote:
> On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>> Thank you for the comment!
>>
>>  Blue Swirl wrote:
>>
>>> On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>>>
>>>> This patch adds check for all SH4 instructions which are
>>>>  executed only in privileged mode.
>>>>
>>> The checks get the privileged mode status from translation context. In
>>> theory, the same TB code block could be used in unprivileged and
>>> privileged mode, so the status that was true at translation time may
>>> no longer be correct at execution time. Of course normally kernel code
>>> is not visible or executable to user processes.
>>>
>>  As you say, this patch has the restriction that you pointed out : the
>>  generated TB cannot used for both unprivileged and privileged.
> 
> Qemu will happily use the same TB for both modes, if the TB flags
> match (cpu-exec.c):
> 
>     if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
>                  tb->flags != flags)) {
>         tb = tb_find_slow(pc, cs_base, flags);
>     }

Thanks!  I have not understood this point.  You mean,
the main problem is that the one TB can be reused for both modes, 
and to check if new translation for the TB is necessary or not,
flags should contain enough information taken from CPU status.

>>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>>  restriction, because those tcg_gen functions take the argument QEMU memory
>>  index flags, which is decided at translation time.  If it is true, the
>>  restriction might be allowed for privilege check.
> 
> The loads and stores have the same problem, the generated code assumes
> that the privilege mode stays the same as what it was during
> translation.

And the other problem is that loads/stores instructions (and privilege
instructions), cannot follow some CPU status changes within one TB.
This problem will be left considering the trade off with performance.

>>> The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
>>> the SH part correctly, the flags copied from env->flags don't contain
>>> the privileged mode bits, isn't that in env->sr & SR_MD?
>>>
>>  Right.  In
>> target-sh4/translate.c:get_intermediate_code_internal(),
>>  the value env->sr & SR_MD used to set ctx->memidx.
>>  We can use ctx->memidx to check the privileged mode at translation time,
>>  and can use env->sr to check at execution time.  Both implementation
>>  can be done, I guess.
> 
> But ctx->memidx value will be accurate only if the TB flags contain
> the SR_MD bit. Then if the bit is different, a new TB will be
> generated using ctx-memidx that reflects the SR_MD bit.

I see.  I made a patch to let TB flags contain SR_MD bit.
At that time, I reviewed any other status of CPU are referred at
translation time.  I found not only SR_MD, some other bits are
referred also : e.g. bits in floating point control registers.
The patch attached to this mail copy such bits into TB flags too.
I hope it reflect your advise enough, doesn't it?

Thanks!

regards,
Shin-ichiro KAWASAKI


Index: trunk/target-sh4/translate.c
===================================================================
--- trunk/target-sh4/translate.c	(revision 5205)
+++ trunk/target-sh4/translate.c	(working copy)
@@ -48,6 +48,12 @@
     int singlestep_enabled;
 } DisasContext;
 
+#if defined(CONFIG_USER_ONLY)
+#define IS_USER(ctx) 1
+#else
+#define IS_USER(ctx) (!(ctx->sr & SR_MD))
+#endif
+
 enum {
     BS_NONE     = 0, /* We go out of the TB without reaching a branch or an
                       * exception condition
@@ -448,6 +454,13 @@
   {tcg_gen_helper_0_0(helper_raise_slot_illegal_instruction); ctx->bstate = BS_EXCP; \
    return;}
 
+#define CHECK_PRIVILEGED                                      \
+  if (IS_USER(ctx)) {                                         \
+      tcg_gen_helper_0_0(helper_raise_illegal_instruction);   \
+      ctx->bstate = BS_EXCP;                                  \
+      return;                                                 \
+  }
+
 void _decode_opc(DisasContext * ctx)
 {
 #if 0
@@ -474,13 +487,11 @@
 	gen_clr_t();
 	return;
     case 0x0038:		/* ldtlb */
-#if defined(CONFIG_USER_ONLY)
-	assert(0);		/* XXXXX */
-#else
+        CHECK_PRIVILEGED
 	tcg_gen_helper_0_0(helper_ldtlb);
-#endif
 	return;
     case 0x002b:		/* rte */
+        CHECK_PRIVILEGED
 	CHECK_NOT_DELAY_SLOT
 	tcg_gen_mov_i32(cpu_sr, cpu_ssr);
 	tcg_gen_mov_i32(cpu_delayed_pc, cpu_spc);
@@ -504,12 +515,8 @@
     case 0x0009:		/* nop */
 	return;
     case 0x001b:		/* sleep */
-	if (ctx->memidx) {
-		tcg_gen_helper_0_0(helper_sleep);
-	} else {
-		tcg_gen_helper_0_0(helper_raise_illegal_instruction);
-		ctx->bstate = BS_EXCP;
-	}
+        CHECK_PRIVILEGED
+	tcg_gen_helper_0_1(helper_sleep, tcg_const_i32(ctx->pc + 2));
 	return;
     }
 
@@ -1350,16 +1357,20 @@
 
     switch (ctx->opcode & 0xf08f) {
     case 0x408e:		/* ldc Rm,Rn_BANK */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(ALTREG(B6_4), REG(B11_8));
 	return;
     case 0x4087:		/* ldc.l @Rm+,Rn_BANK */
+        CHECK_PRIVILEGED
 	tcg_gen_qemu_ld32s(ALTREG(B6_4), REG(B11_8), ctx->memidx);
 	tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
 	return;
     case 0x0082:		/* stc Rm_BANK,Rn */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(REG(B11_8), ALTREG(B6_4));
 	return;
     case 0x4083:		/* stc.l Rm_BANK,@-Rn */
+        CHECK_PRIVILEGED
 	{
 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
@@ -1407,11 +1418,13 @@
 	ctx->flags |= DELAY_SLOT;
 	ctx->delayed_pc = (uint32_t) - 1;
 	return;
-    case 0x400e:		/* lds Rm,SR */
+    case 0x400e:		/* ldc Rm,SR */
+        CHECK_PRIVILEGED
 	tcg_gen_andi_i32(cpu_sr, REG(B11_8), 0x700083f3);
 	ctx->bstate = BS_STOP;
 	return;
-    case 0x4007:		/* lds.l @Rm+,SR */
+    case 0x4007:		/* ldc.l @Rm+,SR */
+        CHECK_PRIVILEGED
 	{
 	    TCGv val = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_qemu_ld32s(val, REG(B11_8), ctx->memidx);
@@ -1421,10 +1434,12 @@
 	    ctx->bstate = BS_STOP;
 	}
 	return;
-    case 0x0002:		/* sts SR,Rn */
+    case 0x0002:		/* stc SR,Rn */
+        CHECK_PRIVILEGED
 	tcg_gen_mov_i32(REG(B11_8), cpu_sr);
 	return;
-    case 0x4003:		/* sts SR,@-Rn */
+    case 0x4003:		/* stc SR,@-Rn */
+        CHECK_PRIVILEGED
 	{
 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
@@ -1433,18 +1448,22 @@
 	    tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);
 	}
 	return;
-#define LDST(reg,ldnum,ldpnum,stnum,stpnum)			\
+#define LDST(reg,ldnum,ldpnum,stnum,stpnum,prechk)		\
   case ldnum:							\
+    prechk    							\
     tcg_gen_mov_i32 (cpu_##reg, REG(B11_8));			\
     return;							\
   case ldpnum:							\
+    prechk    							\
     tcg_gen_qemu_ld32s (cpu_##reg, REG(B11_8), ctx->memidx);	\
     tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);		\
     return;							\
   case stnum:							\
+    prechk    							\
     tcg_gen_mov_i32 (REG(B11_8), cpu_##reg);			\
     return;							\
   case stpnum:							\
+    prechk    							\
     {								\
 	TCGv addr = tcg_temp_new(TCG_TYPE_I32);			\
 	tcg_gen_subi_i32(addr, REG(B11_8), 4);			\
@@ -1453,15 +1472,15 @@
 	tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);		\
     }								\
     return;
-	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013)
-	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023)
-	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033)
-	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043)
-	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2)
-	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002)
-	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012)
-	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022)
-	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052)
+	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013, {})
+	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023, CHECK_PRIVILEGED)
+	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033, CHECK_PRIVILEGED)
+	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043, CHECK_PRIVILEGED)
+	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2, CHECK_PRIVILEGED)
+	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002, {})
+	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012, {})
+	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022, {})
+	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052, {})
     case 0x406a:		/* lds Rm,FPSCR */
 	tcg_gen_helper_0_1(helper_ld_fpscr, REG(B11_8));
 	ctx->bstate = BS_STOP;
Index: trunk/cpu-exec.c
===================================================================
--- trunk/cpu-exec.c	(revision 5205)
+++ trunk/cpu-exec.c	(working copy)
@@ -209,7 +209,10 @@
     cs_base = 0;
     pc = env->pc;
 #elif defined(TARGET_SH4)
-    flags = env->flags;
+    flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
+                    | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))   /* Bits  0- 3 */
+            | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 */
+            | (env->sr & (SR_MD | SR_RB));                     /* Bits 29-30 */
     cs_base = 0;
     pc = env->pc;
 #elif defined(TARGET_ALPHA)

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Aurelien Jarno]

On Mon, Sep 15, 2008 at 10:38:34AM +0900, Shin-ichiro KAWASAKI wrote:
> Blue Swirl wrote:
>> On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>>> Thank you for the comment!
>>>
>>>  Blue Swirl wrote:
>>>
>>>> On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>>>>
>>>>> This patch adds check for all SH4 instructions which are
>>>>>  executed only in privileged mode.
>>>>>
>>>> The checks get the privileged mode status from translation context. In
>>>> theory, the same TB code block could be used in unprivileged and
>>>> privileged mode, so the status that was true at translation time may
>>>> no longer be correct at execution time. Of course normally kernel code
>>>> is not visible or executable to user processes.
>>>>
>>>  As you say, this patch has the restriction that you pointed out : the
>>>  generated TB cannot used for both unprivileged and privileged.
>>
>> Qemu will happily use the same TB for both modes, if the TB flags
>> match (cpu-exec.c):
>>
>>     if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
>>                  tb->flags != flags)) {
>>         tb = tb_find_slow(pc, cs_base, flags);
>>     }
>
> Thanks!  I have not understood this point.  You mean,
> the main problem is that the one TB can be reused for both modes, and to 
> check if new translation for the TB is necessary or not,
> flags should contain enough information taken from CPU status.
>
>>>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>>>  restriction, because those tcg_gen functions take the argument QEMU memory
>>>  index flags, which is decided at translation time.  If it is true, the
>>>  restriction might be allowed for privilege check.
>>
>> The loads and stores have the same problem, the generated code assumes
>> that the privilege mode stays the same as what it was during
>> translation.
>
> And the other problem is that loads/stores instructions (and privilege
> instructions), cannot follow some CPU status changes within one TB.
> This problem will be left considering the trade off with performance.
>
>>>> The TB flags are handled in cpu-exec.c:tb_find_fast(). If I understand
>>>> the SH part correctly, the flags copied from env->flags don't contain
>>>> the privileged mode bits, isn't that in env->sr & SR_MD?
>>>>
>>>  Right.  In
>>> target-sh4/translate.c:get_intermediate_code_internal(),
>>>  the value env->sr & SR_MD used to set ctx->memidx.
>>>  We can use ctx->memidx to check the privileged mode at translation time,
>>>  and can use env->sr to check at execution time.  Both implementation
>>>  can be done, I guess.
>>
>> But ctx->memidx value will be accurate only if the TB flags contain
>> the SR_MD bit. Then if the bit is different, a new TB will be
>> generated using ctx-memidx that reflects the SR_MD bit.
>
> I see.  I made a patch to let TB flags contain SR_MD bit.
> At that time, I reviewed any other status of CPU are referred at
> translation time.  I found not only SR_MD, some other bits are
> referred also : e.g. bits in floating point control registers.
> The patch attached to this mail copy such bits into TB flags too.
> I hope it reflect your advise enough, doesn't it?


Applied, thanks!

> Thanks!
>
> regards,
> Shin-ichiro KAWASAKI
>
>
> Index: trunk/target-sh4/translate.c
> ===================================================================
> --- trunk/target-sh4/translate.c	(revision 5205)
> +++ trunk/target-sh4/translate.c	(working copy)
> @@ -48,6 +48,12 @@
>     int singlestep_enabled;
> } DisasContext;
>
> +#if defined(CONFIG_USER_ONLY)
> +#define IS_USER(ctx) 1
> +#else
> +#define IS_USER(ctx) (!(ctx->sr & SR_MD))
> +#endif
> +
> enum {
>     BS_NONE     = 0, /* We go out of the TB without reaching a branch or an
>                       * exception condition
> @@ -448,6 +454,13 @@
>   {tcg_gen_helper_0_0(helper_raise_slot_illegal_instruction); ctx->bstate = BS_EXCP; \
>    return;}
>
> +#define CHECK_PRIVILEGED                                      \
> +  if (IS_USER(ctx)) {                                         \
> +      tcg_gen_helper_0_0(helper_raise_illegal_instruction);   \
> +      ctx->bstate = BS_EXCP;                                  \
> +      return;                                                 \
> +  }
> +
> void _decode_opc(DisasContext * ctx)
> {
> #if 0
> @@ -474,13 +487,11 @@
> 	gen_clr_t();
> 	return;
>     case 0x0038:		/* ldtlb */
> -#if defined(CONFIG_USER_ONLY)
> -	assert(0);		/* XXXXX */
> -#else
> +        CHECK_PRIVILEGED
> 	tcg_gen_helper_0_0(helper_ldtlb);
> -#endif
> 	return;
>     case 0x002b:		/* rte */
> +        CHECK_PRIVILEGED
> 	CHECK_NOT_DELAY_SLOT
> 	tcg_gen_mov_i32(cpu_sr, cpu_ssr);
> 	tcg_gen_mov_i32(cpu_delayed_pc, cpu_spc);
> @@ -504,12 +515,8 @@
>     case 0x0009:		/* nop */
> 	return;
>     case 0x001b:		/* sleep */
> -	if (ctx->memidx) {
> -		tcg_gen_helper_0_0(helper_sleep);
> -	} else {
> -		tcg_gen_helper_0_0(helper_raise_illegal_instruction);
> -		ctx->bstate = BS_EXCP;
> -	}
> +        CHECK_PRIVILEGED
> +	tcg_gen_helper_0_1(helper_sleep, tcg_const_i32(ctx->pc + 2));
> 	return;
>     }
>
> @@ -1350,16 +1357,20 @@
>
>     switch (ctx->opcode & 0xf08f) {
>     case 0x408e:		/* ldc Rm,Rn_BANK */
> +        CHECK_PRIVILEGED
> 	tcg_gen_mov_i32(ALTREG(B6_4), REG(B11_8));
> 	return;
>     case 0x4087:		/* ldc.l @Rm+,Rn_BANK */
> +        CHECK_PRIVILEGED
> 	tcg_gen_qemu_ld32s(ALTREG(B6_4), REG(B11_8), ctx->memidx);
> 	tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
> 	return;
>     case 0x0082:		/* stc Rm_BANK,Rn */
> +        CHECK_PRIVILEGED
> 	tcg_gen_mov_i32(REG(B11_8), ALTREG(B6_4));
> 	return;
>     case 0x4083:		/* stc.l Rm_BANK,@-Rn */
> +        CHECK_PRIVILEGED
> 	{
> 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
> 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
> @@ -1407,11 +1418,13 @@
> 	ctx->flags |= DELAY_SLOT;
> 	ctx->delayed_pc = (uint32_t) - 1;
> 	return;
> -    case 0x400e:		/* lds Rm,SR */
> +    case 0x400e:		/* ldc Rm,SR */
> +        CHECK_PRIVILEGED
> 	tcg_gen_andi_i32(cpu_sr, REG(B11_8), 0x700083f3);
> 	ctx->bstate = BS_STOP;
> 	return;
> -    case 0x4007:		/* lds.l @Rm+,SR */
> +    case 0x4007:		/* ldc.l @Rm+,SR */
> +        CHECK_PRIVILEGED
> 	{
> 	    TCGv val = tcg_temp_new(TCG_TYPE_I32);
> 	    tcg_gen_qemu_ld32s(val, REG(B11_8), ctx->memidx);
> @@ -1421,10 +1434,12 @@
> 	    ctx->bstate = BS_STOP;
> 	}
> 	return;
> -    case 0x0002:		/* sts SR,Rn */
> +    case 0x0002:		/* stc SR,Rn */
> +        CHECK_PRIVILEGED
> 	tcg_gen_mov_i32(REG(B11_8), cpu_sr);
> 	return;
> -    case 0x4003:		/* sts SR,@-Rn */
> +    case 0x4003:		/* stc SR,@-Rn */
> +        CHECK_PRIVILEGED
> 	{
> 	    TCGv addr = tcg_temp_new(TCG_TYPE_I32);
> 	    tcg_gen_subi_i32(addr, REG(B11_8), 4);
> @@ -1433,18 +1448,22 @@
> 	    tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);
> 	}
> 	return;
> -#define LDST(reg,ldnum,ldpnum,stnum,stpnum)			\
> +#define LDST(reg,ldnum,ldpnum,stnum,stpnum,prechk)		\
>   case ldnum:							\
> +    prechk    							\
>     tcg_gen_mov_i32 (cpu_##reg, REG(B11_8));			\
>     return;							\
>   case ldpnum:							\
> +    prechk    							\
>     tcg_gen_qemu_ld32s (cpu_##reg, REG(B11_8), ctx->memidx);	\
>     tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);		\
>     return;							\
>   case stnum:							\
> +    prechk    							\
>     tcg_gen_mov_i32 (REG(B11_8), cpu_##reg);			\
>     return;							\
>   case stpnum:							\
> +    prechk    							\
>     {								\
> 	TCGv addr = tcg_temp_new(TCG_TYPE_I32);			\
> 	tcg_gen_subi_i32(addr, REG(B11_8), 4);			\
> @@ -1453,15 +1472,15 @@
> 	tcg_gen_subi_i32(REG(B11_8), REG(B11_8), 4);		\
>     }								\
>     return;
> -	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013)
> -	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023)
> -	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033)
> -	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043)
> -	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2)
> -	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002)
> -	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012)
> -	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022)
> -	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052)
> +	LDST(gbr,  0x401e, 0x4017, 0x0012, 0x4013, {})
> +	LDST(vbr,  0x402e, 0x4027, 0x0022, 0x4023, CHECK_PRIVILEGED)
> +	LDST(ssr,  0x403e, 0x4037, 0x0032, 0x4033, CHECK_PRIVILEGED)
> +	LDST(spc,  0x404e, 0x4047, 0x0042, 0x4043, CHECK_PRIVILEGED)
> +	LDST(dbr,  0x40fa, 0x40f6, 0x00fa, 0x40f2, CHECK_PRIVILEGED)
> +	LDST(mach, 0x400a, 0x4006, 0x000a, 0x4002, {})
> +	LDST(macl, 0x401a, 0x4016, 0x001a, 0x4012, {})
> +	LDST(pr,   0x402a, 0x4026, 0x002a, 0x4022, {})
> +	LDST(fpul, 0x405a, 0x4056, 0x005a, 0x4052, {})
>     case 0x406a:		/* lds Rm,FPSCR */
> 	tcg_gen_helper_0_1(helper_ld_fpscr, REG(B11_8));
> 	ctx->bstate = BS_STOP;
> Index: trunk/cpu-exec.c
> ===================================================================
> --- trunk/cpu-exec.c	(revision 5205)
> +++ trunk/cpu-exec.c	(working copy)
> @@ -209,7 +209,10 @@
>     cs_base = 0;
>     pc = env->pc;
> #elif defined(TARGET_SH4)
> -    flags = env->flags;
> +    flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
> +                    | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))   /* Bits  0- 3 */
> +            | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 */
> +            | (env->sr & (SR_MD | SR_RB));                     /* Bits 29-30 */
>     cs_base = 0;
>     pc = env->pc;
> #elif defined(TARGET_ALPHA)
>
>
>

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Blue Swirl]

On 9/15/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> Blue Swirl wrote:
>
> > On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> >
> > > Thank you for the comment!
> > >
> > >  Blue Swirl wrote:
> > >
> > >
> > > > On 9/14/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> > > >
> > > >
> > > > > This patch adds check for all SH4 instructions which are
> > > > >  executed only in privileged mode.
> > > > >
> > > > >
> > > > The checks get the privileged mode status from translation context. In
> > > > theory, the same TB code block could be used in unprivileged and
> > > > privileged mode, so the status that was true at translation time may
> > > > no longer be correct at execution time. Of course normally kernel code
> > > > is not visible or executable to user processes.
> > > >
> > > >
> > >  As you say, this patch has the restriction that you pointed out : the
> > >  generated TB cannot used for both unprivileged and privileged.
> > >
> >
> > Qemu will happily use the same TB for both modes, if the TB flags
> > match (cpu-exec.c):
> >
> >    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> >                 tb->flags != flags)) {
> >        tb = tb_find_slow(pc, cs_base, flags);
> >    }
> >
>
>  Thanks!  I have not understood this point.  You mean,
>  the main problem is that the one TB can be reused for both modes, and to
> check if new translation for the TB is necessary or not,
>  flags should contain enough information taken from CPU status.

Right.

> > >  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
> > >  restriction, because those tcg_gen functions take the argument QEMU
> memory
> > >  index flags, which is decided at translation time.  If it is true, the
> > >  restriction might be allowed for privilege check.
> > >
> >
> > The loads and stores have the same problem, the generated code assumes
> > that the privilege mode stays the same as what it was during
> > translation.
> >
>
>  And the other problem is that loads/stores instructions (and privilege
>  instructions), cannot follow some CPU status changes within one TB.
>  This problem will be left considering the trade off with performance.

The loads/stores will use the ctx->memidx, but that's fine as long as
ctx->memidx is accurate. To ensure this, the supervisor/user bits of
the CPU status may not change during the TB, all instructions that
write to those bits must end the TB. Maybe the other instructions are
fine, but how about  'rte'?

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Shin-ichiro KAWASAKI]

Blue Swirl wrote:
>>>>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>>>>  restriction, because those tcg_gen functions take the argument QEMU
>> memory
>>>>  index flags, which is decided at translation time.  If it is true, the
>>>>  restriction might be allowed for privilege check.
>>>>
>>> The loads and stores have the same problem, the generated code assumes
>>> that the privilege mode stays the same as what it was during
>>> translation.
>>>
>>  And the other problem is that loads/stores instructions (and privilege
>>  instructions), cannot follow some CPU status changes within one TB.
>>  This problem will be left considering the trade off with performance.
> 
> The loads/stores will use the ctx->memidx, but that's fine as long as
> ctx->memidx is accurate. To ensure this, the supervisor/user bits of
> the CPU status may not change during the TB, all instructions that
> write to those bits must end the TB. Maybe the other instructions are
> fine, but how about  'rte'?

'rte' might cause a problem, but it will be a quite rare case, I guess.
Other instructions have problems too.  Those problems seems more important.

'rte' is one of the branch instructions, and it is used to return from
exception handling.  It has a delay slot.  So at the end of TB, delay slot
instruction is placed, and 'rte' is placed just in front of it.  If 'rte'
changes supervisor/user bits, it seems that the instruction in the delay
slot should follow the change. 

But, I found that SH4 manual says that 'rte' has a restriction, that
no exception allowed to happen in the delay slot.  Because memory
load/store instructions can cause TLB exception, I guess most OSes
does not place memory load/store instructions in delay slot.  SH-Linux
places 'nop' in the delay slot.  I'm not sure about other OSes.
How about to check what kind of instruction is in the delay slot of 'rte'?


By the way, special load instructions for SR ('ldc  Rm,SR' and 'ldc  @Rm+,SR'),
can change supervisor/user bits.  Though I guess SH-Linux does not use it to
modify supervisor/user bits, it might be a problem for other OSes.

Similar problems happen for status of floating point unit.  The instructions
'lds Rm,FPSCR', 'lds @Rm+,FPSCR', 'frchg', and 'fschg', might change the
status, and confuse the translated codes.  I guess this will happen so often
on SH-Linux.

Will it be a solution to cut the TB just after these special load instructions?


Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Blue Swirl]

On 9/16/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
> Blue Swirl wrote:
>
> >
> > >
> > > >
> > > > >  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
> > > > >  restriction, because those tcg_gen functions take the argument QEMU
> > > > >
> > > >
> > > memory
> > >
> > > >
> > > > >  index flags, which is decided at translation time.  If it is true,
> the
> > > > >  restriction might be allowed for privilege check.
> > > > >
> > > > >
> > > > The loads and stores have the same problem, the generated code assumes
> > > > that the privilege mode stays the same as what it was during
> > > > translation.
> > > >
> > > >
> > >  And the other problem is that loads/stores instructions (and privilege
> > >  instructions), cannot follow some CPU status changes within one TB.
> > >  This problem will be left considering the trade off with performance.
> > >
> >
> > The loads/stores will use the ctx->memidx, but that's fine as long as
> > ctx->memidx is accurate. To ensure this, the supervisor/user bits of
> > the CPU status may not change during the TB, all instructions that
> > write to those bits must end the TB. Maybe the other instructions are
> > fine, but how about  'rte'?
> >
>
>  'rte' might cause a problem, but it will be a quite rare case, I guess.
>  Other instructions have problems too.  Those problems seems more important.
>
>  'rte' is one of the branch instructions, and it is used to return from
>  exception handling.  It has a delay slot.  So at the end of TB, delay slot
>  instruction is placed, and 'rte' is placed just in front of it.  If 'rte'
>  changes supervisor/user bits, it seems that the instruction in the delay
>  slot should follow the change.
>  But, I found that SH4 manual says that 'rte' has a restriction, that
>  no exception allowed to happen in the delay slot.  Because memory
>  load/store instructions can cause TLB exception, I guess most OSes
>  does not place memory load/store instructions in delay slot.  SH-Linux
>  places 'nop' in the delay slot.  I'm not sure about other OSes.
>  How about to check what kind of instruction is in the delay slot of 'rte'?

I guess the TB will end after the delay slot. If that is the case,
only the delay slot instruction after 'rte' will be executed with user
permissions. Is this in line with the manual?

>  By the way, special load instructions for SR ('ldc  Rm,SR' and 'ldc
> @Rm+,SR'),
>  can change supervisor/user bits.  Though I guess SH-Linux does not use it
> to
>  modify supervisor/user bits, it might be a problem for other OSes.
>
>  Similar problems happen for status of floating point unit.  The
> instructions
>  'lds Rm,FPSCR', 'lds @Rm+,FPSCR', 'frchg', and 'fschg', might change the
>  status, and confuse the translated codes.  I guess this will happen so
> often
>  on SH-Linux.
>
>  Will it be a solution to cut the TB just after these special load
> instructions?

That is the safest way, though I don't know what the bits mean. Maybe
some of the instructions can't change the interesting bits.

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Shin-ichiro KAWASAKI]

Blue Swirl wrote:
> On 9/16/08, Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp> wrote:
>> Blue Swirl wrote:
>>
>>>>>>  I guess the codes generated by tcg_gen_qemu_st/ld() have the same
>>>>>>  restriction, because those tcg_gen functions take the argument QEMU
>>>>>>
>>>> memory
>>>>
>>>>>>  index flags, which is decided at translation time.  If it is true,
>> the
>>>>>>  restriction might be allowed for privilege check.
>>>>>>
>>>>>>
>>>>> The loads and stores have the same problem, the generated code assumes
>>>>> that the privilege mode stays the same as what it was during
>>>>> translation.
>>>>>
>>>>>
>>>>  And the other problem is that loads/stores instructions (and privilege
>>>>  instructions), cannot follow some CPU status changes within one TB.
>>>>  This problem will be left considering the trade off with performance.
>>>>
>>> The loads/stores will use the ctx->memidx, but that's fine as long as
>>> ctx->memidx is accurate. To ensure this, the supervisor/user bits of
>>> the CPU status may not change during the TB, all instructions that
>>> write to those bits must end the TB. Maybe the other instructions are
>>> fine, but how about  'rte'?
>>>
>>  'rte' might cause a problem, but it will be a quite rare case, I guess.
>>  Other instructions have problems too.  Those problems seems more important.
>>
>>  'rte' is one of the branch instructions, and it is used to return from
>>  exception handling.  It has a delay slot.  So at the end of TB, delay slot
>>  instruction is placed, and 'rte' is placed just in front of it.  If 'rte'
>>  changes supervisor/user bits, it seems that the instruction in the delay
>>  slot should follow the change.
>>  But, I found that SH4 manual says that 'rte' has a restriction, that
>>  no exception allowed to happen in the delay slot.  Because memory
>>  load/store instructions can cause TLB exception, I guess most OSes
>>  does not place memory load/store instructions in delay slot.  SH-Linux
>>  places 'nop' in the delay slot.  I'm not sure about other OSes.
>>  How about to check what kind of instruction is in the delay slot of 'rte'?
> 
> I guess the TB will end after the delay slot. If that is the case,
> only the delay slot instruction after 'rte' will be executed with user
> permissions. Is this in line with the manual?

Oh, I reviewed the manual again, and found it.

 The manual is 'SH7751 Series SH7751, SH7751R Hardware Manual'.
(It could be found with searching 'e602201_sh7751.pdf'). In section
7.1, there's a description on the status of 'rte' delay slot. 
It says,
 - instruction access is done with status before modification (supervisor mode, I guess)
 - data acess is done with status after modification (user mode)

>>  By the way, special load instructions for SR ('ldc  Rm,SR' and 'ldc
>> @Rm+,SR'),
>>  can change supervisor/user bits.  Though I guess SH-Linux does not use it
>> to
>>  modify supervisor/user bits, it might be a problem for other OSes.
>>
>>  Similar problems happen for status of floating point unit.  The
>> instructions
>>  'lds Rm,FPSCR', 'lds @Rm+,FPSCR', 'frchg', and 'fschg', might change the
>>  status, and confuse the translated codes.  I guess this will happen so
>> often
>>  on SH-Linux.
>>
>>  Will it be a solution to cut the TB just after these special load
>> instructions?
> 
> That is the safest way, though I don't know what the bits mean. Maybe
> some of the instructions can't change the interesting bits.

Those instructions can change any bits in SR or FPSCR.
SR includes supervisor/user mode bits SR_MD, which influences ctx->memidx.
FPSCR includes the bits which decides,
 - the size of fp calculation, 32bit or 64 bit  (SZ bit and PR bit)
 - bank of register used for fp calculation (FR bit)
Current implementation in "target-sh4/translate.c" refer those
conditions at translation time, and generates the code correspond
to them.


I hope my explanation is clear enough.
Thanks for your response!

Regards,
Shin-ichiro KAWASAKI

Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions

[Paul Mundt]

On Wed, Sep 17, 2008 at 01:44:30AM +0900, Shin-ichiro KAWASAKI wrote:
> By the way, special load instructions for SR ('ldc  Rm,SR' and 'ldc  
> @Rm+,SR'),
> can change supervisor/user bits.  Though I guess SH-Linux does not use it to
> modify supervisor/user bits, it might be a problem for other OSes.
> 
Correct. The supervisor bit is set primarily on the initial SR setup and
largely ignored after that. Other things, like SR_FD, are modified very
regularly. We also see similar behaviour on parts with the SR_DSP bit,
which has roughly the same semantics.

> Similar problems happen for status of floating point unit.  The instructions
> 'lds Rm,FPSCR', 'lds @Rm+,FPSCR', 'frchg', and 'fschg', might change the
> status, and confuse the translated codes.  I guess this will happen so often
> on SH-Linux.
> 
Those FPU instructions are used primarily in save/restore paths of FPU
state in the lazy context switching code. In order to test this, you need
multiple processes that are using the FPU. If you permit the compiler to
emit floating point instructions, this will include basically every
process in the system, so it becomes a very frequent thing.