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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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.

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. I'm not sure
if __cmpxchg() should work differently and do this atomically, or if
this should be done in x86_emulate() and it's not, or if it is done
there somewhere I've missed in the first patch.


Thanks,
Razvan


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