KVM: x86: use proper port value when checking io instruction permission

KVM: x86: use proper port value when checking io instruction permission

[Marcelo Tosatti]

Commit fa4491a6b667304 moved the permission check for io instructions
to the ->check_perm callback. It failed to copy the port value from RDX
register for string and "in,out ax,dx" instructions. Fix it.

Fixes FC8.32 installation.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3bc6b7a..df354a4 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2944,6 +2944,15 @@ static int check_perm_in(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
 
+	switch (c->b) {
+	case 0x6c: /* insb */
+	case 0x6d: /* insw/insd */
+	case 0xec: /* in al,dx */
+	case 0xed: /* in (e/r)ax,dx */
+		c->src.val = c->regs[VCPU_REGS_RDX];
+		break;
+	}
+
 	c->dst.bytes = min(c->dst.bytes, 4u);
 	if (!emulator_io_permited(ctxt, c->src.val, c->dst.bytes))
 		return emulate_gp(ctxt, 0);
@@ -2955,6 +2964,15 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
 {
 	struct decode_cache *c = &ctxt->decode;
 
+	switch (c->b) {
+	case 0x6e: /* outsb */
+	case 0x6f: /* outsw/outsd */
+	case 0xee: /* out dx,al */
+	case 0xef: /* out dx,(e/r)ax */
+		c->dst.val = c->regs[VCPU_REGS_RDX];
+		break;
+	}
+
 	c->src.bytes = min(c->src.bytes, 4u);
 	if (!emulator_io_permited(ctxt, c->dst.val, c->src.bytes))
 		return emulate_gp(ctxt, 0);

Re: KVM: x86: use proper port value when checking io instruction permission

[Gleb Natapov]

On Tue, May 24, 2011 at 02:11:20PM -0300, Marcelo Tosatti wrote:
> 
> Commit fa4491a6b667304 moved the permission check for io instructions
> to the ->check_perm callback. It failed to copy the port value from RDX
> register for string and "in,out ax,dx" instructions. Fix it.
> 
> Fixes FC8.32 installation.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 3bc6b7a..df354a4 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2944,6 +2944,15 @@ static int check_perm_in(struct x86_emulate_ctxt *ctxt)
>  {
>  	struct decode_cache *c = &ctxt->decode;
>  
> +	switch (c->b) {
> +	case 0x6c: /* insb */
> +	case 0x6d: /* insw/insd */
> +	case 0xec: /* in al,dx */
> +	case 0xed: /* in (e/r)ax,dx */
> +		c->src.val = c->regs[VCPU_REGS_RDX];
> +		break;
> +	}
> +
>  	c->dst.bytes = min(c->dst.bytes, 4u);
>  	if (!emulator_io_permited(ctxt, c->src.val, c->dst.bytes))
>  		return emulate_gp(ctxt, 0);
> @@ -2955,6 +2964,15 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>  {
>  	struct decode_cache *c = &ctxt->decode;
>  
> +	switch (c->b) {
> +	case 0x6e: /* outsb */
> +	case 0x6f: /* outsw/outsd */
> +	case 0xee: /* out dx,al */
> +	case 0xef: /* out dx,(e/r)ax */
> +		c->dst.val = c->regs[VCPU_REGS_RDX];
> +		break;
> +	}
> +
>  	c->src.bytes = min(c->src.bytes, 4u);
>  	if (!emulator_io_permited(ctxt, c->dst.val, c->src.bytes))
>  		return emulate_gp(ctxt, 0);
I'd rather do it at decoding stage by adding SrcDX/DstDX.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission

[Avi Kivity]

On 05/24/2011 08:27 PM, Gleb Natapov wrote:
> On Tue, May 24, 2011 at 02:11:20PM -0300, Marcelo Tosatti wrote:
> >
> >  Commit fa4491a6b667304 moved the permission check for io instructions
> >  to the ->check_perm callback. It failed to copy the port value from RDX
> >  register for string and "in,out ax,dx" instructions. Fix it.
> >
> >  Fixes FC8.32 installation.

Ouch.


> >  @@ -2955,6 +2964,15 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
> >   {
> >   	struct decode_cache *c =&ctxt->decode;
> >
> >  +	switch (c->b) {
> >  +	case 0x6e: /* outsb */
> >  +	case 0x6f: /* outsw/outsd */
> >  +	case 0xee: /* out dx,al */
> >  +	case 0xef: /* out dx,(e/r)ax */
> >  +		c->dst.val = c->regs[VCPU_REGS_RDX];
> >  +		break;
> >  +	}
> >  +
> >   	c->src.bytes = min(c->src.bytes, 4u);
> >   	if (!emulator_io_permited(ctxt, c->dst.val, c->src.bytes))
> >   		return emulate_gp(ctxt, 0);
> I'd rather do it at decoding stage by adding SrcDX/DstDX.
>

Note we haven't decoded operands yet.  And this doesn't fix in $imm8, %al.

Maybe we need an additional check site after operands are fetched.

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

Re: KVM: x86: use proper port value when checking io instruction permission

[Gleb Natapov]

On Tue, May 24, 2011 at 10:07:48PM +0300, Avi Kivity wrote:
> >>  @@ -2955,6 +2964,15 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
> >>   {
> >>   	struct decode_cache *c =&ctxt->decode;
> >>
> >>  +	switch (c->b) {
> >>  +	case 0x6e: /* outsb */
> >>  +	case 0x6f: /* outsw/outsd */
> >>  +	case 0xee: /* out dx,al */
> >>  +	case 0xef: /* out dx,(e/r)ax */
> >>  +		c->dst.val = c->regs[VCPU_REGS_RDX];
> >>  +		break;
> >>  +	}
> >>  +
> >>   	c->src.bytes = min(c->src.bytes, 4u);
> >>   	if (!emulator_io_permited(ctxt, c->dst.val, c->src.bytes))
> >>   		return emulate_gp(ctxt, 0);
> >I'd rather do it at decoding stage by adding SrcDX/DstDX.
> >
> 
> Note we haven't decoded operands yet.  And this doesn't fix in $imm8, %al.
> 
We haven't? check_perm is called from x86_emulate_insn() and operands are
decode in x86_decode_insn(). So $imm8, %al should work now. Or am I
missing something?

> Maybe we need an additional check site after operands are fetched.
> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission

[Avi Kivity]

On 05/24/2011 10:18 PM, Gleb Natapov wrote:
> >  Note we haven't decoded operands yet.  And this doesn't fix in $imm8, %al.
> >
> We haven't? check_perm is called from x86_emulate_insn() and operands are
> decode in x86_decode_insn(). So $imm8, %al should work now. Or am I
> missing something?

No, you're right.  SrcDX seems the simplest fix then.

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

KVM: x86: use proper port value when checking io instruction permission (v2)

[Marcelo Tosatti]

Commit fa4491a6b667304 moved the permission check for io instructions
to the ->check_perm callback. It failed to copy the port value from RDX
register for string and "in,out ax,dx" instructions. 

Fix it by reading RDX register at decode stage when appropriate.

Fixes FC8.32 installation.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index ab09ba2..a448212 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -243,7 +243,7 @@ struct decode_cache {
 	struct operand dst;
 	bool has_seg_override;
 	u8 seg_override;
-	unsigned int d;
+	u64 d;
 	int (*execute)(struct x86_emulate_ctxt *ctxt);
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
 	unsigned long regs[NR_VCPU_REGS];
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3bc6b7a..1904d88 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -47,54 +47,56 @@
 #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)
+#define DstDX	    (8<<1)	/* Destination in DX register */
+#define DstMask     (0xf<<1)
 /* Source operand type. */
-#define SrcNone     (0<<4)	/* No source operand. */
-#define SrcReg      (1<<4)	/* Register operand. */
-#define SrcMem      (2<<4)	/* Memory operand. */
-#define SrcMem16    (3<<4)	/* Memory operand (16-bit). */
-#define SrcMem32    (4<<4)	/* Memory operand (32-bit). */
-#define SrcImm      (5<<4)	/* Immediate operand. */
-#define SrcImmByte  (6<<4)	/* 8-bit sign-extended immediate operand. */
-#define SrcOne      (7<<4)	/* Implied '1' */
-#define SrcImmUByte (8<<4)      /* 8-bit unsigned immediate operand. */
-#define SrcImmU     (9<<4)      /* Immediate operand, unsigned */
-#define SrcSI       (0xa<<4)	/* Source is in the DS:RSI */
-#define SrcImmFAddr (0xb<<4)	/* Source is immediate far address */
-#define SrcMemFAddr (0xc<<4)	/* Source is far address in memory */
-#define SrcAcc      (0xd<<4)	/* Source Accumulator */
-#define SrcImmU16   (0xe<<4)    /* Immediate operand, unsigned, 16 bits */
-#define SrcMask     (0xf<<4)
+#define SrcNone     (0<<5)	/* No source operand. */
+#define SrcReg      (1<<5)	/* Register operand. */
+#define SrcMem      (2<<5)	/* Memory operand. */
+#define SrcMem16    (3<<5)	/* Memory operand (16-bit). */
+#define SrcMem32    (4<<5)	/* Memory operand (32-bit). */
+#define SrcImm      (5<<5)	/* Immediate operand. */
+#define SrcImmByte  (6<<5)	/* 8-bit sign-extended immediate operand. */
+#define SrcOne      (7<<5)	/* Implied '1' */
+#define SrcImmUByte (8<<5)      /* 8-bit unsigned immediate operand. */
+#define SrcImmU     (9<<5)      /* Immediate operand, unsigned */
+#define SrcSI       (0xa<<5)	/* Source is in the DS:RSI */
+#define SrcImmFAddr (0xb<<5)	/* Source is immediate far address */
+#define SrcMemFAddr (0xc<<5)	/* Source is far address in memory */
+#define SrcAcc      (0xd<<5)	/* Source Accumulator */
+#define SrcImmU16   (0xe<<5)    /* Immediate operand, unsigned, 16 bits */
+#define SrcDX	    (0xf<<5)
+#define SrcMask     (0xf<<5)
 /* Generic ModRM decode. */
-#define ModRM       (1<<8)
+#define ModRM       (1<<9)
 /* Destination is only written; never read. */
-#define Mov         (1<<9)
-#define BitOp       (1<<10)
-#define MemAbs      (1<<11)      /* Memory operand is absolute displacement */
-#define String      (1<<12)     /* String instruction (rep capable) */
-#define Stack       (1<<13)     /* Stack instruction (push/pop) */
-#define GroupMask   (7<<14)     /* Opcode uses one of the group mechanisms */
-#define Group       (1<<14)     /* Bits 3:5 of modrm byte extend opcode */
-#define GroupDual   (2<<14)     /* Alternate decoding of mod == 3 */
-#define Prefix      (3<<14)     /* Instruction varies with 66/f2/f3 prefix */
-#define RMExt       (4<<14)     /* Opcode extension in ModRM r/m if mod == 3 */
-#define Sse         (1<<17)     /* SSE Vector instruction */
+#define Mov         (1<<10)
+#define BitOp       (1<<11)
+#define MemAbs      (1<<12)      /* Memory operand is absolute displacement */
+#define String      (1<<13)     /* String instruction (rep capable) */
+#define Stack       (1<<14)     /* Stack instruction (push/pop) */
+#define GroupMask   (7<<15)     /* Opcode uses one of the group mechanisms */
+#define Group       (1<<15)     /* Bits 3:5 of modrm byte extend opcode */
+#define GroupDual   (2<<15)     /* Alternate decoding of mod == 3 */
+#define Prefix      (3<<15)     /* Instruction varies with 66/f2/f3 prefix */
+#define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
+#define Sse         (1<<18)     /* SSE Vector instruction */
 /* Misc flags */
-#define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
-#define VendorSpecific (1<<22) /* Vendor specific instruction */
-#define NoAccess    (1<<23) /* Don't access memory (lea/invlpg/verr etc) */
-#define Op3264      (1<<24) /* Operand is 64b in long mode, 32b otherwise */
-#define Undefined   (1<<25) /* No Such Instruction */
-#define Lock        (1<<26) /* lock prefix is allowed for the instruction */
-#define Priv        (1<<27) /* instruction generates #GP if current CPL != 0 */
-#define No64	    (1<<28)
+#define Prot        (1<<22) /* instruction generates #UD if not in prot-mode */
+#define VendorSpecific (1<<23) /* Vendor specific instruction */
+#define NoAccess    (1<<24) /* Don't access memory (lea/invlpg/verr etc) */
+#define Op3264      (1<<25) /* Operand is 64b in long mode, 32b otherwise */
+#define Undefined   (1<<26) /* No Such Instruction */
+#define Lock        (1<<27) /* lock prefix is allowed for the instruction */
+#define Priv        (1<<28) /* instruction generates #GP if current CPL != 0 */
+#define No64	    (1<<29)
 /* Source 2 operand type */
-#define Src2None    (0<<29)
-#define Src2CL      (1<<29)
-#define Src2ImmByte (2<<29)
-#define Src2One     (3<<29)
-#define Src2Imm     (4<<29)
-#define Src2Mask    (7<<29)
+#define Src2None    (0<<30)
+#define Src2CL      (1ULL<<30)
+#define Src2ImmByte (2ULL<<30)
+#define Src2One     (3ULL<<30)
+#define Src2Imm     (4ULL<<30)
+#define Src2Mask    (7ULL<<30)
 
 #define X2(x...) x, x
 #define X3(x...) X2(x), x
@@ -106,7 +108,7 @@
 #define X16(x...) X8(x), X8(x)
 
 struct opcode {
-	u32 flags;
+	u64 flags;
 	u8 intercept;
 	union {
 		int (*execute)(struct x86_emulate_ctxt *ctxt);
@@ -3124,8 +3126,8 @@ static struct opcode opcode_table[256] = {
 	I(DstReg | SrcMem | ModRM | Src2Imm, em_imul_3op),
 	I(SrcImmByte | Mov | Stack, em_push),
 	I(DstReg | SrcMem | ModRM | Src2ImmByte, em_imul_3op),
-	D2bvIP(DstDI | Mov | String, ins, check_perm_in), /* insb, insw/insd */
-	D2bvIP(SrcSI | ImplicitOps | String, outs, check_perm_out), /* outsb, outsw/outsd */
+	D2bvIP(DstDI | SrcDX | Mov | String, ins, check_perm_in), /* insb, insw/insd */
+	D2bvIP(SrcSI | DstDX | String, outs, check_perm_out), /* outsb, outsw/outsd */
 	/* 0x70 - 0x7F */
 	X16(D(SrcImmByte)),
 	/* 0x80 - 0x87 */
@@ -3182,8 +3184,8 @@ static struct opcode opcode_table[256] = {
 	/* 0xE8 - 0xEF */
 	D(SrcImm | Stack), D(SrcImm | ImplicitOps),
 	D(SrcImmFAddr | No64), D(SrcImmByte | ImplicitOps),
-	D2bvIP(SrcNone | DstAcc,     in,  check_perm_in),
-	D2bvIP(SrcAcc | ImplicitOps, out, check_perm_out),
+	D2bvIP(SrcDX | DstAcc, in,  check_perm_in),
+	D2bvIP(SrcAcc | DstDX, out, check_perm_out),
 	/* 0xF0 - 0xF7 */
 	N, DI(ImplicitOps, icebp), N, N,
 	DI(ImplicitOps | Priv, hlt), D(ImplicitOps),
@@ -3580,6 +3582,12 @@ done_prefixes:
 		memop.bytes = c->op_bytes + 2;
 		goto srcmem_common;
 		break;
+	case SrcDX:
+		c->src.type = OP_REG;
+		c->src.bytes = c->op_bytes;
+		c->src.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->src);
+		break;
 	}
 
 	if (rc != X86EMUL_CONTINUE)
@@ -3649,6 +3657,12 @@ done_prefixes:
 		c->dst.addr.mem.seg = VCPU_SREG_ES;
 		c->dst.val = 0;
 		break;
+	case DstDX:
+		c->dst.type = OP_REG;
+		c->dst.bytes = c->op_bytes;
+		c->dst.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->dst);
+		break;
 	case ImplicitOps:
 		/* Special instructions do their own operand decoding. */
 	default:

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Avi Kivity]

On 05/25/2011 09:18 PM, Marcelo Tosatti wrote:
> Commit fa4491a6b667304 moved the permission check for io instructions
> to the ->check_perm callback. It failed to copy the port value from RDX
> register for string and "in,out ax,dx" instructions.
>
> Fix it by reading RDX register at decode stage when appropriate.
>
> Fixes FC8.32 installation.
>
> +#define Sse         (1<<18)     /* SSE Vector instruction */

19/20 are still available, no need to go 64-bit just yet.

>   /* Misc flags */
> -#define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
>
> +	case SrcDX:
> +		c->src.type = OP_REG;
> +		c->src.bytes = c->op_bytes;

Needs to be 2.  Otherwise we'll see extra bits from edx, or lose bits 
from dx if it's a 1-byte instruction.

> +		c->src.addr.reg =&c->regs[VCPU_REGS_RDX];
> +		fetch_register_operand(&c->src);
> +		break;
>   	}
>
>   	if (rc != X86EMUL_CONTINUE)
> @@ -3649,6 +3657,12 @@ done_prefixes:
>   		c->dst.addr.mem.seg = VCPU_SREG_ES;
>   		c->dst.val = 0;
>   		break;
> +	case DstDX:
> +		c->dst.type = OP_REG;
> +		c->dst.bytes = c->op_bytes;

2 again.

> +		c->dst.addr.reg =&c->regs[VCPU_REGS_RDX];
> +		fetch_register_operand(&c->dst);
> +		break;
>   	case ImplicitOps:
>   		/* Special instructions do their own operand decoding. */
>   	default:

We also need to unify Src/Dst decode eventually.

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

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Gleb Natapov]

On Thu, May 26, 2011 at 09:31:50AM +0300, Avi Kivity wrote:
> On 05/25/2011 09:18 PM, Marcelo Tosatti wrote:
> >Commit fa4491a6b667304 moved the permission check for io instructions
> >to the ->check_perm callback. It failed to copy the port value from RDX
> >register for string and "in,out ax,dx" instructions.
> >
> >Fix it by reading RDX register at decode stage when appropriate.
> >
> >Fixes FC8.32 installation.
> >
> >+#define Sse         (1<<18)     /* SSE Vector instruction */
> 
> 19/20 are still available, no need to go 64-bit just yet.
> 
> >  /* Misc flags */
> >-#define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
> >
> >+	case SrcDX:
> >+		c->src.type = OP_REG;
> >+		c->src.bytes = c->op_bytes;
> 
> Needs to be 2.  Otherwise we'll see extra bits from edx, or lose
> bits from dx if it's a 1-byte instruction.
> 
But those extra bits will be dropped by check_perm_in() anyway. Can
c->op_bytes ever be 1?

> >+		c->src.addr.reg =&c->regs[VCPU_REGS_RDX];
> >+		fetch_register_operand(&c->src);
> >+		break;
> >  	}
> >
> >  	if (rc != X86EMUL_CONTINUE)
> >@@ -3649,6 +3657,12 @@ done_prefixes:
> >  		c->dst.addr.mem.seg = VCPU_SREG_ES;
> >  		c->dst.val = 0;
> >  		break;
> >+	case DstDX:
> >+		c->dst.type = OP_REG;
> >+		c->dst.bytes = c->op_bytes;
> 
> 2 again.
> 
> >+		c->dst.addr.reg =&c->regs[VCPU_REGS_RDX];
> >+		fetch_register_operand(&c->dst);
> >+		break;
> >  	case ImplicitOps:
> >  		/* Special instructions do their own operand decoding. */
> >  	default:
> 
> We also need to unify Src/Dst decode eventually.
> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Avi Kivity]

On 05/26/2011 09:55 AM, Gleb Natapov wrote:
> >  >
> >  >+	case SrcDX:
> >  >+		c->src.type = OP_REG;
> >  >+		c->src.bytes = c->op_bytes;
> >
> >  Needs to be 2.  Otherwise we'll see extra bits from edx, or lose
> >  bits from dx if it's a 1-byte instruction.
> >
> But those extra bits will be dropped by check_perm_in() anyway.

It isn't nice to depend on it.

btw, Marcelo, the patch should also make use of the decode during execution:

     case 0xef: /* out dx,(e/r)ax */
         c->dst.val = c->regs[VCPU_REGS_RDX];

^^ can drop


     do_io_out:
         ops->pio_out_emulated(ctxt, c->src.bytes, c->dst.val,
&c->src.val, 1);
         c->dst.type = OP_NONE;    /* Disable writeback. */
         break;

> Can
> c->op_bytes ever be 1?

in %dx, %al

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

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Avi Kivity]

On 05/26/2011 10:02 AM, Avi Kivity wrote:
>
>> Can
>> c->op_bytes ever be 1?
>
> in %dx, %al
>

er, that doesn't change op_bytes.  Still, op_bytes is irrelevant for 
SrcDX, the 16-bit version is always used.

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

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Gleb Natapov]

On Thu, May 26, 2011 at 10:04:24AM +0300, Avi Kivity wrote:
> On 05/26/2011 10:02 AM, Avi Kivity wrote:
> >
> >>Can
> >>c->op_bytes ever be 1?
> >
> >in %dx, %al
> >
> 
> er, that doesn't change op_bytes.
Yep.

>                                    Still, op_bytes is irrelevant for
> SrcDX, the 16-bit version is always used.
> 
If SrcDX/DstDX will be used only for decoding in/out instruction
then yes. Otherwise it is nice to have more general decoder.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Paolo Bonzini]

