[RFC PATCH] Emulate MOVBE

[RFC PATCH] Emulate MOVBE

[Borislav Petkov]

Hi guys,

so I was trying to repro tglx's bug in smpboot.c and for some reason,
the most reliable way to trigger it was to boot an 32-bit atom smp guest
in kvm (don't ask :)).

The problem, however, was that atom wants MOVBE and qemu doesn't emulate
it yet (except Richard's patches which I used in order to be able to
actually even boot a guest).

However, without hw acceleration, qemu is pretty slow, and waiting for
an smp guest to boot in sw-only emulation is not fun. So, just for
funsies, I decided to give the MOVBE emulation a try.

Patch is below, 8 core smp atom guest boots fine and in 6-ish seconds
with it. :-)

I know, I know, it still needs cleaning up and proper rFLAGS handling
but that is for later. For now, I'd very much like to hear whether this
approach even makes sense and if so, what should be improved.

Thanks, and thanks to Andre and Jörg for their help.

Not-yet-signed-off-by: Borislav Petkov <bp@suse.de>

--
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 15f960c06ff7..ae01c765cd77 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -281,6 +281,7 @@ struct x86_emulate_ctxt {
 
 	/* decode cache */
 	u8 twobyte;
+	u8 thirdbyte;
 	u8 b;
 	u8 intercept;
 	u8 lock_prefix;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a335cc6cde72..0ccff339359d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -152,6 +152,7 @@
 #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
 #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
 #define NoWrite     ((u64)1 << 45)  /* No writeback */
+#define ThreeByte   ((u64)1 << 46)  /* Three byte opcodes 0F 38 and 0F 3A */
 
 #define X2(x...) x, x
 #define X3(x...) X2(x), x
@@ -3107,6 +3108,34 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_movbe(struct x86_emulate_ctxt *ctxt)
+{
+	char *valptr = ctxt->src.valptr;
+
+	switch (ctxt->op_bytes) {
+	case 2:
+		*(u16 *)valptr = swab16(*(u16 *)valptr);
+		break;
+	case 4:
+		*(u32 *)valptr = swab32(*(u32 *)valptr);
+
+		/*
+		 * clear upper dword for 32-bit operand size in 64-bit mode.
+		 */
+		if (ctxt->mode == X86EMUL_MODE_PROT64)
+			*((u32 *)valptr + 1) = 0x0;
+		break;
+	case 8:
+		*(u64 *)valptr = swab64(*(u64 *)valptr);
+		break;
+	default:
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+
+	memcpy(ctxt->dst.valptr, ctxt->src.valptr, ctxt->op_bytes);
+	return X86EMUL_CONTINUE;
+}
+
 static int em_cr_write(struct x86_emulate_ctxt *ctxt)
 {
 	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
@@ -3974,7 +4003,8 @@ static const struct opcode twobyte_table[256] = {
 	I(ImplicitOps | VendorSpecific, em_sysenter),
 	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
 	N, N,
-	N, N, N, N, N, N, N, N,
+	I(ModRM | Mov | ThreeByte | VendorSpecific, em_movbe),
+	N, N, N, N, N, N, N,
 	/* 0x40 - 0x4F */
 	X16(D(DstReg | SrcMem | ModRM | Mov)),
 	/* 0x50 - 0x5F */
@@ -4323,6 +4353,15 @@ done_prefixes:
 	}
 	ctxt->d = opcode.flags;
 
+	if (ctxt->d & ThreeByte) {
+		ctxt->thirdbyte = insn_fetch(u8, ctxt);
+
+		if (ctxt->thirdbyte == 0xf0)
+			ctxt->d |= DstReg | SrcMem;
+		else
+			ctxt->d |= DstMem | SrcReg;
+	}
+
 	if (ctxt->d & ModRM)
 		ctxt->modrm = insn_fetch(u8, ctxt);
 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH] Emulate MOVBE

[Borislav Petkov]

On Wed, Apr 10, 2013 at 01:46:02AM +0200, Borislav Petkov wrote:
> +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> +{
> +	char *valptr = ctxt->src.valptr;
> +
> +	switch (ctxt->op_bytes) {
> +	case 2:
> +		*(u16 *)valptr = swab16(*(u16 *)valptr);
> +		break;
> +	case 4:
> +		*(u32 *)valptr = swab32(*(u32 *)valptr);

Note to self: this destroys the src operand but it shouldn't. Fix it
tomorrow.

-- 
Regards/Gruss,
Boris.

Re: [RFC PATCH] Emulate MOVBE

[H. Peter Anvin]

On 04/09/2013 05:03 PM, Borislav Petkov wrote:
> 
> Note to self: this destroys the src operand but it shouldn't. Fix it
> tomorrow.
> 

I thought movbe was already in qemu just not on by default...?

	-hpa

Re: [RFC PATCH] Emulate MOVBE

[Andre Przywara]

On Wed, 10 Apr 2013 01:46:02 +0200
Borislav Petkov <bp@alien8.de> wrote:

> Hi guys,
> 
> so I was trying to repro tglx's bug in smpboot.c and for some reason,
> the most reliable way to trigger it was to boot an 32-bit atom smp
> guest in kvm (don't ask :)).
> 
> The problem, however, was that atom wants MOVBE and qemu doesn't
> emulate it yet (except Richard's patches which I used in order to be
> able to actually even boot a guest).
> 
> However, without hw acceleration, qemu is pretty slow, and waiting for
> an smp guest to boot in sw-only emulation is not fun. So, just for
> funsies, I decided to give the MOVBE emulation a try.
> 
> Patch is below, 8 core smp atom guest boots fine and in 6-ish seconds
> with it. :-)
> 
> I know, I know, it still needs cleaning up and proper rFLAGS handling
> but that is for later. For now, I'd very much like to hear whether
> this approach even makes sense and if so, what should be improved.
> 
> Thanks, and thanks to Andre and Jörg for their help.
> 
> Not-yet-signed-off-by: Borislav Petkov <bp@suse.de>

You are missing an (hackish) older patch which enables the MOVBE CPUID
bit even if the host does not have it (malformed due to c&p):
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -273,7 +273,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2
*entry, u32 function,
		cpuid_mask(&entry->ecx, 4); /* we support x2apic
emulation even if host does not support
		 * it since we emulate x2apic in software */
-		entry->ecx |= F(X2APIC);
+		entry->ecx |= (F(X2APIC) | F(MOVBE));
		break;
	/* function 2 entries are STATEFUL. That is, repeated cpuid
commands
	 * may return different values. This forces us to get_cpu()
before

> 
> --
> diff --git a/arch/x86/include/asm/kvm_emulate.h
> b/arch/x86/include/asm/kvm_emulate.h index 15f960c06ff7..ae01c765cd77
> 100644 --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -281,6 +281,7 @@ struct x86_emulate_ctxt {
>  
>  	/* decode cache */
>  	u8 twobyte;
> +	u8 thirdbyte;
>  	u8 b;
>  	u8 intercept;
>  	u8 lock_prefix;
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a335cc6cde72..0ccff339359d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -152,6 +152,7 @@
>  #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
> +#define ThreeByte   ((u64)1 << 46)  /* Three byte opcodes 0F 38 and
> 0F 3A */ 
>  #define X2(x...) x, x
>  #define X3(x...) X2(x), x
> @@ -3107,6 +3108,34 @@ static int em_mov(struct x86_emulate_ctxt
> *ctxt) return X86EMUL_CONTINUE;
>  }
>  
> +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> +{
> +	char *valptr = ctxt->src.valptr;
> +
> +	switch (ctxt->op_bytes) {
> +	case 2:
> +		*(u16 *)valptr = swab16(*(u16 *)valptr);
> +		break;
> +	case 4:
> +		*(u32 *)valptr = swab32(*(u32 *)valptr);
> +
> +		/*
> +		 * clear upper dword for 32-bit operand size in
> 64-bit mode.
> +		 */
> +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> +			*((u32 *)valptr + 1) = 0x0;
> +		break;
> +	case 8:
> +		*(u64 *)valptr = swab64(*(u64 *)valptr);
> +		break;
> +	default:
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +
> +	memcpy(ctxt->dst.valptr, ctxt->src.valptr, ctxt->op_bytes);
> +	return X86EMUL_CONTINUE;
> +}
> +
On 04/09/2013 05:03 PM, Borislav Petkov wrote:
> 
> Note to self: this destroys the src operand but it shouldn't. Fix it
> tomorrow.
> 

What about this one?

	*(u16 *)(ctxt->dst.valptr) = swab16(*(u16 *)(ctxt->src.valptr));

Respective versions for the other operand sizes, of course, and drop
the final memcpy.

>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
>  	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> @@ -3974,7 +4003,8 @@ static const struct opcode twobyte_table[256] =
> { I(ImplicitOps | VendorSpecific, em_sysenter),
>  	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
>  	N, N,
> -	N, N, N, N, N, N, N, N,
> +	I(ModRM | Mov | ThreeByte | VendorSpecific, em_movbe),
> +	N, N, N, N, N, N, N,

In a real world VendorSpecific should be replaced with something more
meaningful. Depends on KVMs intention to emulate instructions, actually
out of scope for a pure virtualizer.

What is the opinion from the KVM folks on this? Shall we start to
emulate instructions the host does not provide? In this particular case
a relatively simple patch fixes a problem (starting Atom optimized
kernels on non-Atom machines).

(And if one can believe the AMD Fam16h SWOG [1], PS4^Wfuture AMD
processors have MOVBE, so it's not even actually one CPU anymore).

>  	/* 0x40 - 0x4F */
>  	X16(D(DstReg | SrcMem | ModRM | Mov)),
>  	/* 0x50 - 0x5F */
> @@ -4323,6 +4353,15 @@ done_prefixes:
>  	}
>  	ctxt->d = opcode.flags;
>  
> +	if (ctxt->d & ThreeByte) {
> +		ctxt->thirdbyte = insn_fetch(u8, ctxt);
> +
> +		if (ctxt->thirdbyte == 0xf0)
> +			ctxt->d |= DstReg | SrcMem;
> +		else
> +			ctxt->d |= DstMem | SrcReg;
> +	}
> +

As already agreed on on IRC, we should properly check for the other
opcodes in the 0F 38/3A table. Or make it right and implement a real
sub-table.

Regards,
Andre.


[1]http://support.amd.com/us/Processor_TechDocs/52128_16h_Software_Opt_Guide.zip

Re: [RFC PATCH] Emulate MOVBE

[Borislav Petkov]

On Tue, Apr 09, 2013 at 05:04:06PM -0700, H. Peter Anvin wrote:
> On 04/09/2013 05:03 PM, Borislav Petkov wrote:
> > 
> > Note to self: this destroys the src operand but it shouldn't. Fix it
> > tomorrow.
> > 
> 
> I thought movbe was already in qemu just not on by default...?

Yep, this went upstream just last month.

However and AFAICT, this still doesn't help the issue when we run
qemu -enable-kvm and the host doesn't have MOVBE. With my simplistic
thinking, I would expect that kvm would jump to qemu on #UD and let it
emulate the unsupported instruction and go back.

However, as Andre explained it to me, qemu emulation and kvm are
completely unrelated and it is probably very expensive to copy emulation
states to and fro just for a simple instruction. Thus, this simpler
approach to do the emulation straight in kvm as it is done already for a
bunch of other instructions.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH] Emulate MOVBE

[Gleb Natapov]

On Wed, Apr 10, 2013 at 11:29:42AM +0200, Andre Przywara wrote:
> In a real world VendorSpecific should be replaced with something more
> meaningful. Depends on KVMs intention to emulate instructions, actually
> out of scope for a pure virtualizer.
> 
Something like EmulateOnUD.

> What is the opinion from the KVM folks on this? Shall we start to
> emulate instructions the host does not provide? In this particular case
> a relatively simple patch fixes a problem (starting Atom optimized
> kernels on non-Atom machines).
We can add the emulation, but we should not start announcing the instruction
availability to a guest if host cpu does not have it by default. This
may trick a guest into thinking that movbe is the fastest way to do
something when it is not.

> 
> (And if one can believe the AMD Fam16h SWOG [1], PS4^Wfuture AMD
> processors have MOVBE, so it's not even actually one CPU anymore).
If a host CPU has the instruction emulation is not needed unless the
instruction is used for MMIO access.

--
			Gleb.

Re: [RFC PATCH] Emulate MOVBE

[Borislav Petkov]

On Wed, Apr 10, 2013 at 01:08:46PM +0300, Gleb Natapov wrote:
> We can add the emulation, but we should not start announcing the
> instruction availability to a guest if host cpu does not have it
> by default. This may trick a guest into thinking that movbe is the
> fastest way to do something when it is not.

That could be a problem because kernels compiled for atom look at CPUID
to check for MOVBE support and refuse to continue if there's none.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH] Emulate MOVBE

[Gleb Natapov]

On Wed, Apr 10, 2013 at 12:17:21PM +0200, Borislav Petkov wrote:
> On Wed, Apr 10, 2013 at 01:08:46PM +0300, Gleb Natapov wrote:
> > We can add the emulation, but we should not start announcing the
> > instruction availability to a guest if host cpu does not have it
> > by default. This may trick a guest into thinking that movbe is the
> > fastest way to do something when it is not.
> 
> That could be a problem because kernels compiled for atom look at CPUID
> to check for MOVBE support and refuse to continue if there's none.
> 
You will still be able to force it on qemu command line by adding +movbe
to cpu definition, it just not be the default.


--
			Gleb.

Re: [RFC PATCH] Emulate MOVBE

[Andre Przywara]

On Wed, 10 Apr 2013 13:08:46 +0300
Gleb Natapov <gleb@redhat.com> wrote:

> On Wed, Apr 10, 2013 at 11:29:42AM +0200, Andre Przywara wrote:
> > In a real world VendorSpecific should be replaced with something
> > more meaningful. Depends on KVMs intention to emulate instructions,
> > actually out of scope for a pure virtualizer.
> > 
> Something like EmulateOnUD.

Right.

> > What is the opinion from the KVM folks on this? Shall we start to
> > emulate instructions the host does not provide? In this particular
> > case a relatively simple patch fixes a problem (starting Atom
> > optimized kernels on non-Atom machines).
> We can add the emulation, but we should not start announcing the
> instruction availability to a guest if host cpu does not have it by
> default. This may trick a guest into thinking that movbe is the
> fastest way to do something when it is not.

Good point. I'd also like to have a switch which enables this kind of
"non-standard" behavior. Actually this should be requested by QEMU,
right? So that a single guest can override the CPUID masking done by
the kernel if it really really wants to.
 
> > 
> > (And if one can believe the AMD Fam16h SWOG [1], PS4^Wfuture AMD
> > processors have MOVBE, so it's not even actually one CPU anymore).
> If a host CPU has the instruction emulation is not needed unless the
> instruction is used for MMIO access.

I meant to "emulate" such a CPU. -cpu ps4 ;-)

Regards,
Andre.

Re: [RFC PATCH] Emulate MOVBE

[Gleb Natapov]

On Wed, Apr 10, 2013 at 12:39:01PM +0200, Andre Przywara wrote:
> On Wed, 10 Apr 2013 13:08:46 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Wed, Apr 10, 2013 at 11:29:42AM +0200, Andre Przywara wrote:
> > > In a real world VendorSpecific should be replaced with something
> > > more meaningful. Depends on KVMs intention to emulate instructions,
> > > actually out of scope for a pure virtualizer.
> > > 
> > Something like EmulateOnUD.
> 
> Right.
> 
> > > What is the opinion from the KVM folks on this? Shall we start to
> > > emulate instructions the host does not provide? In this particular
> > > case a relatively simple patch fixes a problem (starting Atom
> > > optimized kernels on non-Atom machines).
> > We can add the emulation, but we should not start announcing the
> > instruction availability to a guest if host cpu does not have it by
> > default. This may trick a guest into thinking that movbe is the
> > fastest way to do something when it is not.
> 
> Good point. I'd also like to have a switch which enables this kind of
> "non-standard" behavior. Actually this should be requested by QEMU,
> right? So that a single guest can override the CPUID masking done by
> the kernel if it really really wants to.
>  
Right, the question is how kernel can tell QEMU that the cpuid bit is
supported but should not be set unless explicitly asked by an user.
 
> > > 
> > > (And if one can believe the AMD Fam16h SWOG [1], PS4^Wfuture AMD
> > > processors have MOVBE, so it's not even actually one CPU anymore).
> > If a host CPU has the instruction emulation is not needed unless the
> > instruction is used for MMIO access.
> 
> I meant to "emulate" such a CPU. -cpu ps4 ;-)
> 
Ah, OK.

--
			Gleb.

[PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Wed, Apr 10, 2013 at 03:16:39PM +0300, Gleb Natapov wrote:
> Right, the question is how kernel can tell QEMU that the cpuid bit is
> supported but should not be set unless explicitly asked by an user.

Actually, this seems to work with the patch below based on whether you
have "+movbe" in the -cpu option or not.

Anyway, here's the second version with hopefully all comments and
suggestions addressed.

Thanks.

--
>From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001
From: Borislav Petkov <bp@suse.de>
Date: Thu, 11 Apr 2013 02:06:30 +0200
Subject: [PATCH] kvm: Emulate MOVBE

This basically came from the need to be able to boot 32-bit Atom SMP
guests on an AMD host, i.e. host which doesn't support MOVBE. As a
matter of fact, qemu has since recently received MOVBE support but we
cannot share that with kvm emulation and thus we have to do this in the
host.

We piggyback on the #UD path and emulate the MOVBE functionality. With
it, an 8-core SMP guest boots in under 6 seconds.

Also, requesting MOVBE emulation needs to happen explicitly to work,
i.e. qemu -cpu n270,+movbe...

Signed-off-by: Andre Przywara <andre@andrep.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kvm/cpuid.c   |  2 +-
 arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a20ecb5b6cbf..2d44fc4fd855 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -273,7 +273,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		cpuid_mask(&entry->ecx, 4);
 		/* we support x2apic emulation even if host does not support
 		 * it since we emulate x2apic in software */
-		entry->ecx |= F(X2APIC);
+		entry->ecx |= F(X2APIC) | F(MOVBE);
 		break;
 	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
 	 * may return different values. This forces us to get_cpu() before
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index a335cc6cde72..9011c7a656ad 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -152,6 +152,7 @@
 #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
 #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
 #define NoWrite     ((u64)1 << 45)  /* No writeback */
+#define EmulateOnUD ((u64)1 << 46)  /* emulate if unsupported by the host */
 
 #define X2(x...) x, x
 #define X3(x...) X2(x), x
@@ -3107,6 +3108,30 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_movbe(struct x86_emulate_ctxt *ctxt)
+{
+	switch (ctxt->op_bytes) {
+	case 2:
+		*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);
+		break;
+	case 4:
+		*(u32 *)ctxt->dst.valptr = swab32(*(u32 *)ctxt->src.valptr);
+
+		/*
+		 * clear upper dword for 32-bit operand size in 64-bit mode.
+		 */
+		if (ctxt->mode == X86EMUL_MODE_PROT64)
+			*((u32 *)ctxt->dst.valptr + 1) = 0x0;
+		break;
+	case 8:
+		*(u64 *)ctxt->dst.valptr = swab64(*(u64 *)ctxt->src.valptr);
+		break;
+	default:
+		return X86EMUL_PROPAGATE_FAULT;
+	}
+	return X86EMUL_CONTINUE;
+}
+
 static int em_cr_write(struct x86_emulate_ctxt *ctxt)
 {
 	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
@@ -4033,6 +4058,11 @@ static const struct opcode twobyte_table[256] = {
 	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N
 };
 
+static const struct opcode threebyte_table[] = {
+	[0xf0] = I(DstReg | SrcMem | ModRM | Mov | EmulateOnUD, em_movbe),
+	[0xf1] = I(DstMem | SrcReg | ModRM | Mov | EmulateOnUD, em_movbe),
+};
+
 #undef D
 #undef N
 #undef G
@@ -4320,6 +4350,9 @@ done_prefixes:
 		ctxt->twobyte = 1;
 		ctxt->b = insn_fetch(u8, ctxt);
 		opcode = twobyte_table[ctxt->b];
+
+		if (ctxt->b == 0x38)
+			opcode = threebyte_table[insn_fetch(u8, ctxt)];
 	}
 	ctxt->d = opcode.flags;
 
@@ -4376,8 +4409,10 @@ done_prefixes:
 	if (ctxt->d == 0 || (ctxt->d & Undefined))
 		return EMULATION_FAILED;
 
-	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
-		return EMULATION_FAILED;
+	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn) {
+		if (!(ctxt->d & EmulateOnUD))
+			return EMULATION_FAILED;
+	}
 
 	if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
 		ctxt->op_bytes = 8;
-- 
1.8.2.135.g7b592fa


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Thu, Apr 11, 2013 at 02:18:15AM +0200, Borislav Petkov wrote:
> On Wed, Apr 10, 2013 at 03:16:39PM +0300, Gleb Natapov wrote:
> > Right, the question is how kernel can tell QEMU that the cpuid bit is
> > supported but should not be set unless explicitly asked by an user.
> 
> Actually, this seems to work with the patch below based on whether you
> have "+movbe" in the -cpu option or not.
> 
The problem is that -cpu host will have it unconditionally and this is
definitely not what we want.

> Anyway, here's the second version with hopefully all comments and
> suggestions addressed.
> 
Thanks, will review it later.

> Thanks.
> 
> --
> >From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001
> From: Borislav Petkov <bp@suse.de>
> Date: Thu, 11 Apr 2013 02:06:30 +0200
> Subject: [PATCH] kvm: Emulate MOVBE
> 
> This basically came from the need to be able to boot 32-bit Atom SMP
> guests on an AMD host, i.e. host which doesn't support MOVBE. As a
> matter of fact, qemu has since recently received MOVBE support but we
> cannot share that with kvm emulation and thus we have to do this in the
> host.
> 
> We piggyback on the #UD path and emulate the MOVBE functionality. With
> it, an 8-core SMP guest boots in under 6 seconds.
> 
> Also, requesting MOVBE emulation needs to happen explicitly to work,
> i.e. qemu -cpu n270,+movbe...
> 
> Signed-off-by: Andre Przywara <andre@andrep.de>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kvm/cpuid.c   |  2 +-
>  arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a20ecb5b6cbf..2d44fc4fd855 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -273,7 +273,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		cpuid_mask(&entry->ecx, 4);
>  		/* we support x2apic emulation even if host does not support
>  		 * it since we emulate x2apic in software */
> -		entry->ecx |= F(X2APIC);
> +		entry->ecx |= F(X2APIC) | F(MOVBE);
>  		break;
>  	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
>  	 * may return different values. This forces us to get_cpu() before
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a335cc6cde72..9011c7a656ad 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -152,6 +152,7 @@
>  #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
> +#define EmulateOnUD ((u64)1 << 46)  /* emulate if unsupported by the host */
>  
>  #define X2(x...) x, x
>  #define X3(x...) X2(x), x
> @@ -3107,6 +3108,30 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> +{
> +	switch (ctxt->op_bytes) {
> +	case 2:
> +		*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);
> +		break;
> +	case 4:
> +		*(u32 *)ctxt->dst.valptr = swab32(*(u32 *)ctxt->src.valptr);
> +
> +		/*
> +		 * clear upper dword for 32-bit operand size in 64-bit mode.
> +		 */
> +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> +			*((u32 *)ctxt->dst.valptr + 1) = 0x0;
> +		break;
> +	case 8:
> +		*(u64 *)ctxt->dst.valptr = swab64(*(u64 *)ctxt->src.valptr);
> +		break;
> +	default:
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
>  	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> @@ -4033,6 +4058,11 @@ static const struct opcode twobyte_table[256] = {
>  	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N
>  };
>  
> +static const struct opcode threebyte_table[] = {
> +	[0xf0] = I(DstReg | SrcMem | ModRM | Mov | EmulateOnUD, em_movbe),
> +	[0xf1] = I(DstMem | SrcReg | ModRM | Mov | EmulateOnUD, em_movbe),
> +};
> +
>  #undef D
>  #undef N
>  #undef G
> @@ -4320,6 +4350,9 @@ done_prefixes:
>  		ctxt->twobyte = 1;
>  		ctxt->b = insn_fetch(u8, ctxt);
>  		opcode = twobyte_table[ctxt->b];
> +
> +		if (ctxt->b == 0x38)
> +			opcode = threebyte_table[insn_fetch(u8, ctxt)];
>  	}
>  	ctxt->d = opcode.flags;
>  
> @@ -4376,8 +4409,10 @@ done_prefixes:
>  	if (ctxt->d == 0 || (ctxt->d & Undefined))
>  		return EMULATION_FAILED;
>  
> -	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
> -		return EMULATION_FAILED;
> +	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn) {
> +		if (!(ctxt->d & EmulateOnUD))
> +			return EMULATION_FAILED;
> +	}
>  
>  	if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
>  		ctxt->op_bytes = 8;
> -- 
> 1.8.2.135.g7b592fa
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Thu, Apr 11, 2013 at 05:28:18PM +0300, Gleb Natapov wrote:
> On Thu, Apr 11, 2013 at 02:18:15AM +0200, Borislav Petkov wrote:
> > On Wed, Apr 10, 2013 at 03:16:39PM +0300, Gleb Natapov wrote:
> > > Right, the question is how kernel can tell QEMU that the cpuid bit is
> > > supported but should not be set unless explicitly asked by an user.
> > 
> > Actually, this seems to work with the patch below based on whether you
> > have "+movbe" in the -cpu option or not.
> > 
> The problem is that -cpu host will have it unconditionally and this is
> definitely not what we want.

Hmm, ok, I see what you mean. -cpu host boots the atom kernel just fine.

Well, with my limited qemu exposure, I'd guess
cpu_x86_parse_featurestr() would need to somehow say to
x86_cpu_realizefn that it has parsed a "+movbe" on the command line and
the latter has to keep that bit enabled when doing the checks against
kvm in kvm_check_features_against_host and filter_features_for_kvm().

Unless you have a better idea, that is.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Thu, Apr 11, 2013 at 05:37:33PM +0200, Borislav Petkov wrote:
> On Thu, Apr 11, 2013 at 05:28:18PM +0300, Gleb Natapov wrote:
> > On Thu, Apr 11, 2013 at 02:18:15AM +0200, Borislav Petkov wrote:
> > > On Wed, Apr 10, 2013 at 03:16:39PM +0300, Gleb Natapov wrote:
> > > > Right, the question is how kernel can tell QEMU that the cpuid bit is
> > > > supported but should not be set unless explicitly asked by an user.
> > > 
> > > Actually, this seems to work with the patch below based on whether you
> > > have "+movbe" in the -cpu option or not.
> > > 
> > The problem is that -cpu host will have it unconditionally and this is
> > definitely not what we want.
> 
> Hmm, ok, I see what you mean. -cpu host boots the atom kernel just fine.
> 
> Well, with my limited qemu exposure, I'd guess
> cpu_x86_parse_featurestr() would need to somehow say to
> x86_cpu_realizefn that it has parsed a "+movbe" on the command line and
> the latter has to keep that bit enabled when doing the checks against
> kvm in kvm_check_features_against_host and filter_features_for_kvm().
> 
Then you will filter out movbe even if host cpu supports it directly
which is also not what we want.

Currently userspace assumes that that cpuid configuration returned by
KVM_GET_SUPPORTED_CPUID is the optimal one. What we want here is a way
for KVM to tell userspace that it can emulate movbe though it is not
optimal. Userspace alone cannot figure it out. It can check host's cpuid
directly and assume that if cpuid bit is not present on the host cpu,
but reported as supported by KVM then it is emulated by KVM and this is
not optimal, but sometimes emulation is actually desirable (x2apic), so
such assumption is not always correct.

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Thu, Apr 11, 2013 at 02:18:15AM +0200, Borislav Petkov wrote:
> On Wed, Apr 10, 2013 at 03:16:39PM +0300, Gleb Natapov wrote:
> > Right, the question is how kernel can tell QEMU that the cpuid bit is
> > supported but should not be set unless explicitly asked by an user.
> 
> Actually, this seems to work with the patch below based on whether you
> have "+movbe" in the -cpu option or not.
> 
> Anyway, here's the second version with hopefully all comments and
> suggestions addressed.
> 
> Thanks.
> 
> --
> >From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001
> From: Borislav Petkov <bp@suse.de>
> Date: Thu, 11 Apr 2013 02:06:30 +0200
> Subject: [PATCH] kvm: Emulate MOVBE
> 
> This basically came from the need to be able to boot 32-bit Atom SMP
> guests on an AMD host, i.e. host which doesn't support MOVBE. As a
> matter of fact, qemu has since recently received MOVBE support but we
> cannot share that with kvm emulation and thus we have to do this in the
> host.
> 
> We piggyback on the #UD path and emulate the MOVBE functionality. With
> it, an 8-core SMP guest boots in under 6 seconds.
> 
> Also, requesting MOVBE emulation needs to happen explicitly to work,
> i.e. qemu -cpu n270,+movbe...
> 
> Signed-off-by: Andre Przywara <andre@andrep.de>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kvm/cpuid.c   |  2 +-
>  arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index a20ecb5b6cbf..2d44fc4fd855 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -273,7 +273,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		cpuid_mask(&entry->ecx, 4);
>  		/* we support x2apic emulation even if host does not support
>  		 * it since we emulate x2apic in software */
> -		entry->ecx |= F(X2APIC);
> +		entry->ecx |= F(X2APIC) | F(MOVBE);
>  		break;
>  	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
>  	 * may return different values. This forces us to get_cpu() before
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a335cc6cde72..9011c7a656ad 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -152,6 +152,7 @@
>  #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
> +#define EmulateOnUD ((u64)1 << 46)  /* emulate if unsupported by the host */
>  
Just rename VendorSpecific to EmulateOnUD. The meaning is the same.

>  #define X2(x...) x, x
>  #define X3(x...) X2(x), x
> @@ -3107,6 +3108,30 @@ static int em_mov(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> +{
> +	switch (ctxt->op_bytes) {
> +	case 2:
> +		*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);
What's wrong with: ctxt->dst.val = swab16((u16)ctxt->src.val)?

> +		break;
> +	case 4:
> +		*(u32 *)ctxt->dst.valptr = swab32(*(u32 *)ctxt->src.valptr);
> +
> +		/*
> +		 * clear upper dword for 32-bit operand size in 64-bit mode.
> +		 */
> +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> +			*((u32 *)ctxt->dst.valptr + 1) = 0x0;
It should be clean without this this.

> +		break;
> +	case 8:
> +		*(u64 *)ctxt->dst.valptr = swab64(*(u64 *)ctxt->src.valptr);
> +		break;
> +	default:
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
>  	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> @@ -4033,6 +4058,11 @@ static const struct opcode twobyte_table[256] = {
>  	N, N, N, N, N, N, N, N, N, N, N, N, N, N, N, N
>  };
>  
> +static const struct opcode threebyte_table[] = {
> +	[0xf0] = I(DstReg | SrcMem | ModRM | Mov | EmulateOnUD, em_movbe),
> +	[0xf1] = I(DstMem | SrcReg | ModRM | Mov | EmulateOnUD, em_movbe),
> +};
> +
Populate the whole table explicitly please. You can use X16(N) to make it
short. The encoding is also wrong since 0F 38 F0/F1 instruction decoding
depend on a prefix. Same opcode is decoded as CRC32 with f2 prefix. We
have Prefix mechanism for such instructions, but it currently assumes
that 66 and f2/f3 prefixes are mutually exclusive, but in this case they
are not, ugh.

>  #undef D
>  #undef N
>  #undef G
> @@ -4320,6 +4350,9 @@ done_prefixes:
>  		ctxt->twobyte = 1;
>  		ctxt->b = insn_fetch(u8, ctxt);
>  		opcode = twobyte_table[ctxt->b];
> +
> +		if (ctxt->b == 0x38)
> +			opcode = threebyte_table[insn_fetch(u8, ctxt)];
ctxt->b = insn_fetch(u8, ctxt);
opcode = threebyte_table[ctxt->b];

Otherwise OpImplicit type of decoding will not work for three byte
instructions. Also should rename twobyte into opbytes and set it to 1,
2 or 3. I prefer that three byte instruction decoding will be sent as a
separate patch.

>  	}
>  	ctxt->d = opcode.flags;
>  
> @@ -4376,8 +4409,10 @@ done_prefixes:
>  	if (ctxt->d == 0 || (ctxt->d & Undefined))
>  		return EMULATION_FAILED;
>  
> -	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn)
> -		return EMULATION_FAILED;
> +	if (!(ctxt->d & VendorSpecific) && ctxt->only_vendor_specific_insn) {
> +		if (!(ctxt->d & EmulateOnUD))
> +			return EMULATION_FAILED;
> +	}
>  
>  	if (mode == X86EMUL_MODE_PROT64 && (ctxt->d & Stack))
>  		ctxt->op_bytes = 8;
> -- 
> 1.8.2.135.g7b592fa
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 14, 2013 at 10:41:07AM +0300, Gleb Natapov wrote:
> Currently userspace assumes that that cpuid configuration returned by
> KVM_GET_SUPPORTED_CPUID is the optimal one. What we want here is a way
> for KVM to tell userspace that it can emulate movbe though it is not
> optimal.

Ok, I don't understand.

You want to tell userspace: "yes, we do support a hw feature but we
emulate it."?

> Userspace alone cannot figure it out. It can check host's cpuid
> directly and assume that if cpuid bit is not present on the host cpu,
> but reported as supported by KVM then it is emulated by KVM and this is
> not optimal, but sometimes emulation is actually desirable (x2apic), so
> such assumption is not always correct.

Right, and this is what we have, AFAICT. And if userspace does that what
you exemplify above, you get exactly that - a feature bit not set in
CPUID but KVM reporting it set means, it is emulated. There's no room
for other interpretations here. Which probably means also not optimal
because it is not done in hw.

Or, do you want to have a way to say with KVM_GET_SUPPORTED_CPUID that
"the features I'm reporting to you are those - a subset of them are not
optimally supported because I'm emulating them."

Am I close?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[H. Peter Anvin]

Just to interject here... all this is about host kernel vs userspace, correct?  I.e. nothing that affects the guest.  I assume that the guest sees CPUID and that is what is supported.

Borislav Petkov <bp@alien8.de> wrote:

>On Sun, Apr 14, 2013 at 10:41:07AM +0300, Gleb Natapov wrote:
>> Currently userspace assumes that that cpuid configuration returned by
>> KVM_GET_SUPPORTED_CPUID is the optimal one. What we want here is a
>way
>> for KVM to tell userspace that it can emulate movbe though it is not
>> optimal.
>
>Ok, I don't understand.
>
>You want to tell userspace: "yes, we do support a hw feature but we
>emulate it."?
>
>> Userspace alone cannot figure it out. It can check host's cpuid
>> directly and assume that if cpuid bit is not present on the host cpu,
>> but reported as supported by KVM then it is emulated by KVM and this
>is
>> not optimal, but sometimes emulation is actually desirable (x2apic),
>so
>> such assumption is not always correct.
>
>Right, and this is what we have, AFAICT. And if userspace does that
>what
>you exemplify above, you get exactly that - a feature bit not set in
>CPUID but KVM reporting it set means, it is emulated. There's no room
>for other interpretations here. Which probably means also not optimal
>because it is not done in hw.
>
>Or, do you want to have a way to say with KVM_GET_SUPPORTED_CPUID that
>"the features I'm reporting to you are those - a subset of them are not
>optimally supported because I'm emulating them."
>
>Am I close?
>
>Thanks.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 14, 2013 at 11:36:29AM -0700, H. Peter Anvin wrote:
> Just to interject here... all this is about host kernel vs userspace,
> correct? I.e. nothing that affects the guest. I assume that the guest
> sees CPUID and that is what is supported.

Actually, currently the guest sees MOVBE in CPUID set:

max base function: 0x00000005
CPUID.EAX=0x00000000
EAX=0x00000005, EBX=0x68747541, ECX=0x444d4163, EDX=0x69746e65
CPUID.EAX=0x00000001
EAX=0x000106c2, EBX=0x01000800, ECX=0x80400201, EDX=0x0789fbff
					^
					|- bit 22

even though host (AMD) doesn't support it.

And I'd say we want it that way because the guest should show it in
CPUID so that not only the guest kernel but also guest userspace
binaries built with MOVBE can be run.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[H. Peter Anvin]

That is what I mean yes...

Borislav Petkov <bp@alien8.de> wrote:

>On Sun, Apr 14, 2013 at 11:36:29AM -0700, H. Peter Anvin wrote:
>> Just to interject here... all this is about host kernel vs userspace,
>> correct? I.e. nothing that affects the guest. I assume that the guest
>> sees CPUID and that is what is supported.
>
>Actually, currently the guest sees MOVBE in CPUID set:
>
>max base function: 0x00000005
>CPUID.EAX=0x00000000
>EAX=0x00000005, EBX=0x68747541, ECX=0x444d4163, EDX=0x69746e65
>CPUID.EAX=0x00000001
>EAX=0x000106c2, EBX=0x01000800, ECX=0x80400201, EDX=0x0789fbff
>					^
>					|- bit 22
>
>even though host (AMD) doesn't support it.
>
>And I'd say we want it that way because the guest should show it in
>CPUID so that not only the guest kernel but also guest userspace
>binaries built with MOVBE can be run.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 14, 2013 at 11:43:03AM +0300, Gleb Natapov wrote:
> > +#define EmulateOnUD ((u64)1 << 46)  /* emulate if unsupported by the host */
> >  
> Just rename VendorSpecific to EmulateOnUD. The meaning is the same.

done.

> > +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> > +{
> > +	switch (ctxt->op_bytes) {
> > +	case 2:
> > +		*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);
> What's wrong with: ctxt->dst.val = swab16((u16)ctxt->src.val)?

This doesn't work for the 16-bit swab because of the following
functionality of MOVBE:

"When the operand size is 16 bits, the upper word of the destination
register remains unchanged."

Now here's what gcc produces here:

	movzwl	112(%rdi), %eax	# ctxt_5(D)->src.D.27823.val, tmp83
	rolw	$8, %ax	#, tmp83
	movzwl	%ax, %eax	# tmp83, tmp84
	movq	%rax, 240(%rdi)	# tmp84,

See the zero-extension happening twice (second one should not be needed,
even) and it is actually writing the whole 64-bit value in %rax back
which effectively destroys the upper word?

However, this, a bit uglier version works:

	*(u16 *)&ctxt->dst.val = swab16((u16)ctxt->src.val);

	movzwl  112(%rdi), %eax # ctxt_5(D)->src.D.27823.val, tmp82
	rolw    $8, %ax #, tmp82
	movw    %ax, 240(%rdi)  # tmp82, MEM[(u16 *)ctxt_5(D) + 240B]

Whichever version we take, I'll slap a comment explaining why we're
doing it differently for 16-bit.

> > +	case 4:
> > +		*(u32 *)ctxt->dst.valptr = swab32(*(u32 *)ctxt->src.valptr);
> > +
> > +		/*
> > +		 * clear upper dword for 32-bit operand size in 64-bit mode.
> > +		 */
> > +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> > +			*((u32 *)ctxt->dst.valptr + 1) = 0x0;
> It should be clean without this this.

Right, this should probably work because for 32-bit, we don't care
about the zero-extended upper dword and in 64-bit mode we want to do it
anyway.

> > +		break;
> > +	case 8:
> > +		*(u64 *)ctxt->dst.valptr = swab64(*(u64 *)ctxt->src.valptr);

Ditto for this one.

> > +static const struct opcode threebyte_table[] = {
> > +	[0xf0] = I(DstReg | SrcMem | ModRM | Mov | EmulateOnUD, em_movbe),
> > +	[0xf1] = I(DstMem | SrcReg | ModRM | Mov | EmulateOnUD, em_movbe),
> > +};
> > +
> Populate the whole table explicitly please. You can use X16(N) to make it
> short. The encoding is also wrong since 0F 38 F0/F1 instruction decoding
> depend on a prefix.

... on the absence of a prefix, actually.

> Same opcode is decoded as CRC32 with f2 prefix. We have Prefix
> mechanism for such instructions, but it currently assumes that 66 and
> f2/f3 prefixes are mutually exclusive, but in this case they are not,
> ugh.

Yeah, I'm lazy and wanted to leave the honors to the poor soul who
implements the whole 0F_38h opcode map. :-)

Ok, I probably could do something similar to pfx_vmovntpx with the GP
macro for the F0/F1 opcodes. I'll give it a try when I get a chance
soonish.

> > +
> > +		if (ctxt->b == 0x38)
> > +			opcode = threebyte_table[insn_fetch(u8, ctxt)];
> ctxt->b = insn_fetch(u8, ctxt);
> opcode = threebyte_table[ctxt->b];
> 
> Otherwise OpImplicit type of decoding will not work for three byte
> instructions. Also should rename twobyte into opbytes and set it to 1,
> 2 or 3. I prefer that three byte instruction decoding will be sent as a
> separate patch.

Ok.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Paolo Bonzini]

Il 14/04/2013 23:02, Borislav Petkov ha scritto:
> 	*(u16 *)&ctxt->dst.val = swab16((u16)ctxt->src.val);
> 
> 	movzwl  112(%rdi), %eax # ctxt_5(D)->src.D.27823.val, tmp82
> 	rolw    $8, %ax #, tmp82
> 	movw    %ax, 240(%rdi)  # tmp82, MEM[(u16 *)ctxt_5(D) + 240B]

I think this breaks the C aliasing rules.

Using valptr is okay because it is a char.

Paolo

Re: [RFC PATCH] Emulate MOVBE

[Paolo Bonzini]

Il 10/04/2013 12:08, Gleb Natapov ha scritto:
>> > What is the opinion from the KVM folks on this? Shall we start to
>> > emulate instructions the host does not provide? In this particular case
>> > a relatively simple patch fixes a problem (starting Atom optimized
>> > kernels on non-Atom machines).
> We can add the emulation, but we should not start announcing the instruction
> availability to a guest if host cpu does not have it by default. This
> may trick a guest into thinking that movbe is the fastest way to do
> something when it is not.
> 

This does highlight a weakness in CPU_GET_SUPPORTED_CPUID, but I think
this is not a problem in practice.

With a management layer such as oVirt it's not a problem.  For example,
oVirt has its own library of processors.  It doesn't care if KVM enables
movbe.  If you tell it your datacenter is a mix of Haswells and Sandy
Bridges it will pick the CPUID bits that are common to all.

However, even without a suitable management layer it is also not really
a problem.

The only processors that support MOVBE are Atom and Haswell.  Haswell
adds a whole lot of extra CPUID features, hence "-cpu Haswell,enforce"
will fail with or without movbe emulation.  "-cpu Haswell" will disable
all Haswell new features except movbe will remain slow; that's fine, I
think, anyway it's not what you'ld do except to play with CPU models.

Atom is not defined by QEMU; even if it was, it is unlikely to be
specified for a KVM guest since Atom doesn't support hardware
virtualization itself.

The next AMD processor that has MOVBE will probably have at least
another feature that is not in Opteron_G5, thus it will be the same as
Haswell.

Paolo

Re: [RFC PATCH] Emulate MOVBE

[Borislav Petkov]

On Tue, Apr 16, 2013 at 01:47:46PM +0200, Paolo Bonzini wrote:
> Atom is not defined by QEMU;

$ qemu-system-x86_64 -cpu ?

...

x86             n270  Intel(R) Atom(TM) CPU N270   @ 1.60GHz

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH] Emulate MOVBE

[H. Peter Anvin]

Note that "Atom" isn't a CPU but a line of CPUs. Sadly Qemu's N270 model is broken.

Borislav Petkov <bp@alien8.de> wrote:

>On Tue, Apr 16, 2013 at 01:47:46PM +0200, Paolo Bonzini wrote:
>> Atom is not defined by QEMU;
>
>$ qemu-system-x86_64 -cpu ?
>
>...
>
>x86             n270  Intel(R) Atom(TM) CPU N270   @ 1.60GHz

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC PATCH] Emulate MOVBE

