Re: [PATCH 4/4] KVM: x86 emulator: remove dup code of in/out instruction

[Avi Kivity <avi@redhat.com>]

 On 08/04/2010 10:40 AM, Wei Yongjun wrote:
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

Patch is good, but too big. Please separate into DstImmUByte, change OUT
to use dst instead of src, IN consolidationn and OUT consolidation.

> ---
>  arch/x86/kvm/emulate.c |   50 ++++++++++++++++++++---------------------------
>  1 files changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 1ce3c4f..d197b46 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -54,6 +54,7 @@
>  #define DstAcc      (4<<1)	/* Destination Accumulator */
>  #define DstDI       (5<<1)	/* Destination is in ES:(E)DI */
>  #define DstMem64    (6<<1)	/* 64bit memory operand */
> +#define DstImmUByte (7<<1)	/* 8-bit unsigned immediate operand */
>  #define DstMask     (7<<1)
>  /* Source operand type. */
>  #define SrcNone     (0<<4)	/* No source operand. */
> @@ -2199,12 +2200,12 @@ static struct opcode opcode_table[256] = {
>  	/* 0xE0 - 0xE7 */
>  	N, N, N, N,
>  	D(ByteOp | SrcImmUByte | DstAcc), D(SrcImmUByte | DstAcc),
> -	D(ByteOp | SrcImmUByte | DstAcc), D(SrcImmUByte | DstAcc),
> +	D(ByteOp | SrcAcc | DstImmUByte), D(SrcAcc | DstImmUByte),
>  	/* 0xE8 - 0xEF */
>  	D(SrcImm | Stack), D(SrcImm | ImplicitOps),
>  	D(SrcImmFAddr | No64), D(SrcImmByte | ImplicitOps),
>  	D(SrcNone | ByteOp | DstAcc), D(SrcNone | DstAcc),
> -	D(SrcNone | ByteOp | DstAcc), D(SrcNone | DstAcc),
> +	D(ByteOp | SrcAcc | ImplicitOps), D(SrcAcc | ImplicitOps),
>  	/* 0xF0 - 0xF7 */
>  	N, N, N, N,
>  	D(ImplicitOps | Priv), D(ImplicitOps), G(ByteOp, group3), G(0, group3),
> @@ -2573,6 +2574,12 @@ done_prefixes:
>  		decode_register_operand(&c->dst, c,
>  			 c->twobyte && (c->b == 0xb6 || c->b == 0xb7));
>  		break;
> +	case DstImmUByte:
> +		c->dst.type = OP_IMM;
> +		c->dst.addr.mem = c->eip;
> +		c->dst.bytes = 1;
> +		c->dst.val = insn_fetch(u8, 1, c->eip);
> +		break;
>  	case DstMem:
>  	case DstMem64:
>  		c->dst = memop;
> @@ -2803,29 +2810,12 @@ special_insn:
>  		break;
>  	case 0x6c:		/* insb */
>  	case 0x6d:		/* insw/insd */
> -		c->dst.bytes = min(c->dst.bytes, 4u);
> -		if (!emulator_io_permited(ctxt, ops, c->regs[VCPU_REGS_RDX],
> -					  c->dst.bytes)) {
> -			emulate_gp(ctxt, 0);
> -			goto done;
> -		}
> -		if (!pio_in_emulated(ctxt, ops, c->dst.bytes,
> -				     c->regs[VCPU_REGS_RDX], &c->dst.val))
> -			goto done; /* IO is needed, skip writeback */
> -		break;
> +		c->src.val = c->regs[VCPU_REGS_RDX];
> +		goto do_io_in;
>  	case 0x6e:		/* outsb */
>  	case 0x6f:		/* outsw/outsd */
> -		c->src.bytes = min(c->src.bytes, 4u);
> -		if (!emulator_io_permited(ctxt, ops, c->regs[VCPU_REGS_RDX],
> -					  c->src.bytes)) {
> -			emulate_gp(ctxt, 0);
> -			goto done;
> -		}
> -		ops->pio_out_emulated(c->src.bytes, c->regs[VCPU_REGS_RDX],
> -				      &c->src.val, 1, ctxt->vcpu);
> -
> -		c->dst.type = OP_NONE; /* nothing to writeback */
> -		break;
> +		c->dst.val = c->regs[VCPU_REGS_RDX];
> +		goto do_io_out;
>  	case 0x70 ... 0x7f: /* jcc (short) */
>  		if (test_cc(c->b, ctxt->eflags))
>  			jmp_rel(c, c->src.val);
> @@ -3024,16 +3014,18 @@ special_insn:
>  		break;
>  	case 0xee: /* out dx,al */
>  	case 0xef: /* out dx,(e/r)ax */
> -		c->src.val = c->regs[VCPU_REGS_RDX];
> +		c->dst.val = c->regs[VCPU_REGS_RDX];
>  	do_io_out:
> -		c->dst.bytes = min(c->dst.bytes, 4u);
> -		if (!emulator_io_permited(ctxt, ops, c->src.val, c->dst.bytes)) {
> +		c->src.bytes = min(c->src.bytes, 4u);
> +		if (!emulator_io_permited(ctxt, ops, c->dst.val,
> +					  c->src.bytes)) {
>  			emulate_gp(ctxt, 0);
>  			goto done;
>  		}
> -		ops->pio_out_emulated(c->dst.bytes, c->src.val, &c->dst.val, 1,
> -				      ctxt->vcpu);
> -		c->dst.type = OP_NONE;	/* Disable writeback. */
> +		ops->pio_out_emulated(c->src.bytes, c->dst.val,
> +				      &c->src.val, 1, ctxt->vcpu);
> +
> +		c->dst.type = OP_NONE; /* nothing to writeback */
>  		break;
>  	case 0xf4:              /* hlt */
>  		ctxt->vcpu->arch.halt_request = 1;


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