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

From: "Liu, Jing2" <jing2.liu@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>
Cc: "x86@kernel.org" <x86@kernel.org>,
	"Bae, Chang Seok" <chang.seok.bae@intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"Arjan van de Ven" <arjan@linux.intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Nakajima, Jun" <jun.nakajima@intel.com>,
	Jing Liu <jing2.liu@linux.intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"Cooper, Andrew" <andrew.cooper3@citrix.com>
Subject: RE: [patch 13/31] x86/fpu: Move KVMs FPU swapping to FPU core
Date: Thu, 14 Oct 2021 08:02:23 +0000	[thread overview]
Message-ID: <BL0PR11MB3252511FC48E43484DE79A3CA9B89@BL0PR11MB3252.namprd11.prod.outlook.com> (raw)
In-Reply-To: <ec9c761d-4b5c-71e2-c1fc-d256b6b78c04@redhat.com>

On 10/14/2021 2:50 PM, Paolo Bonzini wrote:
> On 13/10/21 16:06, Thomas Gleixner wrote:
> >> - the guest value stored in vcpu->arch.
> >>
> >> - the "QEMU" value attached to host_fpu.  This one only becomes zero
> >> if QEMU requires AMX (which shouldn't happen).
> >
> > I don't think that makes sense.
> >
> > First of all, if QEMU wants to expose AMX to guests, then it has to
> > ask for permission to do so as any other user space process. We're not
> > going to make that special just because.
> 
> Hmm, I would have preferred if there was no need to enable AMX for the
> QEMU FPU.  But you're saying that guest_fpu needs to swap out to
> current->thread.fpu if the guest is preempted, which would require
> XFD=0; and affect QEMU operation as well.

For preemption, if guest_fpu XFD is used as KVM internal value, then
we can simply set current->thread.fpu XFD the same as KVM internal
value in vmexit so kernel preemption can refer to it.

Thus, I think this issue doesn't much effect if enabling AMX for Qemu
FPU or not.

> 
> In principle I don't like it very much; it would be nicer to say "you
> enable it for QEMU itself via arch_prctl(ARCH_SET_STATE_ENABLE), and for
> the guests via ioctl(KVM_SET_CPUID2)".  But I can see why you want to
> keep things simple, so it's not a strong objection at all.
> 

Does this mean that KVM allocate 3 buffers via 
1) Qemu's request, instead of via 2) guest XCR0 trap? 

For the two ways, I think what we need care is the same: a) allocation time;
b) lazy passthrough time which related to XFD handling and values. Because
we don't want always rdmsr and clear XFD in vmexit, and don't want to
trap different XFD switching in guest.

For 1), Qemu need prctl() and ioctl(ENABLE_DYN_FEATURE).
But *when* does Qemu do ioctl(ENABLE_DYN_FEATURE)? I mean if
guest XCR0 doesn't set bit18, then KVM doesn't need alloc 3 buffers
at all.

Thus, XCR0 trap is a simple way?

Meanwhile, for lazy passthrough, do we want to make it when guest
wrmsr trap (i.e. guest changes XFD, not inits XFD) if using 1) qemu's
request?  Or using 2) via XCR0 trap, directly passthrough when XCR0
trap?

> > Anything else will just create more problems than it solves. Especially
> > #NM handling (think nested guest) and the XFD_ERR additive behaviour
> > will be a nasty playground and easy to get wrong.
> >
> > Not having that at all makes life way simpler, right?
> 
> It is simpler indeed, and it makes sense to start simple.  
I'd like to confirm which is the simpler way we'd like to :)

Thanks,
Jing

I am not sure
> if it will hold, but I agree it's better for the first implementation.
> 
> Paolo