diff mbox series

[v3,07/21] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS

Message ID 20230511040857.6094-8-weijiang.yang@intel.com
State New
Headers
Series Enable CET Virtualization |

Commit Message

Yang, Weijiang May 11, 2023, 4:08 a.m. UTC 
  Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified.
CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all
features enabled via XCR0 | XSS so that guest can allocate correct xsave
buffer.

Note, KVM does not yet support any XSS based features, i.e. supported_xss
is guaranteed to be zero at this time.

Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/cpuid.c | 7 +++++--
 arch/x86/kvm/x86.c   | 6 ++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Chao Gao May 25, 2023, 6:10 a.m. UTC | #1 
On Thu, May 11, 2023 at 12:08:43AM -0400, Yang Weijiang wrote:
>Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified.
>CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all
>features enabled via XCR0 | XSS so that guest can allocate correct xsave
>buffer.
>
>Note, KVM does not yet support any XSS based features, i.e. supported_xss
>is guaranteed to be zero at this time.
>
>Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/cpuid.c | 7 +++++--
> arch/x86/kvm/x86.c   | 6 ++++--
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>index 123bf8b97a4b..cbb1b8a65502 100644
>--- a/arch/x86/kvm/cpuid.c
>+++ b/arch/x86/kvm/cpuid.c
>@@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
> 
> 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
> 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
>-		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>-		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>+		cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {

Align indentation.

 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
		     cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {

>+		u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
>+
>+		best->ebx = xstate_required_size(xstate, true);
>+	}
> 
> 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 33a780fe820b..ab3360a10933 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> 		 */
> 		if (data & ~kvm_caps.supported_xss)

Shouldn't we check against the supported value of _this_ guest? similar to
guest_supported_xcr0.

> 			return 1;
>-		vcpu->arch.ia32_xss = data;
>-		kvm_update_cpuid_runtime(vcpu);
>+		if (vcpu->arch.ia32_xss != data) {
>+			vcpu->arch.ia32_xss = data;
>+			kvm_update_cpuid_runtime(vcpu);
>+		}
> 		break;
> 	case MSR_SMI_COUNT:
> 		if (!msr_info->host_initiated)
>-- 
>2.27.0
>
Yang, Weijiang May 30, 2023, 3:51 a.m. UTC | #2 
On 5/25/2023 2:10 PM, Chao Gao wrote:
> On Thu, May 11, 2023 at 12:08:43AM -0400, Yang Weijiang wrote:
>> Update CPUID(EAX=0DH,ECX=1) when the guest's XSS is modified.
>> CPUID(EAX=0DH,ECX=1).EBX reports current required storage size for all
>> features enabled via XCR0 | XSS so that guest can allocate correct xsave
>> buffer.
>>
>> Note, KVM does not yet support any XSS based features, i.e. supported_xss
>> is guaranteed to be zero at this time.
>>
>> Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/cpuid.c | 7 +++++--
>> arch/x86/kvm/x86.c   | 6 ++++--
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 123bf8b97a4b..cbb1b8a65502 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -277,8 +277,11 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>>
>> 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
>> 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
>> -		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>> -		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>> +		cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {
> Align indentation.

OK. Thanks!

>
>   	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {
>
>> +		u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
>> +
>> +		best->ebx = xstate_required_size(xstate, true);
>> +	}
>>
>> 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
>> 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 33a780fe820b..ab3360a10933 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 		 */
>> 		if (data & ~kvm_caps.supported_xss)
> Shouldn't we check against the supported value of _this_ guest? similar to
> guest_supported_xcr0.

I don't think it requires an extra variable to serve per guest purpose.

For guest XSS settings, now we don't add extra constraints like XCR0, 
thus all KVM supported

bits can be accessed by guest.  There's already another variable 
vcpu->arch.ia32_xss

to play similar role. In future, if there's requirement to serve per VM 
control purpose,

then will align it to XCR0 settings.


>
>> 			return 1;
>> -		vcpu->arch.ia32_xss = data;
>> -		kvm_update_cpuid_runtime(vcpu);
>> +		if (vcpu->arch.ia32_xss != data) {
>> +			vcpu->arch.ia32_xss = data;
>> +			kvm_update_cpuid_runtime(vcpu);
>> +		}
>> 		break;
>> 	case MSR_SMI_COUNT:
>> 		if (!msr_info->host_initiated)
>> -- 
>> 2.27.0
>>
Chao Gao May 30, 2023, 12:08 p.m. UTC | #3 
>> > --- a/arch/x86/kvm/x86.c
>> > +++ b/arch/x86/kvm/x86.c
>> > @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> > 		 */
>> > 		if (data & ~kvm_caps.supported_xss)
>> Shouldn't we check against the supported value of _this_ guest? similar to
>> guest_supported_xcr0.
>
>I don't think it requires an extra variable to serve per guest purpose.
>
>For guest XSS settings, now we don't add extra constraints like XCR0, thus

QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
certain supervisor state components cannot be managed by XSAVES, even
though KVM supports them. IOW, guests may differ in the supported values
for the IA32_XSS MSR.
Yang, Weijiang May 31, 2023, 1:11 a.m. UTC | #4 
On 5/30/2023 8:08 PM, Chao Gao wrote:
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>> 		 */
>>>> 		if (data & ~kvm_caps.supported_xss)
>>> Shouldn't we check against the supported value of _this_ guest? similar to
>>> guest_supported_xcr0.
>> I don't think it requires an extra variable to serve per guest purpose.
>>
>> For guest XSS settings, now we don't add extra constraints like XCR0, thus
> QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
> certain supervisor state components cannot be managed by XSAVES, even
> though KVM supports them. IOW, guests may differ in the supported values
> for the IA32_XSS MSR.

OK, will change this part to align with xcr0 settings. Thanks!
Sean Christopherson June 15, 2023, 11:45 p.m. UTC | #5 
On Wed, May 31, 2023, Weijiang Yang wrote:
> 
> On 5/30/2023 8:08 PM, Chao Gao wrote:
> > > > > --- a/arch/x86/kvm/x86.c
> > > > > +++ b/arch/x86/kvm/x86.c
> > > > > @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > > > 		 */
> > > > > 		if (data & ~kvm_caps.supported_xss)
> > > > Shouldn't we check against the supported value of _this_ guest? similar to
> > > > guest_supported_xcr0.
> > > I don't think it requires an extra variable to serve per guest purpose.
> > > 
> > > For guest XSS settings, now we don't add extra constraints like XCR0, thus
> > QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
> > certain supervisor state components cannot be managed by XSAVES, even
> > though KVM supports them. IOW, guests may differ in the supported values
> > for the IA32_XSS MSR.
> 
> OK, will change this part to align with xcr0 settings. Thanks!

Please write KVM-Unit-Tests to verify KVM correctly handles the various MSRs related
to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, though testing
the combinations of CPUID bits will require multiple x86/unittests.cfg entries.
Might be time to split up msr.c into a library and then multiple tests.
Yang, Weijiang June 16, 2023, 1:58 a.m. UTC | #6 
On 6/16/2023 7:45 AM, Sean Christopherson wrote:
> On Wed, May 31, 2023, Weijiang Yang wrote:
>> On 5/30/2023 8:08 PM, Chao Gao wrote:
>>>>>> --- a/arch/x86/kvm/x86.c
>>>>>> +++ b/arch/x86/kvm/x86.c
>>>>>> @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>>>> 		 */
>>>>>> 		if (data & ~kvm_caps.supported_xss)
>>>>> Shouldn't we check against the supported value of _this_ guest? similar to
>>>>> guest_supported_xcr0.
>>>> I don't think it requires an extra variable to serve per guest purpose.
>>>>
>>>> For guest XSS settings, now we don't add extra constraints like XCR0, thus
>>> QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
>>> certain supervisor state components cannot be managed by XSAVES, even
>>> though KVM supports them. IOW, guests may differ in the supported values
>>> for the IA32_XSS MSR.
>> OK, will change this part to align with xcr0 settings. Thanks!
> Please write KVM-Unit-Tests to verify KVM correctly handles the various MSRs related
> to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, though testing
> the combinations of CPUID bits will require multiple x86/unittests.cfg entries.
> Might be time to split up msr.c into a library and then multiple tests.

Since there's already a CET specific unit test app, do you mind adding 
all CET related stuffs to

the app to make it inclusive? e.g.,  validate constraints between CET 
CPUIDs vs. CET/XSS MSRs?
Sean Christopherson June 23, 2023, 11:21 p.m. UTC | #7 
On Fri, Jun 16, 2023, Weijiang Yang wrote:
> 
> On 6/16/2023 7:45 AM, Sean Christopherson wrote:
> > On Wed, May 31, 2023, Weijiang Yang wrote:
> > > On 5/30/2023 8:08 PM, Chao Gao wrote:
> > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > > > > > 		 */
> > > > > > > 		if (data & ~kvm_caps.supported_xss)
> > > > > > Shouldn't we check against the supported value of _this_ guest? similar to
> > > > > > guest_supported_xcr0.
> > > > > I don't think it requires an extra variable to serve per guest purpose.
> > > > > 
> > > > > For guest XSS settings, now we don't add extra constraints like XCR0, thus
> > > > QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
> > > > certain supervisor state components cannot be managed by XSAVES, even
> > > > though KVM supports them. IOW, guests may differ in the supported values
> > > > for the IA32_XSS MSR.
> > > OK, will change this part to align with xcr0 settings. Thanks!
> > Please write KVM-Unit-Tests to verify KVM correctly handles the various MSRs related
> > to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, though testing
> > the combinations of CPUID bits will require multiple x86/unittests.cfg entries.
> > Might be time to split up msr.c into a library and then multiple tests.
> 
> Since there's already a CET specific unit test app, do you mind adding all
> CET related stuffs to the app to make it inclusive? e.g.,�validate constraints
> between CET CPUIDs vs. CET/XSS MSRs?

Hmm, that will get a bit kludgy since the MSR testcases will want to toggle IBT
and SHSTK on and off.

Actually, I take back my suggestion to add a KUT test.  Except for a few special
cases, e.g. 32-bit support, selftests is a better framework for testing MSRs than
KUT, as it's relatively easy to create a custom vCPU model in selftests, whereas
in KUT it requires handcoding an entry in unittests.cfg, and having corresponding
code in the test itself.

The biggest gap in selftests was the lack of decent reporting in guest code, but
Aaron is working on closing that gap[*].

I'm thinking something like this as a framework.  

	struct msr_data {
		const uint32_t idx;
		const char *name;
		const struct kvm_x86_cpu_feature feature1;
		const struct kvm_x86_cpu_feature feature2;
		const uint32_t nr_values;
		const uint64_t *values;
	};

	#define TEST_MSR2(msr, f1, f2) { .idx = msr, .name = #msr, .feature1 = f1, .feature2 = f2, .nr_values = ARRAY_SIZE(msr_VALUES), .values = msr_VALUES }
	#define TEST_MSR(msr, f) TEST_MSR2(msr, f, <a dummy value?>)
	#define TEST_MSR0(msr) TEST_MSR(msr, <a dummy value?>)

With CET usage looking like

	static const uint64_t MSR_IA32_S_CET_VALUES[] = {
		<super interesting values>
	};

	TEST_MSR2(MSR_IA32_S_CET, X86_FEATURE_IBT, X86_FEATURE_SHSTK);

Then the test could iterate over each entry and test the various combinations of
features being enabled (if supported by KVM).  And it could also test ioctls(),
which are all but impossible to test in KUT, e.g. verify that supported MSRs are
reported in KVM_GET_MSR_INDEX_LIST, verify that userspace can read/write MSRs
regardless of guest CPUID, etc.  Ooh, and we can even test MSR filtering.

I don't know that we'd want to cram all of those things in a single test, but we
can worry about that later as it shouldn't be difficult to put the framework and
MSR definitions in common code.

[*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@google.com
Yang, Weijiang June 26, 2023, 9:24 a.m. UTC | #8 
On 6/24/2023 7:21 AM, Sean Christopherson wrote:
> On Fri, Jun 16, 2023, Weijiang Yang wrote:
>> On 6/16/2023 7:45 AM, Sean Christopherson wrote:
>>> On Wed, May 31, 2023, Weijiang Yang wrote:
>>>> On 5/30/2023 8:08 PM, Chao Gao wrote:
>>>>>>>> --- a/arch/x86/kvm/x86.c
>>>>>>>> +++ b/arch/x86/kvm/x86.c
>>>>>>>> @@ -3776,8 +3776,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>>>>>> 		 */
>>>>>>>> 		if (data & ~kvm_caps.supported_xss)
>>>>>>> Shouldn't we check against the supported value of _this_ guest? similar to
>>>>>>> guest_supported_xcr0.
>>>>>> I don't think it requires an extra variable to serve per guest purpose.
>>>>>>
>>>>>> For guest XSS settings, now we don't add extra constraints like XCR0, thus
>>>>> QEMU can impose constraints by configuring guest CPUID.0xd.1 to indicate
>>>>> certain supervisor state components cannot be managed by XSAVES, even
>>>>> though KVM supports them. IOW, guests may differ in the supported values
>>>>> for the IA32_XSS MSR.
>>>> OK, will change this part to align with xcr0 settings. Thanks!
>>> Please write KVM-Unit-Tests to verify KVM correctly handles the various MSRs related
>>> to CET, e.g. a test_cet_msrs() subtest in msr.c would do nicely.  Hmm, though testing
>>> the combinations of CPUID bits will require multiple x86/unittests.cfg entries.
>>> Might be time to split up msr.c into a library and then multiple tests.
>> Since there's already a CET specific unit test app, do you mind adding all
>> CET related stuffs to the app to make it inclusive? e.g.,�validate constraints
>> between CET CPUIDs vs. CET/XSS MSRs?
> Hmm, that will get a bit kludgy since the MSR testcases will want to toggle IBT
> and SHSTK on and off.
>
> Actually, I take back my suggestion to add a KUT test.  Except for a few special
> cases, e.g. 32-bit support, selftests is a better framework for testing MSRs than
> KUT, as it's relatively easy to create a custom vCPU model in selftests, whereas
> in KUT it requires handcoding an entry in unittests.cfg, and having corresponding
> code in the test itself.
>
> The biggest gap in selftests was the lack of decent reporting in guest code, but
> Aaron is working on closing that gap[*].
>
> I'm thinking something like this as a framework.
>
> 	struct msr_data {
> 		const uint32_t idx;
> 		const char *name;
> 		const struct kvm_x86_cpu_feature feature1;
> 		const struct kvm_x86_cpu_feature feature2;
> 		const uint32_t nr_values;
> 		const uint64_t *values;
> 	};
>
> 	#define TEST_MSR2(msr, f1, f2) { .idx = msr, .name = #msr, .feature1 = f1, .feature2 = f2, .nr_values = ARRAY_SIZE(msr_VALUES), .values = msr_VALUES }
> 	#define TEST_MSR(msr, f) TEST_MSR2(msr, f, <a dummy value?>)
> 	#define TEST_MSR0(msr) TEST_MSR(msr, <a dummy value?>)
>
> With CET usage looking like
>
> 	static const uint64_t MSR_IA32_S_CET_VALUES[] = {
> 		<super interesting values>
> 	};
>
> 	TEST_MSR2(MSR_IA32_S_CET, X86_FEATURE_IBT, X86_FEATURE_SHSTK);
>
> Then the test could iterate over each entry and test the various combinations of
> features being enabled (if supported by KVM).  And it could also test ioctls(),
> which are all but impossible to test in KUT, e.g. verify that supported MSRs are
> reported in KVM_GET_MSR_INDEX_LIST, verify that userspace can read/write MSRs
> regardless of guest CPUID, etc.  Ooh, and we can even test MSR filtering.
>
> I don't know that we'd want to cram all of those things in a single test, but we
> can worry about that later as it shouldn't be difficult to put the framework and
> MSR definitions in common code.

OK, I'll add a new selftest app which initially only includes CET MSRs 
testing but practice

the above ideas.

>
> [*] https://lore.kernel.org/all/20230607224520.4164598-1-aaronlewis@google.com
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 123bf8b97a4b..cbb1b8a65502 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -277,8 +277,11 @@  static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 
 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
-		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
-		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
+		cpuid_entry_has(best, X86_FEATURE_XSAVEC))) {
+		u64 xstate = vcpu->arch.xcr0 | vcpu->arch.ia32_xss;
+
+		best->ebx = xstate_required_size(xstate, true);
+	}
 
 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 33a780fe820b..ab3360a10933 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3776,8 +3776,10 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		 */
 		if (data & ~kvm_caps.supported_xss)
 			return 1;
-		vcpu->arch.ia32_xss = data;
-		kvm_update_cpuid_runtime(vcpu);
+		if (vcpu->arch.ia32_xss != data) {
+			vcpu->arch.ia32_xss = data;
+			kvm_update_cpuid_runtime(vcpu);
+		}
 		break;
 	case MSR_SMI_COUNT:
 		if (!msr_info->host_initiated)