[PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Mohammed Gamal]

This patch adds 'cld' and 'std' instructions to the emulator

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>

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

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index d5da7f1..9c72a77 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1755,6 +1755,14 @@ special_insn:
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
+	case 0xfc: /* cld */
+		ctxt->eflags &= ~EFLG_DF;
+		c->dst.type = OP_NONE;  /* Disable writeback. */
+		break;
+	case 0xfd: /* std */
+		ctxt->eflags |= EFLG_DF;
+		c->dst.type = OP_NONE;  /* Disable writeback. */
+		break;
 	case 0xfe ... 0xff:	/* Grp4/Grp5 */
 		rc = emulate_grp45(ctxt, ops);
 		if (rc != 0)
-- 
1.5.4.3

Re: [PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Avi Kivity]

Mohammed Gamal wrote:
> This patch adds 'cld' and 'std' instructions to the emulator
>
>   

I presume for big real mode?  Which guest wants it?

> @@ -1755,6 +1755,14 @@ special_insn:
>  		ctxt->eflags |= X86_EFLAGS_IF;
>  		c->dst.type = OP_NONE;	/* Disable writeback. */
>  		break;
> +	case 0xfc: /* cld */
> +		ctxt->eflags &= ~EFLG_DF;
> +		c->dst.type = OP_NONE;  /* Disable writeback. */
> +		break;
> +	case 0xfd: /* std */
> +		ctxt->eflags |= EFLG_DF;
> +		c->dst.type = OP_NONE;  /* Disable writeback. */
> +		break;
>   

You need to add non-zero entries in opcode_table[] for this to work.  
Also, why disable writeback?  If there is not DstSomething in 
opcode_table[], c->dst.type will be OP_NONE anyway.

I would like to see test cases for emulator changes.  See 
users/test/x86/emulator.c.  Of course, right now it is impossible to 
cause cld and std to execute in the emulator.  But with the proposed 
invalid_guest_state() change, it should be easy to to cause this state 
and force emulation.

So how about adding optional invalid_guest_state() support (controlled 
by a module parameter), then start adding instructions and test cases, 
then removing the module parameter when everything works?

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

Re: [PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Mohammed Gamal]

On Thu, Jul 31, 2008 at 12:57 PM, Avi Kivity <avi@qumranet.com> wrote:
> Mohammed Gamal wrote:
>>
>> This patch adds 'cld' and 'std' instructions to the emulator
>>
>>
>
> I presume for big real mode?  Which guest wants it?

Actually I was experimenting with checking guest state and emulating
on mode switches. When I was doing so, it used to happen with all
guests, so I guess it maybe something related to the BIOS not a
specific guest.

BTW, the other way we can do is still handle vmentry failures. I
discovered that the problem that caused occasional exits with
Guillaume's patches is very likely to be that not enough checks on the
guest state.

The only check was if CS RPL equals SS RPL, so what happens is that
when a vmexit and then a vmentry occurs CS and SS RPL maybe equal, but
other checks might cause a vmentry failure. I've written a function to
check that guest state is VMX valid - although not all checks are
implemented yet, and I no more get the occasional vmexits.

>
>> @@ -1755,6 +1755,14 @@ special_insn:
>>                ctxt->eflags |= X86_EFLAGS_IF;
>>                c->dst.type = OP_NONE;  /* Disable writeback. */
>>                break;
>> +       case 0xfc: /* cld */
>> +               ctxt->eflags &= ~EFLG_DF;
>> +               c->dst.type = OP_NONE;  /* Disable writeback. */
>> +               break;
>> +       case 0xfd: /* std */
>> +               ctxt->eflags |= EFLG_DF;
>> +               c->dst.type = OP_NONE;  /* Disable writeback. */
>> +               break;
>>
>
> You need to add non-zero entries in opcode_table[] for this to work.  Also,
> why disable writeback?  If there is not DstSomething in opcode_table[],
> c->dst.type will be OP_NONE anyway.
>

Oh! completely forgot about that, sorry!

