Re: [PATCH 1/2] KVM: x86 emulator: put register operand write back to a function

2010-08-16 Thread Avi Kivity 
 On 08/16/2010 04:58 AM, Wei Yongjun wrote:
 
 It's cleaner to take val and bytes from struct operand, and do the
 assignment from the callers, no?
   
 take val and bytes from struct operand may have other issue, when we
 writeback
 the source register, we need do the assignment from the caller, and then
 change
 the val back before write src val to dst val. Such as xadd:
 c-src.val = c-dst.val;
 write_register_operand(c-src);
 c-src.val = c-src.orig_val;
 goto add;

Or avoid the 'goto add'. XADD is not ADD.

write_register_operand(struct operand *) is easy to understand. With the
two additional arguments it becomes confusing since it uses some parts
of the operand but ignores others.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
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 1/2] KVM: x86 emulator: put register operand write back to a function

2010-08-15 Thread Avi Kivity 
 On 08/12/2010 04:38 PM, Wei Yongjun wrote:
 Introduce function write_register_operand() to write back the
 register operand.


  
 +static void write_register_operand(struct operand *op, unsigned long val,
 +unsigned int bytes)
 +{
 + /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
 + switch (bytes) {
 + case 1:
 + *(u8 *)op-addr.reg = (u8)val;
 + break;
 + case 2:
 + *(u16 *)op-addr.reg = (u16)val;
 + break;
 + case 4:
 + *op-addr.reg = (u32)val;
 + break;  /* 64b: zero-extend */
 + case 8:
 + *op-addr.reg = val;
 + break;
 + }
 +}

It's cleaner to take val and bytes from struct operand, and do the
assignment from the callers, no?

-- 
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 1/2] KVM: x86 emulator: put register operand write back to a function

2010-08-15 Thread Wei Yongjun 

  On 08/12/2010 04:38 PM, Wei Yongjun wrote:
   
 Introduce function write_register_operand() to write back the
 register operand.


  
 +static void write_register_operand(struct operand *op, unsigned long val,
 +   unsigned int bytes)
 +{
 +/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
 +switch (bytes) {
 +case 1:
 +*(u8 *)op-addr.reg = (u8)val;
 +break;
 +case 2:
 +*(u16 *)op-addr.reg = (u16)val;
 +break;
 +case 4:
 +*op-addr.reg = (u32)val;
 +break;  /* 64b: zero-extend */
 +case 8:
 +*op-addr.reg = val;
 +break;
 +}
 +}
 
 It's cleaner to take val and bytes from struct operand, and do the
 assignment from the callers, no?
   

take val and bytes from struct operand may have other issue, when we
writeback
the source register, we need do the assignment from the caller, and then
change
the val back before write src val to dst val. Such as xadd:
c-src.val = c-dst.val;
write_register_operand(c-src);
c-src.val = c-src.orig_val;
goto add;




--
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 1/2] KVM: x86 emulator: put register operand write back to a function

2010-08-12 Thread Wei Yongjun 
Introduce function write_register_operand() to write back the
register operand.

Signed-off-by: Wei Yongjun yj...@cn.fujitsu.com
---
 arch/x86/kvm/emulate.c |   53 +++
 1 files changed, 22 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c476a67..8bf80a9 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1020,6 +1020,26 @@ exception:
return X86EMUL_PROPAGATE_FAULT;
 }
 
+static void write_register_operand(struct operand *op, unsigned long val,
+  unsigned int bytes)
+{
+   /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
+   switch (bytes) {
+   case 1:
+   *(u8 *)op-addr.reg = (u8)val;
+   break;
+   case 2:
+   *(u16 *)op-addr.reg = (u16)val;
+   break;
+   case 4:
+   *op-addr.reg = (u32)val;
+   break;  /* 64b: zero-extend */
+   case 8:
+   *op-addr.reg = val;
+   break;
+   }
+}
+
 static inline int writeback(struct x86_emulate_ctxt *ctxt,
struct x86_emulate_ops *ops)
 {
@@ -1029,23 +1049,7 @@ static inline int writeback(struct x86_emulate_ctxt 
*ctxt,
 
switch (c-dst.type) {
case OP_REG:
-   /* The 4-byte case *is* correct:
-* in 64-bit mode we zero-extend.
-*/
-   switch (c-dst.bytes) {
-   case 1:
-   *(u8 *)c-dst.addr.reg = (u8)c-dst.val;
-   break;
-   case 2:
-   *(u16 *)c-dst.addr.reg = (u16)c-dst.val;
-   break;
-   case 4:
-   *c-dst.addr.reg = (u32)c-dst.val;
-   break;  /* 64b: zero-ext */
-   case 8:
-   *c-dst.addr.reg = c-dst.val;
-   break;
-   }
+   write_register_operand(c-dst, c-dst.val, c-dst.bytes);
break;
case OP_MEM:
if (c-lock_prefix)
@@ -2971,20 +2975,7 @@ special_insn:
case 0x86 ... 0x87: /* xchg */
xchg:
/* Write back the register source. */
-   switch (c-dst.bytes) {
-   case 1:
-   *(u8 *) c-src.addr.reg = (u8) c-dst.val;
-   break;
-   case 2:
-   *(u16 *) c-src.addr.reg = (u16) c-dst.val;
-   break;
-   case 4:
-   *c-src.addr.reg = (u32) c-dst.val;
-   break;  /* 64b reg: zero-extend */
-   case 8:
-   *c-src.addr.reg = c-dst.val;
-   break;
-   }
+   write_register_operand(c-src, c-dst.val, c-dst.bytes);
/*
 * Write back the memory destination with implicit LOCK
 * prefix.
-- 
1.7.0.4


--
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