On 05/26/2011 09:07 AM, Gleb Natapov wrote:
> > Still, op_bytes is irrelevant for
> > SrcDX, the 16-bit version is always used.
>
> If SrcDX/DstDX will be used only for decoding in/out instruction
> then yes. Otherwise it is nice to have more general decoder.

Not counting instructions that read/write many registers (rdmsr/wrmsr, 
mul/imul/div/idiv, rdtsc, etc.), I think the only other instruction with 
an implicit DstDX is cwd/cdq/cqo.  Since cwd/cdq/cqo needs c->dst.bytes 
= c->src.bytes (not op_bytes) I think DstDX is not really reusable 
beyond port instructions.

Paolo

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Gleb Natapov]

On Thu, May 26, 2011 at 09:49:21AM +0200, Paolo Bonzini wrote:
> On 05/26/2011 09:07 AM, Gleb Natapov wrote:
> >> Still, op_bytes is irrelevant for
> >> SrcDX, the 16-bit version is always used.
> >
> >If SrcDX/DstDX will be used only for decoding in/out instruction
> >then yes. Otherwise it is nice to have more general decoder.
> 
> Not counting instructions that read/write many registers
> (rdmsr/wrmsr, mul/imul/div/idiv, rdtsc, etc.), I think the only
> other instruction with an implicit DstDX is cwd/cdq/cqo.  Since
> cwd/cdq/cqo needs c->dst.bytes = c->src.bytes (not op_bytes) I think
> DstDX is not really reusable beyond port instructions.
> 
Why would c->dst.bytes != c->src.bytes for cwd/cdq/cqo if we'll set
c->dst.bytes to op_bytes during decode?

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Paolo Bonzini]

