[PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Vitaly Kuznetsov]

state_test/smm_test selftests are failing on AMD with:
"Unexpected result from KVM_GET_MSRS, r: 51 (failed MSR was 0x345)"

MSR_IA32_PERF_CAPABILITIES is an emulated MSR indeed but only on Intel,
make svm_has_emulated_msr() skip it so it is not returned by
KVM_GET_MSR_INDEX_LIST.

Fixes: 27461da31089 ("KVM: x86/pmu: Support full width counting")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8ccfa4197d9c..2c423d64fb8f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3539,6 +3539,8 @@ static bool svm_has_emulated_msr(u32 index)
 	case MSR_IA32_MCG_EXT_CTL:
 	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
 		return false;
+	case MSR_IA32_PERF_CAPABILITIES:
+		return false;
 	default:
 		break;
 	}
-- 
2.25.4

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Jim Mattson]

On Tue, Jun 16, 2020 at 9:14 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> state_test/smm_test selftests are failing on AMD with:
> "Unexpected result from KVM_GET_MSRS, r: 51 (failed MSR was 0x345)"
>
> MSR_IA32_PERF_CAPABILITIES is an emulated MSR indeed but only on Intel,
> make svm_has_emulated_msr() skip it so it is not returned by
> KVM_GET_MSR_INDEX_LIST.

Do we need to support this MSR under SVM for cross-vendor migration?
Or, have we given up on that?

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Tue, Jun 16, 2020 at 9:14 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> state_test/smm_test selftests are failing on AMD with:
>> "Unexpected result from KVM_GET_MSRS, r: 51 (failed MSR was 0x345)"
>>
>> MSR_IA32_PERF_CAPABILITIES is an emulated MSR indeed but only on Intel,
>> make svm_has_emulated_msr() skip it so it is not returned by
>> KVM_GET_MSR_INDEX_LIST.
>
> Do we need to support this MSR under SVM for cross-vendor migration?
> Or, have we given up on that?

To be honest I'm not sure about the status of cross-vendor migration in
general and PMU implications in particular, hope Paolo/Sean can shed
some light. In this particular case my shallow understanding is that
MSR_IA32_PERF_CAPABILITIES has only one known feature bit which unlocks
an MSR range with additional counters. If the feature bit is not set
this, I guess, can easily be migrated (basically, let's allow writing
'0' there on AMD and return '0' on read). But what if the feature was
enabled? We'll have to support the new MSR range and do something with
it after migration (run intel_pmu in fully emulated mode?).

Anyway, the immediate issue I'm trying to fix here is: whatever is
returned by KVM_GET_MSR_INDEX_LIST can be successfully queried with
KVM_GET_MSRS as some userspaces count on that.

-- 
Vitaly

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Jim Mattson]

On Tue, Jun 16, 2020 at 9:45 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Jim Mattson <jmattson@google.com> writes:
>
> > On Tue, Jun 16, 2020 at 9:14 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >>
> >> state_test/smm_test selftests are failing on AMD with:
> >> "Unexpected result from KVM_GET_MSRS, r: 51 (failed MSR was 0x345)"
> >>
> >> MSR_IA32_PERF_CAPABILITIES is an emulated MSR indeed but only on Intel,
> >> make svm_has_emulated_msr() skip it so it is not returned by
> >> KVM_GET_MSR_INDEX_LIST.
> >
> > Do we need to support this MSR under SVM for cross-vendor migration?
> > Or, have we given up on that?
>
> To be honest I'm not sure about the status of cross-vendor migration in
> general and PMU implications in particular, hope Paolo/Sean can shed
> some light. In this particular case my shallow understanding is that
> MSR_IA32_PERF_CAPABILITIES has only one known feature bit which unlocks
> an MSR range with additional counters. If the feature bit is not set
> this, I guess, can easily be migrated (basically, let's allow writing
> '0' there on AMD and return '0' on read). But what if the feature was
> enabled? We'll have to support the new MSR range and do something with
> it after migration (run intel_pmu in fully emulated mode?).
>
> Anyway, the immediate issue I'm trying to fix here is: whatever is
> returned by KVM_GET_MSR_INDEX_LIST can be successfully queried with
> KVM_GET_MSRS as some userspaces count on that.

