[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions |
Date: |
Mon, 15 Sep 2008 10:49:29 +0200 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
On Mon, Sep 15, 2008 at 10:38:34AM +0900, Shin-ichiro KAWASAKI wrote:
> 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?
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
`. `' address@hidden | address@hidden
`- people.debian.org/~aurel32 | www.aurel32.net
- [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Shin-ichiro KAWASAKI, 2008/09/14
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Blue Swirl, 2008/09/14
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Shin-ichiro KAWASAKI, 2008/09/14
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Blue Swirl, 2008/09/14
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Shin-ichiro KAWASAKI, 2008/09/14
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions,
Aurelien Jarno <=
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Blue Swirl, 2008/09/15
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Shin-ichiro KAWASAKI, 2008/09/16
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Blue Swirl, 2008/09/16
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Shin-ichiro KAWASAKI, 2008/09/16
- Re: [Qemu-devel] [PATCH] SH4: Privilege check for instructions, Paul Mundt, 2008/09/17