On Mon, Dec 26, 2022, Like Xu wrote: > From: Like Xu <likexu@xxxxxxxxxxx> > > Higher version PMU features are obviously not supported on hosts with > lower version Arch pmu, such as trying to access FIXED_CTR registers > and PERF_GLOBAL registers from pmu.version >1 on a host with version 1. > > Ignore host userspace reads and writes of '0' to those PMU MSRs that > KVM reports in the MSR-to-save list, but the MSRs are ultimately > unsupported. All MSRs in said list must be writable by userspace, e.g. > if userspace sends the list back at KVM without filtering out the MSRs > it doesn't need. > > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx> > --- > arch/x86/kvm/x86.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index f570367463c8..fcb9c317df59 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3881,6 +3881,11 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_IA32_DS_AREA: > case MSR_PEBS_DATA_CFG: > case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5: > + case MSR_ARCH_PERFMON_FIXED_CTR0 ... MSR_ARCH_PERFMON_FIXED_CTR_MAX: > + case MSR_CORE_PERF_FIXED_CTR_CTRL: > + case MSR_CORE_PERF_GLOBAL_STATUS: > + case MSR_CORE_PERF_GLOBAL_CTRL: > + case MSR_CORE_PERF_GLOBAL_OVF_CTRL: > if (kvm_pmu_is_valid_msr(vcpu, msr)) > return kvm_pmu_set_msr(vcpu, msr_info); > /* > @@ -3984,6 +3989,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_IA32_DS_AREA: > case MSR_PEBS_DATA_CFG: > case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5: > + case MSR_ARCH_PERFMON_FIXED_CTR0 ... MSR_ARCH_PERFMON_FIXED_CTR_MAX: > + case MSR_CORE_PERF_FIXED_CTR_CTRL: > + case MSR_CORE_PERF_GLOBAL_STATUS: > + case MSR_CORE_PERF_GLOBAL_CTRL: > + case MSR_CORE_PERF_GLOBAL_OVF_CTRL: Having to manually handle each MSR is again a maintenance burden. Rather than manually add the affect PMU MSRs, I think we should just allow benign accesses to all "MSRs to save". The lookup will be a linear search, but the array isn't _that_ big and this should be a rare occurrence. That might also make it easier to handle non-PMU MSRs that want similar treatment. I'll send a series with the patches I've proposed, along with the patch to clean up the unimplemented MSR printks[*], which was never formally posted. [*] https://lore.kernel.org/all/Y1wCqAzJwvz4s8OR@xxxxxxxxxx --- arch/x86/kvm/x86.c | 51 ++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 87bb7024e18f..4ad7e3065c69 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3561,6 +3561,18 @@ static void record_steal_time(struct kvm_vcpu *vcpu) mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa)); } +static bool kvm_is_msr_to_save(u32 msr_index) +{ + unsigned int i; + + for (i = 0; i < num_msrs_to_save; i++) { + if (msrs_to_save[i] == msr_index) + return true; + } + + return false; +} + int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) { bool pr = false; @@ -3884,20 +3896,18 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) vcpu->arch.guest_fpu.xfd_err = data; break; #endif - case MSR_IA32_PEBS_ENABLE: - case MSR_IA32_DS_AREA: - case MSR_PEBS_DATA_CFG: - case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5: + default: if (kvm_pmu_is_valid_msr(vcpu, msr)) return kvm_pmu_set_msr(vcpu, msr_info); + /* * Userspace is allowed to write '0' to MSRs that KVM reports * as to-be-saved, even if an MSRs isn't fully supported. */ - return !msr_info->host_initiated || data; - default: - if (kvm_pmu_is_valid_msr(vcpu, msr)) - return kvm_pmu_set_msr(vcpu, msr_info); + if (msr_info->host_initiated && !data && + kvm_is_msr_to_save(msr)) + break; + return KVM_MSR_RET_INVALID; } return 0; @@ -3987,20 +3997,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) case MSR_DRAM_ENERGY_STATUS: /* DRAM controller */ msr_info->data = 0; break; - case MSR_IA32_PEBS_ENABLE: - case MSR_IA32_DS_AREA: - case MSR_PEBS_DATA_CFG: - case MSR_F15H_PERF_CTL0 ... MSR_F15H_PERF_CTR5: - if (kvm_pmu_is_valid_msr(vcpu, msr_info->index)) - return kvm_pmu_get_msr(vcpu, msr_info); - /* - * Userspace is allowed to read MSRs that KVM reports as - * to-be-saved, even if an MSR isn't fully supported. - */ - if (!msr_info->host_initiated) - return 1; - msr_info->data = 0; - break; case MSR_K7_EVNTSEL0 ... MSR_K7_EVNTSEL3: case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3: case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1: @@ -4256,6 +4252,17 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) default: if (kvm_pmu_is_valid_msr(vcpu, msr_info->index)) return kvm_pmu_get_msr(vcpu, msr_info); + + /* + * Userspace is allowed to read MSRs that KVM reports as + * to-be-saved, even if an MSR isn't fully supported. + */ + if (msr_info->host_initiated && + kvm_is_msr_to_save(msr_info->index)) { + msr_info->data = 0; + break; + } + return KVM_MSR_RET_INVALID; } return 0; base-commit: eee17ec1d2c43ab3fcba604c4b88c6eb2d728fcd --
