On Fri, Aug 30, 2019 at 11:26 AM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Wed, Aug 28, 2019 at 04:41:30PM -0700, Oliver Upton wrote:
> > Creating a helper function to check the validity of the
>
> Changelogs should use imperative mood, e.g.:
>
> Create a helper function to check...
>
> > {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL wrt the PMU's global_ctrl_mask.
>
> As is, this needs the SDM quote from patch 4/7 as it's not clear what
> global_ctrl_mask contains, e.g. the check looks inverted to the
> uninitiated. Adding a helper without a user is also discouraged,
> e.g. if the helper is broken then bisection would point at the next
> patch, so this should really be folded in to patch 4/7 anyways.
>
> That being said, if you tweak the prototype (see below) then you can
> use it intel_pmu_set_msr(), in which case a standalone patch does make
> sense.
>
> > Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
> > ---
> > arch/x86/kvm/pmu.h | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index 58265f761c3b..b7d9efff208d 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -79,6 +79,17 @@ static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
> > return kvm_x86_ops->pmu_ops->pmc_is_enabled(pmc);
> > }
> >
> > +static inline bool kvm_is_valid_perf_global_ctrl(struct kvm_vcpu *vcpu,
>
> If this takes 'struct kvm_pmu *pmu' instead of a vcpu then it can also
> be used in intel_pmu_set_msr().
>
> > + u64 global_ctrl)
> > +{
> > + struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> > +
> > + if (pmu->global_ctrl_mask & global_ctrl)
>
> intel_pmu_set_msr() allows 'global_ctrl == data' regardless of the mask,
> do we need simliar code here?
Indeed. For all of the special-cased MSRs in the VMCS, the validity
checks should be the same as those performed by WRMSR emulation.
> > + return false;
> > +
> > + return true;
>
> Or simply
>
> return !(pmu->global_ctrl_mask & global_ctrl);
>
> > +}
> > +
> > /* returns general purpose PMC with the specified MSR. Note that it can be
> > * used for both PERFCTRn and EVNTSELn; that is why it accepts base as a
> > * paramenter to tell them apart.
> > --
> > 2.23.0.187.g17f5b7556c-goog
> >
