Re: [RFC v18 08/15] i386: split smm helper (softmmu)

[Claudio Fontana <cfontana@suse.de>]

On 2/15/21 3:39 PM, Claudio Fontana wrote:
> On 2/15/21 3:13 PM, Paolo Bonzini wrote:
>> On 15/02/21 15:05, Claudio Fontana wrote:
>>> On 2/15/21 2:30 PM, Paolo Bonzini wrote:
>>>> On 15/02/21 13:59, Claudio Fontana wrote:
>>>>> Yes. The difference between before the patch and after the patch 
>>>>> is that before we were still going through all the code in
>>>>> tcg_gen_callN, via the call to gen_helper_rsm macro, only to call
>>>>> finally an empty function for CONFIG_USER_ONLY (helper_rsm()
>>>>> {}),
>>>>>
>>>>> while now we do not generate anything, we do not call the
>>>>> gen_helper_rsm macro at all, so we don't go through
>>>>> tcg_gen_callN.
>>>>>
>>>>
>>>> Can we even have an abort() for such cases?
>>>>
>>>> Paolo
>>>>
>>>
>>> Hi Paolo,
>>>
>>> where are you suggesting to have an abort()?
>>>
>>> You mean that we should abort() QEMU as soon as we detect in
>>> translate.c an RSM instruction in user-mode?
>>
>> Translating it is okay (it's just a guaranteed SIGILL), but I'm thinking
>> of aborting if s->flags & HF_SMM_MASK is true.  Likewise if we see
>> CPU_INTERRUPT_SMI.
>>
>> Paolo
>>
> 
> Ok, will rework as you suggest, thanks!


By the way, in the case of gen_bpt_io, is it a similar situation,
where we should abort in user-mode if we see s->flags & HF_IOBPT_MASK ?


static void gen_bpt_io(DisasContext *s, TCGv_i32 t_port, int ot)
{
#ifndef CONFIG_USER_ONLY
    if (s->flags & HF_IOBPT_MASK) {
        TCGv_i32 t_size = tcg_const_i32(1 << ot);
        TCGv t_next = tcg_const_tl(s->pc - s->cs_base);

        gen_helper_bpt_io(cpu_env, t_port, t_size, t_next);
        tcg_temp_free_i32(t_size);
        tcg_temp_free(t_next);
    }
#endif /* !CONFIG_USER_ONLY */
}



What about other cases like


        case 0xd8: /* VMRUN */

            if (!(s->flags & HF_SVME_MASK) || !s->pe) {
                goto illegal_op;
            }

          ...

            gen_helper_vmrun(cpu_env, tcg_const_i32(s->aflag - 1),
                             tcg_const_i32(s->pc - pc_start));


should we abort there as well if CONFIG_USER_ONLY?

And there are many more probably, should it be its own patch?

Ciao,

Claudio

> 
>>>
>>>     case 0x1aa: /* rsm */
>>>         gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM);
>>>         if (!(s->flags & HF_SMM_MASK))
>>>             goto illegal_op;
>>>         gen_update_cc_op(s);
>>>         gen_jmp_im(s, s->pc - s->cs_base);
>>> #ifndef CONFIG_USER_ONLY
>>>         gen_helper_rsm(cpu_env);
>>> #endif /* CONFIG_USER_ONLY */
>>>         gen_eob(s);
>>>         break;
>>
>>
> 
>