Re: [Qemu-devel] [PATCH v9 11/15] target-or32: Add system instructions

[Blue Swirl]

On Sun, Jul 15, 2012 at 12:39 AM, Jia Liu <address@hidden> wrote:
> Hi Blue
>
> On Sat, Jul 14, 2012 at 10:42 PM, Blue Swirl <address@hidden> wrote:
>> On Sat, Jul 14, 2012 at 2:08 PM, Jia Liu <address@hidden> wrote:
>>> Hi Max
>>>
>>> On Sat, Jul 14, 2012 at 9:49 PM, Max Filippov <address@hidden> wrote:
>>>>>> I don't think so, please check for example target-ppc/translate.c:4192
>>>>>> on how supervisor only mfsr is handled there.
>>>>>>
>>>>>
>>>>> Thank you for comment, Blue.
>>>>>
>>>>> is this code OK?
>>>>
>>>> Shouldn't there also be an exception in softmmu mode
>>>> if the CPU is not in supervisor mode?
>>>>
>>>
>>> Sorry, I...
>>> May you give me more comment? I'm not sure about this.
>>
>> If a user tries to execute a supervisor instruction (only allowed for
>> kernel level code, not applications), the instruction won't be
>> executed but an exception will be raised.
>>
>> This is the PPC mfsr instruction part of target-ppc/translate.c:
>> /* mfsr */
>> static void gen_mfsr(DisasContext *ctx)
>> {
>> #if defined(CONFIG_USER_ONLY)
>>     gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
>> #else
>>     TCGv t0;
>>     if (unlikely(!ctx->mem_idx)) {
>>
>> Here the MMU mode is checked for user mode. I'd use more explicit
>> check ctx->mmu_idx == MMU_USER_IDX.
>>
>>         gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
>>         return;
>>     }
>>     t0 = tcg_const_tl(SR(ctx->opcode));
>>     gen_helper_load_sr(cpu_gpr[rD(ctx->opcode)], cpu_env, t0);
>>     tcg_temp_free(t0);
>> #endif
>> }
>>
>
> Thank you very much for this nice example.
> Is this code OK?
>     case 0x2d:    /* l.mfspr */
>         LOG_DIS("l.mfspr r%d, r%d, %d\n", rd, ra, I16);
>         {
> #if defined(CONFIG_USER_ONLY)
>             gen_illegal_exception(dc);
> #else
>             TCGv_i32 ti = tcg_const_i32(I16);
>             if (dc->mem_idx == MMU_USER_IDX) {
>                 gen_illegal_exception(dc);
>                 return;
>             }
>             gen_helper_mfspr(cpu_R[rd], cpu_env, cpu_R[rd], cpu_R[ra], ti);
>             tcg_temp_free_i32(ti);
> #endif
>         }
>         break;
>
>     case 0x30:    /* l.mtspr */
>         LOG_DIS("l.mtspr %d, r%d, r%d, %d\n", I5, ra, rb, I11);
>         {
> #if defined(CONFIG_USER_ONLY)
>             gen_illegal_exception(dc);

Maybe return?

> #else
>             TCGv_i32 im = tcg_const_i32(tmp);
>             if (dc->mem_idx == MMU_USER_IDX) {
>                 gen_illegal_exception(dc);
>                 return;
>             }
>             gen_helper_mtspr(cpu_env, cpu_R[ra], cpu_R[rb], im);
>             tcg_temp_free_i32(im);
> #endif
>         }
>         break;


Otherwise looks OK. Perhaps there are other supervisor mode only
instructions which need this kind of checks?

>
>
>>>
>>>>>
>>>>>     case 0x2d:    /* l.mfspr */
>>>>>         LOG_DIS("l.mfspr r%d, r%d, %d\n", rd, ra, I16);
>>>>>         {
>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>             gen_illegal_exception(dc);
>>>>> #else
>>>>>             TCGv_i32 ti = tcg_const_i32(I16);
>>>>>             gen_helper_mfspr(cpu_R[rd], cpu_env, cpu_R[rd], cpu_R[ra], 
>>>>> ti);
>>>>>             tcg_temp_free_i32(ti);
>>>>> #endif
>>>>>         }
>>>>>         break;
>>>>>
>>>>>     case 0x30:    /* l.mtspr */
>>>>>         LOG_DIS("l.mtspr %d, r%d, r%d, %d\n", I5, ra, rb, I11);
>>>>>         {
>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>             gen_illegal_exception(dc);
>>>>> #else
>>>>>             TCGv_i32 im = tcg_const_i32(tmp);
>>>>>             gen_helper_mtspr(cpu_env, cpu_R[ra], cpu_R[rb], im);
>>>>>             tcg_temp_free_i32(im);
>>>>> #endif
>>>>>         }
>>>>>         break;
>>>>>
>>>>
>>>> --
>>>> Thanks.
>>>> -- Max
>>>
>>> Regards,
>>> Jia.
>
> Regards,
> Jia