> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Tuesday, December 14, 2021 9:16 PM
>
> On Tue, Dec 14 2021 at 11:21, Paolo Bonzini wrote:
> > On 12/14/21 07:05, Tian, Kevin wrote:
> >>> + if (guest_fpu) {
> >>> + newfps->is_guest = true;
> >>> + newfps->is_confidential = curfps->is_confidential;
> >>> + newfps->in_use = curfps->in_use;
> >>> + guest_fpu->xfeatures |= xfeatures;
> >>> + }
> >>> +
> >> As you explained guest fpstate is not current active in the restoring
> >> path, thus it's not correct to always inherit attributes from the
> >> active one.
>
> Good catch!
>
> > Indeed, guest_fpu->fpstate should be used instead of curfps.
>
> Something like the below.
Looks good. Just two nits.
> + /*
> + * When a guest FPU is supplied, use @guest_fpu->fpstate
> + * as reference independent whether it is in use or not.
> + */
> + curfps = guest_fpu ? guest_fpu->fpstate : fpu->fpstate;
> +
> + /* Determine whether @curfps is the active fpstate */
> + in_use = fpu->fpstate == curfps;
> +
> + if (guest_fpu) {
> + newfps->is_guest = true;
> + newfps->is_confidential = curfps->is_confidential;
> + newfps->in_use = curfps->in_use;
What is the purpose of this 'in_use' field? Currently it's only
touched in three places:
- set when entering guest;
- cleared when exiting to userspace;
- checked when freeing a guest FPU;
The last one can be easily checked by comparing to current fps.
Any other intended usage which is currently missed for guest FPU?
>
> + if (guest_fpu) {
> + curfps = xchg(&guest_fpu->fpstate, newfps);
This can be a direct value update to guest_fpu->fpstate since
curfps has already been acquired in the start.
> + /* If curfps is active, update the FPU fpstate pointer */
> + if (in_use)
> + fpu->fpstate = newfps;
> + } else {
> + curfps = xchg(&fpu->fpstate, newfps);
ditto.
Thanks
Kevin
