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 16:27:37 +0800 |
| |
On 2021/11/8 12:11, Like Xu wrote: > On 8/11/2021 12:07 pm, Liuxiangdong wrote: >> >> >> 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" ? > > Oops, > > u64 pmu_mask = MSR_IA32_MISC_ENABLE_EMON | > MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL; >
Yes. bit[12] is also read-only, so we can keep this bit unchanged also.
And, because write operation will not change this bit by "pmu_mask", do we still need this if statement?
/* RO bits */ if (!msr_info->host_initiated && ((old_val ^ data) & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL)) return 1;
"(old_val ^ data) & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL" means some operation tries to change this bit, so we cannot allow it. But, if there is no this judgement, "pmu_mask" will still make this bit[12] no change.
The only difference is that we can not change other bit (except bit 12 and bit 7) once "old_val[12] != data[12]" if there exists this statement and we can change other bit if there is no judgement.
For both MSR_IA32_MISC_ENABLE_EMON and MSR_IA32_MISC_ENABLE_EMON are read-only, maybe we can keep their behavioral consistency. Either both judge, or neither.
Do you think so?
> I'll send the fix after sync with Lingshan. > >> >>> /* 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. > > Thanks for your feedback! Enjoy the feature. > >>> 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)) >>>> >>
| |