On 05/26/2011 10:26 AM, Gleb Natapov wrote:
> Why would c->dst.bytes != c->src.bytes for cwd/cdq/cqo if we'll set
> c->dst.bytes to op_bytes during decode?

Duh, you're right, cwd/cdq/cqo uses SrcAcc which has

                 c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;

so in practice c->src.bytes = c->op_bytes.

I still find it confusing that DstDX would use c->op_bytes without 
honoring ByteOp unlike pretty much everything else; but yes, there is a 
possible use of DstDX outside in/out.

Paolo

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Gleb Natapov]

On Thu, May 26, 2011 at 11:00:24AM +0200, Paolo Bonzini wrote:
> On 05/26/2011 10:26 AM, Gleb Natapov wrote:
> >Why would c->dst.bytes != c->src.bytes for cwd/cdq/cqo if we'll set
> >c->dst.bytes to op_bytes during decode?
> 
> Duh, you're right, cwd/cdq/cqo uses SrcAcc which has
> 
>                 c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
> 
> so in practice c->src.bytes = c->op_bytes.
> 
> I still find it confusing that DstDX would use c->op_bytes without
> honoring ByteOp unlike pretty much everything else; but yes, there
> is a possible use of DstDX outside in/out.
> 
We can make it honor ByteOp. There will be no instruction that will
specify DstDX | ByteOp though.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Paolo Bonzini]

