[PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Yang, Sheng]

[-- Attachment #1: Type: text/plain, Size: 5194 bytes --]

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot 
survived more than ten seconds, then encounterd kernel panic. The basic 
concept of "crashme" is graduating random assembly code and trying to execute 
them in a fork process.

After some fix on emulator valid judgment, we found it's hard to get the 
current emulator handle the invalid instructions correctly, for the #UD trap 
for hypercall patching caused troubles. The problem is, if the opcode itself 
was OK, but combination of opcode and modrm_reg was invalid, and one operand 
of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory 
operand first rather than judged the validity, and may encounter error there. 
For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.

In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, 
then return from emulate_instruction() and inject a #UD to guest. With the 
patch, the guest had been run for more than 12 hours.

Signed-off-by: Feng (Eric) Liu <eric.e.liu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/kvm/svm.c         |    2 +-
 arch/x86/kvm/vmx.c         |    2 +-
 arch/x86/kvm/x86.c         |   18 +++++++++++++++---
 include/asm-x86/kvm_host.h |    4 +++-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 11e5baf..2da9e11 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -942,7 +942,7 @@ static int ud_interception(struct vcpu_svm *svm, struct 
kvm_run *kvm_run)
 {
 	int er;
 
-	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, 0);
+	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 	if (er != EMULATE_DONE)
 		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
 	return 1;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4741806..5346e42 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1862,7 +1862,7 @@ static int handle_exception(struct kvm_vcpu *vcpu, 
struct kvm_run *kvm_run)
 	}
 
 	if (is_invalid_opcode(intr_info)) {
-		er = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
+		er = emulate_instruction(vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 		if (er != EMULATE_DONE)
 			kvm_queue_exception(vcpu, UD_VECTOR);
 		return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c5b4825..2a659ce 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1840,9 +1840,10 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			struct kvm_run *run,
 			unsigned long cr2,
 			u16 error_code,
-			int no_decode)
+			int emulation_type)
 {
 	int r;
+	struct decode_cache *c;
 
 	vcpu->arch.mmio_fault_cr2 = cr2;
 	kvm_x86_ops->cache_regs(vcpu);
@@ -1850,7 +1851,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 	vcpu->mmio_is_write = 0;
 	vcpu->arch.pio.string = 0;
 
-	if (!no_decode) {
+	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
 		int cs_db, cs_l;
 		kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
 
@@ -1884,6 +1885,16 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 					get_segment_base(vcpu, VCPU_SREG_FS);
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+
+		/* Reject the instructions other than VMCALL/VMMCALL when
+		 * try to emulate invalid opcode */
+		c = &vcpu->arch.emulate_ctxt.decode;
+		if ((emulation_type & EMULTYPE_TRAP_UD) &&
+		    (!(c->twobyte && c->b == 0x01 &&
+		      (c->modrm_reg == 0 || c->modrm_reg == 3) &&
+		       c->modrm_mod == 3 && c->modrm_rm == 1)))
+			return EMULATE_FAIL;
+
 		++vcpu->stat.insn_emulation;
 		if (r)  {
 			++vcpu->stat.insn_emulation_fail;
@@ -2639,7 +2650,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
struct kvm_run *kvm_run)
 		vcpu->mmio_read_completed = 1;
 		vcpu->mmio_needed = 0;
 		r = emulate_instruction(vcpu, kvm_run,
-					vcpu->arch.mmio_fault_cr2, 0, 1);
+					vcpu->arch.mmio_fault_cr2, 0,
+					EMULTYPE_NO_DECODE);
 		if (r == EMULATE_DO_MMIO) {
 			/*
 			 * Read-modify-write.  Back to userspace.
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 20597bc..4702b04 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -415,8 +415,10 @@ enum emulation_result {
 	EMULATE_FAIL,         /* can't emulate this instruction */
 };
 
+#define EMULTYPE_NO_DECODE	    (1 << 0)
+#define EMULTYPE_TRAP_UD	    (1 << 1)
 int emulate_instruction(struct kvm_vcpu *vcpu, struct kvm_run *run,
-			unsigned long cr2, u16 error_code, int no_decode);
+			unsigned long cr2, u16 error_code, int emulation_type);
 void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char 
*context);
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
-- 
1.5.3.4


[-- Attachment #2: 0001-KVM-emulator-Only-allow-VMCALL-VMMCALL-trapped-by.patch --]
[-- Type: text/x-diff, Size: 5153 bytes --]

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them.

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there. For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.

In the patch, we simply checked that if the invalid opcode wasn't vmcall/vmmcall, then return from emulate_instruction() and inject a #UD to guest. With the patch, the guest had been running for more than 12 hours.

Signed-off-by: Feng (Eric) Liu <eric.e.liu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/kvm/svm.c         |    2 +-
 arch/x86/kvm/vmx.c         |    2 +-
 arch/x86/kvm/x86.c         |   18 +++++++++++++++---
 include/asm-x86/kvm_host.h |    4 +++-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 11e5baf..2da9e11 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -942,7 +942,7 @@ static int ud_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 {
 	int er;
 
-	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, 0);
+	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 	if (er != EMULATE_DONE)
 		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
 	return 1;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4741806..5346e42 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1862,7 +1862,7 @@ static int handle_exception(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	}
 
 	if (is_invalid_opcode(intr_info)) {
-		er = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
+		er = emulate_instruction(vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 		if (er != EMULATE_DONE)
 			kvm_queue_exception(vcpu, UD_VECTOR);
 		return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c5b4825..2a659ce 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1840,9 +1840,10 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			struct kvm_run *run,
 			unsigned long cr2,
 			u16 error_code,
-			int no_decode)
+			int emulation_type)
 {
 	int r;
+	struct decode_cache *c;
 
 	vcpu->arch.mmio_fault_cr2 = cr2;
 	kvm_x86_ops->cache_regs(vcpu);
@@ -1850,7 +1851,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 	vcpu->mmio_is_write = 0;
 	vcpu->arch.pio.string = 0;
 
-	if (!no_decode) {
+	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
 		int cs_db, cs_l;
 		kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
 
@@ -1884,6 +1885,16 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 					get_segment_base(vcpu, VCPU_SREG_FS);
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+
+		/* Reject the instructions other than VMCALL/VMMCALL when
+		 * try to emulate invalid opcode */
+		c = &vcpu->arch.emulate_ctxt.decode;
+		if ((emulation_type & EMULTYPE_TRAP_UD) &&
+		    (!(c->twobyte && c->b == 0x01 &&
+		      (c->modrm_reg == 0 || c->modrm_reg == 3) &&
+		       c->modrm_mod == 3 && c->modrm_rm == 1)))
+			return EMULATE_FAIL;
+
 		++vcpu->stat.insn_emulation;
 		if (r)  {
 			++vcpu->stat.insn_emulation_fail;
@@ -2639,7 +2650,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 		vcpu->mmio_read_completed = 1;
 		vcpu->mmio_needed = 0;
 		r = emulate_instruction(vcpu, kvm_run,
-					vcpu->arch.mmio_fault_cr2, 0, 1);
+					vcpu->arch.mmio_fault_cr2, 0,
+					EMULTYPE_NO_DECODE);
 		if (r == EMULATE_DO_MMIO) {
 			/*
 			 * Read-modify-write.  Back to userspace.
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 20597bc..4702b04 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -415,8 +415,10 @@ enum emulation_result {
 	EMULATE_FAIL,         /* can't emulate this instruction */
 };
 
+#define EMULTYPE_NO_DECODE	    (1 << 0)
+#define EMULTYPE_TRAP_UD	    (1 << 1)
 int emulate_instruction(struct kvm_vcpu *vcpu, struct kvm_run *run,
-			unsigned long cr2, u16 error_code, int no_decode);
+			unsigned long cr2, u16 error_code, int emulation_type);
 void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char *context);
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
 void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
-- 
1.5.3.4


[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Anthony Liguori]

Yang, Sheng wrote:
> From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Wed, 2 Jan 2008 14:49:22 +0800
> Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD
>
> When executing a test program called "crashme", we found the KVM guest cannot 
> survived more than ten seconds, then encounterd kernel panic. The basic 
> concept of "crashme" is graduating random assembly code and trying to execute 
> them in a fork process.
>
> After some fix on emulator valid judgment, we found it's hard to get the 
> current emulator handle the invalid instructions correctly, for the #UD trap 
> for hypercall patching caused troubles. The problem is, if the opcode itself 
> was OK, but combination of opcode and modrm_reg was invalid, and one operand 
> of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory 
> operand first rather than judged the validity, and may encounter error there. 
>   

Nice catch!

Regards,

Anthony Liguori

> For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.
>
> In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, 
> then return from emulate_instruction() and inject a #UD to guest. With the 
> patch, the guest had been run for more than 12 hours.
>
> Signed-off-by: Feng (Eric) Liu <eric.e.liu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/kvm/svm.c         |    2 +-
>  arch/x86/kvm/vmx.c         |    2 +-
>  arch/x86/kvm/x86.c         |   18 +++++++++++++++---
>  include/asm-x86/kvm_host.h |    4 +++-
>  4 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 11e5baf..2da9e11 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -942,7 +942,7 @@ static int ud_interception(struct vcpu_svm *svm, struct 
> kvm_run *kvm_run)
>  {
>  	int er;
>  
> -	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, 0);
> +	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
>  	if (er != EMULATE_DONE)
>  		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>  	return 1;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4741806..5346e42 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1862,7 +1862,7 @@ static int handle_exception(struct kvm_vcpu *vcpu, 
> struct kvm_run *kvm_run)
>  	}
>  
>  	if (is_invalid_opcode(intr_info)) {
> -		er = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
> +		er = emulate_instruction(vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
>  		if (er != EMULATE_DONE)
>  			kvm_queue_exception(vcpu, UD_VECTOR);
>  		return 1;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c5b4825..2a659ce 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1840,9 +1840,10 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  			struct kvm_run *run,
>  			unsigned long cr2,
>  			u16 error_code,
> -			int no_decode)
> +			int emulation_type)
>  {
>  	int r;
> +	struct decode_cache *c;
>  
>  	vcpu->arch.mmio_fault_cr2 = cr2;
>  	kvm_x86_ops->cache_regs(vcpu);
> @@ -1850,7 +1851,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  	vcpu->mmio_is_write = 0;
>  	vcpu->arch.pio.string = 0;
>  
> -	if (!no_decode) {
> +	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
>  		int cs_db, cs_l;
>  		kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
>  
> @@ -1884,6 +1885,16 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
>  					get_segment_base(vcpu, VCPU_SREG_FS);
>  
>  		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
> +
> +		/* Reject the instructions other than VMCALL/VMMCALL when
> +		 * try to emulate invalid opcode */
> +		c = &vcpu->arch.emulate_ctxt.decode;
> +		if ((emulation_type & EMULTYPE_TRAP_UD) &&
> +		    (!(c->twobyte && c->b == 0x01 &&
> +		      (c->modrm_reg == 0 || c->modrm_reg == 3) &&
> +		       c->modrm_mod == 3 && c->modrm_rm == 1)))
> +			return EMULATE_FAIL;
> +
>  		++vcpu->stat.insn_emulation;
>  		if (r)  {
>  			++vcpu->stat.insn_emulation_fail;
> @@ -2639,7 +2650,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
> struct kvm_run *kvm_run)
>  		vcpu->mmio_read_completed = 1;
>  		vcpu->mmio_needed = 0;
>  		r = emulate_instruction(vcpu, kvm_run,
> -					vcpu->arch.mmio_fault_cr2, 0, 1);
> +					vcpu->arch.mmio_fault_cr2, 0,
> +					EMULTYPE_NO_DECODE);
>  		if (r == EMULATE_DO_MMIO) {
>  			/*
>  			 * Read-modify-write.  Back to userspace.
> diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
> index 20597bc..4702b04 100644
> --- a/include/asm-x86/kvm_host.h
> +++ b/include/asm-x86/kvm_host.h
> @@ -415,8 +415,10 @@ enum emulation_result {
>  	EMULATE_FAIL,         /* can't emulate this instruction */
>  };
>  
> +#define EMULTYPE_NO_DECODE	    (1 << 0)
> +#define EMULTYPE_TRAP_UD	    (1 << 1)
>  int emulate_instruction(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -			unsigned long cr2, u16 error_code, int no_decode);
> +			unsigned long cr2, u16 error_code, int emulation_type);
>  void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char 
> *context);
>  void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>  void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);
>   
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> ------------------------------------------------------------------------
>
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dong, Eddie]