That's a nice property. Is it documented somewhere?

Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Tue, Jun 16, 2020 at 9:45 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Jim Mattson <jmattson@google.com> writes:
>>
>> > On Tue, Jun 16, 2020 at 9:14 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> >>
>> >> state_test/smm_test selftests are failing on AMD with:
>> >> "Unexpected result from KVM_GET_MSRS, r: 51 (failed MSR was 0x345)"
>> >>
>> >> MSR_IA32_PERF_CAPABILITIES is an emulated MSR indeed but only on Intel,
>> >> make svm_has_emulated_msr() skip it so it is not returned by
>> >> KVM_GET_MSR_INDEX_LIST.
>> >
>> > Do we need to support this MSR under SVM for cross-vendor migration?
>> > Or, have we given up on that?
>>
>> To be honest I'm not sure about the status of cross-vendor migration in
>> general and PMU implications in particular, hope Paolo/Sean can shed
>> some light. In this particular case my shallow understanding is that
>> MSR_IA32_PERF_CAPABILITIES has only one known feature bit which unlocks
>> an MSR range with additional counters. If the feature bit is not set
>> this, I guess, can easily be migrated (basically, let's allow writing
>> '0' there on AMD and return '0' on read). But what if the feature was
>> enabled? We'll have to support the new MSR range and do something with
>> it after migration (run intel_pmu in fully emulated mode?).
>>
>> Anyway, the immediate issue I'm trying to fix here is: whatever is
>> returned by KVM_GET_MSR_INDEX_LIST can be successfully queried with
>> KVM_GET_MSRS as some userspaces count on that.
>
> That's a nice property. Is it documented somewhere?
>

Hm, good question.

Documentation/virt/kvm/api.rst says:

"KVM_GET_MSR_INDEX_LIST returns the guest msrs that are supported.  The list
varies by kvm version and host processor, but does not change otherwise.

[...]

KVM_GET_MSR_FEATURE_INDEX_LIST returns the list of MSRs that can be passed
to the KVM_GET_MSRS system ioctl.  This lets userspace probe host capabilities
and processor features that are exposed via MSRs (e.g., VMX capabilities)."

Side note: MSR_IA32_PERF_CAPABILITIES can be returned by both
KVM_GET_MSR_INDEX_LIST and KVM_GET_MSR_FEATURE_INDEX_LIST as we have it
both as an emulated MSR filtered by kvm_x86_ops.has_emulated_msr() and
a feature msr filtered by kvm_x86_ops.get_msr_feature(). But the later
is a whitelist so MSR_IA32_PERF_CAPABILITIES won't appear on AMD and the
promise "can be passed to the KVM_GET_MSRS" is kept.

For KVM_GET_MSR_INDEX_LIST, the promise is "guest msrs that are
supported" and I'm not exactly sure what this means. Personally, I see
no point in returning MSRs which can't be read with KVM_GET_MSRS (as
this also means the guest can't read them) and KVM selftests seem to
rely on that (vcpu_save_state()) but this is not a documented feature.

> Reviewed-by: Jim Mattson <jmattson@google.com>
>

Thanks!

-- 
Vitaly

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Jim Mattson]

On Wed, Jun 17, 2020 at 4:38 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Side note: MSR_IA32_PERF_CAPABILITIES can be returned by both
> KVM_GET_MSR_INDEX_LIST and KVM_GET_MSR_FEATURE_INDEX_LIST as we have it
> both as an emulated MSR filtered by kvm_x86_ops.has_emulated_msr() and
> a feature msr filtered by kvm_x86_ops.get_msr_feature(). But the later
> is a whitelist so MSR_IA32_PERF_CAPABILITIES won't appear on AMD and the
> promise "can be passed to the KVM_GET_MSRS" is kept.