> I would like to see test cases for emulator changes.  See
> users/test/x86/emulator.c.  Of course, right now it is impossible to cause
> cld and std to execute in the emulator.  But with the proposed
> invalid_guest_state() change, it should be easy to to cause this state and
> force emulation.
>
> So how about adding optional invalid_guest_state() support (controlled by a
> module parameter), then start adding instructions and test cases, then
> removing the module parameter when everything works?
>

Sounds good

Re: [PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Avi Kivity]

Mohammed Gamal wrote:
> On Thu, Jul 31, 2008 at 12:57 PM, Avi Kivity <avi@qumranet.com> wrote:
>   
>> Mohammed Gamal wrote:
>>     
>>> This patch adds 'cld' and 'std' instructions to the emulator
>>>
>>>
>>>       
>> I presume for big real mode?  Which guest wants it?
>>     
>
> Actually I was experimenting with checking guest state and emulating
> on mode switches. When I was doing so, it used to happen with all
> guests, so I guess it maybe something related to the BIOS not a
> specific guest.
>   

There is indeed a cld in rombios.c (after rombios32_05), but it appears 
to be after all segments have been initialized.

> BTW, the other way we can do is still handle vmentry failures. I
> discovered that the problem that caused occasional exits with
> Guillaume's patches is very likely to be that not enough checks on the
> guest state.
>
> The only check was if CS RPL equals SS RPL, so what happens is that
> when a vmexit and then a vmentry occurs CS and SS RPL maybe equal, but
> other checks might cause a vmentry failure. I've written a function to
> check that guest state is VMX valid - although not all checks are
> implemented yet, and I no more get the occasional vmexits.
>   

I'm still worried about undoing the interrupt injection setup.

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

Re: [PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Mohammed Gamal]

Here is an update patch.

This adds 'cld' and 'std' instructions. The group table entries are 
added.

Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/x86_emulate.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 07a277c..4c7ca7f 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -182,7 +182,7 @@ static u16 opcode_table[256] = {
 	ImplicitOps, ImplicitOps, Group | Group3_Byte, Group | Group3,
 	/* 0xF8 - 0xFF */
 	ImplicitOps, 0, ImplicitOps, ImplicitOps,
-	0, 0, Group | Group4, Group | Group5,
+	ImplicitOps, ImplicitOps, Group | Group4, Group | Group5,
 };
 
 static u16 twobyte_table[256] = {
@@ -1757,6 +1757,14 @@ special_insn:
 		ctxt->eflags |= X86_EFLAGS_IF;
 		c->dst.type = OP_NONE;	/* Disable writeback. */
 		break;
+	case 0xfc: /* cld */
+		ctxt->eflags &= ~EFLG_DF;
+		c->dst.type = OP_NONE;	/* Disable writeback. */
+		break;
+	case 0xfd: /* std */
+		ctxt->eflags |= EFLG_DF;
+		c->dst.type = OP_NONE;	/* Disable writeback. */
+		break;
 	case 0xfe ... 0xff:	/* Grp4/Grp5 */
 		rc = emulate_grp45(ctxt, ops);
 		if (rc != 0)
-- 
1.5.4.3

Re: [PATCH][RESEND] x86_emulator: Emulate cld and std instruction

[Andrea Arcangeli]

On Thu, Jul 31, 2008 at 01:25:40PM +0300, Mohammed Gamal wrote:
> The only check was if CS RPL equals SS RPL, so what happens is that
> when a vmexit and then a vmentry occurs CS and SS RPL maybe equal, but

Yes same problem here, vmentry fails because CS is set to >16 bit
value (I think) but the last two bits of CS and SS are the same. That
surely is the wrong check to know if we've to invoke emulation on
vmentry failure.

> other checks might cause a vmentry failure. I've written a function to
> check that guest state is VMX valid - although not all checks are
> implemented yet, and I no more get the occasional vmexits.

Can you send me your latest patch so I can test. I tried to force
unconditional emulation on invalid guest state exception generated by
vmentry failure but even emulation fails for me, and then your patch
sets SS to sane value but that's meaningless here as it's CS that is
set to >16bit value. I've yet to trace where emulation fails, but I
suspect it's fetching instructions from the wrong place by not
emulating the segment-caches set by protected mode, maybe not.