On 05/26/2011 11:02 AM, Gleb Natapov wrote:
> We can make it honor ByteOp. There will be no instruction that will
> specify DstDX | ByteOp though.

"in %dx, %al" and "out %al, %dx" will via D2bv.

Paolo

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Gleb Natapov]

On Thu, May 26, 2011 at 11:23:44AM +0200, Paolo Bonzini wrote:
> On 05/26/2011 11:02 AM, Gleb Natapov wrote:
> >We can make it honor ByteOp. There will be no instruction that will
> >specify DstDX | ByteOp though.
> 
> "in %dx, %al" and "out %al, %dx" will via D2bv.
> 
Yeah. Should ignore ByteOp then.

--
			Gleb.

Re: KVM: x86: use proper port value when checking io instruction permission (v2)

[Marcelo Tosatti]

On Thu, May 26, 2011 at 09:49:21AM +0200, Paolo Bonzini wrote:
> On 05/26/2011 09:07 AM, Gleb Natapov wrote:
> >> Still, op_bytes is irrelevant for
> >> SrcDX, the 16-bit version is always used.
> >
> >If SrcDX/DstDX will be used only for decoding in/out instruction
> >then yes. Otherwise it is nice to have more general decoder.

Yes, the use of op_bytes instead of 2 had that in mind.

