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 | Mon, 8 Nov 2021 12:07:39 +0800 |
| |
On 2021/11/8 11:06, Like Xu wrote: > 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; > u64 pmu_mask = MSR_IA32_MISC_ENABLE_EMON | MSR_IA32_MISC_ENABLE_EMON;
Repetitive "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. > */ Yes. It's a little clearer with comments. > 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)) >>
| |