On Fri, Sep 06, 2019 at 02:03:10PM -0700, Oliver Upton wrote:
> The "load IA32_PERF_GLOBAL_CTRL" bit for VM-entry and VM-exit should
> only be exposed to the guest if IA32_PERF_GLOBAL_CTRL is a valid MSR.
> Create a new helper to allow pmu_refresh() to update the VM-entry and
> VM-exit controls to ensure PMU values are initialized when performing
> the is_valid_msr() check.
Can you describe how this is functionally correct? At a glance, it looks
like KVM already handles PERF_GLOBAL_CTRL, this is just allowing it to be
loaded via VMX transitions? Assuming that's true, including such info in
the changelog is extremely helpful, e.g. to differentiate between a minor
enhancement and a significant addition to what KVM virtualizes.
> Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Reviewed-by: Peter Shier <pshier@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx/pmu_intel.c | 3 +++
> arch/x86/kvm/vmx/vmx.c | 21 +++++++++++++++++++++
> arch/x86/kvm/vmx/vmx.h | 1 +
Can the helper be placed in nested.{c.h} instead of vmx.{c,h}?
> 3 files changed, 25 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 963766d631ad..2dc7be724321 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -16,6 +16,7 @@
> #include "cpuid.h"
> #include "lapic.h"
> #include "pmu.h"
> +#include "vmx.h"
>
> static struct kvm_event_hw_type_mapping intel_arch_events[] = {
> /* Index must match CPUID 0x0A.EBX bit vector */
> @@ -314,6 +315,8 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> (boot_cpu_has(X86_FEATURE_HLE) || boot_cpu_has(X86_FEATURE_RTM)) &&
> (entry->ebx & (X86_FEATURE_HLE|X86_FEATURE_RTM)))
> pmu->reserved_bits ^= HSW_IN_TX|HSW_IN_TX_CHECKPOINTED;
> +
> + nested_vmx_pmu_entry_exit_ctls_update(vcpu);
> }
>
> static void intel_pmu_init(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 570a233e272b..5b0664bff23b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6417,6 +6417,27 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
> }
> }
>
> +void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_vmx *vmx;
> +
> + if (!nested_vmx_allowed(vcpu))
> + return;
> +
> + vmx = to_vmx(vcpu);
> + if (intel_pmu_ops.is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL)) {
> + vmx->nested.msrs.entry_ctls_high |=
> + VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
> + vmx->nested.msrs.exit_ctls_high |=
> + VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
> + } else {
> + vmx->nested.msrs.entry_ctls_high &=
> + ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
> + vmx->nested.msrs.exit_ctls_high &=
> + ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
> + }
> +}
> +
> bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
>
> static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 82d0bc3a4d52..e06884cf88ad 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -331,6 +331,7 @@ void vmx_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
> struct shared_msr_entry *find_msr_entry(struct vcpu_vmx *vmx, u32 msr);
> void pt_update_intercept_for_msr(struct vcpu_vmx *vmx);
> void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp);
> +void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu);
>
> #define POSTED_INTR_ON 0
> #define POSTED_INTR_SN 1
> --
> 2.23.0.187.g17f5b7556c-goog
>
