Re: [Patch 6/7] RealMode: Adds support for mov seg, r (0x8e) instruction

Re: [Patch 6/7] RealMode: Adds support for mov seg, r (0x8e) instruction

[Avi Kivity]

Guillaume Thouvenin wrote:
> Adds support for mov r, sreg (0x8c) instruction. 
> We also introduce a table called vcpu_sreg_table[] to avoid an
> unreadable switch. This table is also used to emulate instruction 0X8c. 
>
>  
> +/*
> + * This table is used by instruction like Ox8c to avoid long ugly switch.
> + * The order is important, index is given by c->modrm_reg.
> + */
> +static u16 vcpu_sreg_table[] = {
> +	VCPU_SREG_ES,
> +	VCPU_SREG_CS,
> +	VCPU_SREG_SS,
> +	VCPU_SREG_DS,
> +	VCPU_SREG_FS,
> +	VCPU_SREG_GS,
> +};
> +
>
>   

'static u16' -> 'static const u8'

Also, you can make the ordering explicit:

static const u8 vcpu_sreg_table[] = {
    [0]: VCPU_SREG_ES,
    ...
};


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

Re: [Patch 6/7] RealMode: Adds support for mov seg, r (0x8e) instruction

[Guillaume Thouvenin]

Adds support for mov r, sreg (0x8c) instruction. 
We also introduce a table called vcpu_sreg_table[] to avoid an
unreadable switch. This table is also used to emulate instruction 0X8c.

Changelog:
 - Change 'static u16' to 'static const u8'
 - Make the ordering explicit

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>

