Re: [PATCH] x86,kvm: move qemu/guest FPU switching out to vcpu_run

[David Hildenbrand <david@redhat.com>]

On 14.11.2017 19:07, Rik van Riel wrote:
> On Tue, 2017-11-14 at 17:57 +0100, David Hildenbrand wrote:
>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h
>>> b/arch/x86/include/asm/kvm_host.h
>>> index c73e493adf07..92e66685249e 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>
>> We should also get rid of guest_fpu_loaded now, right?
> 
> Indeed, we no longer need that member. I'll get rid of it.
> 
>> emulator_get_fpu() does a kvm_load_guest_fpu(). Doesn't that mean
>> that
>> this is now not needed anymore? (at least when emulator code is
>> called
>> from inside the loop?)
> 
> Now that is a very good question!
> 
> When called from inside the loop, it is indeed not
> needed.
> 
> My question is, can the in-kernel emulator code ever
> be called from OUTSIDE the KVM_RUN ioctl loop?
> 
> If so, we need to restore the user FPU context before
> returning from the emulator code. Given that the current
> emulator code does not do that, I suspect this is not
> the case. I also see no path from the kvm ioctl into
> the emulator code, other than via KVM_RUN.
> 
> The FPU and XSAVE ioctls all work on the saved
> vcpu->arch.guest_fpu data, and never directly on the
> registers.
> 
> Looks like we can completely get rid of .get_fpu and
> .put_fpu...
> 
> Unless Paolo has any objection, I'll go do that :)


I think we should check all get/put_fpu callers if they need
preempt_disable().

E.g. em_fxrstor() needs disabled preemption as we temporarily
save + restore some host register (via fxsave + fxrstor) under some
circumstances that are not saved/restored when switching to/back from
another process. We should double check.

@Paolo what about complete_userspace_io? It can end up calling
emulate_instruction(). So maybe we have to move load/put fpu further out
or add special handling.

-- 

Thanks,

David / dhildenb