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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/26/2014 05:19 PM, Jan Beulich wrote:
>>>> On 26.08.14 at 16:01, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 08/26/2014 04:56 PM, Jan Beulich wrote:
>>>>>> On 13.08.14 at 17:28, <rcojocaru@xxxxxxxxxxxxxxx> 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.


Thanks,
Razvan Cojocaru

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel