[PATCH 0/7] KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL

[PATCH 0/7] KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL

[Oliver Upton]

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.

Purpose of each patch:

(1) Change the existing code that implemented the VM-exit functionality
    to use kvm_set_msr() to avoid being overwritten by
    atomic_perf_switch_msrs().
(2) Update prepare_vmcs02() to implement the VM-entry functionality,
    again using kvm_set_msr().
(3) Create a helper function for checking the validity of an
    IA32_PERF_GLOBAL_CTRL value against pmu->global_ctrl_mask.
(4) Check guest state on VM-entry as described in the SDM.
(5) Check host state on VM-entry as described in the SDM.
(6) Expose the "load IA32_PERF_GLOBAL_CTRL" VM-entry and VM-exit
    controls if IA32_PERF_GLOBAL_CTRL is a valid MSR.
(7) Tests in kvm-unit-tests to check the VM-entry and VM-exit controls
    work properly



Oliver Upton (6):
  KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit
  KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on vmentry
  KVM: VMX: Add helper to check reserved bits in
    MSR_CORE_PERF_GLOBAL_CTRL
  KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry
  KVM: nVMX: Check HOST_IA32_PERF_GLOBAL_CTRL on VM-entry
  KVM: nVMX: Enable load IA32_PERF_GLOBAL_CTRL vm control if supported

 arch/x86/kvm/pmu.h           | 11 +++++++++++
 arch/x86/kvm/vmx/nested.c    | 35 ++++++++++++++++++++++++++++++++---
 arch/x86/kvm/vmx/pmu_intel.c |  3 +++
 arch/x86/kvm/vmx/vmx.c       | 21 +++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.h       |  1 +
 5 files changed, 68 insertions(+), 3 deletions(-)

-- 
2.23.0.187.g17f5b7556c-goog

[PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Oliver Upton]

The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
could cause this value to be overwritten. Instead, call kvm_set_msr()
which will allow atomic_switch_perf_msrs() to correctly set the values.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ced9fba32598..b0ca34bf4d21 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 				   struct vmcs12 *vmcs12)
 {
 	struct kvm_segment seg;
+	struct msr_data msr_info;
 	u32 entry_failure_code;
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
@@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
 		vcpu->arch.pat = vmcs12->host_ia32_pat;
 	}
-	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
-		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
-			vmcs12->host_ia32_perf_global_ctrl);
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
+		msr_info.host_initiated = false;
+		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
+		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
+		if (kvm_set_msr(vcpu, &msr_info))
+			pr_debug_ratelimited(
+				"%s cannot write MSR (0x%x, 0x%llx)\n",
+				__func__, msr_info.index, msr_info.data);
+	}
 
 	/* Set L1 segment info according to Intel SDM
 	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Krish Sadhukhan]

On 08/28/2019 04:41 PM, Oliver Upton wrote:
> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> could cause this value to be overwritten. Instead, call kvm_set_msr()
> which will allow atomic_switch_perf_msrs() to correctly set the values.
>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>   arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ced9fba32598..b0ca34bf4d21 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>   				   struct vmcs12 *vmcs12)
>   {
>   	struct kvm_segment seg;
> +	struct msr_data msr_info;
>   	u32 entry_failure_code;
>   
>   	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>   		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>   		vcpu->arch.pat = vmcs12->host_ia32_pat;
>   	}
> -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> -			vmcs12->host_ia32_perf_global_ctrl);
> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
> +		msr_info.host_initiated = false;
> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
> +		if (kvm_set_msr(vcpu, &msr_info))
> +			pr_debug_ratelimited(
> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> +				__func__, msr_info.index, msr_info.data);
> +	}
>   
>   	/* Set L1 segment info according to Intel SDM
>   	    27.5.2 Loading Host Segment and Descriptor-Table Registers */

These patches are what I am already working on. I sent the following:

         [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of 
nested guests
         [PATCH 0/4][kvm-unit-test nVMX]: Test "load 
IA32_PERF_GLOBAL_CONTROL" VM-entry control on vmentry of nested guests

a few months back. I got feedback from the alias and am working on v2 
which I will send soon...

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Oliver Upton]

