diff mbox series

[09/19] kvm: x86: Prepare reallocation check

Message ID 20211208000359.2853257-10-yang.zhong@intel.com (mailing list archive)
State New, archived
Headers show
Series AMX Support in KVM | expand

Commit Message

Yang Zhong Dec. 8, 2021, 12:03 a.m. UTC 
From: Jing Liu <jing2.liu@intel.com>

On native fpstate reallocation is triggered by #NM because IA32_XFD
is initialized to 1 for all native tasks.

However #NM in guest is not trapped by KVM. Instead, guest enabling
of a dynamic extended feature can be captured via emulation of
IA32_XFD and XSETBV. Basically having guest XCR0[i]=1 and XFD[i]=0
indicates that the feature[i] is activated by the guest.

This patch provides a helper function for such check, invoked when
either XCR0 or XFD is changed in the emulation path.

Signed-off-by: Jing Liu <jing2.liu@intel.com>
Signed-off-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 arch/x86/kvm/x86.c | 24 ++++++++++++++++++++++++
 arch/x86/kvm/x86.h |  1 +
 2 files changed, 25 insertions(+)

Comments

Paolo Bonzini Dec. 13, 2021, 9:16 a.m. UTC | #1 
On 12/8/21 01:03, Yang Zhong wrote:
> +	u64 xcr0 = vcpu->arch.xcr0 & XFEATURE_MASK_USER_DYNAMIC;
> +
> +	/* For any state which is enabled dynamically */
> +	if ((xfd & xcr0) != xcr0) {
> +		u64 request = (xcr0 ^ xfd) & xcr0;
> +		struct fpu_guest *guest_fpu = &vcpu->arch.guest_fpu;
> +
> +		/*
> +		 * If requested features haven't been enabled, update
> +		 * the request bitmap and tell the caller to request
> +		 * dynamic buffer reallocation.
> +		 */
> +		if ((guest_fpu->user_xfeatures & request) != request) {
> +			vcpu->arch.guest_fpu.realloc_request = request;

This should be "|=".  If you have

	wrmsr(XFD, dynamic-feature-1);
	...
	wrmsr(XFD, dynamic-feature-2);

then the space for both features has to be allocated.

> +			return true;
> +		}
> +	}
> +


This is just:

	struct fpu_guest *guest_fpu = &vcpu->arch.guest_fpu;
	u64 xcr0 = vcpu->arch.xcr0 & XFEATURE_MASK_USER_DYNAMIC;
	u64 dynamic_enabled = xcr0 & ~xfd;
	if (!(dynamic_enabled & ~guest_fpu->user_xfeatures))
		return false;

	/*
	 * This should actually not be in guest_fpu, see review of
	 * patch 2.  Also see above regarding "=" vs "|=".
	 */
	vcpu->arch.guest_fpu.realloc_request |= dynamic_enabled;
	return true;

But without documentation I'm not sure why this exit-to-userspace step 
is needed:

- if (dynamic_enabled & ~guest_fpu->user_perm) != 0, then this is a 
userspace error and you can #GP the guest without any issue.  Userspace 
is buggy

- if (dynamic_enabled & ~guest_fpu->user_xfeatures) != 0, but the 
feature *is* permitted, then it is okay to just go on and reallocate the 
guest FPU.


Paolo
Tian, Kevin Dec. 14, 2021, 7:06 a.m. UTC | #2 
> From: Paolo Bonzini
> Sent: Monday, December 13, 2021 5:16 PM
> 
> 
> - if (dynamic_enabled & ~guest_fpu->user_perm) != 0, then this is a
> userspace error and you can #GP the guest without any issue.  Userspace
> is buggy
> 

Is it a general guideline that an error caused by emulation itself (e.g.
due to no memory) can be reflected into the guest as #GP, even
when from guest p.o.v there is nothing wrong with its setting?

Thanks
Kevin
Paolo Bonzini Dec. 14, 2021, 10:16 a.m. UTC | #3 
On 12/14/21 08:06, Tian, Kevin wrote:
>> - if (dynamic_enabled & ~guest_fpu->user_perm) != 0, then this is a
>> userspace error and you can #GP the guest without any issue.  Userspace
>> is buggy
>
> Is it a general guideline that an error caused by emulation itself (e.g.
> due to no memory) can be reflected into the guest as #GP, even
> when from guest p.o.v there is nothing wrong with its setting?

No memory is a tricky one, if possible it should propagate -ENOMEM up to 
KVM_RUN or KVM_SET_MSR.  But it's basically an impossible case anyway, 
because even with 8K TILEDATA we're within the limit of 
PAGE_ALLOC_COSTLY_ORDER.

So, since it's not easy to do it right now, we can look at it later.

