Re: Possible issue with x86_emulate when writing results back to memory

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

On 09/01/14 16:07, Simon Graham wrote:
>>> Does this seem a reasonable explanation of the issue?
>> Yes - as long as you can also explain why a spin lock operation
>> would make it into the emulation code in the first place.
>>
> Well, that one is tough and I don't have a good answer... the only thing I would say is that in our system we ALWAYS have the shadow memory tracking enabled (to track changes to framebuffers)

Full shadow, or just logdirty?

With logdirty, frequent pagefaults will occur (which are costly in terms
of vmexits), but I would not expect emulation to occur.

Even with full shadow, emulation only kicks in for non-standard RAM,
which is basically IO to qemu, and instructions trying to write to the
pagetables themselves, which are trapped and emulated for safety reasons.

>
>>> The attached patch is for discussion purposes only - if it is deemed
>>> acceptable I'll resubmit a proper patch request against unstable.
>> I'd rather not add limited scope special casing like that, but instead
>> make the copying much more like real hardware (i.e. not just deal
>> with the 16- and 32-bit cases, and especially not rely on memcpy()
>> using 64-bit reads/writes when it can). IOW - don't use memcpy()
>> here at all (and have a single routine doing The Right Thing (tm)
>> rather than having two clones now, and perhaps more later on -
>> I'd in particular think that the read side in shadow code would also
>> need a similar adjustment).
> My concern was that memcpy is (I assume!) highly optimized - it certainly should be if it isn't and I would worry that a change to make it atomic for the purposes of instruction emulation would result in an across the board perf hit when in most cases it isn't necessary that it be atomic.
>
> This would be fine for the writeback code in the shadow module BUT the __hvm_copy routine is used generically in situations where atomicity is not required...

__hvm_copy() is probably too low to be thinking about this.  There are
many things such as grant_copy() which do not want "hardware like" copy
properties, preferring instead to have less overhead.

~Andrew