linux-kernel - RE: [patch 13/31] x86/fpu: Move KVMs FPU swapping to FPU core

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]

Message-ID: <BYAPR11MB3256B3120DEB5FE0DD53B5B9A9B99@BYAPR11MB3256.namprd11.prod.outlook.com>
Date:   Fri, 15 Oct 2021 14:24:43 +0000
From:   "Liu, Jing2" <jing2.liu@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Paolo Bonzini <pbonzini@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
CC:     "x86@...nel.org" <x86@...nel.org>,
        "Bae, Chang Seok" <chang.seok.bae@...el.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Arjan van de Ven" <arjan@...ux.intel.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Nakajima, Jun" <jun.nakajima@...el.com>,
        Jing Liu <jing2.liu@...ux.intel.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "Cooper, Andrew" <andrew.cooper3@...rix.com>
Subject: RE: [patch 13/31] x86/fpu: Move KVMs FPU swapping to FPU core

Hi Thomas,

On 10/15/2021 5:36 PM, Thomas Gleixner wrote:
> Paolo,
> 
> On Thu, Oct 14 2021 at 21:14, Thomas Gleixner wrote:
> > On Thu, Oct 14 2021 at 17:01, Paolo Bonzini wrote:
> >>> vcpu_create()
> >>>
> >>>    fpu_init_fpstate_user(guest_fpu, supported_xcr0)
> >>>
> >>> That will (it does not today) do:
> >>>
> >>>       guest_fpu::__state_perm = supported_xcr0 &
> >>> xstate_get_group_perm();
> >>>
> >>> The you have the information you need right in the guest FPU.
> >>
> >> Good, I wasn't aware of the APIs that will be there.
> >
> > Me neither, but that's a pretty obvious consequence of the work I'm
> > doing for AMX. So I made it up for you. :)
> 
> let me make some more up for you!
> 
> If you carefully look at part 2 of the rework, then you might notice that there
> is a fundamental change which allows to do a real simplification for KVM FPU
> handling:
> 
>    current->thread.fpu.fpstate
> 
> is now a pointer. So you can spare one FPU allocation because we can now
> do:

Trying to understand your point, seems struct fpu will add a guest_fpstate
pointer. And this will be allocated when vcpu_create() by the following
function. Swap between the two pointers in load/put. What I was thinking 
is that vcpu keeps having guest_fpu and delete user_fpu. 

> 
> fpu_attach_guest_fpu(supported_xcr0)
> {
>         guest_fpstate = alloc_fpstate(supported_xcr0);

I supposed this is called when vcpu_create(). Not sure the reason 
of supported_xcr0 input here. supported_xcr0[n]=1 and
guest _state_perm[n]=1 which is requested before vcpu_create(),
so this will allocate full buffer, at vcpu_create() stage? 
Or do you mean vcpu->arch.guest_supported_xcr0.

Please correct me if I misunderstood. Thanks.

>         fpu_init_fpstate_user(guest_fpstate, supported_xcr0);
>         current->thread.fpu.guest_fpstate = guest_fpstate; }
> 


> fpu_swap_kvm_fpu() becomes in the first step:
> 
> fpu_swap_kvm_fpu(bool enter_guest)
> {
>         safe_fpregs_to_fpstate(current->thread.fpu.fpstate);
> 
>         swap(current->thread.fpu.fpstate, current->thread.fpu.guest_fpstate);
> 
>         restore_fpregs_from_fpstate(current->thread.fpu.fpstate);
> }
> 
> @enter guest will allow to do some sanity checks
> 
> In a second step:
> 
> fpu_swap_kvm_fpu(bool enter_guest, u64 guest_needs_features) {
>         possibly_reallocate(enter_guest, guest_needs_features);

When KVM traps guest wrmsr XFD in #NM, I think KVM need allocate
fpstate buffer for full features.
Because in next vmexit, guest might have dynamic state and KVM
can be preempted before running fpu_swap_kvm_fpu().
Thus, here the current->thread.fpu.fpstate already has enough space
for saving guest.

Thanks,
Jing

>         safe_fpregs_to_fpstate(current->thread.fpu.fpstate);
> 
>         swap(current->thread.fpu.fpstate, current->thread.fpu.guest_fpstate);
> 
>         restore_fpregs_from_fpstate(current->thread.fpu.fpstate);
>         possibly_reallocate(enter_guest, guest_needs_features); }
> 
> @guest_needs_features is the information which you gather via guest XCR0
> and guest XFD.
> 
> So fpu_swap_kvm_fpu() is going to be the place where reallocation happens
> and that's good enough for both cases:
> 
> vcpu_run()
> 
>      fpu_swap_kvm_fpu(); <- 1
> 
>      while (...)
>            vmenter();
> 
>      fpu_swap_kvm_fpu(); <- 2
> 
> #1 QEMU user space used feature and has already large fpstate
> 
> #2 Guest requires feature but has not used it yet (XCR0/XFD trapping)
> 
> See?
> 
> It's not only correct, it's also simple and truly beautiful.
> 
> Thanks,
> 
>         tglx