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

[Shin-ichiro KAWASAKI]

Blue Swirl wrote:
> On 9/14/08, Shin-ichiro KAWASAKI <address@hidden> wrote:
> > Thank you for the comment!
> >
> >  Blue Swirl wrote:
> >
> > > On 9/14/08, Shin-ichiro KAWASAKI <address@hidden> 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)