Emulate MOVBE

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

Quoted 4 lines · Borislav Petkov

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...?

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 <[email protected]>

--

(quoted diff) +0 -0

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

(quoted diff) +1 -0

@@ -281,6 +281,7 @@ struct x86_emulate_ctxt {
 
 	/* decode cache */
 	u8 twobyte;
+	u8 thirdbyte;
 	u8 b;
 	u8 intercept;
 	u8 lock_prefix;

arch/x86/kvm/emulate.c +1 -0

index a335cc6cde72..0ccff339359d 100644
@@ -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

arch/x86/kvm/emulate.c +13 -0

@@ -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.

arch/x86/kvm/emulate.c +15 -0

+		 */
+		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;
+}
+

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

arch/x86/kvm/emulate.c +0 -0

 	/* 0x40 - 0x4F */
 	X16(D(DstReg | SrcMem | ModRM | Mov)),
 	/* 0x50 - 0x5F */

@@ -4323,6 +4353,15 @@ done_prefixes:
 	}
 	ctxt->d = opcode.flags;

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).

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.

Quoted 4 lines · Gleb Natapov

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.

Quoted 4 lines · Andre Przywara

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.

Quoted 4 lines

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.

Quoted 3 lines

(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.

Quoted 6 lines · On Wed, 10 Apr 2013 13:08:46 +0300 Gleb Natapov <[email protected]> wrote:

Quoted 4 lines · Andre Przywara

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 3 lines

(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 ;-)

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.

From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001

Quoted 2 lines · Gleb Natapov

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.

--

Quoted 1 line

From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001

From: Borislav Petkov <[email protected]>
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 <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kvm/cpuid.c   |  2 +-
 arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

arch/x86/kvm/cpuid.c +1 -1

index a20ecb5b6cbf..2d44fc4fd855 100644
@@ -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

arch/x86/kvm/emulate.c +37 -2

index a335cc6cde72..9011c7a656ad 100644
@@ -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;

Signature · 9 lines

1.8.2.135.g7b592fa


-- 
Regards/Gruss,
    Boris.

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

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Gleb Natapov

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.

Quoted 10 lines · Gleb Natapov

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Gleb Natapov

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().

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.

Quoted 2 lines · Gleb Natapov

Currently userspace assumes that that cpuid configuration returned by KVM_GET_SUPPORTED_CPUID is the optimal one. What we want here is a

way

Quoted 2 lines

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."?

Quoted 3 lines

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

Quoted 1 line

not optimal, but sometimes emulation is actually desirable (x2apic),

so

Quoted 1 line

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.

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.

Quoted 3 lines · 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.

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.

Quoted 4 lines · Gleb Natapov

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."?

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines · Gleb Natapov

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

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?

Quoted 2 lines

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.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

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.

Quoted 1 line

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.

Quoted 3 lines · Borislav Petkov

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.

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.

Quoted 14 lines

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.

Quoted 2 lines

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).

Quoted 3 lines · Gleb Natapov

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.

Quoted 14 lines

Quoted 14 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 2 lines

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

Agree, but not nastier than expecting ->function to have special value :)

Quoted 8 lines

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.

Quoted 10 lines · Borislav Petkov

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.

Quoted 2 lines

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

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

From looking at do_cpuid_ent(), kvm uses the 0x40000000 so we probably

Quoted 10 lines · Borislav Petkov

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.

Quoted 2 lines

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

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

Quoted 2 lines · Gleb Natapov

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.

--

Quoted 1 line

From 612fc75a732ad16332f270b7c52a68c89e3565ca Mon Sep 17 00:00:00 2001

From: Borislav Petkov <[email protected]>
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 <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kvm/cpuid.c   |  2 +-
 arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

arch/x86/kvm/cpuid.c +1 -1

index a20ecb5b6cbf..2d44fc4fd855 100644
@@ -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

arch/x86/kvm/emulate.c +0 -0

index a335cc6cde72..9011c7a656ad 100644

arch/x86/kvm/emulate.c +10 -0

+		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))

arch/x86/kvm/emulate.c +4 -2

 	}
 	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;

Signature · 9 lines

1.8.2.135.g7b592fa


-- 
Regards/Gruss,
    Boris.

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

Quoted 2 lines

+#define EmulateOnUD ((u64)1 << 46) /* emulate if unsupported by the host */

Just rename VendorSpecific to EmulateOnUD. The meaning is the same.

arch/x86/kvm/emulate.c static int em_mov(struct x86_emulate_ctxt *ctxt) · line 3115

Quoted 5 lines

arch/x86/kvm/emulate.c +5 -0

+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)?

arch/x86/kvm/emulate.c static int em_mov(struct x86_emulate_ctxt *ctxt) · line 3124

Quoted 8 lines

arch/x86/kvm/emulate.c +8 -0

+	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.

arch/x86/kvm/emulate.c static int em_mov(struct x86_emulate_ctxt *ctxt) · line 3127

Quoted 3 lines

arch/x86/kvm/emulate.c +3 -0

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

arch/x86/kvm/emulate.c static const struct opcode twobyte_table[256] = { · line 4065

Quoted 5 lines

arch/x86/kvm/emulate.c +5 -0

+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.

arch/x86/kvm/emulate.c done_prefixes: · line 4355

Quoted 3 lines

arch/x86/kvm/emulate.c +3 -0

+
+		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.

	*(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]

Il 14/04/2013 23:02, Borislav Petkov ha scritto:

Quoted 5 lines

	*(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.

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

Quoted 1 line

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.

Il 21/04/2013 14:23, Borislav Petkov ha scritto:

Quoted 19 lines · Borislav Petkov

Quoted 1 line

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;

Quoted 1 line

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?

Quoted 20 lines · Paolo Bonzini

Il 21/04/2013 14:23, Borislav Petkov ha scritto:

Quoted 19 lines · Borislav Petkov

Quoted 1 line

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;

Quoted 3 lines

Quoted 1 line

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.

Quoted 2 lines

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?

Quoted 7 lines

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;
        }
}

Quoted 19 lines · Gleb Natapov

Quoted 7 lines

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;

Quoted 8 lines

        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.

Quoted 20 lines · Paolo Bonzini

Il 21/04/2013 14:23, Borislav Petkov ha scritto:

Quoted 19 lines · Borislav Petkov

Quoted 1 line

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;

Quoted 3 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

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.

Atom is not defined by QEMU;

Quoted 1 line · Paolo Bonzini

Atom is not defined by QEMU;

$ qemu-system-x86_64 -cpu ?

...

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

Il 10/04/2013 12:08, Gleb Natapov ha scritto:

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 8 lines

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.

Il 16/04/2013 19:28, Gleb Natapov ha scritto:

Quoted 17 lines

Quoted 16 lines

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.

Quoted 3 lines

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.