Re: [PATCH 19/24] KVM: x86 emulator: fix in/out emulation.

[Gleb Natapov <gleb@redhat.com>]

On Tue, Mar 09, 2010 at 04:47:24PM +0200, Avi Kivity wrote:
> On 03/09/2010 04:09 PM, Gleb Natapov wrote:
> >in/out emulation is broken now. The breakage is different depending
> >on where IO device resides. If it is in userspace emulator reports
> >emulation failure since it incorrectly interprets kvm_emulate_pio()
> >return value. If IO device is in the kernel emulation of 'in' will do
> >nothing since kvm_emulate_pio() stores result directly into vcpu
> >registers, so emulator will overwrite result of emulation during
> >commit of shadowed register.
> >
> >
> >index def4877..315e8a8 100644
> >--- a/arch/x86/kvm/svm.c
> >+++ b/arch/x86/kvm/svm.c
> >@@ -1488,29 +1488,9 @@ static int shutdown_interception(struct vcpu_svm *svm)
> >
> >  static int io_interception(struct vcpu_svm *svm)
> >  {
> >-	u32 io_info = svm->vmcb->control.exit_info_1; /* address size bug? */
> >-	int size, in, string;
> >-	unsigned port;
> >-
> >  	++svm->vcpu.stat.io_exits;
> >
> >-	svm->next_rip = svm->vmcb->control.exit_info_2;
> >-
> >-	string = (io_info&  SVM_IOIO_STR_MASK) != 0;
> >-
> >-	if (string) {
> >-		if (emulate_instruction(&svm->vcpu,
> >-					0, 0, 0) == EMULATE_DO_MMIO)
> >-			return 0;
> >-		return 1;
> >-	}
> >-
> >-	in = (io_info&  SVM_IOIO_TYPE_MASK) != 0;
> >-	port = io_info>>  16;
> >-	size = (io_info&  SVM_IOIO_SIZE_MASK)>>  SVM_IOIO_SIZE_SHIFT;
> >-
> >-	skip_emulated_instruction(&svm->vcpu);
> >-	return kvm_emulate_pio(&svm->vcpu, in, size, port);
> >+	return !(emulate_instruction(&svm->vcpu, 0, 0, 0) == EMULATE_DO_MMIO);
> >  }
> 
> We don't want to enter the emulator for non-string in/out.  Leftover
> test code?
> 
No, unfortunately this is not leftover. I just don't see a way how we
can bypass emulator and still have emulator be able to emulate in/out
(for big real mode for instance). The problem is basically described in
the commit message. If we have function outside of emulator that does
in/out emulation on vcpu directly, then emulator can't  use it since
committing shadowed registers will overwrite the result of emulation.
Having two different emulations (one outside of emulator and another in
emulator) is also problematic since when userspace returns after IO exit
we don't know which emulation to continue. If we want to avoid
instruction decoding we can fill in emulation context from exit info as
if instruction was already decoded and call emulator.

--
			Gleb.