RE: [patch 4/6] x86/fpu: Add guest support to xfd_enable_feature()

Tian, Kevin
2021-12-15T05:46:59Z

> From: Thomas Gleixner <tglx@linutronix.de>
> 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