On Wed, Aug 28, 2019 at 06:30:29PM -0700, Krish Sadhukhan wrote:
> 
> 
> On 08/28/2019 04:41 PM, Oliver Upton wrote:
> > The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> > on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> > could cause this value to be overwritten. Instead, call kvm_set_msr()
> > which will allow atomic_switch_perf_msrs() to correctly set the values.
> > 
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >   arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index ced9fba32598..b0ca34bf4d21 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> >   				   struct vmcs12 *vmcs12)
> >   {
> >   	struct kvm_segment seg;
> > +	struct msr_data msr_info;
> >   	u32 entry_failure_code;
> >   	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> > @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> >   		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
> >   		vcpu->arch.pat = vmcs12->host_ia32_pat;
> >   	}
> > -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> > -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> > -			vmcs12->host_ia32_perf_global_ctrl);
> > +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
> > +		msr_info.host_initiated = false;
> > +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> > +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
> > +		if (kvm_set_msr(vcpu, &msr_info))
> > +			pr_debug_ratelimited(
> > +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> > +				__func__, msr_info.index, msr_info.data);
> > +	}
> >   	/* Set L1 segment info according to Intel SDM
> >   	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
> 
> These patches are what I am already working on. I sent the following:
> 
>         [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested
> guests
>         [PATCH 0/4][kvm-unit-test nVMX]: Test "load
> IA32_PERF_GLOBAL_CONTROL" VM-entry control on vmentry of nested guests
> 
> a few months back. I got feedback from the alias and am working on v2 which
> I will send soon...
>
Yes, I saw your previous mail for this feature. I started work on this
because of a need for this feature + mentioned you in the cover letter.
However, it would be better to give codeveloper credit with your
permission.

May I resend this patchset with a 'Co-Developed-by' tag crediting you?

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Krish Sadhukhan]

On 8/28/19 7:02 PM, Oliver Upton wrote:
> On Wed, Aug 28, 2019 at 06:30:29PM -0700, Krish Sadhukhan wrote:
>>
>> On 08/28/2019 04:41 PM, Oliver Upton wrote:
>>> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
>>> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
>>> could cause this value to be overwritten. Instead, call kvm_set_msr()
>>> which will allow atomic_switch_perf_msrs() to correctly set the values.
>>>
>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>> Signed-off-by: Oliver Upton <oupton@google.com>
>>> ---
>>>    arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>> index ced9fba32598..b0ca34bf4d21 100644
>>> --- a/arch/x86/kvm/vmx/nested.c
>>> +++ b/arch/x86/kvm/vmx/nested.c
>>> @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>>>    				   struct vmcs12 *vmcs12)
>>>    {
>>>    	struct kvm_segment seg;
>>> +	struct msr_data msr_info;
>>>    	u32 entry_failure_code;
>>>    	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
>>> @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>>>    		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>>>    		vcpu->arch.pat = vmcs12->host_ia32_pat;
>>>    	}
>>> -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
>>> -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
>>> -			vmcs12->host_ia32_perf_global_ctrl);
>>> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
>>> +		msr_info.host_initiated = false;
>>> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
>>> +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
>>> +		if (kvm_set_msr(vcpu, &msr_info))
>>> +			pr_debug_ratelimited(
>>> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
>>> +				__func__, msr_info.index, msr_info.data);
>>> +	}
>>>    	/* Set L1 segment info according to Intel SDM
>>>    	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
>> These patches are what I am already working on. I sent the following:
>>
>>          [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested
>> guests
>>          [PATCH 0/4][kvm-unit-test nVMX]: Test "load
>> IA32_PERF_GLOBAL_CONTROL" VM-entry control on vmentry of nested guests
>>
>> a few months back. I got feedback from the alias and am working on v2 which
>> I will send soon...
>>
> Yes, I saw your previous mail for this feature. I started work on this
> because of a need for this feature
I understand. I know that I have been bit late on this...
> + mentioned you in the cover letter.
> However, it would be better to give codeveloper credit with your
> permission.
>
> May I resend this patchset with a 'Co-Developed-by' tag crediting you?

Sure. Thank you !

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Oliver Upton]

On Thu, Aug 29, 2019 at 12:19:21AM -0700, Krish Sadhukhan wrote:
> 
> On 8/28/19 7:02 PM, Oliver Upton wrote:
> > On Wed, Aug 28, 2019 at 06:30:29PM -0700, Krish Sadhukhan wrote:
> > > 
> > > On 08/28/2019 04:41 PM, Oliver Upton wrote:
> > > > The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> > > > on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> > > > could cause this value to be overwritten. Instead, call kvm_set_msr()
> > > > which will allow atomic_switch_perf_msrs() to correctly set the values.
> > > > 
> > > > Suggested-by: Jim Mattson <jmattson@google.com>
> > > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > > ---
> > > >    arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
> > > >    1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > > index ced9fba32598..b0ca34bf4d21 100644
> > > > --- a/arch/x86/kvm/vmx/nested.c
> > > > +++ b/arch/x86/kvm/vmx/nested.c
> > > > @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> > > >    				   struct vmcs12 *vmcs12)
> > > >    {
> > > >    	struct kvm_segment seg;
> > > > +	struct msr_data msr_info;
> > > >    	u32 entry_failure_code;
> > > >    	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> > > > @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> > > >    		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
> > > >    		vcpu->arch.pat = vmcs12->host_ia32_pat;
> > > >    	}
> > > > -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> > > > -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> > > > -			vmcs12->host_ia32_perf_global_ctrl);
> > > > +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
> > > > +		msr_info.host_initiated = false;
> > > > +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> > > > +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
> > > > +		if (kvm_set_msr(vcpu, &msr_info))
> > > > +			pr_debug_ratelimited(
> > > > +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> > > > +				__func__, msr_info.index, msr_info.data);
> > > > +	}
> > > >    	/* Set L1 segment info according to Intel SDM
> > > >    	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
> > > These patches are what I am already working on. I sent the following:
> > > 
> > >          [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested
> > > guests
> > >          [PATCH 0/4][kvm-unit-test nVMX]: Test "load
> > > IA32_PERF_GLOBAL_CONTROL" VM-entry control on vmentry of nested guests
> > > 
> > > a few months back. I got feedback from the alias and am working on v2 which
> > > I will send soon...
> > > 
> > Yes, I saw your previous mail for this feature. I started work on this
> > because of a need for this feature
> I understand. I know that I have been bit late on this...
No worries here! Glad I had a good starting point to go from :-)
> > + mentioned you in the cover letter.
> > However, it would be better to give codeveloper credit with your
> > permission.
> > 
> > May I resend this patchset with a 'Co-Developed-by' tag crediting you?
> 
> Sure. Thank you !

One last thing, need a Signed-off-by tag corresponding to this. Do I
have your permission to do so in the resend?

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Krish Sadhukhan]

On 8/29/19 1:07 AM, Oliver Upton wrote:
> On Thu, Aug 29, 2019 at 12:19:21AM -0700, Krish Sadhukhan wrote:
>> On 8/28/19 7:02 PM, Oliver Upton wrote:
>>> On Wed, Aug 28, 2019 at 06:30:29PM -0700, Krish Sadhukhan wrote:
>>>> On 08/28/2019 04:41 PM, Oliver Upton wrote:
>>>>> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
>>>>> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
>>>>> could cause this value to be overwritten. Instead, call kvm_set_msr()
>>>>> which will allow atomic_switch_perf_msrs() to correctly set the values.
>>>>>
>>>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>>>> Signed-off-by: Oliver Upton <oupton@google.com>
>>>>> ---
>>>>>     arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
>>>>>     1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>>> index ced9fba32598..b0ca34bf4d21 100644
>>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>>> @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>>>>>     				   struct vmcs12 *vmcs12)
>>>>>     {
>>>>>     	struct kvm_segment seg;
>>>>> +	struct msr_data msr_info;
>>>>>     	u32 entry_failure_code;
>>>>>     	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
>>>>> @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>>>>>     		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>>>>>     		vcpu->arch.pat = vmcs12->host_ia32_pat;
>>>>>     	}
>>>>> -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
>>>>> -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
>>>>> -			vmcs12->host_ia32_perf_global_ctrl);
>>>>> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
>>>>> +		msr_info.host_initiated = false;
>>>>> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
>>>>> +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
>>>>> +		if (kvm_set_msr(vcpu, &msr_info))
>>>>> +			pr_debug_ratelimited(
>>>>> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
>>>>> +				__func__, msr_info.index, msr_info.data);
>>>>> +	}
>>>>>     	/* Set L1 segment info according to Intel SDM
>>>>>     	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
>>>> These patches are what I am already working on. I sent the following:
>>>>
>>>>           [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested
>>>> guests
>>>>           [PATCH 0/4][kvm-unit-test nVMX]: Test "load
>>>> IA32_PERF_GLOBAL_CONTROL" VM-entry control on vmentry of nested guests
>>>>
>>>> a few months back. I got feedback from the alias and am working on v2 which
>>>> I will send soon...
>>>>
>>> Yes, I saw your previous mail for this feature. I started work on this
>>> because of a need for this feature
>> I understand. I know that I have been bit late on this...
> No worries here! Glad I had a good starting point to go from :-)
>>> + mentioned you in the cover letter.
>>> However, it would be better to give codeveloper credit with your
>>> permission.
>>>
>>> May I resend this patchset with a 'Co-Developed-by' tag crediting you?
>> Sure. Thank you !
> One last thing, need a Signed-off-by tag corresponding to this. Do I
> have your permission to do so in the resend?
Absolutely. Thanks !

Re: [PATCH 1/7] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

[Jim Mattson]

On Wed, Aug 28, 2019 at 4:41 PM Oliver Upton <oupton@google.com> wrote:
>
> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> could cause this value to be overwritten. Instead, call kvm_set_msr()
> which will allow atomic_switch_perf_msrs() to correctly set the values.

Also, as Sean pointed out, kvm needs to negotiate its usage of the
performance counters with the Linux perf subsystem. Going through
kvm_set_msr is the correct way to do that. Setting the hardware MSR
directly (going behind the back of the perf subsystem) is not allowed.

[My apologies for the double mail some of you will receive.]

[PATCH 2/7] KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on vmentry

[Oliver Upton]

If the "load IA32_PERF_GLOBAL_CTRL" bit on the VM-entry control is
set, the IA32_PERF_GLOBAL_CTRL MSR is loaded from GUEST_IA32_PERF_GLOBAL_CTRL
on VM-entry. Adding condition to prepare_vmcs02 to set
MSR_CORE_PERF_GLOBAL_CTRL if the "load IA32_PERF_GLOBAL_CTRL" bit is set.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b0ca34bf4d21..9ba90b38d74b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2281,6 +2281,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct hv_enlightened_vmcs *hv_evmcs = vmx->nested.hv_evmcs;
+	struct msr_data msr_info;
 	bool load_guest_pdptrs_vmcs12 = false;
 
 	if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
@@ -2404,6 +2405,16 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	if (!enable_ept)
 		vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) {
+		msr_info.host_initiated = false;
+		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
+		msr_info.data = vmcs12->guest_ia32_perf_global_ctrl;
+		if (kvm_set_msr(vcpu, &msr_info))
+			pr_debug_ratelimited(
+				"%s cannot write MSR (0x%x, 0x%llx)\n",
+				__func__, msr_info.index, msr_info.data);
+	}
+
 	kvm_rsp_write(vcpu, vmcs12->guest_rsp);
 	kvm_rip_write(vcpu, vmcs12->guest_rip);
 	return 0;
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 2/7] KVM: nVMX: Load GUEST_IA32_PERF_GLOBAL_CTRL MSR on vmentry

[Sean Christopherson]

On Wed, Aug 28, 2019 at 04:41:29PM -0700, Oliver Upton wrote:
> If the "load IA32_PERF_GLOBAL_CTRL" bit on the VM-entry control is
> set, the IA32_PERF_GLOBAL_CTRL MSR is loaded from GUEST_IA32_PERF_GLOBAL_CTRL
> on VM-entry. Adding condition to prepare_vmcs02 to set
> MSR_CORE_PERF_GLOBAL_CTRL if the "load IA32_PERF_GLOBAL_CTRL" bit is set.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b0ca34bf4d21..9ba90b38d74b 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2281,6 +2281,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	struct hv_enlightened_vmcs *hv_evmcs = vmx->nested.hv_evmcs;
> +	struct msr_data msr_info;
>  	bool load_guest_pdptrs_vmcs12 = false;
>  
>  	if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
> @@ -2404,6 +2405,16 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>  	if (!enable_ept)
>  		vcpu->arch.walk_mmu->inject_page_fault = vmx_inject_page_fault_nested;
>  
> +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) {
> +		msr_info.host_initiated = false;
> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> +		msr_info.data = vmcs12->guest_ia32_perf_global_ctrl;
> +		if (kvm_set_msr(vcpu, &msr_info))
> +			pr_debug_ratelimited(
> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> +				__func__, msr_info.index, msr_info.data);

Blech, I was going to say you should add a helper to consolidate this code
for patches 1/7 and 2/7, but there are something like 10 different places
in KVM that have similar blobs.  I'll put together a patch or three to
add helpers, no need to address a wider problem with this series.

> +	}
> +
>  	kvm_rsp_write(vcpu, vmcs12->guest_rsp);
>  	kvm_rip_write(vcpu, vmcs12->guest_rip);
>  	return 0;
> -- 
> 2.23.0.187.g17f5b7556c-goog
> 

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

[Oliver Upton]

Creating a helper function to check the validity of the
{HOST,GUEST}_IA32_PERF_GLOBAL_CTRL wrt the PMU's global_ctrl_mask.

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,
+						 u64 global_ctrl)
+{
+	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+
+	if (pmu->global_ctrl_mask & global_ctrl)
+		return false;
+
+	return true;
+}
+
 /* 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

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

[Sean Christopherson]

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
> 

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

[Jim Mattson]

On Fri, Aug 30, 2019 at 11:26 AM Sean Christopherson
<sean.j.christopherson@intel.com> 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@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?

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

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

[Oliver Upton]

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@intel.com> 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@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?
> 
> 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
> > >

[PATCH 4/7] KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry

[Oliver Upton]

According to the SDM 26.3.1.1, "If the "load IA32_PERF_GLOBAL_CTRL" VM-entry
control is 1, bits reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
field for that register".

Adding condition to nested_vmx_check_guest_state() to check the validity of
GUEST_IA32_PERF_GLOBAL_CTRL if the "load IA32_PERF_GLOBAL_CTRL" bit is
set on the VM-entry control.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 9ba90b38d74b..8d6f0144b1bd 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -10,6 +10,7 @@
 #include "hyperv.h"
 #include "mmu.h"
 #include "nested.h"
+#include "pmu.h"
 #include "trace.h"
 #include "x86.h"
 
@@ -2748,6 +2749,11 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 	}
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	    !kvm_is_valid_perf_global_ctrl(vcpu,
+					   vmcs12->guest_ia32_perf_global_ctrl))
+		return -EINVAL;
+
 	/*
 	 * If the load IA32_EFER VM-entry control is 1, the following checks
 	 * are performed on the field for the IA32_EFER MSR:
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 4/7] KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry

[Sean Christopherson]

On Wed, Aug 28, 2019 at 04:41:31PM -0700, Oliver Upton wrote:
> According to the SDM 26.3.1.1, "If the "load IA32_PERF_GLOBAL_CTRL" VM-entry
> control is 1, bits reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> field for that register".
> 
> Adding condition to nested_vmx_check_guest_state() to check the validity of
> GUEST_IA32_PERF_GLOBAL_CTRL if the "load IA32_PERF_GLOBAL_CTRL" bit is
> set on the VM-entry control.

Same comment on mood.  And for this case, it's probably overkill to
give a play-by-play of the code, just state that you're adding a check
as described in the SDM, e.g.:

Add a nested VM-Enter consistency check when loading the guest's
IA32_PERF_GLOBAL_CTRL MSR from vmcs12.  Per Intel's SDM:

  If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, bits
  reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the field for
  that register.

> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 9ba90b38d74b..8d6f0144b1bd 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -10,6 +10,7 @@
>  #include "hyperv.h"
>  #include "mmu.h"
>  #include "nested.h"
> +#include "pmu.h"
>  #include "trace.h"
>  #include "x86.h"
>  
> @@ -2748,6 +2749,11 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  	}
>  
> +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
> +	    !kvm_is_valid_perf_global_ctrl(vcpu,
> +					   vmcs12->guest_ia32_perf_global_ctrl))
> +		return -EINVAL;
> +
>  	/*
>  	 * If the load IA32_EFER VM-entry control is 1, the following checks
>  	 * are performed on the field for the IA32_EFER MSR:
> -- 
> 2.23.0.187.g17f5b7556c-goog
> 

Re: [PATCH 4/7] KVM: nVMX: check GUEST_IA32_PERF_GLOBAL_CTRL on VM-Entry

[Oliver Upton]

On Fri, Aug 30, 2019 at 11:37:03AM -0700, Sean Christopherson wrote:
> On Wed, Aug 28, 2019 at 04:41:31PM -0700, Oliver Upton wrote:
> > According to the SDM 26.3.1.1, "If the "load IA32_PERF_GLOBAL_CTRL" VM-entry
> > control is 1, bits reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> > field for that register".
> > 
> > Adding condition to nested_vmx_check_guest_state() to check the validity of
> > GUEST_IA32_PERF_GLOBAL_CTRL if the "load IA32_PERF_GLOBAL_CTRL" bit is
> > set on the VM-entry control.
> 
> Same comment on mood.  And for this case, it's probably overkill to
> give a play-by-play of the code, just state that you're adding a check
> as described in the SDM, e.g.:
> 
> Add a nested VM-Enter consistency check when loading the guest's
> IA32_PERF_GLOBAL_CTRL MSR from vmcs12.  Per Intel's SDM:
> 
>   If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, bits
>   reserved in the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the field for
>   that register.

Ack. This is a style problem throughout, I will make sure to address in
the next set I send out. Thanks for the suggested text as well, reads a
lot better than what I had before!
> > 
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 9ba90b38d74b..8d6f0144b1bd 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -10,6 +10,7 @@
> >  #include "hyperv.h"
> >  #include "mmu.h"
> >  #include "nested.h"
> > +#include "pmu.h"
> >  #include "trace.h"
> >  #include "x86.h"
> >  
> > @@ -2748,6 +2749,11 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
> > +	    !kvm_is_valid_perf_global_ctrl(vcpu,
> > +					   vmcs12->guest_ia32_perf_global_ctrl))
> > +		return -EINVAL;
> > +
> >  	/*
> >  	 * If the load IA32_EFER VM-entry control is 1, the following checks
> >  	 * are performed on the field for the IA32_EFER MSR:
> > -- 
> > 2.23.0.187.g17f5b7556c-goog
> > 

[PATCH 5/7] KVM: nVMX: Check HOST_IA32_PERF_GLOBAL_CTRL on VM-entry

[Oliver Upton]

According to the SDM 26.2.2, "If the "load IA32_PERF_GLOBAL_CTRL"
VM-exit control is 1, bits reserved in the IA32_PERF_GLOBAL_CTRL
MSR must be 0 in the field for that register"

Adding condition to nested_vmx_check_host_state that checks the validity
of HOST_IA32_PERF_GLOBAL_CTRL if "load IA32_PERF_GLOBAL_CTRL" is
set on the VM-exit control.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8d6f0144b1bd..d294b7d2d2cd 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2650,6 +2650,11 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    !kvm_pat_valid(vmcs12->host_ia32_pat))
 		return -EINVAL;
 
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	    !kvm_is_valid_perf_global_ctrl(vcpu,
+					   vmcs12->host_ia32_perf_global_ctrl))
+		return -EINVAL;
+
 	ia32e = (vmcs12->vm_exit_controls &
 		 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
 
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH 6/7] KVM: nVMX: Enable load IA32_PERF_GLOBAL_CTRL vm control if supported

[Oliver Upton]

The "load IA32_PERF_GLOBAL_CTRL" high bit for VM-entry and VM-exit should only
be set and made available to the guest iff IA32_PERF_GLOBAL_CTRL is a valid
MSR. Otherwise, the high bit should be cleared.

Creating a new helper function in vmx.c to allow the pmu_refresh code to
update the VM-entry and VM-exit controls. This was done instead of
adding to 'nested_vmx_entry_exit_ctls_update' as the PMU isn't yet
initialized with its values at the time it is called, causing the
'is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL)' check to fail
unconditionally.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c |  3 +++
 arch/x86/kvm/vmx/vmx.c       | 21 +++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.h       |  1 +
 3 files changed, 25 insertions(+)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 4dea0e0e7e39..6d42d9b41ddc 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 42ed3faa6af8..2cad761c913c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6407,6 +6407,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

Re: [PATCH 6/7] KVM: nVMX: Enable load IA32_PERF_GLOBAL_CTRL vm control if supported

[Jim Mattson]

On Wed, Aug 28, 2019 at 4:41 PM Oliver Upton <oupton@google.com> wrote:
>
> The "load IA32_PERF_GLOBAL_CTRL" high bit for VM-entry and VM-exit should only
> be set and made available to the guest iff IA32_PERF_GLOBAL_CTRL is a valid
> MSR. Otherwise, the high bit should be cleared.
>
> Creating a new helper function in vmx.c to allow the pmu_refresh code to
> update the VM-entry and VM-exit controls. This was done instead of
> adding to 'nested_vmx_entry_exit_ctls_update' as the PMU isn't yet
> initialized with its values at the time it is called, causing the
> 'is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL)' check to fail
> unconditionally.
>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>

One thing that's clear from this change (though it's not anything new)
is that KVM_SET_CPUID should be called before KVM_SET_MSRS, or
unexpected things might happen. Perhaps it's worth adding a note to
api.txt?

[kvm-unit-tests PATCH 7/7] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Oliver Upton]

Tests to verify that KVM performs the correct checks on Host/Guest state
at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
Control Registers and MSRs".

Test that KVM does the following:

    If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
    reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
    GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
    should fail with an exit reason of "VM-entry failure due to invalid
    guest state" (33).

    If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
    reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
    HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
    should fail with a VM-instruction error of "VM entry with invalid
    host-state field(s)" (8).

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 x86/vmx_tests.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 185 insertions(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 94be937da41d..ac734fea7f3d 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -6837,6 +6837,189 @@ static void test_host_efer(void)
 	test_efer(HOST_EFER, "HOST_EFER", EXI_CONTROLS, EXI_LOAD_EFER);
 }
 
+union cpuid10_eax {
+	struct {
+		unsigned int version_id:8;
+		unsigned int num_counters:8;
+		unsigned int bit_width:8;
+		unsigned int mask_length:8;
+	} split;
+	unsigned int full;
+};
+
+union cpuid10_edx {
+	struct {
+		unsigned int num_counters_fixed:5;
+		unsigned int bit_width_fixed:8;
+		unsigned int reserved:19;
+	} split;
+	unsigned int full;
+};
+
+static bool valid_pgc(u64 val)
+{
+	struct cpuid id;
+	union cpuid10_eax eax;
+	union cpuid10_edx edx;
+	u64 mask;
+
+	id = cpuid(0xA);
+	eax.full = id.a;
+	edx.full = id.d;
+	mask = ~(((1ull << eax.split.num_counters) - 1) |
+		(((1ull << edx.split.num_counters_fixed) - 1) << 32));
+
+	return !(val & mask);
+}
+
+static void test_pgc_vmlaunch(u32 xerror, bool xfail, bool host)
+{
+	u32 inst_err;
+	u64 guest_rip, inst_len;
+	bool success;
+
+	if (host) {
+		success = vmlaunch_succeeds();
+	} else {
+		if (xfail)
+			enter_guest_with_invalid_guest_state();
+		else
+			enter_guest();
+		success = VMX_VMCALL == (vmcs_read(EXI_REASON) & 0xff);
+		guest_rip = vmcs_read(GUEST_RIP);
+		inst_len = vmcs_read(EXI_INST_LEN);
+		if (success)
+			vmcs_write(GUEST_RIP, guest_rip + inst_len);
+	}
+	if (!success) {
+		inst_err = vmcs_read(VMX_INST_ERROR);
+		report("vmlaunch failed, VMX Inst Error is %d (expected %d)",
+		       xerror == inst_err, inst_err, xerror);
+	} else {
+		report("vmlaunch succeeded", success != xfail);
+	}
+}
+
+/*
+ * test_load_pgc is a generic function for testing the
+ * "load IA32_PERF_GLOBAL_CTRL" VM-{entry,exit} control. This test function
+ * will test the provided ctrl_val disabled and enabled.
+ *
+ * @nr - VMCS field number corresponding to the Host/Guest state field
+ * @name - Name of the above VMCS field for printing in test report
+ * @ctrl_nr - VMCS field number corresponding to the VM-{entry,exit} control
+ * @ctrl_val - Bit to set on the ctrl field.
+ */
+static void test_load_pgc(u32 nr, const char * name, u32 ctrl_nr,
+			  const char * ctrl_name, u64 ctrl_val)
+{
+	u64 ctrl_saved = vmcs_read(ctrl_nr);
+	u64 pgc_saved = vmcs_read(nr);
+	u64 i, val;
+	bool host = nr == HOST_PERF_GLOBAL_CTRL;
+
+	if (!host) {
+		vmx_set_test_stage(1);
+		test_set_guest(guest_state_test_main);
+	}
+	vmcs_write(ctrl_nr, ctrl_saved & ~ctrl_val);
+	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=0 on %s",
+			    ctrl_name);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		vmcs_write(nr, val);
+		report_prefix_pushf("%s = 0x%lx", name, val);
+		/*
+		 * If the "load IA32_PERF_GLOBAL_CTRL" bit is 0 then
+		 * the {HOST,GUEST}_IA32_PERF_GLOBAL_CTRL field is ignored,
+		 * thus setting reserved bits in this field does not cause
+		 * vmlaunch to fail.
+		 */
+		test_pgc_vmlaunch(0, false, host);
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	vmcs_write(ctrl_nr, ctrl_saved | ctrl_val);
+	report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\"=1 on %s",
+			    ctrl_name);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		vmcs_write(nr, val);
+		report_prefix_pushf("%s = 0x%lx", name, val);
+		if (valid_pgc(val)) {
+			test_pgc_vmlaunch(0, false, host);
+		} else {
+			/*
+			 * [SDM 30.4]
+			 *
+			 * Invalid host state fields result in an VM
+			 * instruction error with error number 8
+			 * (VMXERR_ENTRY_INVALID_HOST_STATE_FIELD)
+			 */
+			if (nr == HOST_PERF_GLOBAL_CTRL) {
+				test_pgc_vmlaunch(
+					VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
+					true, host);
+			/*
+			 * [SDM 26.1]
+			 *
+			 * If a VM-Entry fails according to one of
+			 * the guest-state checks, the exit reason on the VMCS
+			 * will be set to reason number 33 (VMX_FAIL_STATE)
+			 */
+			} else {
+				test_pgc_vmlaunch(
+					0,
+					true, host);
+				TEST_ASSERT_EQ(
+					VMX_ENTRY_FAILURE | VMX_FAIL_STATE,
+					vmcs_read(EXI_REASON));
+			}
+		}
+		report_prefix_pop();
+	}
+
+	report_prefix_pop();
+
+	if (nr == GUEST_PERF_GLOBAL_CTRL) {
+		/*
+		 * Let the guest finish execution
+		 */
+		vmx_set_test_stage(2);
+		vmcs_write(ctrl_nr, ctrl_saved);
+		vmcs_write(nr, pgc_saved);
+		enter_guest();
+	}
+
+	vmcs_write(ctrl_nr, ctrl_saved);
+	vmcs_write(nr, pgc_saved);
+}
+
+static void test_host_load_pgc(void)
+{
+	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
+		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		       "exit control not supported\n");
+		return;
+	}
+
+	test_load_pgc(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
+		      EXI_CONTROLS, "EXI_CONTROLS", EXI_LOAD_PERF);
+}
+
+
+static void test_guest_load_pgc(void)
+{
+	if (!(ctrl_enter_rev.clr & ENT_LOAD_PERF)) {
+		printf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		       "entry control not supported\n");
+	}
+
+	test_load_pgc(GUEST_PERF_GLOBAL_CTRL, "GUEST_PERF_GLOBAL_CTRL",
+		      ENT_CONTROLS, "ENT_CONTROLS", ENT_LOAD_PERF);
+}
+
 /*
  * PAT values higher than 8 are uninteresting since they're likely lumped
  * in with "8". We only test values above 8 one bit at a time,
@@ -7128,6 +7311,7 @@ static void vmx_host_state_area_test(void)
 	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
 
 	test_host_efer();
+	test_host_load_pgc();
 	test_load_host_pat();
 	test_host_segment_regs();
 	test_host_desc_tables();
@@ -8564,7 +8748,6 @@ static int invalid_msr_entry_failure(struct vmentry_failure *failure)
 	return VMX_TEST_VMEXIT;
 }
 
-
 #define TEST(name) { #name, .v2 = name }
 
 /* name/init/guest_main/exit_handler/syscall_handler/guest_regs */