Anthony Liguori wrote:
> Yang, Sheng wrote:
>> From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00
>> 2001 
>> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Date: Wed, 2 Jan 2008 14:49:22 +0800
>> Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by
>> #UD 
>> 
>> When executing a test program called "crashme", we found the KVM
>> guest 
>> cannot survived more than ten seconds, then encounterd kernel panic.
>> The basic concept of "crashme" is graduating random assembly code and
>> trying to execute them in a fork process.
>> 
>> After some fix on emulator valid judgment, we found it's hard to get
>> the current emulator handle the invalid instructions correctly, for
>> the #UD trap for hypercall patching caused troubles. The problem is,
>> if the opcode itself was OK, but combination of opcode and modrm_reg
>> was invalid, and one operand of the opcode was memory(SrcMem or
>> DstMem), emulator would fetched the memory operand first rather than
>> judged the validity, and may encounter error there. 
>> 
> 
> Nice catch!
> 
> Regards,
> 
> Anthony Liguori
> 
Anthony:
	Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa.  BTW, what
is the nenefit to remove hypercall page, which provide more clean
approach IMO?
thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dor Laor]

[-- Attachment #1.1: Type: text/plain, Size: 2143 bytes --]

Dong, Eddie wrote:
> Anthony Liguori wrote:
>   
>> Yang, Sheng wrote:
>>     
>>> From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00
>>> 2001 
>>> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Date: Wed, 2 Jan 2008 14:49:22 +0800
>>> Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by
>>> #UD 
>>>
>>> When executing a test program called "crashme", we found the KVM
>>> guest 
>>> cannot survived more than ten seconds, then encounterd kernel panic.
>>> The basic concept of "crashme" is graduating random assembly code and
>>> trying to execute them in a fork process.
>>>
>>> After some fix on emulator valid judgment, we found it's hard to get
>>> the current emulator handle the invalid instructions correctly, for
>>> the #UD trap for hypercall patching caused troubles. The problem is,
>>> if the opcode itself was OK, but combination of opcode and modrm_reg
>>> was invalid, and one operand of the opcode was memory(SrcMem or
>>> DstMem), emulator would fetched the memory operand first rather than
>>> judged the validity, and may encounter error there. 
>>>
>>>       
>> Nice catch!
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>     
> Anthony:
> 	Actually I am wondering if the binary used for VMMCALL could be
> assumed will never be used by Intel processor or vice versa.  BTW, what
> is the nenefit to remove hypercall page, which provide more clean
> approach IMO?
> thx,eddie
>
>   
The benefit is that one can migrate a running guest from Intel to Amd 
and vise versa without
any effort  of the guest/host knowing that.
Can you offer other binary code for vmmcall that you're sure it won't be 
used in the future?
Regards,
Dor
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   


[-- Attachment #1.2: Type: text/html, Size: 3199 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Anthony Liguori]

Dong, Eddie wrote:
> Anthony:
> 	Actually I am wondering if the binary used for VMMCALL could be
> assumed will never be used by Intel processor or vice versa.  BTW, what
> is the nenefit to remove hypercall page, which provide more clean
> approach IMO?
>   

A hypercall page doesn't help the situation with respect to migration 
between an AMD and Intel system.  It was bad enough that Intel and AMD 
couldn't just agree on a common instruction in the first place.  It 
would just be mean if you reused the other guys VMCALL instruction for 
something else.  You guys can't be that mean, right ;-)

Regards,

Anthony Liguori

> thx,eddie
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dong, Eddie]

Anthony Liguori wrote:
> Dong, Eddie wrote:
>> Anthony:
>> 	Actually I am wondering if the binary used for VMMCALL could be
>> assumed will never be used by Intel processor or vice versa.  BTW,
>> what is the nenefit to remove hypercall page, which provide more
>> clean approach IMO? 
>> 
> 
> A hypercall page doesn't help the situation with respect to migration
> between an AMD and Intel system. 

I guess I miss something. As if hypercall page is read only after
creation
(by VMM), later memory migration won't overlap it. The page could be 
a "Reserved" in e820 table.

thx, eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Dong, Eddie wrote:
> Anthony Liguori wrote:
>   
>> Dong, Eddie wrote:
>>     
>>> Anthony:
>>> 	Actually I am wondering if the binary used for VMMCALL could be
>>> assumed will never be used by Intel processor or vice versa.  BTW,
>>> what is the nenefit to remove hypercall page, which provide more
>>> clean approach IMO? 
>>>
>>>       
>> A hypercall page doesn't help the situation with respect to migration
>> between an AMD and Intel system. 
>>     
>
> I guess I miss something. As if hypercall page is read only after
> creation
> (by VMM), later memory migration won't overlap it. The page could be 
> a "Reserved" in e820 table.
>   

If migration happens while rip is in the hypercall page, and if the 
hypercall page is different in the target host, you may need to apply 
fixups.  The fixups (and the hypercall page itself) may be different 
according to the guest mode (32-bit or 64-bit).

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dong, Eddie]

