[RFC] KVM: x86: emulate movdqa

[RFC] KVM: x86: emulate movdqa

[Stefan Hajnoczi]

An Ubuntu 9.10 Karmic Koala guest is unable to boot or install due to
missing movdqa emulation:

kvm_exit: reason EXCEPTION_NMI rip 0x7fef3e025a7b info 7fef3e799000 80000b0e
kvm_page_fault: address 7fef3e799000 error_code f
kvm_emulate_insn: 0:7fef3e025a7b: 66 0f 7f 07 (prot64)

movdqa %xmm0,(%rdi)

This patch adds movdqa to the emulator but does not implement #GP when
the memory operand is unaligned to 16 bytes.  I'm not sure whether
alignment checking can be implemented as an opcode .flag or if it needs
to be done in em_movdqa().

A more fundamental question: why do we have to emulate this guest
userspace SSE instruction in the first place?  This host machine lacks
EPT but can't we service the page fault and then retry execution inside
the guest?

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 arch/x86/kvm/emulate.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8547958..0424569 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2653,6 +2653,13 @@ static int em_mov_sreg_rm(struct x86_emulate_ctxt *ctxt)
 	return load_segment_descriptor(ctxt, sel, ctxt->modrm_reg);
 }
 
+static int em_movdqa(struct x86_emulate_ctxt *ctxt)
+{
+	/* TODO alignment */
+	memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
+	return X86EMUL_CONTINUE;
+}
+
 static int em_movdqu(struct x86_emulate_ctxt *ctxt)
 {
 	memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
@@ -3115,7 +3122,7 @@ static struct opcode group11[] = {
 };
 
 static struct gprefix pfx_0f_6f_0f_7f = {
-	N, N, N, I(Sse, em_movdqu),
+	N, I(Sse, em_movdqa), N, I(Sse, em_movdqu),
 };
 
 static struct opcode opcode_table[256] = {
-- 
1.7.7.3

Re: [RFC] KVM: x86: emulate movdqa

[Avi Kivity]

On 01/07/2012 12:26 PM, Stefan Hajnoczi wrote:
> An Ubuntu 9.10 Karmic Koala guest is unable to boot or install due to
> missing movdqa emulation:
>
> kvm_exit: reason EXCEPTION_NMI rip 0x7fef3e025a7b info 7fef3e799000 80000b0e
> kvm_page_fault: address 7fef3e799000 error_code f
> kvm_emulate_insn: 0:7fef3e025a7b: 66 0f 7f 07 (prot64)

Install udis86 (and udsi86-devel) and build trace-cmd; it will decode
the opcode for you.

>
> movdqa %xmm0,(%rdi)
>
> This patch adds movdqa to the emulator but does not implement #GP when
> the memory operand is unaligned to 16 bytes.  I'm not sure whether
> alignment checking can be implemented as an opcode .flag or if it needs
> to be done in em_movdqa().

It should actually be automatic when the Sse flag is present, since it's
the norm for almost all SSE instructions.  There should be a .flag to
override it for movdqu.

> A more fundamental question: why do we have to emulate this guest
> userspace SSE instruction in the first place?  This host machine lacks
> EPT but can't we service the page fault and then retry execution inside
> the guest?

Not when the target is mmio - there is no possible mapping.  With your
patch, is there a kvm_mmio trace right after the movdqa emulation?

>  
> +static int em_movdqa(struct x86_emulate_ctxt *ctxt)
> +{
> +	/* TODO alignment */
> +	memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
> +	return X86EMUL_CONTINUE;
> +}

em_mov() should be adjusted to work here.

> +
>  static int em_movdqu(struct x86_emulate_ctxt *ctxt)
>  {
>  	memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
> @@ -3115,7 +3122,7 @@ static struct opcode group11[] = {
>  };
>  
>  static struct gprefix pfx_0f_6f_0f_7f = {
> -	N, N, N, I(Sse, em_movdqu),
> +	N, I(Sse, em_movdqa), N, I(Sse, em_movdqu),
>  };
>  

Need the Mov flag too (I see it's missing for movdqu as well); otherwise
the emulator will RMW the destination.

Don't forget a unit test (for both the aligned and unaligned case).

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

Re: [RFC] KVM: x86: emulate movdqa

[Stefan Hajnoczi]

On Sun, Jan 8, 2012 at 10:32 AM, Avi Kivity <avi@redhat.com> wrote:
> On 01/07/2012 12:26 PM, Stefan Hajnoczi wrote:
>>
>> movdqa %xmm0,(%rdi)
>>
>> This patch adds movdqa to the emulator but does not implement #GP when
>> the memory operand is unaligned to 16 bytes.  I'm not sure whether
>> alignment checking can be implemented as an opcode .flag or if it needs
>> to be done in em_movdqa().
>
> It should actually be automatic when the Sse flag is present, since it's
> the norm for almost all SSE instructions.  There should be a .flag to
> override it for movdqu.

When writing a kvm-unit-test for movdqa I found that alignment
checking happens before the page fault (makes sense).  That means
misalignment is detected by the CPU while still in guest mode.  The
emulator never sees the instruction because #GP is raised and handled
in the guest.

I also didn't see other instances of alignment checking in the
emulator (e.g. eflags AC).  I guess the same situation applies there.

Can you think of a case where we need to perform alignment checking in
the emulator?

>> A more fundamental question: why do we have to emulate this guest
>> userspace SSE instruction in the first place?  This host machine lacks
>> EPT but can't we service the page fault and then retry execution inside
>> the guest?
>
> Not when the target is mmio - there is no possible mapping.  With your
> patch, is there a kvm_mmio trace right after the movdqa emulation?

You are right.  These instructions are accessing the VGA area at 0xa0000.

>> +static int em_movdqa(struct x86_emulate_ctxt *ctxt)
>> +{
>> +     /* TODO alignment */
>> +     memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
>> +     return X86EMUL_CONTINUE;
>> +}
>
> em_mov() should be adjusted to work here.

Ok.

>> +
>>  static int em_movdqu(struct x86_emulate_ctxt *ctxt)
>>  {
>>       memcpy(&ctxt->dst.vec_val, &ctxt->src.vec_val, ctxt->op_bytes);
>> @@ -3115,7 +3122,7 @@ static struct opcode group11[] = {
>>  };
>>
>>  static struct gprefix pfx_0f_6f_0f_7f = {
>> -     N, N, N, I(Sse, em_movdqu),
>> +     N, I(Sse, em_movdqa), N, I(Sse, em_movdqu),
>>  };
>>
>
> Need the Mov flag too (I see it's missing for movdqu as well); otherwise
> the emulator will RMW the destination.

The Mov is already given by the GP() group prefix.

Stefan

Re: [RFC] KVM: x86: emulate movdqa

[Gleb Natapov]

On Sun, Jan 08, 2012 at 04:21:56PM +0000, Stefan Hajnoczi wrote:
> On Sun, Jan 8, 2012 at 10:32 AM, Avi Kivity <avi@redhat.com> wrote:
> > On 01/07/2012 12:26 PM, Stefan Hajnoczi wrote:
> >>
> >> movdqa %xmm0,(%rdi)
> >>
> >> This patch adds movdqa to the emulator but does not implement #GP when
> >> the memory operand is unaligned to 16 bytes.  I'm not sure whether
> >> alignment checking can be implemented as an opcode .flag or if it needs
> >> to be done in em_movdqa().
> >
> > It should actually be automatic when the Sse flag is present, since it's
> > the norm for almost all SSE instructions.  There should be a .flag to
> > override it for movdqu.
> 
> When writing a kvm-unit-test for movdqa I found that alignment
> checking happens before the page fault (makes sense).  That means
> misalignment is detected by the CPU while still in guest mode.  The
> emulator never sees the instruction because #GP is raised and handled
> in the guest.
> 
> I also didn't see other instances of alignment checking in the
> emulator (e.g. eflags AC).  I guess the same situation applies there.
> 
> Can you think of a case where we need to perform alignment checking in
> the emulator?
> 
Any instruction can be fed to emulator on smp guest. Make one vcpu
enter emulator by some instruction that does io and replace it by
another instruction on a second vcpu. If replacing will happen before
first vcpu emulates fetch, instruction written by the second vcpu will be
emulated. This is why emulator has to perform all security checks even
if cpu does them before exit. Alignment checking is probably not
security sensitive though.

--
			Gleb.

Re: [RFC] KVM: x86: emulate movdqa

[Avi Kivity]

On 01/08/2012 06:21 PM, Stefan Hajnoczi wrote:
> On Sun, Jan 8, 2012 at 10:32 AM, Avi Kivity <avi@redhat.com> wrote:
> > On 01/07/2012 12:26 PM, Stefan Hajnoczi wrote:
> >>
> >> movdqa %xmm0,(%rdi)
> >>
> >> This patch adds movdqa to the emulator but does not implement #GP when
> >> the memory operand is unaligned to 16 bytes.  I'm not sure whether
> >> alignment checking can be implemented as an opcode .flag or if it needs
> >> to be done in em_movdqa().
> >
> > It should actually be automatic when the Sse flag is present, since it's
> > the norm for almost all SSE instructions.  There should be a .flag to
> > override it for movdqu.
>
> When writing a kvm-unit-test for movdqa I found that alignment
> checking happens before the page fault (makes sense).  That means
> misalignment is detected by the CPU while still in guest mode.  The
> emulator never sees the instruction because #GP is raised and handled
> in the guest.

Right.  The way to test this is to change the instruction while the vcpu
is processing it, see for example x86/svm.c's
corrupt_cr3_intercept_bypass().

> I also didn't see other instances of alignment checking in the
> emulator (e.g. eflags AC).  I guess the same situation applies there.

Yes.

> Can you think of a case where we need to perform alignment checking in
> the emulator?

The case that x86/svm.c is checking for is a security issue.

Guests don't normally turn eflags.AC on, so I don't expect issues
there.  I also don't see issues with SSE alignment, especially as AVX
removes it (for VEX encoded instructions).  However, it's good to stick
to the spec, there are always surprise issues when you don't.

>
> >> A more fundamental question: why do we have to emulate this guest
> >> userspace SSE instruction in the first place?  This host machine lacks
> >> EPT but can't we service the page fault and then retry execution inside
> >> the guest?
> >
> > Not when the target is mmio - there is no possible mapping.  With your
> > patch, is there a kvm_mmio trace right after the movdqa emulation?
>
> You are right.  These instructions are accessing the VGA area at 0xa0000.

> >
> > Need the Mov flag too (I see it's missing for movdqu as well); otherwise
> > the emulator will RMW the destination.
>
> The Mov is already given by the GP() group prefix.
>

Ah.  Not sure it was a good idea.  The whole trend towards em_foo() is
motivated by the idea of having everything about an instruction in one
place, and this doesn't help there.  On the other hand, I can see many
SSE instructions repeating this pattern.  We'll just have to see how it
evolves.

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

Re: [RFC] KVM: x86: emulate movdqa

[Takuya Yoshikawa]

Avi Kivity <avi@redhat.com> wrote:

> > Can you think of a case where we need to perform alignment checking in
> > the emulator?
> 
> The case that x86/svm.c is checking for is a security issue.
> 
> Guests don't normally turn eflags.AC on, so I don't expect issues
> there.  I also don't see issues with SSE alignment, especially as AVX
> removes it (for VEX encoded instructions).  However, it's good to stick
> to the spec, there are always surprise issues when you don't.
> 

Isn't it make sense to add Documentation/virtual/kvm/emulator.txt at some
point in the future?

In some cases it is difficult to know the underlying assumptions.

Explanations about the emulator specific macros may also be helpful.

	Takuya

Re: [RFC] KVM: x86: emulate movdqa

[Avi Kivity]

On 01/09/2012 10:52 AM, Takuya Yoshikawa wrote:
> Avi Kivity <avi@redhat.com> wrote:
>
> > > Can you think of a case where we need to perform alignment checking in
> > > the emulator?
> > 
> > The case that x86/svm.c is checking for is a security issue.
> > 
> > Guests don't normally turn eflags.AC on, so I don't expect issues
> > there.  I also don't see issues with SSE alignment, especially as AVX
> > removes it (for VEX encoded instructions).  However, it's good to stick
> > to the spec, there are always surprise issues when you don't.
> > 
>
> Isn't it make sense to add Documentation/virtual/kvm/emulator.txt at some
> point in the future?
>
> In some cases it is difficult to know the underlying assumptions.

Sure.

> Explanations about the emulator specific macros may also be helpful.

Those might be better as code comments, where those macros are defined.

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