From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kf9m3-0003oD-2t for qemu-devel@nongnu.org; Mon, 15 Sep 2008 04:49:35 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kf9m2-0003mu-3F for qemu-devel@nongnu.org; Mon, 15 Sep 2008 04:49:34 -0400 Received: from [199.232.76.173] (port=35408 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kf9m1-0003md-Ox for qemu-devel@nongnu.org; Mon, 15 Sep 2008 04:49:33 -0400 Received: from hall.aurel32.net ([91.121.138.14]:53435) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Kf9m1-0000IT-45 for qemu-devel@nongnu.org; Mon, 15 Sep 2008 04:49:33 -0400 Received: from volta.aurel32.net ([2002:52e8:2fb:1:21e:8cff:feb0:693b]) by hall.aurel32.net with esmtpsa (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1Kf9ly-0007U4-N6 for qemu-devel@nongnu.org; Mon, 15 Sep 2008 10:49:31 +0200 Received: from aurel32 by volta.aurel32.net with local (Exim 4.69) (envelope-from ) id 1Kf9lx-0007pW-Tw for qemu-devel@nongnu.org; Mon, 15 Sep 2008 10:49:29 +0200 Date: Mon, 15 Sep 2008 10:49:29 +0200 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions Message-ID: <20080915084929.GE9801@volta.aurel32.net> References: <48CC8D3E.1040401@juno.dti.ne.jp> <48CCE727.7000203@juno.dti.ne.jp> <48CDBC9A.2070504@juno.dti.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <48CDBC9A.2070504@juno.dti.ne.jp> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Mon, Sep 15, 2008 at 10:38:34AM +0900, Shin-ichiro KAWASAKI wrote: > Blue Swirl wrote: >> On 9/14/08, Shin-ichiro KAWASAKI wrote: >>> Thank you for the comment! >>> >>> Blue Swirl wrote: >>> >>>> On 9/14/08, Shin-ichiro KAWASAKI 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