[Gleb Natapov]

On Tue, Apr 16, 2013 at 01:47:46PM +0200, Paolo Bonzini wrote:
> Il 10/04/2013 12:08, Gleb Natapov ha scritto:
> >> > What is the opinion from the KVM folks on this? Shall we start to
> >> > emulate instructions the host does not provide? In this particular case
> >> > a relatively simple patch fixes a problem (starting Atom optimized
> >> > kernels on non-Atom machines).
> > We can add the emulation, but we should not start announcing the instruction
> > availability to a guest if host cpu does not have it by default. This
> > may trick a guest into thinking that movbe is the fastest way to do
> > something when it is not.
> > 
> 
> This does highlight a weakness in CPU_GET_SUPPORTED_CPUID, but I think
> this is not a problem in practice.
> 
> With a management layer such as oVirt it's not a problem.  For example,
> oVirt has its own library of processors.  It doesn't care if KVM enables
> movbe.  If you tell it your datacenter is a mix of Haswells and Sandy
> Bridges it will pick the CPUID bits that are common to all.
> 
> However, even without a suitable management layer it is also not really
> a problem.
> 
> The only processors that support MOVBE are Atom and Haswell.  Haswell
> adds a whole lot of extra CPUID features, hence "-cpu Haswell,enforce"
> will fail with or without movbe emulation.  "-cpu Haswell" will disable
> all Haswell new features except movbe will remain slow; that's fine, I
> think, anyway it's not what you'ld do except to play with CPU models.
> 
No that's not fine. KVM should not trick userspace (QEMU is just one of
them) into nonoptimal configuration. And you forgot about -cpu host in your
analysis.

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Sun, Apr 14, 2013 at 07:32:16PM +0200, Borislav Petkov wrote:
> On Sun, Apr 14, 2013 at 10:41:07AM +0300, Gleb Natapov wrote:
> > Currently userspace assumes that that cpuid configuration returned by
> > KVM_GET_SUPPORTED_CPUID is the optimal one. What we want here is a way
> > for KVM to tell userspace that it can emulate movbe though it is not
> > optimal.
> 
> Ok, I don't understand.
> 
> You want to tell userspace: "yes, we do support a hw feature but we
> emulate it."?
> 
I am contemplating this, yes.

