[PATCH] KVM: fix mov immediate emulation for 64-bit operands

[PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Nadav Amit]

MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
The previous emulation implementation assumes the operand is no longer than 32.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
arch/x86/kvm/emulate.c |    6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 05a562b..65d1d31 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3502,7 +3502,8 @@ static unsigned imm_size(struct x86_emulate_ctxt *ctxt)
	unsigned size;

	size = (ctxt->d & ByteOp) ? 1 : ctxt->op_bytes;
-	if (size == 8)
+	/* Immediates are usually no longer than 4 bytes */
+	if (size == 8 && ((ctxt->b & 0xF8) != 0xB8 || ctxt->twobyte))
		size = 4;
	return size;
}
@@ -3526,6 +3527,9 @@ static int decode_imm(struct x86_emulate_ctxt *ctxt, struct operand *op,
	case 4:
		op->val = insn_fetch(s32, ctxt);
		break;
+	case 8:
+		op->val = insn_fetch(s64, ctxt);
+		break;
	}
	if (!sign_extension) {
		switch (op->bytes) {
-- 
1.7.4.1

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[H. Peter Anvin]

On 01/07/2012 12:21 PM, Nadav Amit wrote:
> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> The previous emulation implementation assumes the operand is no longer than 32.
> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>

There are exactly two such instructions: MOV immediate (B8-BF) and MOV
moff (A0-A3); you may want to check the latter too.

	-hpa

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Nadav Amit]

On Jan 7, 2012, at 10:25 PM, H. Peter Anvin wrote:

> On 01/07/2012 12:21 PM, Nadav Amit wrote:
>> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
>> The previous emulation implementation assumes the operand is no longer than 32.
>> 
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> 
> There are exactly two such instructions: MOV immediate (B8-BF) and MOV
> moff (A0-A3); you may want to check the latter too.
> 
> 	-hpa
> 

These instructions (A0-A3) seem to be already covered by the decode_abs function.

Regards,
Nadav

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Takuya Yoshikawa]

Hi,

Nadav Amit <nadav.amit@gmail.com> wrote: 
> On Jan 7, 2012, at 10:25 PM, H. Peter Anvin wrote:
> 
> > On 01/07/2012 12:21 PM, Nadav Amit wrote:
> >> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> >> The previous emulation implementation assumes the operand is no longer than 32.
> >> 
> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> > 
> > There are exactly two such instructions: MOV immediate (B8-BF) and MOV
> > moff (A0-A3); you may want to check the latter too.
> > 
> > 	-hpa
> > 
> 
> These instructions (A0-A3) seem to be already covered by the decode_abs function.

Like these how about introducing a new flag and change the following entries in the
decode table to indicate possible 64bit immediate:

	/* 0xB8 - 0xBF */
	X8(I(DstReg | SrcImm | Mov, em_mov)),	

Checking the opcode byte at the operand decoding stage, like below, does not look nice:
	(IMO so better ask Avi)

+	if (size == 8 && ((ctxt->b & 0xF8) != 0xB8 || ctxt->twobyte))
		size = 4;

I am now cleaning up x86_decode_insn() to make each decoding stage clearer.

	Takuya

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Nadav Amit]

On Sun, Jan 8, 2012 at 3:26 AM, Takuya Yoshikawa
<takuya.yoshikawa@gmail.com> wrote:
> Hi,
>
> Nadav Amit <nadav.amit@gmail.com> wrote:
>> On Jan 7, 2012, at 10:25 PM, H. Peter Anvin wrote:
>>
>> > On 01/07/2012 12:21 PM, Nadav Amit wrote:
>> >> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
>> >> The previous emulation implementation assumes the operand is no longer than 32.
>> >>
>> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>> >
>> > There are exactly two such instructions: MOV immediate (B8-BF) and MOV
>> > moff (A0-A3); you may want to check the latter too.
>> >
>> >     -hpa
>> >
>>
>> These instructions (A0-A3) seem to be already covered by the decode_abs function.
>
> Like these how about introducing a new flag and change the following entries in the
> decode table to indicate possible 64bit immediate:
>
>        /* 0xB8 - 0xBF */
>        X8(I(DstReg | SrcImm | Mov, em_mov)),
>
> Checking the opcode byte at the operand decoding stage, like below, does not look nice:
>        (IMO so better ask Avi)
>
> +       if (size == 8 && ((ctxt->b & 0xF8) != 0xB8 || ctxt->twobyte))
>                size = 4;
>

