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

2010-03-10 Thread Avi Kivity 

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.

2010-03-10 Thread Gleb Natapov 
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.

2010-03-09 Thread Gleb Natapov 
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.

2010-03-09 Thread Avi Kivity 

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.

2010-03-09 Thread Gleb Natapov 
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