Messages in this thread Patch in this message | | | Subject | Re: [PATCH V10 05/18] KVM: x86/pmu: Set MSR_IA32_MISC_ENABLE_EMON bit when vPMU is enabled | From | Liuxiangdong <> | Date | Sun, 7 Nov 2021 18:14:17 +0800 |
| |
Hi, like and lingshan.
As said, IA32_MISC_ENABLE[7] bit depends on the PMU is enabled for the guest, so a software write openration to this bit will be ignored.
But, in this patch, all the openration that writes msr_ia32_misc_enable in guest could make this bit become 0.
Suppose: When we start vm with "enable_pmu", vcpu->arch.ia32_misc_enable_msr may be 0x80 first. And next, guest writes msr_ia32_misc_enable value 0x1. What we want could be 0x81, but unfortunately, it will be 0x1 because of "data &= ~MSR_IA32_MISC_ENABLE_EMON;" And even if guest writes msr_ia32_misc_enable value 0x81, it will be 0x1 also.
What we want is write operation will not change this bit. So, how about this?
--- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3321,6 +3321,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) } break; case MSR_IA32_MISC_ENABLE: + data &= ~MSR_IA32_MISC_ENABLE_EMON; + data |= (vcpu->arch.ia32_misc_enable_msr & MSR_IA32_MISC_ENABLE_EMON); if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT) && ((vcpu->arch.ia32_misc_enable_msr ^ data) & MSR_IA32_MISC_ENABLE_MWAIT)) { if (!guest_cpuid_has(vcpu, X86_FEATURE_XMM3))
Or is there anything in your design intention I don't understand?
Thanks!
Xiangdong Liu
On 2021/8/6 21:37, Zhu Lingshan wrote: > From: Like Xu <like.xu@linux.intel.com> > > On Intel platforms, the software can use the IA32_MISC_ENABLE[7] bit to > detect whether the processor supports performance monitoring facility. > > It depends on the PMU is enabled for the guest, and a software write > operation to this available bit will be ignored. The proposal to ignore > the toggle in KVM is the way to go and that behavior matches bare metal. > > Cc: Yao Yuan <yuan.yao@intel.com> > Signed-off-by: Like Xu <like.xu@linux.intel.com> > Reviewed-by: Venkatesh Srinivas <venkateshs@chromium.org> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/x86/kvm/vmx/pmu_intel.c | 1 + > arch/x86/kvm/x86.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 9efc1a6b8693..d9dbebe03cae 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -488,6 +488,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu) > if (!pmu->version) > return; > > + vcpu->arch.ia32_misc_enable_msr |= MSR_IA32_MISC_ENABLE_EMON; > perf_get_x86_pmu_capability(&x86_pmu); > > pmu->nr_arch_gp_counters = min_t(int, eax.split.num_counters, > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index efd11702465c..f6b6984e26ef 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3321,6 +3321,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > } > break; > case MSR_IA32_MISC_ENABLE: > + data &= ~MSR_IA32_MISC_ENABLE_EMON; > if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT) && > ((vcpu->arch.ia32_misc_enable_msr ^ data) & MSR_IA32_MISC_ENABLE_MWAIT)) { > if (!guest_cpuid_has(vcpu, X86_FEATURE_XMM3))
| |