Avi Kivity wrote:
> Dong, Eddie wrote:
>> Anthony Liguori wrote:
>> 
>>> Dong, Eddie wrote:
>>> 
>>>> Anthony:
>>>> 	Actually I am wondering if the binary used for VMMCALL could be
>>>> assumed will never be used by Intel processor or vice versa.  BTW,
>>>> what is the nenefit to remove hypercall page, which provide more
>>>> clean approach IMO? 
>>>> 
>>>> 
>>> A hypercall page doesn't help the situation with respect to
>>> migration between an AMD and Intel system.
>>> 
>> 
>> I guess I miss something. As if hypercall page is read only after
>> creation (by VMM), later memory migration won't overlap it. The page
>> could be a "Reserved" in e820 table.
>> 
> 
> If migration happens while rip is in the hypercall page, and if the

I didn't quit catch here. The source VM vCPU is in Qemu migration part,
The target VM VCPU is always waiting for migration data/command.
If you mean SMP case, all target VCPUs are in waiting for data/cmd,
and I assume source VCPUs are all in Qemu known state, not?


> hypercall page is different in the target host, you may need to apply
> fixups.  The fixups (and the hypercall page itself) may be different
> according to the guest mode (32-bit or 64-bit).

thx, eddie


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Dong, Eddie wrote:
> Avi Kivity wrote:
>   
>> Dong, Eddie wrote:
>>     
>>> Anthony Liguori wrote:
>>>
>>>       
>>>> Dong, Eddie wrote:
>>>>
>>>>         
>>>>> Anthony:
>>>>> 	Actually I am wondering if the binary used for VMMCALL could be
>>>>> assumed will never be used by Intel processor or vice versa.  BTW,
>>>>> what is the nenefit to remove hypercall page, which provide more
>>>>> clean approach IMO? 
>>>>>
>>>>>
>>>>>           
>>>> A hypercall page doesn't help the situation with respect to
>>>> migration between an AMD and Intel system.
>>>>
>>>>         
>>> I guess I miss something. As if hypercall page is read only after
>>> creation (by VMM), later memory migration won't overlap it. The page
>>> could be a "Reserved" in e820 table.
>>>
>>>       
>> If migration happens while rip is in the hypercall page, and if the
>>     
>
> I didn't quit catch here. The source VM vCPU is in Qemu migration part,
> The target VM VCPU is always waiting for migration data/command.
> If you mean SMP case, all target VCPUs are in waiting for data/cmd,
> and I assume source VCPUs are all in Qemu known state, not?
>
>
>   