> Not counting instructions that read/write many registers
> (rdmsr/wrmsr, mul/imul/div/idiv, rdtsc, etc.), I think the only
> other instruction with an implicit DstDX is cwd/cdq/cqo.  Since
> cwd/cdq/cqo needs c->dst.bytes = c->src.bytes (not op_bytes) I think
> DstDX is not really reusable beyond port instructions.
> 
> Paolo

OK, will switch to 2 then, thanks.

KVM: x86: use proper port value when checking io instruction permission (v3)

[Marcelo Tosatti]

Commit fa4491a6b667304 moved the permission check for io instructions
to the ->check_perm callback. It failed to copy the port value from RDX
register for string and "in,out ax,dx" instructions.

Fix it by reading RDX register at decode stage when appropriate.

Fixes FC8.32 installation.

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3bc6b7a..fc3d2d9 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -47,7 +47,7 @@
 #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)
+#define DstMask     ((7<<1) | (1<<18))
 /* Source operand type. */
 #define SrcNone     (0<<4)	/* No source operand. */
 #define SrcReg      (1<<4)	/* Register operand. */
@@ -64,7 +64,7 @@
 #define SrcMemFAddr (0xc<<4)	/* Source is far address in memory */
 #define SrcAcc      (0xd<<4)	/* Source Accumulator */
 #define SrcImmU16   (0xe<<4)    /* Immediate operand, unsigned, 16 bits */