So, how is MSR_IA32_PERF_CAPABILITIES different from, say,
MSR_K7_HWCR, which by its very name doesn't sound like it would be
supported on Intel CPUs? Why not just emulate
MSR_IA32_PERF_CAPABILITIES on AMD, just as we emulate MSR_K7_HWCR on
Intel?

My concern is that we don't seem to have a standard here. Each
individual MSR is handled ad hoc, which adds unnecessary complexity.

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Paolo Bonzini]

On 17/06/20 13:38, Vitaly Kuznetsov wrote:
> 
> For KVM_GET_MSR_INDEX_LIST, the promise is "guest msrs that are
> supported" and I'm not exactly sure what this means. Personally, I see
> no point in returning MSRs which can't be read with KVM_GET_MSRS (as
> this also means the guest can't read them) and KVM selftests seem to
> rely on that (vcpu_save_state()) but this is not a documented feature.

Yes, this is intended.  KVM_GET_MSR_INDEX_LIST is not the full list of
supported MSRs or KVM_GET_MSRS (especially PMU MSRs are missing) but it
certainly should be a sufficient condition for KVM_GET_MSRS support.

In this case your patch is sort-of correct because AMD machines won't
have X86_FEATURE_PDCM.  However, even in that case there are two things
we can do that are better:

1) force-set X86_FEATURE_PDCM in vmx_set_cpu_caps instead of having it
in kvm_set_cpu_caps.  The latter is incorrect because if AMD for
whatever reason added it we'd lack the support.  This would be basically
a refined version of your patch.

2) emulate the MSR on AMD too (returning zero) if somebody for whatever
reason enables PDCM in there too: this would include returning it in
KVM_GET_FEATURE_MSR_INDEX_LIST, and using kvm_get_msr_feature to set a
default value in kvm_pmu_refresh.  The feature bit then would be
force-set in kvm_set_cpu_caps.  This would be nicer since we have the
value in vcpu->arch already instead of struct vcpu_vmx.

Thanks,

Paolo

Re: [PATCH] KVM: SVM: drop MSR_IA32_PERF_CAPABILITIES from emulated MSRs

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 17/06/20 13:38, Vitaly Kuznetsov wrote:
>> 
>> For KVM_GET_MSR_INDEX_LIST, the promise is "guest msrs that are
>> supported" and I'm not exactly sure what this means. Personally, I see
>> no point in returning MSRs which can't be read with KVM_GET_MSRS (as
>> this also means the guest can't read them) and KVM selftests seem to
>> rely on that (vcpu_save_state()) but this is not a documented feature.
>
> Yes, this is intended.  KVM_GET_MSR_INDEX_LIST is not the full list of
> supported MSRs or KVM_GET_MSRS (especially PMU MSRs are missing) but it
> certainly should be a sufficient condition for KVM_GET_MSRS support.
>
> In this case your patch is sort-of correct because AMD machines won't
> have X86_FEATURE_PDCM.  However, even in that case there are two things
> we can do that are better:
>
> 1) force-set X86_FEATURE_PDCM in vmx_set_cpu_caps instead of having it
> in kvm_set_cpu_caps.  The latter is incorrect because if AMD for
> whatever reason added it we'd lack the support.  This would be basically
> a refined version of your patch.
>
> 2) emulate the MSR on AMD too (returning zero) if somebody for whatever
> reason enables PDCM in there too: this would include returning it in
> KVM_GET_FEATURE_MSR_INDEX_LIST, and using kvm_get_msr_feature to set a
> default value in kvm_pmu_refresh.  The feature bit then would be
> force-set in kvm_set_cpu_caps.  This would be nicer since we have the
> value in vcpu->arch already instead of struct vcpu_vmx.

Let's try the hard way :-) I'll send v2 implementing 2) (hope I got the
idea right), thanks!

-- 
Vitaly