> > Userspace alone cannot figure it out. It can check host's cpuid
> > directly and assume that if cpuid bit is not present on the host cpu,
> > but reported as supported by KVM then it is emulated by KVM and this is
> > not optimal, but sometimes emulation is actually desirable (x2apic), so
> > such assumption is not always correct.
> 
> Right, and this is what we have, AFAICT. And if userspace does that what
> you exemplify above, you get exactly that - a feature bit not set in
> CPUID but KVM reporting it set means, it is emulated. There's no room
> for other interpretations here. Which probably means also not optimal
> because it is not done in hw.
> 
This is not true for all emulated CPUID bits. X2APIC is emulated and it
is preferable for a guest to use it for example.

> Or, do you want to have a way to say with KVM_GET_SUPPORTED_CPUID that
> "the features I'm reporting to you are those - a subset of them are not
> optimally supported because I'm emulating them."
> 
> Am I close?
> 
Yes, you are. I am considering such interface. Adding new specialized interfaces
is last resort though. I am open to other ideas.

--
			Gleb.

Re: [RFC PATCH] Emulate MOVBE

[Paolo Bonzini]

Il 16/04/2013 19:28, Gleb Natapov ha scritto:
> > This does highlight a weakness in CPU_GET_SUPPORTED_CPUID, but I think
> > this is not a problem in practice.
> > 
> > With a management layer such as oVirt it's not a problem.  For example,
> > oVirt has its own library of processors.  It doesn't care if KVM enables
> > movbe.  If you tell it your datacenter is a mix of Haswells and Sandy
> > Bridges it will pick the CPUID bits that are common to all.
> > 
> > However, even without a suitable management layer it is also not really
> > a problem.
> > 
> > The only processors that support MOVBE are Atom and Haswell.  Haswell
> > adds a whole lot of extra CPUID features, hence "-cpu Haswell,enforce"
> > will fail with or without movbe emulation.  "-cpu Haswell" will disable
> > all Haswell new features except movbe will remain slow; that's fine, I
> > think, anyway it's not what you'ld do except to play with CPU models.
>
> No that's not fine. KVM should not trick userspace (QEMU is just one of
> them) into nonoptimal configuration. And you forgot about -cpu host in your
> analysis.