-#define SrcMask     (0xf<<4)
+#define SrcMask     ((0xf<<4) | (1<<19))
 /* Generic ModRM decode. */
 #define ModRM       (1<<8)
 /* Destination is only written; never read. */
@@ -79,6 +79,8 @@
 #define Prefix      (3<<14)     /* Instruction varies with 66/f2/f3 prefix */
 #define RMExt       (4<<14)     /* Opcode extension in ModRM r/m if mod == 3 */
 #define Sse         (1<<17)     /* SSE Vector instruction */
+#define DstDX       (1<<18)	/* Destination is in DX register */
+#define SrcDX       (1<<19)	/* Source is in DX register */
 /* Misc flags */
 #define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
 #define VendorSpecific (1<<22) /* Vendor specific instruction */
@@ -3124,8 +3126,8 @@ static struct opcode opcode_table[256] = {
 	I(DstReg | SrcMem | ModRM | Src2Imm, em_imul_3op),
 	I(SrcImmByte | Mov | Stack, em_push),
 	I(DstReg | SrcMem | ModRM | Src2ImmByte, em_imul_3op),
-	D2bvIP(DstDI | Mov | String, ins, check_perm_in), /* insb, insw/insd */
-	D2bvIP(SrcSI | ImplicitOps | String, outs, check_perm_out), /* outsb, outsw/outsd */
+	D2bvIP(DstDI | SrcDX | Mov | String, ins, check_perm_in), /* insb, insw/insd */
+	D2bvIP(SrcSI | DstDX | String, outs, check_perm_out), /* outsb, outsw/outsd */
 	/* 0x70 - 0x7F */
 	X16(D(SrcImmByte)),
 	/* 0x80 - 0x87 */
@@ -3182,8 +3184,8 @@ static struct opcode opcode_table[256] = {
 	/* 0xE8 - 0xEF */
 	D(SrcImm | Stack), D(SrcImm | ImplicitOps),
 	D(SrcImmFAddr | No64), D(SrcImmByte | ImplicitOps),
-	D2bvIP(SrcNone | DstAcc,     in,  check_perm_in),
-	D2bvIP(SrcAcc | ImplicitOps, out, check_perm_out),
+	D2bvIP(SrcDX | DstAcc, in,  check_perm_in),
+	D2bvIP(SrcAcc | DstDX, out, check_perm_out),
 	/* 0xF0 - 0xF7 */
 	N, DI(ImplicitOps, icebp), N, N,
 	DI(ImplicitOps | Priv, hlt), D(ImplicitOps),
@@ -3580,6 +3582,12 @@ done_prefixes:
 		memop.bytes = c->op_bytes + 2;
 		goto srcmem_common;
 		break;
+	case SrcDX:
+		c->src.type = OP_REG;
+		c->src.bytes = 2;
+		c->src.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->src);
+		break;
 	}
 
 	if (rc != X86EMUL_CONTINUE)
@@ -3649,6 +3657,12 @@ done_prefixes:
 		c->dst.addr.mem.seg = VCPU_SREG_ES;
 		c->dst.val = 0;
 		break;
+	case DstDX:
+		c->dst.type = OP_REG;
+		c->dst.bytes = 2;
+		c->dst.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->dst);
+		break;
 	case ImplicitOps:
 		/* Special instructions do their own operand decoding. */
 	default:
@@ -3993,7 +4007,6 @@ special_insn:
 		break;
 	case 0xec: /* in al,dx */
 	case 0xed: /* in (e/r)ax,dx */
-		c->src.val = c->regs[VCPU_REGS_RDX];
 	do_io_in:
 		if (!pio_in_emulated(ctxt, c->dst.bytes, c->src.val,
 				     &c->dst.val))
@@ -4001,7 +4014,6 @@ special_insn:
 		break;
 	case 0xee: /* out dx,al */
 	case 0xef: /* out dx,(e/r)ax */
-		c->dst.val = c->regs[VCPU_REGS_RDX];
 	do_io_out:
 		ops->pio_out_emulated(ctxt, c->src.bytes, c->dst.val,
 				      &c->src.val, 1);

Re: KVM: x86: use proper port value when checking io instruction permission (v3)

[Avi Kivity]