I'm talking about the guest rip.  The guest is not aware of the migration.

Suppose that on the last copy that the guest rip is (hypercall_page_virt 
+ 3).  This address might be in the middle of some instruction on the 
hypercall page on the target machine.  You need to fix up rip and 
possibly modify registers so that when it resumes it works correctly.



-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dong, Eddie]

>>> If migration happens while rip is in the hypercall page, and if the
>>> 
>> 
>> I didn't quit catch here. The source VM vCPU is in Qemu migration
>> part, The target VM VCPU is always waiting for migration
>> data/command. If you mean SMP case, all target VCPUs are in waiting
>> for data/cmd, and I assume source VCPUs are all in Qemu known state,
>> not? 
>> 
>> 
>> 
> 
> I'm talking about the guest rip.  The guest is not aware of the
> migration. 
> 
> Suppose that on the last copy that the guest rip is
> (hypercall_page_virt + 3).  This address might be in the middle of
> some instruction on the 
> hypercall page on the target machine.  You need to fix up rip and

This depends on how the hypercall page instruction is generated.
In Xen's construction, the code in hypercall page is exactly same
between SVM & VMX except the VMCALL/VMMCALL instruction itself.

> possibly modify registers so that when it resumes it works correctly. 

If we construct hypercall page in same manner, we don't need to fix up.

thx, eddie

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Dong, Eddie wrote:
>>>> If migration happens while rip is in the hypercall page, and if the
>>>>
>>>>         
>>> I didn't quit catch here. The source VM vCPU is in Qemu migration
>>> part, The target VM VCPU is always waiting for migration
>>> data/command. If you mean SMP case, all target VCPUs are in waiting
>>> for data/cmd, and I assume source VCPUs are all in Qemu known state,
>>> not? 
>>>
>>>
>>>
>>>       
>> I'm talking about the guest rip.  The guest is not aware of the
>> migration. 
>>
>> Suppose that on the last copy that the guest rip is
>> (hypercall_page_virt + 3).  This address might be in the middle of
>> some instruction on the 
>> hypercall page on the target machine.  You need to fix up rip and
>>     
>
> This depends on how the hypercall page instruction is generated.
> In Xen's construction, the code in hypercall page is exactly same
> between SVM & VMX except the VMCALL/VMMCALL instruction itself.
>
>   

