Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
Hi Paolo,
On 2021/1/27 14:04, Like Xu wrote:
On 2021/1/26 17:42, Paolo Bonzini wrote:
On 08/01/21 02:36, Like Xu wrote:
@@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
pmu->fixed_counters[i].current_config = 0;
}
+
+ vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu,
X86_FEATURE_PDCM) ?
+ vmx_get_perf_capabilities() : 0;
There is one thing I don't understand with this patch: intel_pmu_init is
not called when CPUID is changed. So I would have thought that anything
that uses guest_cpuid_has must stay in intel_pmu_refresh. As I
understand it vcpu->arch.perf_capabilities is always set to 0
(vmx_get_perf_capabilities is never called), and kvm_set_msr_common
would fail to set any bit in the MSR. What am I missing?
In addition, the code of patch 4:
+ if (!intel_pmu_lbr_is_enabled(vcpu)) {
+ vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
+ lbr_desc->records.nr = 0;
+ }
is not okay after MSR changes. The value written by the host must be
either rejected (with "return 1") or applied unchanged.
Fortunately I think this code is dead if you move the check in
kvm_set_msr from patch 9 to patch 4. However, in patch 9
vmx_get_perf_capabilities() must only set the LBR format bits if
intel_pmu_lbr_is_compatible(vcpu).
Thanks for the guidance. How about handling it in this way:
In the intel_pmu_init():
vcpu->arch.perf_capabilities = 0;
lbr_desc->records.nr = 0;
In the intel_pmu_refresh():
if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) {
vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();
if (!lbr_desc->records.nr)
vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
}
In the vmx_set_msr():
case MSR_IA32_PERF_CAPABILITIES:
// set up lbr_desc->records.nr
if (!intel_pmu_lbr_is_compatible(vcpu))
return 1;
ret = kvm_set_msr_common(vcpu, msr_info);
In the kvm_set_msr_common():
case MSR_IA32_PERF_CAPABILITIES:
vcpu->arch.perf_capabilities = data;
kvm_pmu_refresh(vcpu);
The new version will make the vcpu->arch.perf_capabilities crud as simple
as possible.
The patches look good apart from these issues and the other nits I
pointed out. However, you need testcases here, for both kvm-unit-tests
and tools/testing/selftests/kvm.
For KVM, it would be at least a basic check that looks for the MSR LBR
(using the MSR indices for the various processors), does a branch, and
checks that the FROM_IP/TO_IP are good. You can write the
kvm-unit-tests using the QEMU option "-cpu host,migratable=no": if you
do this, QEMU will pick the KVM_GET_SUPPORTED_CPUID bits and move them
more or less directly into the guest CPUID.
For tools/testing/selftests/kvm, your test need to check the effect of
various CPUID settings on the PERF_CAPABILITIES MSR, check that whatever
you write with KVM_SET_MSR is _not_ modified and can be retrieved with
KVM_GET_MSR, and check that invalid LBR formats are rejected.
Thanks, I will add the above tests in the next version.
I'm really, really sorry for leaving these patches on my todo list for
months, but you guys need to understand the main reason for this: they
come with no testcases. A large patch series adding userspace APIs and
complicated CPUID/MSR processing *automatically* goes to the bottom of
my queue, because:
Please review the new version
https://lore.kernel.org/kvm/20210201051039.255478-1-like...@linux.intel.com/T/#t
and kvm-unit-tests:
https://lore.kernel.org/kvm/20210201045751.243231-1-like...@linux.intel.com,
in case the patch set is not *automatically* processed to the bottom of
your queue.
I'll also add tests for other new vPMU features.
---
thx, likexu
- I need to go with a fine comb over all the userspace API changes, I
cannot just look at test code and see if it works.
- I will have no way to test its correctness after it's committed.
For you, the work ends when your patch is accepted. For me, that's when
the work begins, and I need to make sure that the patch will be
maintainable in the future.
Thanks, and sorry again for the delay.
Paolo
Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
On 27/01/21 07:04, Like Xu wrote:
On 2021/1/26 17:42, Paolo Bonzini wrote:
On 08/01/21 02:36, Like Xu wrote:
@@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
pmu->fixed_counters[i].current_config = 0;
}
+
+ vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu,
X86_FEATURE_PDCM) ?
+ vmx_get_perf_capabilities() : 0;
There is one thing I don't understand with this patch: intel_pmu_init
is not called when CPUID is changed. So I would have thought that
anything that uses guest_cpuid_has must stay in intel_pmu_refresh. As
I understand it vcpu->arch.perf_capabilities is always set to 0
(vmx_get_perf_capabilities is never called), and kvm_set_msr_common
would fail to set any bit in the MSR. What am I missing?
In addition, the code of patch 4:
+ if (!intel_pmu_lbr_is_enabled(vcpu)) {
+ vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
+ lbr_desc->records.nr = 0;
+ }
is not okay after MSR changes. The value written by the host must be
either rejected (with "return 1") or applied unchanged.
Fortunately I think this code is dead if you move the check in
kvm_set_msr from patch 9 to patch 4. However, in patch 9
vmx_get_perf_capabilities() must only set the LBR format bits if
intel_pmu_lbr_is_compatible(vcpu).
Thanks for the guidance. How about handling it in this way:
In the intel_pmu_init():
vcpu->arch.perf_capabilities = 0;
lbr_desc->records.nr = 0;
In the intel_pmu_refresh():
if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) {
vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();
if (!lbr_desc->records.nr)
vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
}
In the vmx_set_msr():
case MSR_IA32_PERF_CAPABILITIES:
// set up lbr_desc->records.nr
if (!intel_pmu_lbr_is_compatible(vcpu))
Maybe pass msr_info.data to intel_pmu_lbr_is_compatible? Otherwise
seems okay, thanks Like.
Paolo
Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
On 2021/1/26 17:42, Paolo Bonzini wrote:
On 08/01/21 02:36, Like Xu wrote:
@@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
pmu->fixed_counters[i].current_config = 0;
}
+
+ vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu,
X86_FEATURE_PDCM) ?
+ vmx_get_perf_capabilities() : 0;
There is one thing I don't understand with this patch: intel_pmu_init is
not called when CPUID is changed. So I would have thought that anything
that uses guest_cpuid_has must stay in intel_pmu_refresh. As I understand
it vcpu->arch.perf_capabilities is always set to 0
(vmx_get_perf_capabilities is never called), and kvm_set_msr_common would
fail to set any bit in the MSR. What am I missing?
In addition, the code of patch 4:
+ if (!intel_pmu_lbr_is_enabled(vcpu)) {
+ vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
+ lbr_desc->records.nr = 0;
+ }
is not okay after MSR changes. The value written by the host must be
either rejected (with "return 1") or applied unchanged.
Fortunately I think this code is dead if you move the check in kvm_set_msr
from patch 9 to patch 4. However, in patch 9 vmx_get_perf_capabilities()
must only set the LBR format bits if intel_pmu_lbr_is_compatible(vcpu).
Thanks for the guidance. How about handling it in this way:
In the intel_pmu_init():
vcpu->arch.perf_capabilities = 0;
lbr_desc->records.nr = 0;
In the intel_pmu_refresh():
if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) {
vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();
if (!lbr_desc->records.nr)
vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
}
In the vmx_set_msr():
case MSR_IA32_PERF_CAPABILITIES:
// set up lbr_desc->records.nr
if (!intel_pmu_lbr_is_compatible(vcpu))
return 1;
ret = kvm_set_msr_common(vcpu, msr_info);
In the kvm_set_msr_common():
case MSR_IA32_PERF_CAPABILITIES:
vcpu->arch.perf_capabilities = data;
kvm_pmu_refresh(vcpu);
The patches look good apart from these issues and the other nits I pointed
out. However, you need testcases here, for both kvm-unit-tests and
tools/testing/selftests/kvm.
For KVM, it would be at least a basic check that looks for the MSR LBR
(using the MSR indices for the various processors), does a branch, and
checks that the FROM_IP/TO_IP are good. You can write the kvm-unit-tests
using the QEMU option "-cpu host,migratable=no": if you do this, QEMU will
pick the KVM_GET_SUPPORTED_CPUID bits and move them more or less directly
into the guest CPUID.
For tools/testing/selftests/kvm, your test need to check the effect of
various CPUID settings on the PERF_CAPABILITIES MSR, check that whatever
you write with KVM_SET_MSR is _not_ modified and can be retrieved with
KVM_GET_MSR, and check that invalid LBR formats are rejected.
Thanks, I will add the above tests in the next version.
I'm really, really sorry for leaving these patches on my todo list for
months, but you guys need to understand the main reason for this: they come
with no testcases. A large patch series adding userspace APIs and
complicated CPUID/MSR processing *automatically* goes to the bottom of my
queue, because:
- I need to go with a fine comb over all the userspace API changes, I
cannot just look at test code and see if it works.
- I will have no way to test its correctness after it's committed.
For you, the work ends when your patch is accepted. For me, that's when
the work begins, and I need to make sure that the patch will be
maintainable in the future.
Thanks, and sorry again for the delay.
Paolo
Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
On 08/01/21 02:36, Like Xu wrote:
@@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
pmu->fixed_counters[i].current_config = 0;
}
+
+ vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu, X86_FEATURE_PDCM) ?
+ vmx_get_perf_capabilities() : 0;
There is one thing I don't understand with this patch: intel_pmu_init is
not called when CPUID is changed. So I would have thought that anything
that uses guest_cpuid_has must stay in intel_pmu_refresh. As I
understand it vcpu->arch.perf_capabilities is always set to 0
(vmx_get_perf_capabilities is never called), and kvm_set_msr_common
would fail to set any bit in the MSR. What am I missing?
In addition, the code of patch 4:
+ if (!intel_pmu_lbr_is_enabled(vcpu)) {
+ vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
+ lbr_desc->records.nr = 0;
+ }
is not okay after MSR changes. The value written by the host must be
either rejected (with "return 1") or applied unchanged.
Fortunately I think this code is dead if you move the check in
kvm_set_msr from patch 9 to patch 4. However, in patch 9
vmx_get_perf_capabilities() must only set the LBR format bits if
intel_pmu_lbr_is_compatible(vcpu).
The patches look good apart from these issues and the other nits I
pointed out. However, you need testcases here, for both kvm-unit-tests
and tools/testing/selftests/kvm.
For KVM, it would be at least a basic check that looks for the MSR LBR
(using the MSR indices for the various processors), does a branch, and
checks that the FROM_IP/TO_IP are good. You can write the
kvm-unit-tests using the QEMU option "-cpu host,migratable=no": if you
do this, QEMU will pick the KVM_GET_SUPPORTED_CPUID bits and move them
more or less directly into the guest CPUID.
For tools/testing/selftests/kvm, your test need to check the effect of
various CPUID settings on the PERF_CAPABILITIES MSR, check that whatever
you write with KVM_SET_MSR is _not_ modified and can be retrieved with
KVM_GET_MSR, and check that invalid LBR formats are rejected.
I'm really, really sorry for leaving these patches on my todo list for
months, but you guys need to understand the main reason for this: they
come with no testcases. A large patch series adding userspace APIs and
complicated CPUID/MSR processing *automatically* goes to the bottom of
my queue, because:
- I need to go with a fine comb over all the userspace API changes, I
cannot just look at test code and see if it works.
- I will have no way to test its correctness after it's committed.
For you, the work ends when your patch is accepted. For me, that's when
the work begins, and I need to make sure that the patch will be
maintainable in the future.
Thanks, and sorry again for the delay.
Paolo
[RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
On Intel platforms, KVM agent could configure MSR_IA32_PERF_CAPABILITIES (such as unmask some vmx-supported bits in vcpu->arch.perf_capabilities) to adjust the visibility of guest PMU features for vPMU-enabled guests. Once MSR_IA32_PERF_CAPABILITIES is changed via vmx_set_msr() validly, the adjustment in intel_pmu_refresh() will be triggered. To ensure the sustainability of the new value, the default initialization path is moved to intel_pmu_init(). Signed-off-by: Like Xu Reviewed-by: Andi Kleen --- arch/x86/kvm/vmx/pmu_intel.c | 6 +++--- arch/x86/kvm/vmx/vmx.c | 5 + arch/x86/kvm/x86.c | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index a886a47daebd..f8083ecf8c7b 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -327,7 +327,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu) pmu->counter_bitmask[KVM_PMC_FIXED] = 0; pmu->version = 0; pmu->reserved_bits = 0x0020ull; - vcpu->arch.perf_capabilities = 0; entry = kvm_find_cpuid_entry(vcpu, 0xa, 0); if (!entry) @@ -340,8 +339,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu) return; perf_get_x86_pmu_capability(_pmu); - if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) - vcpu->arch.perf_capabilities = vmx_get_perf_capabilities(); pmu->nr_arch_gp_counters = min_t(int, eax.split.num_counters, x86_pmu.num_counters_gp); @@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu) pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED; pmu->fixed_counters[i].current_config = 0; } + + vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu, X86_FEATURE_PDCM) ? + vmx_get_perf_capabilities() : 0; } static void intel_pmu_reset(struct kvm_vcpu *vcpu) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 035bd6d279a4..085a14579bbe 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2209,6 +2209,11 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if ((data >> 32) != 0) return 1; goto find_uret_msr; + case MSR_IA32_PERF_CAPABILITIES: + if (data && !vcpu_to_pmu(vcpu)->version) + return 1; + ret = kvm_set_msr_common(vcpu, msr_info); + break; default: find_uret_msr: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c765fd72a66c..a1b251ae648a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3037,7 +3037,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) return 1; vcpu->arch.perf_capabilities = data; - + kvm_pmu_refresh(vcpu); return 0; } case MSR_EFER: -- 2.29.2
[PATCH RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility
On Intel platforms, KVM agent could configure MSR_IA32_PERF_CAPABILITIES (such as unmask some vmx-supported bits in vcpu->arch.perf_capabilities) to adjust the visibility of guest PMU features for vPMU-enabled guests. Once MSR_IA32_PERF_CAPABILITIES is changed via vmx_set_msr() validly, the adjustment in intel_pmu_refresh() will be triggered. To ensure the sustainability of the new value, the default initialization path is moved to intel_pmu_init(). Signed-off-by: Like Xu --- arch/x86/kvm/vmx/pmu_intel.c | 6 +++--- arch/x86/kvm/vmx/vmx.c | 5 + arch/x86/kvm/x86.c | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index a886a47daebd..f8083ecf8c7b 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -327,7 +327,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu) pmu->counter_bitmask[KVM_PMC_FIXED] = 0; pmu->version = 0; pmu->reserved_bits = 0x0020ull; - vcpu->arch.perf_capabilities = 0; entry = kvm_find_cpuid_entry(vcpu, 0xa, 0); if (!entry) @@ -340,8 +339,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu) return; perf_get_x86_pmu_capability(_pmu); - if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) - vcpu->arch.perf_capabilities = vmx_get_perf_capabilities(); pmu->nr_arch_gp_counters = min_t(int, eax.split.num_counters, x86_pmu.num_counters_gp); @@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu) pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED; pmu->fixed_counters[i].current_config = 0; } + + vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu, X86_FEATURE_PDCM) ? + vmx_get_perf_capabilities() : 0; } static void intel_pmu_reset(struct kvm_vcpu *vcpu) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 4085c90c8fc2..b9fde795dd96 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2210,6 +2210,11 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if ((data >> 32) != 0) return 1; goto find_uret_msr; + case MSR_IA32_PERF_CAPABILITIES: + if (data && !vcpu_to_pmu(vcpu)->version) + return 1; + ret = kvm_set_msr_common(vcpu, msr_info); + break; default: find_uret_msr: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 021791991e05..410d5209a76f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3029,7 +3029,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) return 1; vcpu->arch.perf_capabilities = data; - + kvm_pmu_refresh(vcpu); return 0; } case MSR_EFER: -- 2.21.3
