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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.17 at 17:49, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 03/29/2017 06:04 PM, Razvan Cojocaru wrote:
>> On 03/29/2017 05:00 PM, Razvan Cojocaru wrote:
>>> On 03/29/2017 04:55 PM, Jan Beulich wrote:
>>>>>>> On 28.03.17 at 12:50, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> On 03/28/2017 01:47 PM, Jan Beulich wrote:
>>>>>>>>> On 28.03.17 at 12:27, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>> On 03/28/2017 01:03 PM, Jan Beulich wrote:
>>>>>>>>>>> On 28.03.17 at 11:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>>>> I'm not sure that the RETRY model is what the guest OS expects. 
>>>>>>>>> AFAIK, a
>>>>>>>>> failed CMPXCHG should happen just once, with the proper registers and 
>>>>>>>>> ZF
>>>>>>>>> set. The guest surely expects neither that the instruction resume 
>>>>>>>>> until
>>>>>>>>> it succeeds, nor that some hidden loop goes on for an undeterminate
>>>>>>>>> ammount of time until a CMPXCHG succeeds.
>>>>>>>>
>>>>>>>> The guest doesn't observe the CMPXCHG failing - RETRY leads to
>>>>>>>> the instruction being restarted instead of completed.
>>>>>>>
>>>>>>> Indeed, but it works differently with hvm_emulate_one_vm_event() where
>>>>>>> RETRY currently would have the instruction be re-executed (properly
>>>>>>> re-executed, not just re-emulated) by the guest.
>>>>>>
>>>>>> Right - see my other reply to Andrew: The function likely would
>>>>>> need to tell apart guest CMPXCHG uses from us using the insn to
>>>>>> carry out the write by some other one. That may involve
>>>>>> adjustments to the memory write logic in x86_emulate() itself, as
>>>>>> the late failure of the comparison then would also need to be
>>>>>> communicated back (via ZF clear) to the guest.
>>>>>
>>>>> Exactly, it would require quite some reworking of x86_emulate().
>>>>
>>>> I had imagined it to be less intrusive (outside of x86_emulate()),
>>>> but I've now learned why Andrew was able to get rid of
>>>> X86EMUL_CMPXCHG_FAILED - the apparently intended behavior
>>>> was never implemented. Attached a first take at it, which has
>>>> seen smoke testing, but nothing more. The way it ends up being
>>>> I don't think this can reasonably be considered for 4.9 at this
>>>> point in time. (Also Cc-ing Tim for the shadow code changes,
>>>> even if this isn't really a proper patch submission.)
>>>
>>> Thanks! I'll give a spin with a modified version of my CMPXCHG patch as
>>> soon as possible.
>> 
>> With the attached patch with hvmemul_cmpxchg() now returning
>> X86EMUL_CMPXCHG_FAILED if __cmpxchg() fails my (32-bit) Windows 7 guest
>> gets stuck at the "Starting Windows" screen.

That's with or without monitoring in use? I specifically did try a
32-bit Win7 guest, and I didn't have an issue. But then again a
single run may not mean much.

> And again this change:
> 
> 1162     if ( __cmpxchg(map, old, new, bytes) != old )
> 1163     {
> 1164         memcpy(p_old, map, bytes);
> 1165         rc = X86EMUL_CMPXCHG_FAILED;
> 1166     }
> 
> i.e. doing the accumulator <- destination part of a failed CMPXCHG which
> might be missing from your patch leads me again to BSODs.

Missing from my patch? Why and/or where? It's not clear to me which
function the above code fragment is supposed to go into. I might
guess hvmemul_cmpxchg(), but then my patch doesn't alter its
behavior (from forwarding to hvmeml_write()), and hence I don't
see why my patch would need to do such an adjustment.

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.

Jan


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