"-cpu host" enables bits that are not in the host, but that's a QEMU
bug.  If the host lacks x2apic, for example, "-cpu host" should not
enable it even if it is there in KVM_GET_SUPPORTED_CPUID.

Now, KVM_GET_SUPPORTED_CPUID is a pretty bad interface for implementing
a sane "CPU as similar to the host's as possible" policy.  That's
another story and I agree.  It's a very sane policy for simple
userspaces like lkvm.

Paolo

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Tue, Apr 16, 2013 at 08:42:36PM +0300, Gleb Natapov wrote:
> > Right, and this is what we have, AFAICT. And if userspace does that what
> > you exemplify above, you get exactly that - a feature bit not set in
> > CPUID but KVM reporting it set means, it is emulated. There's no room
> > for other interpretations here. Which probably means also not optimal
> > because it is not done in hw.
> > 
> This is not true for all emulated CPUID bits. X2APIC is emulated and it
> is preferable for a guest to use it for example.

Right, and on boxes without X2APIC support you will not find
CPUID(1).ECX[21] set. kvm will tell you it is supported, though, which
means, kvm emulates it.

> > Or, do you want to have a way to say with KVM_GET_SUPPORTED_CPUID that
> > "the features I'm reporting to you are those - a subset of them are not
> > optimally supported because I'm emulating them."
> > 
> > Am I close?
> > 
> Yes, you are. I am considering such interface. Adding new specialized
> interfaces is last resort though. I am open to other ideas.

