linux-kernel - Re: [PATCH] x86,kvm: move qemu/guest FPU switching out to vcpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Nov 2017 20:40:22 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Rik van Riel <riel@...hat.com>, pbonzini@...hat.com
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, rkrcmar@...hat.com, borntraeger@...ibm.com
Subject: Re: [PATCH] x86,kvm: move qemu/guest FPU switching out to vcpu_run

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