Re: [PATCH v2 25/30] KVM: x86 emulator: fix in/out emulation.

[Gleb Natapov <gleb@redhat.com>]

On Sun, Mar 14, 2010 at 06:54:11PM +0200, Avi Kivity wrote:
> On 03/14/2010 06:21 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.
> >
> >
> >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >index 8f5e4c8..344e17b 100644
> >--- a/arch/x86/kvm/emulate.c
> >+++ b/arch/x86/kvm/emulate.c
> >@@ -210,13 +210,13 @@ static u32 opcode_table[256] = {
> >  	0, 0, 0, 0, 0, 0, 0, 0,
> >  	/* 0xE0 - 0xE7 */
> >  	0, 0, 0, 0,
> >-	ByteOp | SrcImmUByte, SrcImmUByte,
> >-	ByteOp | SrcImmUByte, SrcImmUByte,
> >+	ByteOp | SrcImmUByte | DstAcc, SrcImmUByte | DstAcc,
> >+	ByteOp | SrcImmUByte | DstAcc, SrcImmUByte | DstAcc,
> 
> REX prefix shouldn't expand DstAcc to 64 bits here.  Might cause
> problems further down in the pipeline.+
Is REX prefix allowed with this opcodes?

If yes:

if (c->dst.bytes == 8)
	c->dst.bytes = 4;

inside IN/OUT emulation will fix this.

> >  	port = io_info>>  16;
> >  	size = (io_info&  SVM_IOIO_SIZE_MASK)>>  SVM_IOIO_SIZE_SHIFT;
> >+	svm->next_rip = svm->vmcb->control.exit_info_2;
> >+        skip_emulated_instruction(&svm->vcpu);
> 
> whitespace damage.
> 
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 3c5ffa2..37c6178 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -3351,6 +3351,86 @@ emul_write:
> >  	return emulator_write_emulated(addr, new, bytes, vcpu);
> >  }
> >
> >+static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
> >+{
> >+	/* TODO: String I/O for in kernel device */
> 
> ISTR this is actually needed for something.  Is this a regression?
> 
This patch does not add this function, just moves it. The function exists
currently in the source with this comment. With my patch series applied
string pio to kernel device should work without problems.

--
			Gleb.