---
 arch/x86/kvm/x86_emulate.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 21d7ff6..b852fcb 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -138,7 +138,8 @@ static u16 opcode_table[256] = {
 	/* 0x88 - 0x8F */
 	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
 	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
-	0, ModRM | DstReg, 0, Group | Group1A,
+	0, ModRM | DstReg,
+	DstReg | SrcMem | ModRM | Mov, Group | Group1A,
 	/* 0x90 - 0x9F */
 	0, 0, 0, 0, 0, 0, 0, 0,
 	0, 0, 0, 0, ImplicitOps | Stack, ImplicitOps | Stack, 0, 0,
@@ -288,6 +289,19 @@ static u16 group2_table[] = {
 	SrcMem16 | ModRM | Mov, 0,
 };
 
+/*
+ * This table is used by instruction like Ox8c to avoid long ugly switch.
+ * The order is important, index is given by c->modrm_reg.
+ */
+static const u8 vcpu_sreg_table[] = {
+	[0] = VCPU_SREG_ES,
+	[1] = VCPU_SREG_CS,
+	[2] = VCPU_SREG_SS,
+	[3] = VCPU_SREG_DS,
+	[4] = VCPU_SREG_FS,
+	[5] = VCPU_SREG_GS,
+};
+
 /* EFLAGS bit definitions. */
 #define EFLG_OF (1<<11)
 #define EFLG_DF (1<<10)
@@ -1520,6 +1534,28 @@ special_insn:
 	case 0x8d: /* lea r16/r32, m */
 		c->dst.val = c->modrm_ea;
 		break;
+	case 0x8e: { /* mov seg, r/m16 */
+		uint16_t sel;
+		int type_bits;
+		int err;
+
+		sel = c->src.val;
+		if (c->modrm_reg <= 5) {
+			type_bits = (c->modrm_reg == 1) ? 9 : 1;
+			err = kvm_load_segment_descriptor(ctxt->vcpu, sel, type_bits,
+							  vcpu_sreg_table[c->modrm_reg]);
+		} else {
+			printk(KERN_INFO "Invalid segreg in modrm byte 0x%02x\n",
+					c->modrm);
+			goto cannot_emulate;
+		}
+
+		if (err < 0)
+			goto cannot_emulate;
+
+		c->dst.type = OP_NONE;  /* Disable writeback. */
+		break;
+	}
 	case 0x8f:		/* pop (sole member of Grp1a) */
 		rc = emulate_grp1a(ctxt, ops);
 		if (rc != 0)
-- 
1.5.5.1

Re: [Patch 7/7] RealMode: Adds support for mov r, sreg (0x8c) instruction

[Avi Kivity]

Guillaume Thouvenin wrote:
> Adds support for mov r, sreg (0x8c) instruction
>
> Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
> Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
>
> ---
>  arch/x86/kvm/x86_emulate.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index 106d714..35767e7 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -138,7 +138,7 @@ static u16 opcode_table[256] = {
>  	/* 0x88 - 0x8F */
>  	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
>  	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
> -	0, ModRM | DstReg,
> +	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
>  	DstReg | SrcMem | ModRM | Mov, Group | Group1A,
>  	/* 0x90 - 0x9F */
>  	0, 0, 0, 0, 0, 0, 0, 0,
> @@ -1531,6 +1531,25 @@ special_insn:
>  		break;
>  	case 0x88 ... 0x8b:	/* mov */
>  		goto mov;
> +	case 0x8c: { /* mov r/m, sreg */
> +		struct kvm_segment segreg;
> +
> +		if (c->modrm_mod == 0x3)
> +			c->src.val = c->modrm_val;
>   

What's this bit?

> +
> +		if (c->modrm_reg <= 5)
> +			kvm_get_segment(ctxt->vcpu, &segreg, vcpu_sreg_table[c->modrm_reg]);
> +		else {
> +			printk(KERN_INFO "0x8c: Invalid segreg in modrm byte 0x%02x\n",
> +					 c->modrm);
> +			goto cannot_emulate;
> +		}
> +		c->dst.val = segreg.selector;
> +		c->dst.bytes = 2;
> +		c->dst.ptr = (unsigned long *)decode_register(c->modrm_rm, c->regs,
> +							      c->d & ByteOp);
>   

Aren't dst.bytes and dst.ptr already set by the decoder?

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

Re: [Patch 7/7] RealMode: Adds support for mov r, sreg (0x8c) instruction

[Guillaume Thouvenin]

On Tue, 27 May 2008 13:14:36 +0300
Avi Kivity <avi@qumranet.com> wrote:

> Guillaume Thouvenin wrote:
> > Adds support for mov r, sreg (0x8c) instruction
> >
> > Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
> > Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
> >
> > ---
> >  arch/x86/kvm/x86_emulate.c |   21 ++++++++++++++++++++-
> >  1 files changed, 20 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> > index 106d714..35767e7 100644
> > --- a/arch/x86/kvm/x86_emulate.c
> > +++ b/arch/x86/kvm/x86_emulate.c
> > @@ -138,7 +138,7 @@ static u16 opcode_table[256] = {
> >  	/* 0x88 - 0x8F */
> >  	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
> >  	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
> > -	0, ModRM | DstReg,
> > +	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
> >  	DstReg | SrcMem | ModRM | Mov, Group | Group1A,
> >  	/* 0x90 - 0x9F */
> >  	0, 0, 0, 0, 0, 0, 0, 0,
> > @@ -1531,6 +1531,25 @@ special_insn:
> >  		break;
> >  	case 0x88 ... 0x8b:	/* mov */
> >  		goto mov;
> > +	case 0x8c: { /* mov r/m, sreg */
> > +		struct kvm_segment segreg;
> > +
> > +		if (c->modrm_mod == 0x3)
> > +			c->src.val = c->modrm_val;
> >   
> 
> What's this bit?

Right it's unneeded because c->src.val is update in "SrcReg:"

> > +
> > +		if (c->modrm_reg <= 5)
> > +			kvm_get_segment(ctxt->vcpu, &segreg, vcpu_sreg_table[c->modrm_reg]);
> > +		else {
> > +			printk(KERN_INFO "0x8c: Invalid segreg in modrm byte 0x%02x\n",
> > +					 c->modrm);
> > +			goto cannot_emulate;
> > +		}
> > +		c->dst.val = segreg.selector;
> > +		c->dst.bytes = 2;
> > +		c->dst.ptr = (unsigned long *)decode_register(c->modrm_rm, c->regs,
> > +							      c->d & ByteOp);
> >   
> 
> Aren't dst.bytes and dst.ptr already set by the decoder?

Yes you're right. So I'll remove the c->dst.ptr. For the c->dst.bytes
I copy it from Xen code where I see the following code:

if (c->dst.type == OP_MEM) 
	c->dst.bytes = 2;

I added a trace and I didn't see any case where c->dst.type == OP_MEM.
So I will also remove the update of c->dst.bytes that seems well done
by the decoder.

Regards,
Guillaume

Re: [Patch 7/7] RealMode: Adds support for mov r, sreg (0x8c) instruction

[Guillaume Thouvenin]

Adds support for mov r, sreg (0x8c) instruction

Changelog:
  - Remove code that is unneeded in the instruction because it is
already executed by the decoder.

Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net> 
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>

---
 arch/x86/kvm/x86_emulate.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index b852fcb..1093281 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -138,7 +138,7 @@ static u16 opcode_table[256] = {
 	/* 0x88 - 0x8F */
 	ByteOp | DstMem | SrcReg | ModRM | Mov, DstMem | SrcReg | ModRM | Mov,
 	ByteOp | DstReg | SrcMem | ModRM | Mov, DstReg | SrcMem | ModRM | Mov,
-	0, ModRM | DstReg,
+	DstMem | SrcReg | ModRM | Mov, ModRM | DstReg,
 	DstReg | SrcMem | ModRM | Mov, Group | Group1A,
 	/* 0x90 - 0x9F */
 	0, 0, 0, 0, 0, 0, 0, 0,
@@ -1531,6 +1531,19 @@ special_insn:
 		break;
 	case 0x88 ... 0x8b:	/* mov */
 		goto mov;
+	case 0x8c: { /* mov r/m, sreg */
+		struct kvm_segment segreg;
+
+		if (c->modrm_reg <= 5)
+			kvm_get_segment(ctxt->vcpu, &segreg, vcpu_sreg_table[c->modrm_reg]);
+		else {
+			printk(KERN_INFO "0x8c: Invalid segreg in modrm byte 0x%02x\n",
+					 c->modrm);
+			goto cannot_emulate;
+		}
+		c->dst.val = segreg.selector;
+		break;
+	}
 	case 0x8d: /* lea r16/r32, m */
 		c->dst.val = c->modrm_ea;
 		break;
-- 
1.5.5.1

Re: [Patch 4/7] RealMode: Adds support for jmp far 0xea

[Anthony Liguori]

Guillaume Thouvenin wrote:
>  Adds support for jmp far 0xea instruction
>
>
> Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
> Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
>
> ---
>  arch/x86/kvm/x86_emulate.c |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
> index a928aa6..48b62cc 100644
> --- a/arch/x86/kvm/x86_emulate.c
> +++ b/arch/x86/kvm/x86_emulate.c
> @@ -168,7 +168,8 @@ static u16 opcode_table[256] = {
>  	/* 0xE0 - 0xE7 */
>  	0, 0, 0, 0, 0, 0, 0, 0,
>  	/* 0xE8 - 0xEF */
> -	ImplicitOps | Stack, SrcImm|ImplicitOps, 0, SrcImmByte|ImplicitOps,
> +	ImplicitOps | Stack, SrcImm | ImplicitOps,
> +	ImplicitOps, SrcImmByte | ImplicitOps,
>  	0, 0, 0, 0,
>  	/* 0xF0 - 0xF7 */
>  	0, 0, 0, 0,
> @@ -1661,7 +1662,33 @@ special_insn:
>  		break;
>  	}
>  	case 0xe9: /* jmp rel */
> -	case 0xeb: /* jmp rel short */
> +		goto jmp;
>   

The cases don't have to be in ascending order.  Much better to leave 
0xeb here then have a jmp cross case boundaries.

Regards,

Anthony Liguori

> +	case 0xea: /* jmp far */ {
> +		uint32_t eip;
> +		uint16_t sel;
> +
> +		switch (c->op_bytes) {
> +		case 2:
> +			eip = insn_fetch(u16, 2, c->eip);
> +			break;
> +		case 4:
> +			eip = insn_fetch(u32, 4, c->eip);
> +			break;
> +		default:
> +			DPRINTF("jmp far: Invalid op_bytes\n");
> +			goto cannot_emulate;
> +		}
> +		sel = insn_fetch(u16, 2, c->eip);
> +		if (kvm_load_segment_descriptor(ctxt->vcpu, sel, 9, VCPU_SREG_CS) < 0) {
> +			DPRINTF("jmp far: Failed to load CS descriptor\n");
> +			goto cannot_emulate;
> +		}
> +
> +		c->eip = eip;
> +		break;
> +	}
> +	case 0xeb:
> +	      jmp:		/* jmp rel short */
>  		jmp_rel(c, c->src.val);
>  		c->dst.type = OP_NONE; /* Disable writeback. */
>  		break;
>   

Re: [Patch 4/7] RealMode: Adds support for jmp far 0xea

[Avi Kivity]

Anthony Liguori wrote:
>> @@ -1661,7 +1662,33 @@ special_insn:
>>          break;
>>      }
>>      case 0xe9: /* jmp rel */
>> -    case 0xeb: /* jmp rel short */
>> +        goto jmp;
>>   
>
> The cases don't have to be in ascending order.  Much better to leave 
> 0xeb here then have a jmp cross case boundaries.

I find that sorting helps finding opcodes quickly.  A simple search 
doesn't work due to ranges.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.