From:   Thomas Gleixner <tglx@linutronix.de>
To:     Paolo Bonzini <pbonzini@redhat.com>,
        Yang Zhong <yang.zhong@intel.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com
Cc:     seanjc@google.com, jun.nakajima@intel.com, kevin.tian@intel.com,
        jing2.liu@linux.intel.com, jing2.liu@intel.com
Subject: Re: [PATCH 02/19] x86/fpu: Prepare KVM for dynamically enabled states
In-Reply-To: <16c938e2-2427-c8dd-94a1-eba8f967283b@redhat.com>
Date:   Mon, 13 Dec 2021 20:50:17 +0100
Message-ID: <87v8zsthc6.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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