Re: [PATCH 3/7] KVM: VMX: Add helper to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL

[Sean Christopherson <sean.j.christopherson@intel.com>]

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@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  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?

> +		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
>