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

["Jan Beulich" <JBeulich@suse.com>]

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

Jan