@@ -8614,6 +8797,7 @@ struct vmx_test vmx_tests[] = {
 	TEST(vmx_host_state_area_test),
 	TEST(vmx_guest_state_area_test),
 	TEST(vmentry_movss_shadow_test),
+	TEST(test_guest_load_pgc),
 	/* APICv tests */
 	TEST(vmx_eoi_bitmap_ioapic_scan_test),
 	TEST(vmx_hlt_with_rvi_test),
-- 
2.23.0.187.g17f5b7556c-goog

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Jim Mattson]

On Wed, Aug 28, 2019 at 4:41 PM Oliver Upton <oupton@google.com> wrote:
>
> Tests to verify that KVM performs the correct checks on Host/Guest state
> at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
> Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
> Control Registers and MSRs".
>
> Test that KVM does the following:
>
>     If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
>     reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>     GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>     should fail with an exit reason of "VM-entry failure due to invalid
>     guest state" (33).
>
>     If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
>     reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
>     HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
>     should fail with a VM-instruction error of "VM entry with invalid
>     host-state field(s)" (8).

Could you add a simple functionality test as well? That is, after a
successful nested VM-entry, the L2 guest should be able to observe the
GUEST_IA32_PERF_GLOBAL_CTRL value when it reads the
IA32_PERF_GLOBAL_CTRL MSR, and after a subsequent nested VM-exit, the
L1 guest should be able to observe the HOST_IA32_PERF_GLOBAL_CTRL
value when it reads the IA32_PERF_GLOBAL_CTRL MSR.

