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

[Jia Liu <proljc@gmail.com>]

Hi Blue,

On Sun, Jul 15, 2012 at 3:54 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Jul 15, 2012 at 12:39 AM, Jia Liu <proljc@gmail.com> wrote:
>> Hi Blue
>>
>> On Sat, Jul 14, 2012 at 10:42 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sat, Jul 14, 2012 at 2:08 PM, Jia Liu <proljc@gmail.com> wrote:
>>>> Hi Max
>>>>
>>>> On Sat, Jul 14, 2012 at 9:49 PM, Max Filippov <jcmvbkbc@gmail.com> 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?

Thank you! I think return is a better way.

>
>> #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?
>

I'll make a check, thank you.

>>
>>
>>>>
>>>>>>
>>>>>>     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

Regards,
Jia