Hmm, ok, so basically we want qemu to query the host CPUID and see which
features are *really* supported, then do KVM_GET_SUPPORTED_CPUID and see
which are emulated.

How's that? Too simple?

I probably am missing something as a novice qemu user.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH] Emulate MOVBE

[Gleb Natapov]

On Wed, Apr 17, 2013 at 12:42:30PM +0200, Paolo Bonzini wrote:
> Il 16/04/2013 19:28, Gleb Natapov ha scritto:
> > > This does highlight a weakness in CPU_GET_SUPPORTED_CPUID, but I think
> > > this is not a problem in practice.
> > > 
> > > With a management layer such as oVirt it's not a problem.  For example,
> > > oVirt has its own library of processors.  It doesn't care if KVM enables
> > > movbe.  If you tell it your datacenter is a mix of Haswells and Sandy
> > > Bridges it will pick the CPUID bits that are common to all.
> > > 
> > > However, even without a suitable management layer it is also not really
> > > a problem.
> > > 
> > > The only processors that support MOVBE are Atom and Haswell.  Haswell
> > > adds a whole lot of extra CPUID features, hence "-cpu Haswell,enforce"
> > > will fail with or without movbe emulation.  "-cpu Haswell" will disable
> > > all Haswell new features except movbe will remain slow; that's fine, I
> > > think, anyway it's not what you'ld do except to play with CPU models.
> >
> > No that's not fine. KVM should not trick userspace (QEMU is just one of
> > them) into nonoptimal configuration. And you forgot about -cpu host in your
> > analysis.
> 
> "-cpu host" enables bits that are not in the host, but that's a QEMU
> bug.  If the host lacks x2apic, for example, "-cpu host" should not
> enable it even if it is there in KVM_GET_SUPPORTED_CPUID.
> 
Why? We always said that if performance is the only thing user cares
about he should use "-cpu host". Not exposing emulated x2apic to a guest
will not provide best configuration possible. If user uses -cpu guest_cpu_model
and the guest_cpu_model is the same as host cpu model and host cpu
model does not have x2apic then guest should not see x2apic either, but
this works correctly today already.

