Re: [PATCH v3 4/4] KVM: x86: emulate FXSAVE and FXRSTOR

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-11-09 00:25+0100, Paolo Bonzini:
> On 08/11/2016 20:54, Radim Krčmář wrote:
>> 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.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v3:
>>  - remove fxsave64 and extra colons at the end of asm to make old GCCs
>>    happy  (fxsave64 could have been implemented using other nmemonics,
>>    but there is no point when it won't be used + removing it makes the
>>    code nicer.)
>>  v2:
>>  - throws #GP to the guest when reserved MXCSR are set [Nadav]
>>  - returns internal emulation error if an exception is hit during
>>    execution
>>  - preserves XMM 8-15
>> ---
>>  arch/x86/kvm/emulate.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 112 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 6af3cac6ec89..1b3fab1fb8d3 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -3883,6 +3883,115 @@ 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);
>> +
>> +	/*
>> +	 * Don't emulate a case that should never be hit, instead of working
>> +	 * around a lack of fxsave64/fxrstor64 on old compilers.
>> +	 */
>> +	if (ctxt->mode >= X86EMUL_MODE_PROT64)
>> +		return X86EMUL_UNHANDLEABLE;
>> +
>> +	return X86EMUL_CONTINUE;
>> +}
>> +
>> +/*
>> + * FXSAVE and FXRSTOR have 4 different formats depending on execution mode,
>> + *  1) 16 bit mode
>> + *  2) 32 bit mode
>> + *     - like (1), but FIP and FDP (foo) are only 16 bit.  At least Intel CPUs
>> + *       preserve whole 32 bit values, though, so (1) and (2) are the same wrt.
>> + *       save and restore
>> + *  3) 64-bit mode with REX.W prefix
>> + *     - like (2), but XMM 8-15 are being saved and restored
>> + *  4) 64-bit mode without REX.W prefix
>> + *     - like (3), but FIP and FDP are 64 bit
>> + *
>> + * Emulation uses (3) for (1) and (2) and preserves XMM 8-15 to reach the
>> + * desired result.  (4) is not emulated.
>> + *
>> + * XXX: Guest and host CPUID.(EAX=07H,ECX=0H):EBX[bit 13] (deprecate FPU CS
>> + * and FPU DS) should match.
>> + */
>> +static int em_fxsave(struct x86_emulate_ctxt *ctxt)
>> +{
>> +	struct fxregs_state fx_state;
>> +	size_t size = 288; /* up to XMM7 */
> 
> Sorry for noticing this only now; if CR4.OSFXSR is 0, XMM and MXCSR
> should not be saved.

Intel processors don't save it, but the spec allows saving even when
CR4.OSFXSR is 0:

  If the OSFXSR bit in control register CR4 is not set, the FXSAVE
  instruction may not save this register (these registers).
  This behavior is implementation dependent.

I let "implementation dependent" behavior be the one with less code, but
haven't checked AMD spec, which doesn't seem to make it implementation
dependent ... I'll add it.  (On intel, OSFXSR gets written with 0 and
XMM 0-7 isn't modified without OSFXSR, so I'll just assume that AMD
won't break with that.)

> I can apply the first three patches already if you prefer.

Yes, they would not change, thanks.