Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH 02/19] x86/fpu: Prepare KVM for dynamically enabled states | Date | Mon, 13 Dec 2021 20:50:17 +0100 |
| |
Paolo,
On Mon, Dec 13 2021 at 13:45, Paolo Bonzini wrote: > On 12/13/21 13:00, Thomas Gleixner wrote: >> On Mon, Dec 13 2021 at 10:12, Paolo Bonzini wrote: >>> Please rename to alloc_xfeatures >> >> That name makes no sense at all. This has nothing to do with alloc. > > Isn't that the features for which space is currently allocated?
It is, but from the kernel POV this is user. :)
> Reading "user_xfeatures" in there is cryptic, it seems like it's > something related to the userspace thread or group that has invoked the > KVM ioctl. If it's renamed to alloc_xfeatures, then this: > > + missing = request & ~guest_fpu->alloc_xfeatures; > + if (missing) { > + vcpu->arch.guest_fpu.realloc_request |= missing; > + return true; > + } > > makes it obvious that the allocation is for features that are requested > but haven't been allocated in the xstate yet.
Let's rename it to xfeatures and perm and be done with it.
>> Why? Yet another export of FPU internals just because? > > It's one function more and one field less. I prefer another export of > FPU internals, to a write to a random field with undocumented > invariants.
We want less not more exports. :)
> For example, why WARN_ON_ONCE if enter_guest == true? If you enter the > guest after the host has restored MSR_IA32_XFD with KVM_SET_MSR, the
Indeed restoring a guest might require buffer reallocation, I missed that, duh!
On restore the following components are involved:
XCR0, XFD, XSTATE
XCR0 and XFD have to be restored _before_ XSTATE and that needs to be enforced.
But independent of the ordering of XCR0 and XFD restore the following check applies to both the restore and the runtime logic:
int kvm_fpu_realloc(struct kvm_vcpu *vcpu, u64 xcr0, u64 xfd) { u64 expand, enabled = xcr0 & ~xfd;
expand = enabled & ~vcpu->arch.guest_fpu.xfeatures; if (!expand) return 0; return fpu_enable_guest_features(&vcpu->arch.guest_fpu, expand); }
int fpu_enable_guest_features(struct guest_fpu *gfpu, u64 which) { permission_checks(); ... return fpstate_realloc(.....) }
fpstate_realloc() needs to be careful about flipping the pointers depending on the question whether guest_fpu->fpstate is actually active, i.e.:
current->thread.fpu.fpstate == gfpu->fpstate
I'm halfways done with that. Will send something soonish.
Thanks,
tglx
| |