If you make the assumption that the hypercall is a single 3-byte 
instruction, then you might as well patch it directly.  Of course it 
depends on Intel and AMD not reusing each other's opcodes.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Dong, Eddie]

Avi Kivity wrote:
> Dong, Eddie wrote:
>>>>> If migration happens while rip is in the hypercall page, and if
>>>>> the 
>>>>> 
>>>>> 
>>>> I didn't quit catch here. The source VM vCPU is in Qemu migration
>>>> part, The target VM VCPU is always waiting for migration
>>>> data/command. If you mean SMP case, all target VCPUs are in waiting
>>>> for data/cmd, and I assume source VCPUs are all in Qemu known
>>>> state, not? 
>>>> 
>>>> 
>>>> 
>>>> 
>>> I'm talking about the guest rip.  The guest is not aware of the
>>> migration. 
>>> 
>>> Suppose that on the last copy that the guest rip is
>>> (hypercall_page_virt + 3).  This address might be in the middle of
>>> some instruction on the hypercall page on the target machine.  You
>>> need to fix up rip and 
>>> 
>> 
>> This depends on how the hypercall page instruction is generated.
>> In Xen's construction, the code in hypercall page is exactly same
>> between SVM & VMX except the VMCALL/VMMCALL instruction itself.
>> 
>> 
> 
> If you make the assumption that the hypercall is a single 3-byte

Mmm, I am not and actually it doesn't care what opcode is used 
and how long it is. We need to assume the pre-instructions and post
instructions are same, and it is totally in VMM plate.

Well, using #UD to emulate cross-architecture VMCALL/VMMCALL
need to assume the opcode won't be used in other architecture.

> instruction, then you might as well patch it directly.  Of course it
> depends on Intel and AMD not reusing each other's opcodes.  

Besides the opcode concern and potential bug like Eric & Sheng fixed,
 the performance of using #UD method is worse than using hypercall page.

thx,eddie

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Yang, Sheng wrote:
> From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Wed, 2 Jan 2008 14:49:22 +0800
> Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD
>
> When executing a test program called "crashme", we found the KVM guest cannot 
> survived more than ten seconds, then encounterd kernel panic. The basic 
> concept of "crashme" is graduating random assembly code and trying to execute 
> them in a fork process.
>
> After some fix on emulator valid judgment, we found it's hard to get the 
> current emulator handle the invalid instructions correctly, for the #UD trap 
> for hypercall patching caused troubles. The problem is, if the opcode itself 
> was OK, but combination of opcode and modrm_reg was invalid, and one operand 
> of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory 
> operand first rather than judged the validity, and may encounter error there. 
> For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.
>
> In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, 
> then return from emulate_instruction() and inject a #UD to guest. With the 
> patch, the guest had been run for more than 12 hours.
>
>   

Applied, thanks.  As Anthony says, good catch indeed.

I have a vague plan for improving decode; basically extend the decode 
tables to add group decoding.  We add a bit to opcode_table and 
twobyte_table that is set for all instructions which need group 
decoding.  When the bit is set, the rest of the value in opcode_table is 
interpreted as an index (together with modrm_reg) into a new 
group_table, so we can have different decoding for such instructions.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Yang, Sheng]

On Sunday 06 January 2008 16:40:16 Avi Kivity wrote:
> Yang, Sheng wrote:
> > From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
> > From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Date: Wed, 2 Jan 2008 14:49:22 +0800
> > Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD
> >
> > When executing a test program called "crashme", we found the KVM guest
> > cannot survived more than ten seconds, then encounterd kernel panic. The
> > basic concept of "crashme" is graduating random assembly code and trying
> > to execute them in a fork process.
> >
> > After some fix on emulator valid judgment, we found it's hard to get the
> > current emulator handle the invalid instructions correctly, for the #UD
> > trap for hypercall patching caused troubles. The problem is, if the
> > opcode itself was OK, but combination of opcode and modrm_reg was
> > invalid, and one operand of the opcode was memory(SrcMem or DstMem),
> > emulator would fetched the memory operand first rather than judged the
> > validity, and may encounter error there. For example, ".byte 0xfe, 0x34,
> > 0xcd" got this trouble.
> >
> > In the patch, we simply check that if the invalid opcode isn't
> > vmcall/vmmcall, then return from emulate_instruction() and inject a #UD
> > to guest. With the patch, the guest had been run for more than 12 hours.
>
> Applied, thanks.  As Anthony says, good catch indeed.
>
> I have a vague plan for improving decode; basically extend the decode
> tables to add group decoding.  We add a bit to opcode_table and
> twobyte_table that is set for all instructions which need group
> decoding.  When the bit is set, the rest of the value in opcode_table is
> interpreted as an index (together with modrm_reg) into a new
> group_table, so we can have different decoding for such instructions.

