Messages in this thread Patch in this message | | | Date | Mon, 8 Nov 2021 11:06:40 +0800 | From | Like Xu <> | Subject | Re: [PATCH V10 05/18] KVM: x86/pmu: Set MSR_IA32_MISC_ENABLE_EMON bit when vPMU is enabled |
| |
On 7/11/2021 6:14 pm, Liuxiangdong wrote: > 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. >
Yes and thank you. The fix has been committed on my private tree for a long time.
> > 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)) > >
How about this for the final state considering PEBS enabling:
case MSR_IA32_MISC_ENABLE: { u64 old_val = vcpu->arch.ia32_misc_enable_msr; u64 pmu_mask = MSR_IA32_MISC_ENABLE_EMON | MSR_IA32_MISC_ENABLE_EMON;
/* RO bits */ if (!msr_info->host_initiated && ((old_val ^ data) & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL)) return 1;
/* * For a dummy user space, the order of setting vPMU capabilities and * initialising MSR_IA32_MISC_ENABLE is not strictly guaranteed, so to * avoid inconsistent functionality we keep the vPMU bits unchanged here. */ data &= ~pmu_mask; data |= old_val & pmu_mask; if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT) && ((old_val ^ data) & MSR_IA32_MISC_ENABLE_MWAIT)) { if (!guest_cpuid_has(vcpu, X86_FEATURE_XMM3)) return 1; vcpu->arch.ia32_misc_enable_msr = data; kvm_update_cpuid_runtime(vcpu); } else { vcpu->arch.ia32_misc_enable_msr = data; } break; }
> 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)) >
| |