Re: [PATCH 19/24] KVM: x86 emulator: fix in/out emulation.
On 03/09/2010 08:09 PM, Gleb Natapov wrote: 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. Alternatively, another entry point would be fine. in/out is a fast path (used for virtio for example). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/24] KVM: x86 emulator: fix in/out emulation.
On Wed, Mar 10, 2010 at 11:12:34AM +0200, Avi Kivity wrote: On 03/09/2010 08:09 PM, Gleb Natapov wrote: 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. Alternatively, another entry point would be fine. in/out is a fast path (used for virtio for example). You mean another entry point into emulator, not separate implementation for emulated in/out and intercepted one. If yes this is what I mean by faking decoding stage. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 19/24] KVM: x86 emulator: fix in/out emulation.
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. Signed-off-by: Gleb Natapov g...@redhat.com --- arch/x86/include/asm/kvm_emulate.h |7 ++ arch/x86/kvm/emulate.c | 17 ++-- arch/x86/kvm/svm.c | 22 + arch/x86/kvm/vmx.c | 19 +--- arch/x86/kvm/x86.c | 203 +--- 5 files changed, 139 insertions(+), 129 deletions(-) diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h index 4268330..7d323d5 100644 --- a/arch/x86/include/asm/kvm_emulate.h +++ b/arch/x86/include/asm/kvm_emulate.h @@ -119,6 +119,13 @@ struct x86_emulate_ops { const void *new, unsigned int bytes, struct kvm_vcpu *vcpu); + + int (*pio_in_emulated)(int size, unsigned short port, void *val, + unsigned int count, struct kvm_vcpu *vcpu); + + int (*pio_out_emulated)(int size, unsigned short port, const void *val, + unsigned int count, struct kvm_vcpu *vcpu); + bool (*get_cached_descriptor)(struct desc_struct *desc, int seg, struct kvm_vcpu *vcpu); void (*set_cached_descriptor)(struct desc_struct *desc, diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 81ecf47..0ec7b9b 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -208,12 +208,12 @@ static u32 opcode_table[256] = { 0, 0, 0, 0, 0, 0, 0, 0, /* 0xE0 - 0xE7 */ 0, 0, 0, 0, - ByteOp | SrcImmUByte, SrcImmUByte, + ByteOp | SrcImmUByte | DstAcc, SrcImmUByte | DstAcc, ByteOp | SrcImmUByte, SrcImmUByte, /* 0xE8 - 0xEF */ SrcImm | Stack, SrcImm | ImplicitOps, SrcImmU | Src2Imm16 | No64, SrcImmByte | ImplicitOps, - SrcNone | ByteOp | ImplicitOps, SrcNone | ImplicitOps, + SrcNone | ByteOp | DstAcc, SrcNone | DstAcc, SrcNone | ByteOp | ImplicitOps, SrcNone | ImplicitOps, /* 0xF0 - 0xF7 */ 0, 0, 0, 0, @@ -2915,12 +2915,13 @@ special_insn: kvm_inject_gp(ctxt-vcpu, 0); goto done; } - if (kvm_emulate_pio(ctxt-vcpu, io_dir_in, - (c-d ByteOp) ? 1 : c-op_bytes, - port) != 0) { - c-eip = saved_eip; - goto cannot_emulate; - } + if (io_dir_in) + ops-pio_in_emulated((c-d ByteOp) ? 1 : c-op_bytes, +port, c-dst.val, 1, ctxt-vcpu); + else + ops-pio_out_emulated((c-d ByteOp) ? 1 : c-op_bytes, + port, c-regs[VCPU_REGS_RAX], 1, + ctxt-vcpu); break; case 0xf4: /* hlt */ ctxt-vcpu-arch.halt_request = 1; diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c 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); } static int nmi_interception(struct vcpu_svm *svm) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ae3217d..7f33d8e 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2974,26 +2974,9 @@ static int handle_triple_fault(struct kvm_vcpu *vcpu) static int handle_io(struct kvm_vcpu *vcpu) { - unsigned long exit_qualification; - int size, in, string; - unsigned port; - ++vcpu-stat.io_exits; -
Re: [PATCH 19/24] KVM: x86 emulator: fix in/out emulation.
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? static int nmi_interception(struct vcpu_svm *svm) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ae3217d..7f33d8e 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2974,26 +2974,9 @@ static int handle_triple_fault(struct kvm_vcpu *vcpu) static int handle_io(struct kvm_vcpu *vcpu) { - unsigned long exit_qualification; - int size, in, string; - unsigned port; - ++vcpu-stat.io_exits; - exit_qualification = vmcs_readl(EXIT_QUALIFICATION); - string = (exit_qualification 16) != 0; - if (string) { - if (emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO) - return 0; - return 1; - } - - size = (exit_qualification 7) + 1; - in = (exit_qualification 8) != 0; - port = exit_qualification 16; - - skip_emulated_instruction(vcpu); - return kvm_emulate_pio(vcpu, in, size, port); + return !(emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DO_MMIO); } Ditto. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/24] KVM: x86 emulator: fix in/out emulation.
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. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html