On 05/26/2011 02:56 PM, Marcelo Tosatti wrote:
> Commit fa4491a6b667304 moved the permission check for io instructions
> to the ->check_perm callback. It failed to copy the port value from RDX
> register for string and "in,out ax,dx" instructions.
>
> Fix it by reading RDX register at decode stage when appropriate.
>
> Fixes FC8.32 installation.
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 3bc6b7a..fc3d2d9 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -47,7 +47,7 @@
>   #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)
> +#define DstMask     ((7<<1) | (1<<18))

Ouch, why??

My comment about bits 19-20 was only about the shifting of of Prot etc. 
from 21+ to 22+.

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

KVM: x86: use proper port value when checking io instruction permission (v4)

[Marcelo Tosatti]

Commit fa4491a6b667304 moved the permission check for io instructions
to the ->check_perm callback. It failed to copy the port value from RDX
register for string and "in,out ax,dx" instructions.

Fix it by reading RDX register at decode stage when appropriate.

Fixes FC8.32 installation.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 3bc6b7a..01c295f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -47,38 +47,40 @@
 #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)
+#define DstDX       (8<<1)	/* Destination is in DX register */
+#define DstMask     (0xf<<1)
 /* Source operand type. */
-#define SrcNone     (0<<4)	/* No source operand. */
-#define SrcReg      (1<<4)	/* Register operand. */
-#define SrcMem      (2<<4)	/* Memory operand. */
-#define SrcMem16    (3<<4)	/* Memory operand (16-bit). */
-#define SrcMem32    (4<<4)	/* Memory operand (32-bit). */
-#define SrcImm      (5<<4)	/* Immediate operand. */
-#define SrcImmByte  (6<<4)	/* 8-bit sign-extended immediate operand. */
-#define SrcOne      (7<<4)	/* Implied '1' */
-#define SrcImmUByte (8<<4)      /* 8-bit unsigned immediate operand. */
-#define SrcImmU     (9<<4)      /* Immediate operand, unsigned */
-#define SrcSI       (0xa<<4)	/* Source is in the DS:RSI */
-#define SrcImmFAddr (0xb<<4)	/* Source is immediate far address */
-#define SrcMemFAddr (0xc<<4)	/* Source is far address in memory */
-#define SrcAcc      (0xd<<4)	/* Source Accumulator */
-#define SrcImmU16   (0xe<<4)    /* Immediate operand, unsigned, 16 bits */
-#define SrcMask     (0xf<<4)
+#define SrcNone     (0<<5)	/* No source operand. */
+#define SrcReg      (1<<5)	/* Register operand. */
+#define SrcMem      (2<<5)	/* Memory operand. */
+#define SrcMem16    (3<<5)	/* Memory operand (16-bit). */
+#define SrcMem32    (4<<5)	/* Memory operand (32-bit). */
+#define SrcImm      (5<<5)	/* Immediate operand. */
+#define SrcImmByte  (6<<5)	/* 8-bit sign-extended immediate operand. */
+#define SrcOne      (7<<5)	/* Implied '1' */
+#define SrcImmUByte (8<<5)      /* 8-bit unsigned immediate operand. */
+#define SrcImmU     (9<<5)      /* Immediate operand, unsigned */
+#define SrcSI       (0xa<<5)	/* Source is in the DS:RSI */
+#define SrcImmFAddr (0xb<<5)	/* Source is immediate far address */
+#define SrcMemFAddr (0xc<<5)	/* Source is far address in memory */
+#define SrcAcc      (0xd<<5)	/* Source Accumulator */
+#define SrcImmU16   (0xe<<5)    /* Immediate operand, unsigned, 16 bits */
+#define SrcDX       (0xf<<5)	/* Source is in DX register */
+#define SrcMask     (0xf<<5)
 /* Generic ModRM decode. */
-#define ModRM       (1<<8)
+#define ModRM       (1<<9)
 /* Destination is only written; never read. */
-#define Mov         (1<<9)
-#define BitOp       (1<<10)
-#define MemAbs      (1<<11)      /* Memory operand is absolute displacement */
-#define String      (1<<12)     /* String instruction (rep capable) */
-#define Stack       (1<<13)     /* Stack instruction (push/pop) */
-#define GroupMask   (7<<14)     /* Opcode uses one of the group mechanisms */
-#define Group       (1<<14)     /* Bits 3:5 of modrm byte extend opcode */
-#define GroupDual   (2<<14)     /* Alternate decoding of mod == 3 */
-#define Prefix      (3<<14)     /* Instruction varies with 66/f2/f3 prefix */
-#define RMExt       (4<<14)     /* Opcode extension in ModRM r/m if mod == 3 */
-#define Sse         (1<<17)     /* SSE Vector instruction */
+#define Mov         (1<<10)
+#define BitOp       (1<<11)
+#define MemAbs      (1<<12)      /* Memory operand is absolute displacement */
+#define String      (1<<13)     /* String instruction (rep capable) */
+#define Stack       (1<<14)     /* Stack instruction (push/pop) */
+#define GroupMask   (7<<15)     /* Opcode uses one of the group mechanisms */
+#define Group       (1<<15)     /* Bits 3:5 of modrm byte extend opcode */
+#define GroupDual   (2<<15)     /* Alternate decoding of mod == 3 */
+#define Prefix      (3<<15)     /* Instruction varies with 66/f2/f3 prefix */
+#define RMExt       (4<<15)     /* Opcode extension in ModRM r/m if mod == 3 */
+#define Sse         (1<<18)     /* SSE Vector instruction */
 /* Misc flags */
 #define Prot        (1<<21) /* instruction generates #UD if not in prot-mode */
 #define VendorSpecific (1<<22) /* Vendor specific instruction */
