Re: [PATCH v2 15/16] x86/PV: use generic emulator for privileged instruction handling

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

On 28/09/16 09:18, Jan Beulich wrote:
> There's a new emulator return code being added to allow bypassing
> certain operations (see the code comment). Its handling in the epilogue
> code involves moving the raising of the single step trap until after
> registers were updated. This should probably have been that way from
> the beginning, to allow the inject_hw_exception() hook to see updated
> register state (in case it cares) - it's a trap, after all.

I agree.  (However, given the complexity of this patch, it really would
be better to split changes like the #DB handling out into a separate patch).

>
> The other small tweak to the emulator is to single iteration handling
> of INS and OUTS: Since we don't want to handle any other memory access
> instructions, we want these to be handled by the rep_ins() / rep_outs()
> hooks here too. The read() / write() hook pointers get checked for that
> purpose.

Moving the non-rep INS/OUTS instructions into rep_ins/outs() (perhaps
with dropping the rep_ prefix from the callback names) seems sensible.

However, making this implicit on a check against the read/write hooks
doesn't seem sensible.  Anyone looking at the code is going to get
thoroughly confused.

Can't we make the ins/outs hook deal properly with a rep of 1, and have
x86_emulate() know not to update %ecx in this case?

>
> And finally handling of exceptions gets changed for REP INS / REP OUTS:
> If the hook return X86EMUL_EXCEPTION, register state will still get
> updated if some iterations have been performed (but the rIP update will
> get suppressed if not all of them did get handled).

Isn't this what happens on real hardware anyway?

>  While on the HVM side
> the VA -> LA -> PA translation process clips the number of repetitions,
> doing so would unduly complicate the PV side code being added here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> One thing to be considered is that despite avoiding the handling of
> memory reads and writes (other than for INS and OUTS) the set of insns
> now getting potentially handled by the emulator is much larger than
> before. A possible solution to this would be a new hook to be called
> between decode and execution stages, allowing further restrictions to
> be enforced. Of course this could easily be a follow-up patch, as the
> one here is quite big already.

I think this would be a very sensible precaution.  I would suggest even
that this patch doesn't get committed without being adjacent to such a
patch.

>
> Another thing to consider is to the extend the X86EMUL_EXCEPTION
> handling change mentioned above to other string instructions. In that
> case this should probably be broken out into a prereq patch.

Yes.

~Andrew

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