| Message ID | 20240126085444.324918-19-xiong.y.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/pmu: Introduce passthrough vPM | expand |
On Fri, Jan 26, 2024, Xiong Zhang wrote: > From: Mingwei Zhang <mizhang@google.com> > > Intercept full-width GP counter MSRs in passthrough PMU if guest does not > have the capability to write in full-width. In addition, opportunistically > add a warning if non-full-width counter MSRs are also intercepted, in which > case it is a clear mistake. > > Co-developed-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > arch/x86/kvm/vmx/pmu_intel.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 7f6cabb2c378..49df154fbb5b 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -429,6 +429,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > default: > if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || > (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) { > + if (is_passthrough_pmu_enabled(vcpu) && > + !(msr & MSR_PMC_FULL_WIDTH_BIT) && > + !msr_info->host_initiated) { > + pr_warn_once("passthrough PMU never intercepts non-full-width PMU counters\n"); > + return 1; This is broken, KVM must be prepared to handle WRMSR (and RDMSR and RDPMC) that come in through the emulator.
On Thu, Apr 11, 2024 at 2:23 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Jan 26, 2024, Xiong Zhang wrote: > > From: Mingwei Zhang <mizhang@google.com> > > > > Intercept full-width GP counter MSRs in passthrough PMU if guest does not > > have the capability to write in full-width. In addition, opportunistically > > add a warning if non-full-width counter MSRs are also intercepted, in which > > case it is a clear mistake. > > > > Co-developed-by: Xiong Zhang <xiong.y.zhang@intel.com> > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > Signed-off-by: Mingwei Zhang <mizhang@google.com> > > --- > > arch/x86/kvm/vmx/pmu_intel.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > > index 7f6cabb2c378..49df154fbb5b 100644 > > --- a/arch/x86/kvm/vmx/pmu_intel.c > > +++ b/arch/x86/kvm/vmx/pmu_intel.c > > @@ -429,6 +429,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > default: > > if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || > > (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) { > > + if (is_passthrough_pmu_enabled(vcpu) && > > + !(msr & MSR_PMC_FULL_WIDTH_BIT) && > > + !msr_info->host_initiated) { > > + pr_warn_once("passthrough PMU never intercepts non-full-width PMU counters\n"); > > + return 1; > > This is broken, KVM must be prepared to handle WRMSR (and RDMSR and RDPMC) that > come in through the emulator. Don't tell me that we are still supporting CPUs that don't have "unrestricted guest"! Sigh.
On Thu, Apr 11, 2024, Jim Mattson wrote: > On Thu, Apr 11, 2024 at 2:23 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Fri, Jan 26, 2024, Xiong Zhang wrote: > > > From: Mingwei Zhang <mizhang@google.com> > > > > > > Intercept full-width GP counter MSRs in passthrough PMU if guest does not > > > have the capability to write in full-width. In addition, opportunistically > > > add a warning if non-full-width counter MSRs are also intercepted, in which > > > case it is a clear mistake. > > > > > > Co-developed-by: Xiong Zhang <xiong.y.zhang@intel.com> > > > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com> > > > Signed-off-by: Mingwei Zhang <mizhang@google.com> > > > --- > > > arch/x86/kvm/vmx/pmu_intel.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > > > index 7f6cabb2c378..49df154fbb5b 100644 > > > --- a/arch/x86/kvm/vmx/pmu_intel.c > > > +++ b/arch/x86/kvm/vmx/pmu_intel.c > > > @@ -429,6 +429,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > default: > > > if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || > > > (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) { > > > + if (is_passthrough_pmu_enabled(vcpu) && > > > + !(msr & MSR_PMC_FULL_WIDTH_BIT) && > > > + !msr_info->host_initiated) { > > > + pr_warn_once("passthrough PMU never intercepts non-full-width PMU counters\n"); > > > + return 1; > > > > This is broken, KVM must be prepared to handle WRMSR (and RDMSR and RDPMC) that > > come in through the emulator. > > Don't tell me that we are still supporting CPUs that don't have > "unrestricted guest"! Sigh. Heh, KVM still supports CPUs without VMX virtual NMIs :-) Practically speaking, if we want to eliminate things like emulated WRMSR/RDMSR, a Kconfig to build a reduced emulator would be the way to go. But while a reduced emulator would be nice for host security, I don't think it would buy us much from a code perspective, since KVM still needs to handle host userspace MSR accesses. E.g. KVM could have conditional sanity checks for MSRs that are supposed to be passed through, but unless a reduced emulator is a hard requirement for passthrough PMUs, we'd still need the code to handle the emulated accesses. And even if a reduced emulator were a hard requirement, I'd still push for a WARN-and-continue approach, not a "inject a bogus #GP because KVM screwed up" approach.
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index 7f6cabb2c378..49df154fbb5b 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -429,6 +429,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) default: if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) || (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) { + if (is_passthrough_pmu_enabled(vcpu) && + !(msr & MSR_PMC_FULL_WIDTH_BIT) && + !msr_info->host_initiated) { + pr_warn_once("passthrough PMU never intercepts non-full-width PMU counters\n"); + return 1; + } + if ((msr & MSR_PMC_FULL_WIDTH_BIT) && (data & ~pmu->counter_bitmask[KVM_PMC_GP])) return 1; @@ -801,7 +808,8 @@ void intel_passthrough_pmu_msrs(struct kvm_vcpu *vcpu) for (i = 0; i < vcpu_to_pmu(vcpu)->nr_arch_gp_counters; i++) { vmx_set_intercept_for_msr(vcpu, MSR_ARCH_PERFMON_EVENTSEL0 + i, MSR_TYPE_RW, false); vmx_set_intercept_for_msr(vcpu, MSR_IA32_PERFCTR0 + i, MSR_TYPE_RW, false); - vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW, false); + if (fw_writes_is_enabled(vcpu)) + vmx_set_intercept_for_msr(vcpu, MSR_IA32_PMC0 + i, MSR_TYPE_RW, false); } vmx_set_intercept_for_msr(vcpu, MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_TYPE_RW, false);