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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 03/30/2017 03:05 PM, Jan Beulich wrote:
>>>> On 29.03.17 at 17:49, <rcojocaru@bitdefender.com> 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@bitdefender.com> wrote:
>>>>>> On 03/28/2017 01:47 PM, Jan Beulich wrote:
>>>>>>>>>> On 28.03.17 at 12:27, <rcojocaru@bitdefender.com> wrote:
>>>>>>>> On 03/28/2017 01:03 PM, Jan Beulich wrote:
>>>>>>>>>>>> On 28.03.17 at 11:14, <rcojocaru@bitdefender.com> 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.

With monitoring in use - specifically using hvm_emulate_one_vm_event().
Sorry for the ommision.

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

Right, I was thinking about this bit:

6704         if ( _regs.eflags & X86_EFLAGS_ZF )
6705             dst.type = OP_NONE;
6706         else
6707         {
6708             /* Failure: write the value we saw to EAX. */
6709             dst.type = OP_REG;
6710             dst.reg  = (unsigned long *)&_regs.r(ax);
6711         }

For some reason I had missed it, but I now see it does the writeback. My
mistake.

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

True, I'll update my tests.


Thanks,
Razvan

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