I also have tried to propose a table for Grp opcode, but can't find a easy 
way. Using the rest of the value in opcode_table is a good idea, but I'm 
afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0
(Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add 
more factors to this, it would become unclear and more tricky, the table also 
may become larger... 

Currently, if we want to using group_table, I think the straightforward way is 
better: another big "switch"... The only exception is 1a, and we may use 0 
instead of it. 

-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Yang, Sheng wrote:
>> I have a vague plan for improving decode; basically extend the decode
>> tables to add group decoding.  We add a bit to opcode_table and
>> twobyte_table that is set for all instructions which need group
>> decoding.  When the bit is set, the rest of the value in opcode_table is
>> interpreted as an index (together with modrm_reg) into a new
>> group_table, so we can have different decoding for such instructions.
>>     
>
> I also have tried to propose a table for Grp opcode, but can't find a easy 
> way. Using the rest of the value in opcode_table is a good idea, but I'm 
> afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0
> (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add 
> more factors to this, it would become unclear and more tricky, the table also 
> may become larger... 
>
> Currently, if we want to using group_table, I think the straightforward way is 
> better: another big "switch"... The only exception is 1a, and we may use 0 
> instead of it. 
>   

Not sure what you mean.  I thought of adding code like


    if (c->d & Group) {
          c->group = c->d & GroupMask;
          // fetch modrm_reg
          c->d = group_table[c->group * 8 + modrm_reg];
    }

Instruction execution could continue to be in the regular switch, but 
the decode flags can be different for every instruction in the group.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Yang, Sheng]

On Monday 07 January 2008 17:22:43 Avi Kivity wrote:
> Yang, Sheng wrote:
> >> I have a vague plan for improving decode; basically extend the decode
> >> tables to add group decoding.  We add a bit to opcode_table and
> >> twobyte_table that is set for all instructions which need group
> >> decoding.  When the bit is set, the rest of the value in opcode_table is
> >> interpreted as an index (together with modrm_reg) into a new
> >> group_table, so we can have different decoding for such instructions.
> >
> > I also have tried to propose a table for Grp opcode, but can't find a
> > easy way. Using the rest of the value in opcode_table is a good idea, but
> > I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and
> > 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If
> > we add more factors to this, it would become unclear and more tricky, the
> > table also may become larger...
> >
> > Currently, if we want to using group_table, I think the straightforward
> > way is better: another big "switch"... The only exception is 1a, and we
> > may use 0 instead of it.
>
> Not sure what you mean.  I thought of adding code like
>
>
>     if (c->d & Group) {
>           c->group = c->d & GroupMask;

I meant the c->d & GroupMask is not sufficient to indicate different group. 
For example, 0x82(Grp1) and 0xc0(Grp2) have same c->d & GroupMask = ByteOp | 
DstMem | SrcImm | ModRM.

>           // fetch modrm_reg
>           c->d = group_table[c->group * 8 + modrm_reg];

In this case, how can you deal with c->group =  ByteOp | DstMem | SrcImm | 
ModRM, and modrm_reg = 6. Is it a XOR or nothing?

>     }
>
> Instruction execution could continue to be in the regular switch, but
> the decode flags can be different for every instruction in the group.



-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Avi Kivity]