Re: [kvm-unit-tests PATCH 7/7] x86: VMX: Add tests for nested "load IA32_PERF_GLOBAL_CTRL"

[Oliver Upton]

On Thu, Aug 29, 2019 at 10:33:48AM -0700, Jim Mattson wrote:
> On Wed, Aug 28, 2019 at 4:41 PM Oliver Upton <oupton@google.com> wrote:
> >
> > Tests to verify that KVM performs the correct checks on Host/Guest state
> > at VM-entry, as described in SDM 26.3.1.1 "Checks on Guest Control
> > Registers, Debug Registers, and MSRs" and SDM 26.2.2 "Checks on Host
> > Control Registers and MSRs".
> >
> > Test that KVM does the following:
> >
> >     If the "load IA32_PERF_GLOBAL_CTRL" VM-entry control is 1, the
> >     reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> >     GUEST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
> >     should fail with an exit reason of "VM-entry failure due to invalid
> >     guest state" (33).
> >
> >     If the "load IA32_PERF_GLOBAL_CTRL" VM-exit control is 1, the
> >     reserved bits of the IA32_PERF_GLOBAL_CTRL MSR must be 0 in the
> >     HOST_IA32_PERF_GLOBAL_CTRL VMCS field. Otherwise, the VM-entry
> >     should fail with a VM-instruction error of "VM entry with invalid
> >     host-state field(s)" (8).
> 
> Could you add a simple functionality test as well? That is, after a
> successful nested VM-entry, the L2 guest should be able to observe the
> GUEST_IA32_PERF_GLOBAL_CTRL value when it reads the
> IA32_PERF_GLOBAL_CTRL MSR, and after a subsequent nested VM-exit, the
> L1 guest should be able to observe the HOST_IA32_PERF_GLOBAL_CTRL
> value when it reads the IA32_PERF_GLOBAL_CTRL MSR.

Definitely should. I think I can just add it as an additional check to
the common test code for host and guest tests, since we already have the
guest brought up and configured correctly anyway.

I have a couple style nits to correct in this patch as well. I'll add
this to the v2 I'm going to send out for the series.