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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.16 at 23:06, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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).

Okay.

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

The former I'll need to check carefully, but I don't expect the
existing handlers to have an issue with count being 1. However
I think the idea not to use the rep handler in that case was that
the other hook would be cheaper, so I'm not sure we really want
to change this universally. Yet I couldn't think of an elegant way
to have the caller control the desired behavior here.

The latter I think already gets taken care of by put_rep_prefix().

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

Yes, but this case so far was of no interest (and hence not
implemented that way), since the way the HVM emulation code
would only ever return X86EMUL_OKAY when doing a partial
access (due to clipping count suitably before doing the first
iteration, which is not a model we can easily use in the PV case).

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

Okay, I'll work towards that.

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

Okay.

Jan


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