Yang, Sheng wrote:
> On Monday 07 January 2008 17:22:43 Avi Kivity wrote:
>   
>> Yang, Sheng wrote:
>>     
>>>> I have a vague plan for improving decode; basically extend the decode
>>>> tables to add group decoding.  We add a bit to opcode_table and
>>>> twobyte_table that is set for all instructions which need group
>>>> decoding.  When the bit is set, the rest of the value in opcode_table is
>>>> interpreted as an index (together with modrm_reg) into a new
>>>> group_table, so we can have different decoding for such instructions.
>>>>         
>>> I also have tried to propose a table for Grp opcode, but can't find a
>>> easy way. Using the rest of the value in opcode_table is a good idea, but
>>> I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and
>>> 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If
>>> we add more factors to this, it would become unclear and more tricky, the
>>> table also may become larger...
>>>
>>> Currently, if we want to using group_table, I think the straightforward
>>> way is better: another big "switch"... The only exception is 1a, and we
>>> may use 0 instead of it.
>>>       
>> Not sure what you mean.  I thought of adding code like
>>
>>
>>     if (c->d & Group) {
>>           c->group = c->d & GroupMask;
>>     
>
> I meant the c->d & GroupMask is not sufficient to indicate different group. 
> For example, 0x82(Grp1) and 0xc0(Grp2) have same c->d & GroupMask = ByteOp | 
> DstMem | SrcImm | ModRM.
>
>   

I now see the source of confusion...

>>           // fetch modrm_reg
>>           c->d = group_table[c->group * 8 + modrm_reg];
>>     
>
> In this case, how can you deal with c->group =  ByteOp | DstMem | SrcImm | 
> ModRM, and modrm_reg = 6. Is it a XOR or nothing?
>   

In this case, we would have

   opcode_table[0x82] == Group | Grp1  (c->group == Grp1 == 1)
   opcode_table[0xc0] == Group | Grp2  (c->group == Grp2 == 2)
   group_table[Grp1 * 8 + 6] == ByteOp | DstMem | SrcImm | ModRM
   group_table[Grp2 * 8 + 0] == ByteOp | DstMem | SrcImm | ModRM

If Group is set in opcode_table, the rest of the value is the group 
number, with the reg part used to pick the final decode flags.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

[Yang, Sheng]

On Monday 07 January 2008 18:43:52 Avi Kivity wrote:
> Yang, Sheng wrote:
> > On Monday 07 January 2008 17:22:43 Avi Kivity wrote:
> >> Yang, Sheng wrote:
> >>>> I have a vague plan for improving decode; basically extend the decode
> >>>> tables to add group decoding.  We add a bit to opcode_table and
> >>>> twobyte_table that is set for all instructions which need group
> >>>> decoding.  When the bit is set, the rest of the value in opcode_table
> >>>> is interpreted as an index (together with modrm_reg) into a new
> >>>> group_table, so we can have different decoding for such instructions.
> >>>
> >>> I also have tried to propose a table for Grp opcode, but can't find a
> >>> easy way. Using the rest of the value in opcode_table is a good idea,
> >>> but I'm afraid the same value for different group exists, e.g.
> >>> 0x82(Grp1) and 0xc0 (Grp2) got the same value as: ByteOp | DstMem |
> >>> SrcImm | ModRM. If we add more factors to this, it would become unclear
> >>> and more tricky, the table also may become larger...
> >>>
> >>> Currently, if we want to using group_table, I think the straightforward
> >>> way is better: another big "switch"... The only exception is 1a, and we
> >>> may use 0 instead of it.
> >>
> >> Not sure what you mean.  I thought of adding code like
> >>
> >>
> >>     if (c->d & Group) {
> >>           c->group = c->d & GroupMask;
> >
> > I meant the c->d & GroupMask is not sufficient to indicate different
> > group. For example, 0x82(Grp1) and 0xc0(Grp2) have same c->d & GroupMask
> > = ByteOp | DstMem | SrcImm | ModRM.
>
> I now see the source of confusion...
>
> >>           // fetch modrm_reg
> >>           c->d = group_table[c->group * 8 + modrm_reg];
> >
> > In this case, how can you deal with c->group =  ByteOp | DstMem | SrcImm
> > | ModRM, and modrm_reg = 6. Is it a XOR or nothing?
>
> In this case, we would have
>
>    opcode_table[0x82] == Group | Grp1  (c->group == Grp1 == 1)
>    opcode_table[0xc0] == Group | Grp2  (c->group == Grp2 == 2)
>    group_table[Grp1 * 8 + 6] == ByteOp | DstMem | SrcImm | ModRM
>    group_table[Grp2 * 8 + 0] == ByteOp | DstMem | SrcImm | ModRM
>
> If Group is set in opcode_table, the rest of the value is the group
> number, with the reg part used to pick the final decode flags.

Faint... 

Yeah, I think that's pretty good. Sorry for misunderstanding...

-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/