Re: [Xen-devel] [PATCH RFC] x86/emulate: implement hvmemul_cmpxchg() with an actual CMPXCHG

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/30/2017 05:21 PM, Jan Beulich wrote:
>>>> On 30.03.17 at 16:08, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 03/30/2017 03:56 PM, Razvan Cojocaru wrote:
>>> On 03/30/2017 03:05 PM, Jan Beulich wrote:
>>>> What I do note though is that you don't copy back the value
>>>> __cmpxchg() returns, yet that's what is needed. *map may
>>>> have changed again already.
>>>
>>> Changing the code to:
>>>
>>> 1162     ret = __cmpxchg(map, old, new, bytes);
>>> 1163
>>> 1164     if ( ret != old )
>>> 1165     {
>>> 1166         memcpy(p_old, &ret, bytes);
>>> 1167         rc = X86EMUL_CMPXCHG_FAILED;
>>> 1168     }
>>>
>>> where ret is an unsigned long still triggers BSODs when I add my patch
>>> to yours. I'll need to dig deeper.
>>
>> Nevermind, I've found the culprit: hvm_emulate_one_vm_event()'s code
>> needs to be wrapped in a loop that checks for X86EMUL_RETRY again, since
>> hvmemul_cmpxchg() may return RETRY even for some mapping problems, in
>> which case we again end up with the guest trying to re-execute an
>> emulable CMPXCHG.
> 
> This seems wrong to me - note how my patch changes behavior
> regarding the return value from paging_cmpxchg_guest_entry()
> in ptwr_emulated_update().
> 
>> However, this gets me back to my original problem when I "solved" it in
>> the same manner (looping until emulation succeeds) back when
>> hvmemul_cmpxchg() failures were reported with RETRY: eventually the
>> guest BSODs with code 101. RETRY failures are still possible coming from
>> the hvmemul_vaddr_to_mfn() code in my patch.
>>
>> I wonder if I should just return X86EMUL_CMPXCHG_FAILED for all those as
>> well and just never end up returning RETRY from hvmemul_cmpxchg().
> 
> That would seem similarly wrong to me - it ought to be
> UNHANDLEABLE, I would think. In any event, never returning
> RETRY would also be wrong in case you hit emulated MMIO, but
> that's really the only case where RETRY should be passed back.

Sorry, I don't follow: hvm_emulate_one_vm_event() calls
hvm_emulate_one() and then does this with the return value:

switch ( rc )
{
case X86EMUL_RETRY:
    /*
     * This function is called when handling an EPT-related vm_event
     * reply. As such, nothing else needs to be done here, since simply
     * returning makes the current instruction cause a page fault again,
     * consistent with X86EMUL_RETRY.
     */
    return;
case X86EMUL_UNHANDLEABLE:
    hvm_dump_emulation_state(XENLOG_G_DEBUG "Mem event", &ctx);
    hvm_inject_hw_exception(trapnr, errcode);
    break;
case X86EMUL_EXCEPTION:
    if ( ctx.ctxt.event_pending )
        hvm_inject_event(&ctx.ctxt.event);
    break;
}

hvm_emulate_writeback(&ctx);

If I'd return UNHANDLEABLE from hvmemul_cmpxchg() where I'm now
returning RETRY from hvmemul_vaddr_to_mfn(), that would inject
TRAP_invalid_op in the guest, so it seems rather harsh. And returning
RETRY here does nothing, so the guest will resume at the same IP
(re-trying CMPXCHG, causing a page fault (or crashing the guest), and
re-trying the emulation (if it hasn't)). Please see hvm_do_resume() in
arch/x86/hvm/hvm.c.

Speaking of emulated MMIO, I've got this when the guest was crashing
immediately (pre RETRY loop):

 MMIO emulation failed: d3v8 32bit @ 0008:82679f3c -> f0 0f ba 30 00 72
07 8b cb e8 da 4b ff ff 8b 45

Again, this is all with guest monitoring enabled.


Thanks,
Razvan

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