Re: [PATCH 2/8] KVM: x86 emulator: decode extended accumulator explicity

[Gleb Natapov <gleb@redhat.com>]

On Sat, Feb 09, 2013 at 11:31:45AM +0200, Avi Kivity wrote:
> Single-operand MUL and DIV access an extended accumulator: AX for byte
> instructions, and DX:AX, EDX:EAX, or RDX:RAX for larger-sized instructions.
> Add support for fetching the extended accumulator.
> 
Where this "extended accumulator" definition is coming from? Spec just
says that two registers are used for a result depending on the operand
size.

> In order not to change things too much, RDX is loaded into Src2, which is
> already loaded by fastop().  This avoids increasing register pressure on
> i386.
> 
> Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
> ---
>  arch/x86/kvm/emulate.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 18c86b5..aa8516e 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -60,6 +60,8 @@
>  #define OpGS              25ull  /* GS */
>  #define OpMem8            26ull  /* 8-bit zero extended memory operand */
>  #define OpImm64           27ull  /* Sign extended 16/32/64-bit immediate */
> +#define OpAccLo           29ull  /* Low part of extended acc (AX/AX/EAX/RAX) */
> +#define OpAccHi           30ull  /* High part of extended acc (-/DX/EDX/RDX) */
>  
>  #define OpBits             5  /* Width of operand field */
>  #define OpMask             ((1ull << OpBits) - 1)
> @@ -85,6 +87,7 @@
>  #define DstMem64    (OpMem64 << DstShift)
>  #define DstImmUByte (OpImmUByte << DstShift)
>  #define DstDX       (OpDX << DstShift)
> +#define DstAccLo    (OpAccLo << DstShift)
>  #define DstMask     (OpMask << DstShift)
>  /* Source operand type. */
>  #define SrcShift    6
> @@ -106,6 +109,7 @@
>  #define SrcImm64    (OpImm64 << SrcShift)
>  #define SrcDX       (OpDX << SrcShift)
>  #define SrcMem8     (OpMem8 << SrcShift)
> +#define SrcAccHi    (OpAccHi << SrcShift)
>  #define SrcMask     (OpMask << SrcShift)
>  #define BitOp       (1<<11)
>  #define MemAbs      (1<<12)      /* Memory operand is absolute displacement */
> @@ -154,6 +158,8 @@
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
>  #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
>  
> +#define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
> +
>  #define X2(x...) x, x
>  #define X3(x...) X2(x), x
>  #define X4(x...) X2(x), X2(x)
> @@ -4129,6 +4135,22 @@ static int decode_operand(struct x86_emulate_ctxt *ctxt, struct operand *op,
>  		fetch_register_operand(op);
>  		op->orig_val = op->val;
>  		break;
> +	case OpAccLo:
> +		op->type = OP_REG;
> +		op->bytes = (ctxt->d & ByteOp) ? 2 : ctxt->op_bytes;
> +		op->addr.reg = reg_rmw(ctxt, VCPU_REGS_RAX);
> +		fetch_register_operand(op);
> +		op->orig_val = op->val;
> +		break;
> +	case OpAccHi:
> +		if (ctxt->d & ByteOp)
> +			break;
Why would we set OpAccHi if ByteOp is set in decode tables in the first
place?

> +		op->type = OP_REG;
> +		op->bytes = ctxt->op_bytes;
> +		op->addr.reg = reg_rmw(ctxt, VCPU_REGS_RDX);
> +		fetch_register_operand(op);
> +		op->orig_val = op->val;
> +		break;
>  	case OpDI:
>  		op->type = OP_MEM;
>  		op->bytes = (ctxt->d & ByteOp) ? 1 : ctxt->op_bytes;
> -- 
> 1.8.1.2

--
			Gleb.