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