Paolo
Jing Liu Dec. 14, 2021, 2:41 p.m. UTC | #4 
On 12/14/2021 6:16 PM, Paolo Bonzini wrote:
> 
> On 12/14/21 08:06, Tian, Kevin wrote:
> >> - if (dynamic_enabled & ~guest_fpu->user_perm) != 0, then this is a
> >> userspace error and you can #GP the guest without any issue.
> >> Userspace is buggy
> >
> > Is it a general guideline that an error caused by emulation itself (e.g.
> > due to no memory) can be reflected into the guest as #GP, even when
> > from guest p.o.v there is nothing wrong with its setting?
> 
> No memory is a tricky one, if possible it should propagate -ENOMEM up to
> KVM_RUN or KVM_SET_MSR.  But it's basically an impossible case anyway,
> because even with 8K TILEDATA we're within the limit of
> PAGE_ALLOC_COSTLY_ORDER.
> 
> So, since it's not easy to do it right now, we can look at it later.

For the way handling xcr0 and xfd ioctl failure, xcr0 and xfd have 
different handlings. Current KVM_SET_XCRS returns -EINVAL to 
userspace. KVM_SET_MSR is always allowed as the discussion in 
another thread.

So I'm thinking if reallocation failure in KVM_SET_XCRS and 
KVM_SET_MSR (may due to NOMEM or EPERM or ENOTSUPP), 
what is the way we would like to choose?

Thanks,
Jing
 
> Paolo
Tian, Kevin Dec. 15, 2021, 7:09 a.m. UTC | #5 
> From: Liu, Jing2 <jing2.liu@intel.com>
> Sent: Tuesday, December 14, 2021 10:42 PM
> 
> On 12/14/2021 6:16 PM, Paolo Bonzini wrote:
> >
> > On 12/14/21 08:06, Tian, Kevin wrote:
> > >> - if (dynamic_enabled & ~guest_fpu->user_perm) != 0, then this is a
> > >> userspace error and you can #GP the guest without any issue.
> > >> Userspace is buggy
> > >
> > > Is it a general guideline that an error caused by emulation itself (e.g.
> > > due to no memory) can be reflected into the guest as #GP, even when
> > > from guest p.o.v there is nothing wrong with its setting?
> >
> > No memory is a tricky one, if possible it should propagate -ENOMEM up to
> > KVM_RUN or KVM_SET_MSR.  But it's basically an impossible case anyway,
> > because even with 8K TILEDATA we're within the limit of
> > PAGE_ALLOC_COSTLY_ORDER.
> >
> > So, since it's not easy to do it right now, we can look at it later.
> 
> For the way handling xcr0 and xfd ioctl failure, xcr0 and xfd have
> different handlings. Current KVM_SET_XCRS returns -EINVAL to
> userspace. KVM_SET_MSR is always allowed as the discussion in
> another thread.
> 
> So I'm thinking if reallocation failure in KVM_SET_XCRS and
> KVM_SET_MSR (may due to NOMEM or EPERM or ENOTSUPP),
> what is the way we would like to choose?
> 

KVM_SET_MSRS can definitely accept failure according to msr_io().
I think Paolo's point is more about that the restore path should not
inherit any check related to vCPU capability. It's a different matter
if the error is caused by other host kernel errors.

Given that we don't need any special handling between the two
scenarios (set by guest vs. set by host) in those emulation paths. 
Just return '1' to indicate error and whatever error policy exists 
in those scenarios is just applied.

Thanks
Kevin
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 05f2cda73d69..91cc6f69a7ca 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -956,6 +956,30 @@  void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_load_host_xsave_state);
 
+bool kvm_check_guest_realloc_fpstate(struct kvm_vcpu *vcpu, u64 xfd)
+{
+	u64 xcr0 = vcpu->arch.xcr0 & XFEATURE_MASK_USER_DYNAMIC;
+
+	/* For any state which is enabled dynamically */
+	if ((xfd & xcr0) != xcr0) {
+		u64 request = (xcr0 ^ xfd) & xcr0;
+		struct fpu_guest *guest_fpu = &vcpu->arch.guest_fpu;
+
+		/*
+		 * If requested features haven't been enabled, update
+		 * the request bitmap and tell the caller to request
+		 * dynamic buffer reallocation.
+		 */
+		if ((guest_fpu->user_xfeatures & request) != request) {
+			vcpu->arch.guest_fpu.realloc_request = request;
+			return true;
+		}
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(kvm_check_guest_realloc_fpstate);
+
 static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 {
 	u64 xcr0 = xcr;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 4abcd8d9836d..24a323980146 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -445,6 +445,7 @@  static inline void kvm_machine_check(void)
 
 void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
 void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
+bool kvm_check_guest_realloc_fpstate(struct kvm_vcpu *vcpu, u64 new_xfd);
 int kvm_spec_ctrl_test_value(u64 value);
 bool kvm_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
 int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,