Re: [PATCH RFC V7 1/5] xen: Emulate with no writes

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/26/2014 05:40 PM, Jan Beulich wrote:
>>>> On 26.08.14 at 16:30, <rcojocaru@bitdefender.com> wrote:
>> On 08/26/2014 05:19 PM, Jan Beulich wrote:
>>>>>> On 26.08.14 at 16:01, <rcojocaru@bitdefender.com> wrote:
>>>> On 08/26/2014 04:56 PM, Jan Beulich wrote:
>>>>>>>> On 13.08.14 at 17:28, <rcojocaru@bitdefender.com> wrote:
>>>>>> +void hvm_emulate_one_full(bool_t nowrite, unsigned int trapnr,
>>>>>> +    unsigned int errcode)
>>>>>> +{
>>>>>> +    struct hvm_emulate_ctxt ctx = {{ 0 }};
>>>>>> +    int rc;
>>>>>> +
>>>>>> +    hvm_emulate_prepare(&ctx, guest_cpu_user_regs());
>>>>>> +
>>>>>> +    if ( nowrite )
>>>>>> +        rc = hvm_emulate_one_no_write(&ctx);
>>>>>> +    else
>>>>>> +        rc = hvm_emulate_one(&ctx);
>>>>>> +
>>>>>> +    switch ( rc )
>>>>>> +    {
>>>>>> +    case X86EMUL_UNHANDLEABLE:
>>>>>> +        gdprintk(XENLOG_DEBUG, "Emulation failed @ %04x:%lx: "
>>>>>> +               "%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
>>>>>> +               hvmemul_get_seg_reg(x86_seg_cs, &ctx)->sel,
>>>>>> +               ctx.insn_buf_eip,
>>>>>> +               ctx.insn_buf[0], ctx.insn_buf[1],
>>>>>> +               ctx.insn_buf[2], ctx.insn_buf[3],
>>>>>> +               ctx.insn_buf[4], ctx.insn_buf[5],
>>>>>> +               ctx.insn_buf[6], ctx.insn_buf[7],
>>>>>> +               ctx.insn_buf[8], ctx.insn_buf[9]);
>>>>>> +        hvm_inject_hw_exception(trapnr, errcode);
>>>>>> +        break;
>>>>>> +    case X86EMUL_EXCEPTION:
>>>>>> +        if ( ctx.exn_pending )
>>>>>> +            hvm_inject_hw_exception(ctx.exn_vector, ctx.exn_error_code);
>>>>>> +        break;
>>>>>
>>>>> Shouldn't you act on X86EMUL_RETRY here? Or at least not fall through
>>>>> to the writeback below?
>>>>
>>>> Thanks for the review, I did initially loop around hvm_emulate_one()
>>>> until rc != X86EMUL_RETRY, but I've been told that that might block
>>>> against time calibration rendezvous points.
>>>
>>> In any event it strikes me as odd that you ignore that state
>>> altogether rather than propagating it back up, so that someone
>>> in suitable position to do the retry can invoke it.
>>
>> Since it's being called in the context of handling a mem_event response,
>> the X86EMUL_RETRY case would lead to a retry anyway (since we couldn't
>> emulate the current instruction, and we haven't lifted the page access
>> restrictions). So if we've failed to somehow modify the guest's EIP, the
>> instruction will hit the page again, cause a new mem_event and a new
>> attempt to emulate it - so that would seem to fit with the spirit of
>> X86EMUL_RETRY.
> 
> Makes sense. Please add a brief comment to this effect when you
> add this specific case (bailing without writeback). One thing to
> consider though is which function you're in: Based on its name it
> has no connection to the specific mem-access use, and hence - with
> the behavior you intend to have here not being generically usable -
> renaming the function may be a good idea.

Will do, thank you very much for your comments!


Thanks,
Razvan Cojocaru