> Now, KVM_GET_SUPPORTED_CPUID is a pretty bad interface for implementing
> a sane "CPU as similar to the host's as possible" policy.  That's
> another story and I agree.  It's a very sane policy for simple
> userspaces like lkvm.
> 
Thats because -cpu host is a misnomer. It really should be -cpu best
since its task is too create most efficient configuration given host
cpu. So you can wave -cpu host problem by saying that it should filter
non host cpuid bits from KVM_GET_SUPPORTED_CPUID, but then you will want
to implement -cpu best and you will encounter the same problem, but
without any excuse now :)

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Wed, Apr 17, 2013 at 01:04:34PM +0200, Borislav Petkov wrote:
> On Tue, Apr 16, 2013 at 08:42:36PM +0300, Gleb Natapov wrote:
> > > Right, and this is what we have, AFAICT. And if userspace does that what
> > > you exemplify above, you get exactly that - a feature bit not set in
> > > CPUID but KVM reporting it set means, it is emulated. There's no room
> > > for other interpretations here. Which probably means also not optimal
> > > because it is not done in hw.
> > > 
> > This is not true for all emulated CPUID bits. X2APIC is emulated and it
> > is preferable for a guest to use it for example.
> 
> Right, and on boxes without X2APIC support you will not find
> CPUID(1).ECX[21] set. kvm will tell you it is supported, though, which
> means, kvm emulates it.
> 
Yes, but it is "good" emulation. You want to have it for performance
even if host does not have x2apic. x2apic is emulated no matter if host
cpu has it or not. movbe is different, you want to emulate it only if
your guest requires it.

> > > Or, do you want to have a way to say with KVM_GET_SUPPORTED_CPUID that
> > > "the features I'm reporting to you are those - a subset of them are not
> > > optimally supported because I'm emulating them."
> > > 
> > > Am I close?
> > > 
> > Yes, you are. I am considering such interface. Adding new specialized
> > interfaces is last resort though. I am open to other ideas.
> 
> Hmm, ok, so basically we want qemu to query the host CPUID and see which
> features are *really* supported, then do KVM_GET_SUPPORTED_CPUID and see
> which are emulated.
> 
> How's that? Too simple?
As I said this will filter x2apic on machines which host cpu does not
support it. This is not what we want.

> 
> I probably am missing something as a novice qemu user.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Wed, Apr 17, 2013 at 04:38:29PM +0300, Gleb Natapov wrote:
> Yes, but it is "good" emulation. You want to have it for performance
> even if host does not have x2apic. x2apic is emulated no matter if
> host cpu has it or not. movbe is different, you want to emulate it
> only if your guest requires it.

It sounds to me you want to have feature bits which are *explicitly*
enabled on the command line nandled differently in qemu.

Let me elaborate:

1. x2apic is always enabled in qemu and will be reported through
KVM_GET_SUPPORTED_CPUID, no matter whether emulated or not.

2. movbe, which got enabled on the command line through "-cpu
n270,+movbe" will be enabled for this specific guest only. Anything else
*without* "+movbe" on the command line, will not get the emulation.

Those 2 categories or am I missing a third one?

> > How's that? Too simple?
> As I said this will filter x2apic on machines which host cpu does not
> support it. This is not what we want.

Right, so basically we want to handle features which were explicitly
enabled only for this guest as private, only relevant to this particular
guest run.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Wed, Apr 17, 2013 at 04:02:00PM +0200, Borislav Petkov wrote:
> Right, so basically we want to handle features which were explicitly
> enabled only for this guest as private, only relevant to this
> particular guest run.

Ok,

here are two more ideas Joerg and I had today during lunch:

* reuse KVM_GET_SUPPORTED_CPUID

we hand-in a struct kvm_cpuid_entry2 with ->function and respective bits
in e[abcd]x set for each CPUID leaf we want to query kvm.

Once in the kernel, we do the following:

if ->function is not 0xffffffff, it means userspace wants us to look at
the all set bits in the respective e[abcd]x members.

For each set bit, we check whether we emulate the respective feature
and if so, we leave it untouched before returning it to userspace.
Otherwise, we clear it before OR-ing in the host bits and the
good-emulated bits like x2apic.

Yeah, semantics need to be handled carefully, but it has this
knock-on-door aspect where kvm says that it actually emulates a feature
only if asked, i.e. with the -cpu ...,+<feature> syntax.

* new ioctl KVM_GET_EMULATED_CPUID

Might be overkill and might be used only in a limited fashion since we
don't want to emulate *every* feature in kvm.

Hmmm. I kinda like the first one more while the second one is cleaner.

Opinions?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Fri, Apr 19, 2013 at 12:48:48AM +0200, Borislav Petkov wrote:
> On Wed, Apr 17, 2013 at 04:02:00PM +0200, Borislav Petkov wrote:
> > Right, so basically we want to handle features which were explicitly
> > enabled only for this guest as private, only relevant to this
> > particular guest run.
> 
> Ok,
> 
> here are two more ideas Joerg and I had today during lunch:
> 
> * reuse KVM_GET_SUPPORTED_CPUID
> 
> we hand-in a struct kvm_cpuid_entry2 with ->function and respective bits
> in e[abcd]x set for each CPUID leaf we want to query kvm.
> 
> Once in the kernel, we do the following:
> 
> if ->function is not 0xffffffff, it means userspace wants us to look at
> the all set bits in the respective e[abcd]x members.
There may be userspaces that set ->function to 0xffffffff (just because
they do not init the buffer before calling into the kernel) and this will
break them.

> 
> For each set bit, we check whether we emulate the respective feature
> and if so, we leave it untouched before returning it to userspace.
> Otherwise, we clear it before OR-ing in the host bits and the
> good-emulated bits like x2apic.
> 
> Yeah, semantics need to be handled carefully, but it has this
> knock-on-door aspect where kvm says that it actually emulates a feature
> only if asked, i.e. with the -cpu ...,+<feature> syntax.
> 
> * new ioctl KVM_GET_EMULATED_CPUID
> 
> Might be overkill and might be used only in a limited fashion since we
> don't want to emulate *every* feature in kvm.
> 
> Hmmm. I kinda like the first one more while the second one is cleaner.
> 
The first one needs explicit userspace support to work correctly. This
should be other way around: old userspace should do the right thing, but may
not support new features, new userspace should be able to support new
feature.

We may extend KVM_GET_SUPPORTED_CPUID instead of adding new one. There
is a padding field in kvm_cpuid2 that we could have reused as flags,
but unfortunately current implementation does not error if the field is
not zero, so if there is a userspace that does not zero the padding it may
break. Another options is to reuse high bits of nent as flags (not very
nice, but will work).

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 21, 2013 at 12:46:51PM +0300, Gleb Natapov wrote:
> There may be userspaces that set ->function to 0xffffffff (just
> because they do not init the buffer before calling into the kernel)
> and this will break them.

You don't mean 0xffffffff here but rather something random which is not
properly initialized and by chance is a valid CPUID leaf. Then, if some
of the bits in the register variables e[abcd]x are also set, we return
with emulated bits set.

> > For each set bit, we check whether we emulate the respective feature
> > and if so, we leave it untouched before returning it to userspace.
> > Otherwise, we clear it before OR-ing in the host bits and the
> > good-emulated bits like x2apic.
> > 
> > Yeah, semantics need to be handled carefully, but it has this
> > knock-on-door aspect where kvm says that it actually emulates a feature
> > only if asked, i.e. with the -cpu ...,+<feature> syntax.
> > 
> > * new ioctl KVM_GET_EMULATED_CPUID
> > 
> > Might be overkill and might be used only in a limited fashion since we
> > don't want to emulate *every* feature in kvm.
> > 
> > Hmmm. I kinda like the first one more while the second one is cleaner.
> > 
> The first one needs explicit userspace support to work correctly. This
> should be other way around: old userspace should do the right thing, but may
> not support new features, new userspace should be able to support new
> feature.

Crap, not even qemu is handing in cleared buffers with that g_malloc0()
thing, AFAICT.

> We may extend KVM_GET_SUPPORTED_CPUID instead of adding new one. There
> is a padding field in kvm_cpuid2 that we could have reused as flags,

That would've been lovely.

> but unfortunately current implementation does not error if the field is
> not zero, so if there is a userspace that does not zero the padding it may
> break. Another options is to reuse high bits of nent as flags (not very
> nice, but will work).

Not nice?! It is a very nasty hack - that's what it is. :-)

Frankly speaking, I'd rather prefer adding a new ioctl. Since old
userspace won't support the new features, then we just as well can
simply add the new ioctl.

In all the cases we need to touch userspace - be it to OR in the flags
into ->nent or to implement the new ioctl. So why not do it in a clean
manner from the get-go?

Hmmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Tue, Apr 16, 2013 at 01:36:00PM +0200, Paolo Bonzini wrote:
> Il 14/04/2013 23:02, Borislav Petkov ha scritto:
> > 	*(u16 *)&ctxt->dst.val = swab16((u16)ctxt->src.val);
> > 
> > 	movzwl  112(%rdi), %eax # ctxt_5(D)->src.D.27823.val, tmp82
> > 	rolw    $8, %ax #, tmp82
> > 	movw    %ax, 240(%rdi)  # tmp82, MEM[(u16 *)ctxt_5(D) + 240B]
> 
> I think this breaks the C aliasing rules.
> 
> Using valptr is okay because it is a char.

Yep, good catch.

We normally build with -fno-strict-aliasing but when I change that, gcc
catches it:

arch/x86/kvm/emulate.c: In function ‘em_movbe’:
arch/x86/kvm/emulate.c:3121:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

However, it screams even louder about the valptr variant

	*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);

too:

arch/x86/kvm/emulate.c: In function ‘em_movbe’:
arch/x86/kvm/emulate.c:3117:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
arch/x86/kvm/emulate.c:3117:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
arch/x86/kvm/emulate.c:3117:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
arch/x86/kvm/emulate.c:3117:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
arch/x86/kvm/emulate.c:3117:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

We probably need something with copying values to a temp variable or so.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> We probably need something with copying values to a temp variable or so.

Basically something like that:

        case 2:
                /*
                 * From MOVBE definition: "...When the operand size is 16 bits,
                 * the upper word of the destination register remains unchanged
                 * ..."
                 *
                 * Both casting ->valptr and ->val to u16 breaks strict aliasing
                 * rules so we have to do the operation almost per hand.
                 */
                tmp = (u16)ctxt->src.val;
                ctxt->dst.val &= ~0xffffUL;
                ctxt->dst.val |= (unsigned long)swab16(tmp);
		break;

This passes all gcc checks, even the stricter ones when building with W=3.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Sun, Apr 21, 2013 at 01:30:51PM +0200, Borislav Petkov wrote:
> On Sun, Apr 21, 2013 at 12:46:51PM +0300, Gleb Natapov wrote:
> > There may be userspaces that set ->function to 0xffffffff (just
> > because they do not init the buffer before calling into the kernel)
> > and this will break them.
> 
> You don't mean 0xffffffff here but rather something random which is not
> properly initialized and by chance is a valid CPUID leaf. Then, if some
> of the bits in the register variables e[abcd]x are also set, we return
> with emulated bits set.
> 
Yes.

> > > For each set bit, we check whether we emulate the respective feature
> > > and if so, we leave it untouched before returning it to userspace.
> > > Otherwise, we clear it before OR-ing in the host bits and the
> > > good-emulated bits like x2apic.
> > > 
> > > Yeah, semantics need to be handled carefully, but it has this
> > > knock-on-door aspect where kvm says that it actually emulates a feature
> > > only if asked, i.e. with the -cpu ...,+<feature> syntax.
> > > 
> > > * new ioctl KVM_GET_EMULATED_CPUID
> > > 
> > > Might be overkill and might be used only in a limited fashion since we
> > > don't want to emulate *every* feature in kvm.
> > > 
> > > Hmmm. I kinda like the first one more while the second one is cleaner.
> > > 
> > The first one needs explicit userspace support to work correctly. This
> > should be other way around: old userspace should do the right thing, but may
> > not support new features, new userspace should be able to support new
> > feature.
> 
> Crap, not even qemu is handing in cleared buffers with that g_malloc0()
> thing, AFAICT.
> 
> > We may extend KVM_GET_SUPPORTED_CPUID instead of adding new one. There
> > is a padding field in kvm_cpuid2 that we could have reused as flags,
> 
> That would've been lovely.
> 
> > but unfortunately current implementation does not error if the field is
> > not zero, so if there is a userspace that does not zero the padding it may
> > break. Another options is to reuse high bits of nent as flags (not very
> > nice, but will work).
> 
> Not nice?! It is a very nasty hack - that's what it is. :-)
> 
Agree, but not nastier than expecting ->function to have special value :)

> Frankly speaking, I'd rather prefer adding a new ioctl. Since old
> userspace won't support the new features, then we just as well can
> simply add the new ioctl.
> 
> In all the cases we need to touch userspace - be it to OR in the flags
> into ->nent or to implement the new ioctl. So why not do it in a clean
> manner from the get-go?
> 
Agree here too.

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Paolo Bonzini]

Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
>> We probably need something with copying values to a temp variable or so.
> 
> Basically something like that:
> 
>         case 2:
>                 /*
>                  * From MOVBE definition: "...When the operand size is 16 bits,
>                  * the upper word of the destination register remains unchanged
>                  * ..."
>                  *
>                  * Both casting ->valptr and ->val to u16 breaks strict aliasing
>                  * rules so we have to do the operation almost per hand.
>                  */
>                 tmp = (u16)ctxt->src.val;
>                 ctxt->dst.val &= ~0xffffUL;
>                 ctxt->dst.val |= (unsigned long)swab16(tmp);
> 		break;
> 
> This passes all gcc checks, even the stricter ones when building with W=3.

I thought the valptr one was ok.  I find this one more readable, too.
How does the generated code look like?

Paolo

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Mon, Apr 22, 2013 at 10:53:42AM +0200, Paolo Bonzini wrote:
> Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> > On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> >> We probably need something with copying values to a temp variable or so.
> > 
> > Basically something like that:
> > 
> >         case 2:
> >                 /*
> >                  * From MOVBE definition: "...When the operand size is 16 bits,
> >                  * the upper word of the destination register remains unchanged
> >                  * ..."
> >                  *
> >                  * Both casting ->valptr and ->val to u16 breaks strict aliasing
> >                  * rules so we have to do the operation almost per hand.
> >                  */
> >                 tmp = (u16)ctxt->src.val;
> >                 ctxt->dst.val &= ~0xffffUL;
> >                 ctxt->dst.val |= (unsigned long)swab16(tmp);
> > 		break;
> > 
> > This passes all gcc checks, even the stricter ones when building with W=3.
> 
> I thought the valptr one was ok.

Yep, it looked like that too. And, it could actually really be ok and
the gcc's warning here is bogus. I'll try to talk to gcc people about
it.

> I find this one more readable, too. How does the generated code look
> like?

Well, so so:

	movzwl	112(%rdi), %eax	# ctxt_5(D)->src.D.27823.val, tmp87
	movq	240(%rdi), %rdx	# ctxt_5(D)->dst.D.27823.val, tmp89
	xorw	%dx, %dx	# tmp89
	rolw	$8, %ax	#, tmp87
	movzwl	%ax, %eax	# tmp87, tmp91

I have hard time understanding why it is adding this insn here - it can
simply drop it and continue with the 64-bit OR. It's not like it changes
anything...

	orq	%rdx, %rax	# tmp89, tmp91
	movq	%rax, 240(%rdi)	# tmp91, ctxt_5(D)->dst.D.27823.val

Btw, I wanted to ask: when kvm commits the results, does it look at
ctxt->op_bytes to know exactly how many bytes to write to the guest?
Because if it does, we can save ourselves the trouble here.

Or does it simply write both the full sizeof(unsigned long) bytes of
->src.val and ->dst.val to the guest?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Mon, Apr 22, 2013 at 11:38:10AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 10:53:42AM +0200, Paolo Bonzini wrote:
> > Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> > > On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> > >> We probably need something with copying values to a temp variable or so.
> > > 
> > > Basically something like that:
> > > 
> > >         case 2:
> > >                 /*
> > >                  * From MOVBE definition: "...When the operand size is 16 bits,
> > >                  * the upper word of the destination register remains unchanged
> > >                  * ..."
> > >                  *
> > >                  * Both casting ->valptr and ->val to u16 breaks strict aliasing
> > >                  * rules so we have to do the operation almost per hand.
> > >                  */
> > >                 tmp = (u16)ctxt->src.val;
> > >                 ctxt->dst.val &= ~0xffffUL;
> > >                 ctxt->dst.val |= (unsigned long)swab16(tmp);
> > > 		break;
> > > 
> > > This passes all gcc checks, even the stricter ones when building with W=3.
> > 
> > I thought the valptr one was ok.
> 
> Yep, it looked like that too. And, it could actually really be ok and
> the gcc's warning here is bogus. I'll try to talk to gcc people about
> it.
> 
> > I find this one more readable, too. How does the generated code look
> > like?
> 
> Well, so so:
> 
> 	movzwl	112(%rdi), %eax	# ctxt_5(D)->src.D.27823.val, tmp87
> 	movq	240(%rdi), %rdx	# ctxt_5(D)->dst.D.27823.val, tmp89
> 	xorw	%dx, %dx	# tmp89
> 	rolw	$8, %ax	#, tmp87
> 	movzwl	%ax, %eax	# tmp87, tmp91
> 
> I have hard time understanding why it is adding this insn here - it can
> simply drop it and continue with the 64-bit OR. It's not like it changes
> anything...
> 
> 	orq	%rdx, %rax	# tmp89, tmp91
> 	movq	%rax, 240(%rdi)	# tmp91, ctxt_5(D)->dst.D.27823.val
> 
> Btw, I wanted to ask: when kvm commits the results, does it look at
> ctxt->op_bytes to know exactly how many bytes to write to the guest?
> Because if it does, we can save ourselves the trouble here.
> 
> Or does it simply write both the full sizeof(unsigned long) bytes of
> ->src.val and ->dst.val to the guest?
> 
No, it does this in case of register operand:

static void write_register_operand(struct operand *op)
{
        /* The 4-byte case *is* correct: in 64-bit mode we zero-extend.  */
        switch (op->bytes) {
        case 1:
                *(u8 *)op->addr.reg = (u8)op->val;
                break;
        case 2:
                *(u16 *)op->addr.reg = (u16)op->val;
                break;
        case 4:
                *op->addr.reg = (u32)op->val;
                break;  /* 64b: zero-extend */
        case 8:
                *op->addr.reg = op->val;
                break;
        }
}

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Mon, Apr 22, 2013 at 12:42:46PM +0300, Gleb Natapov wrote:
> > Btw, I wanted to ask: when kvm commits the results, does it look at
> > ctxt->op_bytes to know exactly how many bytes to write to the guest?
> > Because if it does, we can save ourselves the trouble here.
> > 
> > Or does it simply write both the full sizeof(unsigned long) bytes of
> > ->src.val and ->dst.val to the guest?
> > 
> No, it does this in case of register operand:
> 
> static void write_register_operand(struct operand *op)
> {
>         /* The 4-byte case *is* correct: in 64-bit mode we zero-extend.  */
>         switch (op->bytes) {
>         case 1:
>                 *(u8 *)op->addr.reg = (u8)op->val;
>                 break;
>         case 2:
>                 *(u16 *)op->addr.reg = (u16)op->val;
>                 break;
>         case 4:
>                 *op->addr.reg = (u32)op->val;
>                 break;  /* 64b: zero-extend */
>         case 8:
>                 *op->addr.reg = op->val;
>                 break;
>         }
> }

Ok, and for OP_MEM it does look at ctxt->dst.bytes in writeback(),
AFAICT. And I see other emulated instructions like POPF, for example, do
this:

	ctxt->dst.bytes = ctxt->op_bytes;

Which means, we can drop all the bullshit in em_movbe and even destroy
some of the bytes in dst.val but only write out the correct ones. Which
means, a simpler code and a lot less jumping through hoops.

Would that be the more accepted practice?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Mon, Apr 22, 2013 at 11:52:03AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 12:42:46PM +0300, Gleb Natapov wrote:
> > > Btw, I wanted to ask: when kvm commits the results, does it look at
> > > ctxt->op_bytes to know exactly how many bytes to write to the guest?
> > > Because if it does, we can save ourselves the trouble here.
> > > 
> > > Or does it simply write both the full sizeof(unsigned long) bytes of
> > > ->src.val and ->dst.val to the guest?
> > > 
> > No, it does this in case of register operand:
> > 
> > static void write_register_operand(struct operand *op)
> > {
> >         /* The 4-byte case *is* correct: in 64-bit mode we zero-extend.  */
> >         switch (op->bytes) {
> >         case 1:
> >                 *(u8 *)op->addr.reg = (u8)op->val;
> >                 break;
> >         case 2:
> >                 *(u16 *)op->addr.reg = (u16)op->val;
> >                 break;
> >         case 4:
> >                 *op->addr.reg = (u32)op->val;
> >                 break;  /* 64b: zero-extend */
> >         case 8:
> >                 *op->addr.reg = op->val;
> >                 break;
> >         }
> > }
> 
> Ok, and for OP_MEM it does look at ctxt->dst.bytes in writeback(),
> AFAICT. And I see other emulated instructions like POPF, for example, do
> this:
> 
> 	ctxt->dst.bytes = ctxt->op_bytes;
> 
> Which means, we can drop all the bullshit in em_movbe and even destroy
> some of the bytes in dst.val but only write out the correct ones. Which
> means, a simpler code and a lot less jumping through hoops.
> 
> Would that be the more accepted practice?
> 
For most instructions the decoder already sets op->bytes to correct value,
given that all flags a correctly specified in opcode table. Explicit
op->bytes setting should be done only if it cannot be expressed by
opcode flags.

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Mon, Apr 22, 2013 at 12:58:12PM +0300, Gleb Natapov wrote:
> For most instructions the decoder already sets op->bytes to correct
> value, given that all flags a correctly specified in opcode table.
> Explicit op->bytes setting should be done only if it cannot be
> expressed by opcode flags.

MOVBE encodes operands in ModRM and operand size is determined by the
effective operand size. By looking at that switch(mode) thing near
the beginning of x86_decode_insn, we make sure ctxt->op_bytes is set
accordingly. Then, we have the following definitions for MOVBE:

+       [0xf0] = I(DstReg | SrcMem | ModRM | Mov | ThreeByte | EmulateOnUD, em_movbe),
+       [0xf1] = I(DstMem | SrcReg | ModRM | Mov | ThreeByte | EmulateOnUD, em_movbe),

and from looking at decode_operand(), it makes sure that op->bytes gets
the correct value since we have the proper {Src,Dst}{Reg,Mem} flags in
the insn definition.

So everything is fine, I'll make sure it works that way too, though,
when testing.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Sun, Apr 21, 2013 at 03:51:16PM +0300, Gleb Natapov wrote:
> > Not nice?! It is a very nasty hack - that's what it is. :-)
> > 
> Agree, but not nastier than expecting ->function to have special value :)

Probably of the same nastiness level :-)

> > Frankly speaking, I'd rather prefer adding a new ioctl. Since old
> > userspace won't support the new features, then we just as well can
> > simply add the new ioctl.
> > 
> > In all the cases we need to touch userspace - be it to OR in the flags
> > into ->nent or to implement the new ioctl. So why not do it in a clean
> > manner from the get-go?
> > 
> Agree here too.

Btw, in thinking about this more, I'm kinda sceptical we want to use the
CPUID layout for this new KVM_GET_EMULATED_* ioctl. And the reason why
I'm sceptical is that not every instruction is behind a CPUID capability
bit and if we want to tell userspace that we do emulate any insn, even
one for which there's no CPUID bit, we're going to have to either
simulate a kvm-specific CPUID leaf or, maybe even better, come up with
our own format for emulated capabilities. Maybe a bit vector with set
bits for the respective capability, or something more nifty.

In any case, it doesn't really need to be CPUID-like, IMHO.

Hmmm...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[H. Peter Anvin]

On 04/23/2013 04:41 PM, Borislav Petkov wrote:
> 
> Btw, in thinking about this more, I'm kinda sceptical we want to use the
> CPUID layout for this new KVM_GET_EMULATED_* ioctl. And the reason why
> I'm sceptical is that not every instruction is behind a CPUID capability
> bit and if we want to tell userspace that we do emulate any insn, even
> one for which there's no CPUID bit, we're going to have to either
> simulate a kvm-specific CPUID leaf or, maybe even better, come up with
> our own format for emulated capabilities. Maybe a bit vector with set
> bits for the respective capability, or something more nifty.
> 
> In any case, it doesn't really need to be CPUID-like, IMHO.
> 

Using CPUID has the major advantage that it is well-defined.

	-hpa

Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov]

On Tue, Apr 23, 2013 at 04:50:43PM -0700, H. Peter Anvin wrote:
> On 04/23/2013 04:41 PM, Borislav Petkov wrote:
> > 
> > Btw, in thinking about this more, I'm kinda sceptical we want to use the
> > CPUID layout for this new KVM_GET_EMULATED_* ioctl. And the reason why
> > I'm sceptical is that not every instruction is behind a CPUID capability
> > bit and if we want to tell userspace that we do emulate any insn, even
> > one for which there's no CPUID bit, we're going to have to either
> > simulate a kvm-specific CPUID leaf or, maybe even better, come up with
> > our own format for emulated capabilities. Maybe a bit vector with set
> > bits for the respective capability, or something more nifty.
> > 
> > In any case, it doesn't really need to be CPUID-like, IMHO.
> > 
> 
> Using CPUID has the major advantage that it is well-defined.
> 
This. And I really hope vendors will not add instructions without
corespondent CPUID bits nowadays.

--
			Gleb.

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Tue, Apr 23, 2013 at 04:50:43PM -0700, H. Peter Anvin wrote:
> On 04/23/2013 04:41 PM, Borislav Petkov wrote:
> > 
> > Btw, in thinking about this more, I'm kinda sceptical we want to use the
> > CPUID layout for this new KVM_GET_EMULATED_* ioctl. And the reason why
> > I'm sceptical is that not every instruction is behind a CPUID capability
> > bit and if we want to tell userspace that we do emulate any insn, even
> > one for which there's no CPUID bit, we're going to have to either
> > simulate a kvm-specific CPUID leaf or, maybe even better, come up with
> > our own format for emulated capabilities. Maybe a bit vector with set
> > bits for the respective capability, or something more nifty.
> > 
> > In any case, it doesn't really need to be CPUID-like, IMHO.
> > 
> Using CPUID has the major advantage that it is well-defined.

Right, and it is actually a tree of bitvectors of height of 1. :-)

However, in the remote case where we want to state an emulated
capability for which there is no CPUID bit, we probably need something.

>From looking at do_cpuid_ent(), kvm uses the 0x40000000 so we probably
could define such a flag in there. If needed, of course.

So KVM_GET_EMULATED_CPUID it is. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov]

On Mon, Apr 22, 2013 at 11:38:10AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 10:53:42AM +0200, Paolo Bonzini wrote:
> > Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> > > On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> > >> We probably need something with copying values to a temp variable or so.
> > > 
> > > Basically something like that:
> > > 
> > >         case 2:
> > >                 /*
> > >                  * From MOVBE definition: "...When the operand size is 16 bits,
> > >                  * the upper word of the destination register remains unchanged
> > >                  * ..."
> > >                  *
> > >                  * Both casting ->valptr and ->val to u16 breaks strict aliasing
> > >                  * rules so we have to do the operation almost per hand.
> > >                  */
> > >                 tmp = (u16)ctxt->src.val;
> > >                 ctxt->dst.val &= ~0xffffUL;
> > >                 ctxt->dst.val |= (unsigned long)swab16(tmp);
> > > 		break;
> > > 
> > > This passes all gcc checks, even the stricter ones when building with W=3.
> > 
> > I thought the valptr one was ok.
> 
> Yep, it looked like that too. And, it could actually really be ok and
> the gcc's warning here is bogus. I'll try to talk to gcc people about
> it.

Ok, I did and here's the explanation, as far as I understood it. Micha,
please correct me if I'm talking bullsh*t.

So basically, gcc screams because there's a type incompatibility
according to the ICO C standard. IOW, valptr is declared as char[] and
we are casting it to "unsigned short *" and then dereffing it, and both
types are not compatible.

So, I'm looking at
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf, section 6.2.7
says "Two types have compatible type if their types are the same."

Then, section "6.7.2 Type specifiers" talks about the different
types and on the next page, in sentence 5 it says: "Each of the
comma-separated multisets designates the same type,... " And, no wonder
there, char and unsigned short are in different multisets so...

So, what gcc actually warns about is, something which has been declared
as char[] should not be subsequently accessed through "unsigned short *"
because the two types are incompatible.

No wonder we're building the kernel with -fno-strict-aliasing :).

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--