I agree. I remembered these flags are expensive (from the time flags
were set in u32).
I guess I can add OpImm64.
Another less preferable alternative is to add a misc. flag or reuse
another flag.

Avi, please acknowledge adding OpImm64.

Regards,
Nadav

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Avi Kivity]

On 01/08/2012 10:47 AM, Nadav Amit wrote:
> On Sun, Jan 8, 2012 at 3:26 AM, Takuya Yoshikawa
> <takuya.yoshikawa@gmail.com> wrote:
> > Hi,
> >
> > Nadav Amit <nadav.amit@gmail.com> wrote:
> >> On Jan 7, 2012, at 10:25 PM, H. Peter Anvin wrote:
> >>
> >> > On 01/07/2012 12:21 PM, Nadav Amit wrote:
> >> >> MOV immediate instruction (opcodes 0xB8-0xBF) may take 64-bit operand.
> >> >> The previous emulation implementation assumes the operand is no longer than 32.
> >> >>
> >> >> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> >> >
> >> > There are exactly two such instructions: MOV immediate (B8-BF) and MOV
> >> > moff (A0-A3); you may want to check the latter too.
> >> >
> >> >     -hpa
> >> >
> >>
> >> These instructions (A0-A3) seem to be already covered by the decode_abs function.
> >
> > Like these how about introducing a new flag and change the following entries in the
> > decode table to indicate possible 64bit immediate:
> >
> >        /* 0xB8 - 0xBF */
> >        X8(I(DstReg | SrcImm | Mov, em_mov)),
> >
> > Checking the opcode byte at the operand decoding stage, like below, does not look nice:
> >        (IMO so better ask Avi)
> >
> > +       if (size == 8 && ((ctxt->b & 0xF8) != 0xB8 || ctxt->twobyte))
> >                size = 4;
> >
>
> I agree. I remembered these flags are expensive (from the time flags
> were set in u32).
> I guess I can add OpImm64.
> Another less preferable alternative is to add a misc. flag or reuse
> another flag.
>
> Avi, please acknowledge adding OpImm64.

Yes, OpImm64 is the cleanest IMO.  Note it doesn't even cost us a bit.

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

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Avi Kivity]

On 01/08/2012 04:05 PM, Avi Kivity wrote:
> >
> > Avi, please acknowledge adding OpImm64.
>
> Yes, OpImm64 is the cleanest IMO.  Note it doesn't even cost us a bit.

Note, usually I'd ask for a unit test to accompany the fix, but the
current framework only supports testing instructions that have a memroy
operand.  So you're off the hook for now, unless you'd like to extend
the framework.

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

Re: [PATCH] KVM: fix mov immediate emulation for 64-bit operands

[Nadav Amit]

On Sun, Jan 8, 2012 at 4:08 PM, Avi Kivity <avi@redhat.com> wrote:
> On 01/08/2012 04:05 PM, Avi Kivity wrote:
>> >
>> > Avi, please acknowledge adding OpImm64.
>>
>> Yes, OpImm64 is the cleanest IMO.  Note it doesn't even cost us a bit.

Very well - I'll submit a revised patch later.

>
> Note, usually I'd ask for a unit test to accompany the fix, but the
> current framework only supports testing instructions that have a memroy
> operand.  So you're off the hook for now, unless you'd like to extend
> the framework.

Unfortunately, I don't. I am doing some odd things with KVM that cause
me to encounter emulator bugs.
I am likely to submit another patch or two as the emulation code has
additional "issues" handling EPT violations (especially on non-memory
operands).

Regards,
Nadav