[1/2] adds the emulation (and could be split into two patches if you'd like),
[2/2] just refactors the code.
This should fix an issue that users are hitting. Laszlo found several reports:
- https://bugs.launchpad.net/qemu/+bug/1623276
- https://bugzilla.proxmox.com/show_bug.cgi?id=1182
- https://bugs.archlinux.org/task/50778
I have only tested it with a simple kvm-unit-tests, though. Reproducing the
iPXE issue is on the way ...
Radim Krčmář (2):
KVM: x86: emulate fxsave and fxrstor
KVM: x86: save one bit in ctxt->d
arch/x86/kvm/emulate.c | 110 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 94 insertions(+), 16 deletions(-)
--
2.10.1
Internal errors were reported on 16 bit fxsave and fxrstor with iPXE.
Old Intels don't have unrestricted_guest, so we have to emulate them.
The patch takes advantage of the hardware implementation. There should
be no problem as long as the buffer is aligned.
Signed-off-by: Radim Krčmář <[email protected]>
---
I'm not happy with #ifdefs around the x86_64 only code -- ideas?
---
arch/x86/kvm/emulate.c | 93 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 84 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 4e95d3eb2955..f360876d6b7f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -171,6 +171,7 @@
#define NearBranch ((u64)1 << 52) /* Near branches */
#define No16 ((u64)1 << 53) /* No 16 bit operand */
#define IncSP ((u64)1 << 54) /* SP is incremented before ModRM calc */
+#define Aligned16 ((u64)1 << 55) /* Aligned to 16 byte boundary (e.g. FXSAVE) */
#define DstXacc (DstAccLo | SrcAccHi | SrcWrite)
@@ -632,21 +633,24 @@ static void set_segment_selector(struct x86_emulate_ctxt *ctxt, u16 selector,
* depending on whether they're AVX encoded or not.
*
* Also included is CMPXCHG16B which is not a vector instruction, yet it is
- * subject to the same check.
+ * subject to the same check. FXSAVE and FXRSTOR are checked here too as their
+ * 512 bytes of data must be aligned to a 16 byte boundary.
*/
-static bool insn_aligned(struct x86_emulate_ctxt *ctxt, unsigned size)
+static unsigned insn_alignment(struct x86_emulate_ctxt *ctxt, unsigned size)
{
if (likely(size < 16))
- return false;
+ return 1;
if (ctxt->d & Aligned)
- return true;
+ return size;
else if (ctxt->d & Unaligned)
- return false;
+ return 1;
else if (ctxt->d & Avx)
- return false;
+ return 1;
+ else if (ctxt->d & Aligned16)
+ return 16;
else
- return true;
+ return size;
}
static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
@@ -704,7 +708,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
}
break;
}
- if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))
+ if (la & (insn_alignment(ctxt, size) - 1))
return emulate_gp(ctxt, 0);
return X86EMUL_CONTINUE;
bad:
@@ -3856,6 +3860,75 @@ static int em_movsxd(struct x86_emulate_ctxt *ctxt)
return X86EMUL_CONTINUE;
}
+static int check_fxsr(struct x86_emulate_ctxt *ctxt)
+{
+ u32 eax = 1, ebx, ecx = 0, edx;
+
+ ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
+ if (!(edx & FFL(FXSR)))
+ return emulate_ud(ctxt);
+
+ if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
+ return emulate_nm(ctxt);
+
+ return X86EMUL_CONTINUE;
+}
+
+/*
+ * FXSAVE and FXRSTOR have 3 different formats depending on execution mode,
+ * 1) non-64-bit mode
+ * 2) 64-bit mode with REX.W prefix
+ * 3) 64-bit mode without REX.W prefix
+ *
+ * Emulation uses (3) for for (1) mode because only the number of XMM registers
+ * is different.
+ */
+static int em_fxsave(struct x86_emulate_ctxt *ctxt)
+{
+ char fx_state[512] __aligned(16);
+ int rc;
+
+ rc = check_fxsr(ctxt);
+ if (rc != X86EMUL_CONTINUE)
+ return rc;
+
+ ctxt->ops->get_fpu(ctxt);
+#ifdef CONFIG_X86_64
+ if (ctxt->rex_prefix & (1 << 3))
+ asm volatile("fxsave64 %0" : "+m"(fx_state));
+ else
+#endif
+ asm volatile("fxsave %0" : "+m"(fx_state));
+ ctxt->ops->put_fpu(ctxt);
+
+ return segmented_write(ctxt, ctxt->memop.addr.mem, fx_state, 512);
+}
+
+static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
+{
+ char fx_state[512] __aligned(16);
+ int rc;
+
+ rc = check_fxsr(ctxt);
+ if (rc != X86EMUL_CONTINUE)
+ return rc;
+
+ rc = segmented_read(ctxt, ctxt->memop.addr.mem, fx_state, 512);
+ if (rc != X86EMUL_CONTINUE)
+ return rc;
+
+ ctxt->ops->get_fpu(ctxt);
+#ifdef CONFIG_X86_64
+ if (ctxt->rex_prefix & (1 << 3))
+ asm volatile("fxrstor64 %0" : "+m"(fx_state));
+ else
+#endif
+ asm volatile("fxrstor %0" : "+m"(fx_state));
+ ctxt->ops->put_fpu(ctxt);
+
+ return X86EMUL_CONTINUE;
+}
+
static bool valid_cr(int nr)
{
switch (nr) {
@@ -4208,7 +4281,9 @@ static const struct gprefix pfx_0f_ae_7 = {
};
static const struct group_dual group15 = { {
- N, N, N, N, N, N, N, GP(0, &pfx_0f_ae_7),
+ I(ModRM | Aligned16, em_fxsave),
+ I(ModRM | Aligned16, em_fxrstor),
+ N, N, N, N, N, GP(0, &pfx_0f_ae_7),
}, {
N, N, N, N, N, N, N, N,
} };
--
2.10.1
Alignments are exclusive, so 5 modes can be expressed in 3 bits.
Signed-off-by: Radim Krčmář <[email protected]>
---
arch/x86/kvm/emulate.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f360876d6b7f..8fe2dbfb6338 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -158,9 +158,11 @@
#define Src2GS (OpGS << Src2Shift)
#define Src2Mask (OpMask << Src2Shift)
#define Mmx ((u64)1 << 40) /* MMX Vector instruction */
+#define AlignMask ((u64)7 << 41)
#define Aligned ((u64)1 << 41) /* Explicitly aligned (e.g. MOVDQA) */
-#define Unaligned ((u64)1 << 42) /* Explicitly unaligned (e.g. MOVDQU) */
-#define Avx ((u64)1 << 43) /* Advanced Vector Extensions */
+#define Unaligned ((u64)2 << 41) /* Explicitly unaligned (e.g. MOVDQU) */
+#define Avx ((u64)3 << 41) /* Advanced Vector Extensions */
+#define Aligned16 ((u64)4 << 41) /* Aligned to 16 byte boundary (e.g. FXSAVE) */
#define Fastop ((u64)1 << 44) /* Use opcode::u.fastop */
#define NoWrite ((u64)1 << 45) /* No writeback */
#define SrcWrite ((u64)1 << 46) /* Write back src operand */
@@ -171,7 +173,6 @@
#define NearBranch ((u64)1 << 52) /* Near branches */
#define No16 ((u64)1 << 53) /* No 16 bit operand */
#define IncSP ((u64)1 << 54) /* SP is incremented before ModRM calc */
-#define Aligned16 ((u64)1 << 55) /* Aligned to 16 byte boundary (e.g. FXSAVE) */
#define DstXacc (DstAccLo | SrcAccHi | SrcWrite)
@@ -638,19 +639,21 @@ static void set_segment_selector(struct x86_emulate_ctxt *ctxt, u16 selector,
*/
static unsigned insn_alignment(struct x86_emulate_ctxt *ctxt, unsigned size)
{
+ u64 alignment = ctxt->d & AlignMask;
+
if (likely(size < 16))
return 1;
- if (ctxt->d & Aligned)
- return size;
- else if (ctxt->d & Unaligned)
+ switch (alignment) {
+ case Unaligned:
+ case Avx:
return 1;
- else if (ctxt->d & Avx)
- return 1;
- else if (ctxt->d & Aligned16)
+ case Aligned16:
return 16;
- else
+ case Aligned:
+ default:
return size;
+ }
}
static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
--
2.10.1
On 10/26/16 22:50, Radim Krčmář wrote:
> [1/2] adds the emulation (and could be split into two patches if you'd like),
> [2/2] just refactors the code.
>
> This should fix an issue that users are hitting. Laszlo found several reports:
> - https://bugs.launchpad.net/qemu/+bug/1623276
> - https://bugzilla.proxmox.com/show_bug.cgi?id=1182
> - https://bugs.archlinux.org/task/50778
>
> I have only tested it with a simple kvm-unit-tests, though. Reproducing the
> iPXE issue is on the way ...
>
>
> Radim Krčmář (2):
> KVM: x86: emulate fxsave and fxrstor
> KVM: x86: save one bit in ctxt->d
>
> arch/x86/kvm/emulate.c | 110 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 94 insertions(+), 16 deletions(-)
>
I was just about to post iPXE patches that would disable the FXSAVE /
FXRSTOR instructions in the CONFIG=qemu build (*), but you beat me to it
with the KVM emulation code ;)
(*) If you look at the iPXE commit that added them, they are a
workaround for a Tivoli VMM bug; i.e., irrelevant for QEMU/KVM guests.
... Actually, those iPXE patches that conditionalize FXSAVE / FXRSTOR
may still make sense -- we can rebuild iPXE, and bundle the refreshed
binaries with QEMU v2.7.1, and swiftly at that. Whereas the KVM patches
could take more time to propagate to users?... Not sure. What do you
guys think?
Thanks
Laszlo
Hi Radim,
Radim Krčmář <[email protected]> writes:
...
> static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
> @@ -704,7 +708,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
> }
> break;
> }
> - if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))
> + if (la & (insn_alignment(ctxt, size) - 1))
> return emulate_gp(ctxt, 0);
> return X86EMUL_CONTINUE;
> bad:
> @@ -3856,6 +3860,75 @@ static int em_movsxd(struct x86_emulate_ctxt *ctxt)
> return X86EMUL_CONTINUE;
> }
>
> +static int check_fxsr(struct x86_emulate_ctxt *ctxt)
> +{
> + u32 eax = 1, ebx, ecx = 0, edx;
> +
> + ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> + if (!(edx & FFL(FXSR)))
> + return emulate_ud(ctxt);
> +
> + if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
> + return emulate_nm(ctxt);
> +
> + return X86EMUL_CONTINUE;
> +}
> +
> +/*
> + * FXSAVE and FXRSTOR have 3 different formats depending on execution mode,
> + * 1) non-64-bit mode
> + * 2) 64-bit mode with REX.W prefix
> + * 3) 64-bit mode without REX.W prefix
> + *
> + * Emulation uses (3) for for (1) mode because only the number of XMM registers
> + * is different.
> + */
> +static int em_fxsave(struct x86_emulate_ctxt *ctxt)
> +{
> + char fx_state[512] __aligned(16);
> + int rc;
> +
> + rc = check_fxsr(ctxt);
> + if (rc != X86EMUL_CONTINUE)
> + return rc;
> +
> + ctxt->ops->get_fpu(ctxt);
> +#ifdef CONFIG_X86_64
> + if (ctxt->rex_prefix & (1 << 3))
> + asm volatile("fxsave64 %0" : "+m"(fx_state));
> + else
> +#endif
> + asm volatile("fxsave %0" : "+m"(fx_state));
> + ctxt->ops->put_fpu(ctxt);
> +
> + return segmented_write(ctxt, ctxt->memop.addr.mem, fx_state, 512);
> +}
> +
> +static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
> +{
> + char fx_state[512] __aligned(16);
> + int rc;
> +
> + rc = check_fxsr(ctxt);
Is this check enough here ? What I mean is that is it possible that the memory
image that is read from has data in an invalid format/corrupt or is that irrelevant ?
Bandan
> + if (rc != X86EMUL_CONTINUE)
> + return rc;
> +
> + rc = segmented_read(ctxt, ctxt->memop.addr.mem, fx_state, 512);
> + if (rc != X86EMUL_CONTINUE)
> + return rc;
> +
> + ctxt->ops->get_fpu(ctxt);
> +#ifdef CONFIG_X86_64
> + if (ctxt->rex_prefix & (1 << 3))
> + asm volatile("fxrstor64 %0" : "+m"(fx_state));
> + else
> +#endif
> + asm volatile("fxrstor %0" : "+m"(fx_state));
> + ctxt->ops->put_fpu(ctxt);
> +
> + return X86EMUL_CONTINUE;
> +}
> +
> static bool valid_cr(int nr)
> {
> switch (nr) {
> @@ -4208,7 +4281,9 @@ static const struct gprefix pfx_0f_ae_7 = {
> };
>
> static const struct group_dual group15 = { {
> - N, N, N, N, N, N, N, GP(0, &pfx_0f_ae_7),
> + I(ModRM | Aligned16, em_fxsave),
> + I(ModRM | Aligned16, em_fxrstor),
> + N, N, N, N, N, GP(0, &pfx_0f_ae_7),
> }, {
> N, N, N, N, N, N, N, N,
> } };
2016-10-26 20:17-0400, Bandan Das:
> Radim Krčmář <[email protected]> writes:
> ...
>> +static int check_fxsr(struct x86_emulate_ctxt *ctxt)
>> +{
>> + u32 eax = 1, ebx, ecx = 0, edx;
>> +
>> + ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
>> + if (!(edx & FFL(FXSR)))
>> + return emulate_ud(ctxt);
>> +
>> + if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
>> + return emulate_nm(ctxt);
>> +
>> + return X86EMUL_CONTINUE;
>> +}
>> +
>> +/*
>> + * FXSAVE and FXRSTOR have 3 different formats depending on execution mode,
>> + * 1) non-64-bit mode
>> + * 2) 64-bit mode with REX.W prefix
>> + * 3) 64-bit mode without REX.W prefix
>> + *
>> + * Emulation uses (3) for for (1) mode because only the number of XMM registers
>> + * is different.
>> + */
| [...]
>> +
>> +static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
>> +{
>> + char fx_state[512] __aligned(16);
>> + int rc;
>> +
>> + rc = check_fxsr(ctxt);
>
> Is this check enough here ? What I mean is that is it possible that the memory
> image that is read from has data in an invalid format/corrupt or is that irrelevant ?
No, it is not enough, v2 will need testing on bare metal. :)
Nadav mentioned that MXCSR could thrown #GP when setting bits 16-32.
And there are actually 4 different formats: 16 bit mode has only 16 bit
FIP, and other 16 bits are reserved, but KVM's fxrstor would be loading
all 32 bits, so the reserved upper 16 should be cleared beforehand.
The structure has a lot of reserved fields, but they should just be
ignored by the CPU.
Did you notice other problems?
Thanks.
2016-10-26 23:40+0200, Laszlo Ersek:
> On 10/26/16 22:50, Radim Krčmář wrote:
>> [1/2] adds the emulation (and could be split into two patches if you'd like),
>> [2/2] just refactors the code.
>>
>> This should fix an issue that users are hitting. Laszlo found several reports:
>> - https://bugs.launchpad.net/qemu/+bug/1623276
>> - https://bugzilla.proxmox.com/show_bug.cgi?id=1182
>> - https://bugs.archlinux.org/task/50778
>>
>> I have only tested it with a simple kvm-unit-tests, though. Reproducing the
>> iPXE issue is on the way ...
>>
>>
>> Radim Krčmář (2):
>> KVM: x86: emulate fxsave and fxrstor
>> KVM: x86: save one bit in ctxt->d
>>
>> arch/x86/kvm/emulate.c | 110 ++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 94 insertions(+), 16 deletions(-)
>>
>
> I was just about to post iPXE patches that would disable the FXSAVE /
> FXRSTOR instructions in the CONFIG=qemu build (*), but you beat me to it
> with the KVM emulation code ;)
>
> (*) If you look at the iPXE commit that added them, they are a
> workaround for a Tivoli VMM bug; i.e., irrelevant for QEMU/KVM guests.
>
> ... Actually, those iPXE patches that conditionalize FXSAVE / FXRSTOR
> may still make sense -- we can rebuild iPXE, and bundle the refreshed
> binaries with QEMU v2.7.1, and swiftly at that. Whereas the KVM patches
> could take more time to propagate to users?... Not sure. What do you
> guys think?
This series won't get into 4.9, so it would take almost half a year
before the kernel trickles into experimental distros. And updating
QEMU/iPXE isn't as dangerous as updating kernel, so I like the idea.
I am just tempted to drop a KVM patch with positive diffstat that fixes
something that doesn't really need fixing anymore. :)
On 10/27/16 18:41, Radim Krčmář wrote:
> 2016-10-26 23:40+0200, Laszlo Ersek:
>> On 10/26/16 22:50, Radim Krčmář wrote:
>>> [1/2] adds the emulation (and could be split into two patches if you'd like),
>>> [2/2] just refactors the code.
>>>
>>> This should fix an issue that users are hitting. Laszlo found several reports:
>>> - https://bugs.launchpad.net/qemu/+bug/1623276
>>> - https://bugzilla.proxmox.com/show_bug.cgi?id=1182
>>> - https://bugs.archlinux.org/task/50778
>>>
>>> I have only tested it with a simple kvm-unit-tests, though. Reproducing the
>>> iPXE issue is on the way ...
>>>
>>>
>>> Radim Krčmář (2):
>>> KVM: x86: emulate fxsave and fxrstor
>>> KVM: x86: save one bit in ctxt->d
>>>
>>> arch/x86/kvm/emulate.c | 110 ++++++++++++++++++++++++++++++++++++++++++-------
>>> 1 file changed, 94 insertions(+), 16 deletions(-)
>>>
>>
>> I was just about to post iPXE patches that would disable the FXSAVE /
>> FXRSTOR instructions in the CONFIG=qemu build (*), but you beat me to it
>> with the KVM emulation code ;)
>>
>> (*) If you look at the iPXE commit that added them, they are a
>> workaround for a Tivoli VMM bug; i.e., irrelevant for QEMU/KVM guests.
>>
>> ... Actually, those iPXE patches that conditionalize FXSAVE / FXRSTOR
>> may still make sense -- we can rebuild iPXE, and bundle the refreshed
>> binaries with QEMU v2.7.1, and swiftly at that. Whereas the KVM patches
>> could take more time to propagate to users?... Not sure. What do you
>> guys think?
>
> This series won't get into 4.9, so it would take almost half a year
> before the kernel trickles into experimental distros. And updating
> QEMU/iPXE isn't as dangerous as updating kernel, so I like the idea.
>
> I am just tempted to drop a KVM patch with positive diffstat that fixes
> something that doesn't really need fixing anymore. :)
Personally I can't argue either way; I'll just state that the iPXE
patches aren't a done deal yet, either... We're still waiting for
maintainer feedback.
Thank you, Radim!
Laszlo
Radim Krčmář <[email protected]> writes:
> 2016-10-26 20:17-0400, Bandan Das:
>> Radim Krčmář <[email protected]> writes:
>> ...
>>> +static int check_fxsr(struct x86_emulate_ctxt *ctxt)
>>> +{
>>> + u32 eax = 1, ebx, ecx = 0, edx;
>>> +
>>> + ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
>>> + if (!(edx & FFL(FXSR)))
>>> + return emulate_ud(ctxt);
>>> +
>>> + if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
>>> + return emulate_nm(ctxt);
>>> +
>>> + return X86EMUL_CONTINUE;
>>> +}
>>> +
>>> +/*
>>> + * FXSAVE and FXRSTOR have 3 different formats depending on execution mode,
>>> + * 1) non-64-bit mode
>>> + * 2) 64-bit mode with REX.W prefix
>>> + * 3) 64-bit mode without REX.W prefix
>>> + *
>>> + * Emulation uses (3) for for (1) mode because only the number of XMM registers
>>> + * is different.
>>> + */
> | [...]
>>> +
>>> +static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
>>> +{
>>> + char fx_state[512] __aligned(16);
>>> + int rc;
>>> +
>>> + rc = check_fxsr(ctxt);
>>
>> Is this check enough here ? What I mean is that is it possible that the memory
>> image that is read from has data in an invalid format/corrupt or is that irrelevant ?
>
> No, it is not enough, v2 will need testing on bare metal. :)
>
> Nadav mentioned that MXCSR could thrown #GP when setting bits 16-32.
Yeah, this was what I am wondering about. Whether, there are obvious ways
for the guest to abuse the path, you know, common emulator code concerns.
> And there are actually 4 different formats: 16 bit mode has only 16 bit
> FIP, and other 16 bits are reserved, but KVM's fxrstor would be loading
> all 32 bits, so the reserved upper 16 should be cleared beforehand.
> The structure has a lot of reserved fields, but they should just be
> ignored by the CPU.
Ok.
> Did you notice other problems?
Rest looks fine to me even with the #ifdefs that you don't prefer but
I will be sure to take another look when you post a v2.
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html