@@ -3124,8 +3126,8 @@ static struct opcode opcode_table[256] = {
 	I(DstReg | SrcMem | ModRM | Src2Imm, em_imul_3op),
 	I(SrcImmByte | Mov | Stack, em_push),
 	I(DstReg | SrcMem | ModRM | Src2ImmByte, em_imul_3op),
-	D2bvIP(DstDI | Mov | String, ins, check_perm_in), /* insb, insw/insd */
-	D2bvIP(SrcSI | ImplicitOps | String, outs, check_perm_out), /* outsb, outsw/outsd */
+	D2bvIP(DstDI | SrcDX | Mov | String, ins, check_perm_in), /* insb, insw/insd */
+	D2bvIP(SrcSI | DstDX | String, outs, check_perm_out), /* outsb, outsw/outsd */
 	/* 0x70 - 0x7F */
 	X16(D(SrcImmByte)),
 	/* 0x80 - 0x87 */
@@ -3182,8 +3184,8 @@ static struct opcode opcode_table[256] = {
 	/* 0xE8 - 0xEF */
 	D(SrcImm | Stack), D(SrcImm | ImplicitOps),
 	D(SrcImmFAddr | No64), D(SrcImmByte | ImplicitOps),
-	D2bvIP(SrcNone | DstAcc,     in,  check_perm_in),
-	D2bvIP(SrcAcc | ImplicitOps, out, check_perm_out),
+	D2bvIP(SrcDX | DstAcc, in,  check_perm_in),
+	D2bvIP(SrcAcc | DstDX, out, check_perm_out),
 	/* 0xF0 - 0xF7 */
 	N, DI(ImplicitOps, icebp), N, N,
 	DI(ImplicitOps | Priv, hlt), D(ImplicitOps),
@@ -3580,6 +3582,12 @@ done_prefixes:
 		memop.bytes = c->op_bytes + 2;
 		goto srcmem_common;
 		break;
+	case SrcDX:
+		c->src.type = OP_REG;
+		c->src.bytes = 2;
+		c->src.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->src);
+		break;
 	}
 
 	if (rc != X86EMUL_CONTINUE)
@@ -3649,6 +3657,12 @@ done_prefixes:
 		c->dst.addr.mem.seg = VCPU_SREG_ES;
 		c->dst.val = 0;
 		break;
+	case DstDX:
+		c->dst.type = OP_REG;
+		c->dst.bytes = 2;
+		c->dst.addr.reg = &c->regs[VCPU_REGS_RDX];
+		fetch_register_operand(&c->dst);
+		break;
 	case ImplicitOps:
 		/* Special instructions do their own operand decoding. */
 	default:
@@ -3993,7 +4007,6 @@ special_insn:
 		break;
 	case 0xec: /* in al,dx */
 	case 0xed: /* in (e/r)ax,dx */
-		c->src.val = c->regs[VCPU_REGS_RDX];
 	do_io_in:
 		if (!pio_in_emulated(ctxt, c->dst.bytes, c->src.val,
 				     &c->dst.val))
@@ -4001,7 +4014,6 @@ special_insn:
 		break;
 	case 0xee: /* out dx,al */
 	case 0xef: /* out dx,(e/r)ax */
-		c->dst.val = c->regs[VCPU_REGS_RDX];
 	do_io_out:
 		ops->pio_out_emulated(ctxt, c->src.bytes, c->dst.val,
 				      &c->src.val, 1);

Re: KVM: x86: use proper port value when checking io instruction permission (v3)

[Marcelo Tosatti]

On Sun, May 29, 2011 at 11:34:43AM +0300, Avi Kivity wrote:
> On 05/26/2011 02:56 PM, Marcelo Tosatti wrote:
> >Commit fa4491a6b667304 moved the permission check for io instructions
> >to the ->check_perm callback. It failed to copy the port value from RDX
> >register for string and "in,out ax,dx" instructions.
> >
> >Fix it by reading RDX register at decode stage when appropriate.
> >
> >Fixes FC8.32 installation.
> >
> >diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >index 3bc6b7a..fc3d2d9 100644
> >--- a/arch/x86/kvm/emulate.c
> >+++ b/arch/x86/kvm/emulate.c
> >@@ -47,7 +47,7 @@
> >  #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)
> >+#define DstMask     ((7<<1) | (1<<18))
> 
> Ouch, why??
> 
> My comment about bits 19-20 was only about the shifting of of Prot
> etc. from 21+ to 22+.

Oh, OK, please check v4.

Re: KVM: x86: use proper port value when checking io instruction permission (v4)

[Avi Kivity]

On 05/30/2011 09:23 PM, Marcelo Tosatti wrote:
> Commit fa4491a6b667304 moved the permission check for io instructions
> to the ->check_perm callback. It failed to copy the port value from RDX
> register for string and "in,out ax,dx" instructions.
>
> Fix it by reading RDX register at decode stage when appropriate.
>
> Fixes FC8.32 installation.
>

Looks good.

We should really unify Src/Dst encoding and fetching.

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