Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [patch 4/6] x86/fpu: Add guest support to xfd_enable_feature() | Date | Wed, 15 Dec 2021 05:46:53 +0000 |
| |
> 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
| |