Re: [PATCH v5 0/8] KVM: nVMX: Add full nested support for "load IA32_PERF_GLOBAL_CTRL" VM-{Entry,Exit} control

From: Oliver Upton <oupton@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org, "Radim Krčmář" <rkrcmar@redhat.com>,
	"Jim Mattson" <jmattson@google.com>,
	"Peter Shier" <pshier@google.com>,
	"Krish Sadhukhan" <krish.sadhukhan@oracle.com>,
	"Sean Christopherson" <sean.j.christopherson@intel.com>
Subject: Re: [PATCH v5 0/8] KVM: nVMX: Add full nested support for "load IA32_PERF_GLOBAL_CTRL" VM-{Entry,Exit} control
Date: Mon, 18 Nov 2019 21:17:38 -0800	[thread overview]
Message-ID: <20191119051738.GA2386@google.com> (raw)
In-Reply-To: <828732bd-4b22-6ae8-7dd9-a8ec54c927ec@redhat.com>

On Fri, Nov 15, 2019 at 01:19:46PM +0100, Paolo Bonzini wrote:
> On 14/11/19 01:17, Oliver Upton wrote:
> > [v1] https://lore.kernel.org/r/20190828234134.132704-1-oupton@google.com
> > [v2] https://lore.kernel.org/r/20190903213044.168494-1-oupton@google.com
> > [v3] https://lore.kernel.org/r/20190903215801.183193-1-oupton@google.com
> > [v4] https://lore.kernel.org/r/20190906210313.128316-1-oupton@google.com
> > 
> > v1 => v2:
> >  - Add Krish's Co-developed-by and Signed-off-by tags.
> >  - Fix minor nit to kvm-unit-tests to use 'host' local variable
> >    throughout test_load_pgc()
> >  - Teach guest_state_test_main() to check guest state from within nested
> >    VM
> >  - Update proposed tests to use guest/host state checks, wherein the
> >    value is checked from MSR_CORE_PERF_GLOBAL_CTRL.
> >  - Changelog line wrapping
> > 
> > v2 => v3:
> >  - Remove the value unchanged condition from
> >    kvm_is_valid_perf_global_ctrl
> >  - Add line to changelog for patch 3/8
> > 
> > v3 => v4:
> >  - Allow tests to set the guest func multiple times
> >  - Style fixes throughout kvm-unit-tests patches, per Krish's review
> > 
> > v4 => v5:
> >  - Rebased kernel and kvm-unit-tests patches
> >  - Reordered and reworked patches to now WARN on a failed
> >    kvm_set_msr()
> >  - Dropped patch to alow resetting guest in kvm-unit-tests, as the
> >    functionality is no longer needed.
> > 
> > This patchset exposes the "load IA32_PERF_GLOBAL_CTRL" to guests for nested
> > VM-entry and VM-exit. There already was some existing code that supported
> > the VM-exit ctrl, though it had an issue and was not exposed to the guest
> > anyway. These patches are based on the original set that Krish Sadhukhan
> > sent out earlier this year.
> > 
> > Oliver Upton (6):
> >   KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL VM-{Entry,Exit} control
> >   KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on VM-Entry
> >   KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on VM-Exit
> >   KVM: nVMX: Check HOST_IA32_PERF_GLOBAL_CTRL on VM-Entry
> >   KVM: nVMX: Check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry
> >   KVM: VMX: Add helper to check reserved bits in IA32_PERF_GLOBAL_CTRL
> > 
> >  arch/x86/kvm/pmu.h           |  6 ++++++
> >  arch/x86/kvm/vmx/nested.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  arch/x86/kvm/vmx/nested.h    |  1 +
> >  arch/x86/kvm/vmx/pmu_intel.c |  5 ++++-
> > 
> 
> Queued, thanks.
> 
> But I had to squash this in patch 8:
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 3129385..b6233ae 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -7161,6 +7161,7 @@ static void test_perf_global_ctrl(u32 nr, const
> char *name, u32 ctrl_nr,
>  		report_prefix_pop();
>  	}
> 
> +	data->enabled = false;
>  	report_prefix_pop();
>  	vmcs_write(ctrl_nr, ctrl_saved);
>  	vmcs_write(nr, pgc_saved);
> 
> and I'm not sure about how this could have worked for you.
> 
> Paolo

Thanks for the fix. This was an oversight of mine as I had only run the
tests I introduced individually. I'll be more thorough in future
changes, apologies.

--
Best,
Oliver