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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/03/17 11:03, 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.

And this probably is the root of the problem.  CMPXCHG on a contended
location should be observed to fail from the guests point of view.

>
> There is a severe downside to MMIO accesses with this model
> though: Other than for RAM, we shouldn't be reading (and even
> less so writing) the memory location more than once.

With MMIO, the hardware analogy is sending a single PCIe transaction
with the atomic flag set.

However, atomic options on non-RAM locations tend not to actually be
atomic on real hardware anyway, so so-long as we don't make multiple
reads/writes, the lack of atomicity won't be a problem in practice.

>
>> The picture is further complicated by the two-part handling of
>> instructions in x86_emulate(). For example, for CMPXCHG we have:
>> [...]
>> I now see that this is why using a spinlock only around the writeback
>> part did not solve the issue: both the compare part and the writeback
>> part need to be part of the same atomic operation - lock needs to be
>> aquired before the cmp / ZF part.
> No exactly: RMW insns require the lock to be acquired ahead of
> the memory read, and dropped after the memory write.
>
>> Opinions and suggestions are appreciated. If I'm not mistaken, it looks
>> like the smp_lock design is the better solution to the problem.
> Andrew has told me that the re-work of how we do memory accesses
> in the emulator is pretty high on his priority list now. Whether we'd
> want to introduce an interim solution therefore depends on the time
> scale to possibly reach the only ultimately correct solution (no longer
> acting on intermediate copies of the data coming from guest memory,
> which is only correct for plain reads and plain writes).

This re-enforces my opinion that mapping the destination and pointing a
stub at the mapping is the only viable option.

~Andrew

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