On Fri, Aug 30, 2019 at 12:20:12PM -0700, Jim Mattson wrote:
> 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.
Is the 'global_ctrl == data' check present to ensure that
global_ctrl_changed() is only hit when necessary, or is it a condition
to test for valid state?
I'm having a hard time seeing what returning true on 'global_ctrl ==
data' is giving us (in the context of emulating host/guest state checks
on vm-entry). Also, intel_pmu_set_msr() will still need to check it
anyway to decide if it needs to call global_ctrl_changed().
Either way, the check against mask condition can be shared through a
common helper with Sean's suggested tweak to the prototype.
> > > + return false;
> > > +
> > > + return true;
> >
> > Or simply
> >
> > return !(pmu->global_ctrl_mask & global_ctrl);
I'll go this route in the next series.
> > > +}
> > > +
> > > /* 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
> > >
