[KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested guests

[KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested guests

[Krish Sadhukhan]

Patch 1 through 7 add the necessary KVM code for checking and enabling
"load IA32_PERF_GLOBAL_CTRL" VM-{exit,entry} controls.

Patch# 8 adds a unit test for the "load IA32_PERF_GLOBAL_CTRL" VM-exit
control. I will send a separate patch for the unit test for
"load IA32_PERF_GLOBAL_CTRL" VM-entry control.


[PATCH 1/8][KVMnVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control
[PATCH 2/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry
[PATCH 3/8][KVM VMX]: Add a function to check reserved bits in
[PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control
[PATCH 5/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control
[PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested
[PATCH 7/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit}
[PATCH 8/8][KVM nVMX]: Test "load IA32_PERF_GLOBAL_CTRL" controls on vmentry

 arch/x86/include/asm/kvm_host.h  |  1 +
 arch/x86/include/asm/msr-index.h |  7 +++++++
 arch/x86/kvm/vmx/nested.c        | 19 +++++++++++++++++--
 arch/x86/kvm/vmx/vmx.c           | 12 ++++++++++++
 arch/x86/kvm/x86.c               | 20 ++++++++++++++++++++
 5 files changed, 57 insertions(+), 2 deletions(-)

Krish Sadhukhan (7):
      nVMX: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests
      nVMX: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry control for nested guests
      VMX: Add a function to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL
      nVMX: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests
      nVMX: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control on vmentry of nested guests
      nVMX: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests
      nVMX: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit} controls

 x86/vmx_tests.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

Krish Sadhukhan (1):
      nVMX: Test "load IA32_PERF_GLOBAL_CTRL" controls on vmentry of nested guests

[PATCH 1/8][KVMnVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3170e291215d..42a4deb662c6 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5493,7 +5493,8 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
 	msrs->exit_ctls_high |=
 		VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
 		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
-		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT;
+		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT |
+		VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
 
 	/* We support free control of debug control saving. */
 	msrs->exit_ctls_low &= ~VM_EXIT_SAVE_DEBUG_CONTROLS;
-- 
2.17.2

Re: [PATCH 1/8][KVMnVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:17PM -0400, Krish Sadhukhan wrote:
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 3170e291215d..42a4deb662c6 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5493,7 +5493,8 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
>  	msrs->exit_ctls_high |=
>  		VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>  		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
> -		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT;
> +		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT |
> +		VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;

This exposes the control to L1 but doesn't implement the backing
functionality.  The backing functionality, consistency checks and exposure
to L1 should be a single patch.  The consistency checks could be added
earlier, but I don't see much value in doing so given that the checks are
(currently) a few lines.

>  
>  	/* We support free control of debug control saving. */
>  	msrs->exit_ctls_low &= ~VM_EXIT_SAVE_DEBUG_CONTROLS;
> -- 
> 2.17.2
> 

Re: [PATCH 1/8][KVMnVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests

[Krish Sadhukhan]

On 05/13/2019 11:49 AM, Sean Christopherson wrote:
> On Wed, Apr 24, 2019 at 07:17:17PM -0400, Krish Sadhukhan wrote:
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   arch/x86/kvm/vmx/nested.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 3170e291215d..42a4deb662c6 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -5493,7 +5493,8 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
>>   	msrs->exit_ctls_high |=
>>   		VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>>   		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
>> -		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT;
>> +		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT |
>> +		VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
> This exposes the control to L1 but doesn't implement the backing
> functionality.  The backing functionality, consistency checks and exposure
> to L1 should be a single patch.  The consistency checks could be added
> earlier, but I don't see much value in doing so given that the checks are
> (currently) a few lines.

I will combine the exposure of the control and its backing functionality 
into a single patch. But I would prefer to keep the consistency checks 
in separate patches just to make it a gradual progression i.e., first 
enabling the controls in a  patch and then checking their consistency in 
the successive patch.

>
>>   
>>   	/* We support free control of debug control saving. */
>>   	msrs->exit_ctls_low &= ~VM_EXIT_SAVE_DEBUG_CONTROLS;
>> -- 
>> 2.17.2
>>

[PATCH 2/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry control for nested guests

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 42a4deb662c6..83cd887638cb 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5511,7 +5511,8 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
 #endif
 		VM_ENTRY_LOAD_IA32_PAT;
 	msrs->entry_ctls_high |=
-		(VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | VM_ENTRY_LOAD_IA32_EFER);
+		(VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | VM_ENTRY_LOAD_IA32_EFER |
+		VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL);
 
 	/* We support free control of debug control loading. */
 	msrs->entry_ctls_low &= ~VM_ENTRY_LOAD_DEBUG_CONTROLS;
-- 
2.17.2

Re: [PATCH 2/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry control for nested guests

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:18PM -0400, Krish Sadhukhan wrote:
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 42a4deb662c6..83cd887638cb 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5511,7 +5511,8 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps,
>  #endif
>  		VM_ENTRY_LOAD_IA32_PAT;
>  	msrs->entry_ctls_high |=
> -		(VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | VM_ENTRY_LOAD_IA32_EFER);
> +		(VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | VM_ENTRY_LOAD_IA32_EFER |
> +		VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL);

Same comments as the VM-Exit patch.

>  
>  	/* We support free control of debug control loading. */
>  	msrs->entry_ctls_low &= ~VM_ENTRY_LOAD_DEBUG_CONTROLS;
> -- 
> 2.17.2
> 

[PATCH 3/8][KVM VMX]: Add a function to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/include/asm/kvm_host.h  |  1 +
 arch/x86/include/asm/msr-index.h |  7 +++++++
 arch/x86/kvm/x86.c               | 20 ++++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4660ce90de7f..c5b3c63129a6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1311,6 +1311,7 @@ int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
 
 void kvm_enable_efer_bits(u64);
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
+bool kvm_valid_perf_global_ctrl(u64 perf_global);
 int kvm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
 int kvm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
 
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 8e40c2446fd1..d10e8d4b2842 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -775,6 +775,13 @@
 #define MSR_CORE_PERF_GLOBAL_CTRL	0x0000038f
 #define MSR_CORE_PERF_GLOBAL_OVF_CTRL	0x00000390
 
+/* MSR_CORE_PERF_GLOBAL_CTRL bits */
+#define	PERF_GLOBAL_CTRL_PMC0_ENABLE	(1ull << 0)
+#define	PERF_GLOBAL_CTRL_PMC1_ENABLE	(1ull << 1)
+#define	PERF_GLOBAL_CTRL_FIXED0_ENABLE	(1ull << 32)
+#define	PERF_GLOBAL_CTRL_FIXED1_ENABLE	(1ull << 33)
+#define	PERF_GLOBAL_CTRL_FIXED2_ENABLE	(1ull << 34)
+
 /* Geode defined MSRs */
 #define MSR_GEODE_BUSCONT_CONF0		0x00001900
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 02c8e095a239..ecddb8baaa7f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -89,8 +89,19 @@ EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
 #ifdef CONFIG_X86_64
 static
 u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
+static
+u64 __read_mostly perf_global_ctrl_reserved_bits =
+				~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE	|
+					PERF_GLOBAL_CTRL_PMC1_ENABLE	|
+					PERF_GLOBAL_CTRL_FIXED0_ENABLE	|
+					PERF_GLOBAL_CTRL_FIXED1_ENABLE	|
+					PERF_GLOBAL_CTRL_FIXED2_ENABLE));
 #else
 static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
+static
+u64 __read_mostly perf_global_ctrl_reserved_bits =
+				~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE	|
+					PERF_GLOBAL_CTRL_PMC1_ENABLE));
 #endif
 
 #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
@@ -1255,6 +1266,15 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 	return 0;
 }
 
+bool kvm_valid_perf_global_ctrl(u64 perf_global)
+{
+	if (perf_global & perf_global_ctrl_reserved_bits)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(kvm_valid_perf_global_ctrl);
+
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
 	if (efer & efer_reserved_bits)
-- 
2.17.2

Re: [PATCH 3/8][KVM VMX]: Add a function to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:19PM -0400, Krish Sadhukhan wrote:
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/include/asm/kvm_host.h  |  1 +
>  arch/x86/include/asm/msr-index.h |  7 +++++++
>  arch/x86/kvm/x86.c               | 20 ++++++++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4660ce90de7f..c5b3c63129a6 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1311,6 +1311,7 @@ int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
>  
>  void kvm_enable_efer_bits(u64);
>  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
> +bool kvm_valid_perf_global_ctrl(u64 perf_global);
>  int kvm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
>  int kvm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
>  
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 8e40c2446fd1..d10e8d4b2842 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -775,6 +775,13 @@
>  #define MSR_CORE_PERF_GLOBAL_CTRL	0x0000038f
>  #define MSR_CORE_PERF_GLOBAL_OVF_CTRL	0x00000390
>  
> +/* MSR_CORE_PERF_GLOBAL_CTRL bits */
> +#define	PERF_GLOBAL_CTRL_PMC0_ENABLE	(1ull << 0)

BIT and BIT_ULL

> +#define	PERF_GLOBAL_CTRL_PMC1_ENABLE	(1ull << 1)
> +#define	PERF_GLOBAL_CTRL_FIXED0_ENABLE	(1ull << 32)
> +#define	PERF_GLOBAL_CTRL_FIXED1_ENABLE	(1ull << 33)
> +#define	PERF_GLOBAL_CTRL_FIXED2_ENABLE	(1ull << 34)
> +
>  /* Geode defined MSRs */
>  #define MSR_GEODE_BUSCONT_CONF0		0x00001900
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 02c8e095a239..ecddb8baaa7f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -89,8 +89,19 @@ EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
>  #ifdef CONFIG_X86_64
>  static
>  u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
> +static
> +u64 __read_mostly perf_global_ctrl_reserved_bits =
> +				~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE	|
> +					PERF_GLOBAL_CTRL_PMC1_ENABLE	|
> +					PERF_GLOBAL_CTRL_FIXED0_ENABLE	|
> +					PERF_GLOBAL_CTRL_FIXED1_ENABLE	|
> +					PERF_GLOBAL_CTRL_FIXED2_ENABLE));
>  #else
>  static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
> +static

Why is static on a different line?

> +u64 __read_mostly perf_global_ctrl_reserved_bits =
> +				~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE	|
> +					PERF_GLOBAL_CTRL_PMC1_ENABLE));

Why are the fixed bits reserved on a 32-bit build?

>  #endif
>  
>  #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
> @@ -1255,6 +1266,15 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>  	return 0;
>  }
>  
> +bool kvm_valid_perf_global_ctrl(u64 perf_global)
> +{
> +	if (perf_global & perf_global_ctrl_reserved_bits)

If the check were correct, this could be:

	return !(perf_blobal & perf_global_ctrl_reserved_bits);

But the check isn't correct, the number of counters is variable, i.e. the
helper should query the guest's CPUID 0xA (ignoring for the moment the
fact that this bypasses the PMU handling of guest vs. host ownership).

> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(kvm_valid_perf_global_ctrl);
> +
>  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
>  	if (efer & efer_reserved_bits)
> -- 
> 2.17.2
> 

Re: [PATCH 3/8][KVM VMX]: Add a function to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL

[Jim Mattson]

On Mon, May 13, 2019 at 11:57 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Apr 24, 2019 at 07:17:19PM -0400, Krish Sadhukhan wrote:
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h  |  1 +
> >  arch/x86/include/asm/msr-index.h |  7 +++++++
> >  arch/x86/kvm/x86.c               | 20 ++++++++++++++++++++
> >  3 files changed, 28 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 4660ce90de7f..c5b3c63129a6 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1311,6 +1311,7 @@ int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> >
> >  void kvm_enable_efer_bits(u64);
> >  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
> > +bool kvm_valid_perf_global_ctrl(u64 perf_global);
> >  int kvm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
> >  int kvm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr);
> >
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index 8e40c2446fd1..d10e8d4b2842 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -775,6 +775,13 @@
> >  #define MSR_CORE_PERF_GLOBAL_CTRL    0x0000038f
> >  #define MSR_CORE_PERF_GLOBAL_OVF_CTRL        0x00000390
> >
> > +/* MSR_CORE_PERF_GLOBAL_CTRL bits */
> > +#define      PERF_GLOBAL_CTRL_PMC0_ENABLE    (1ull << 0)
>
> BIT and BIT_ULL
>
> > +#define      PERF_GLOBAL_CTRL_PMC1_ENABLE    (1ull << 1)
> > +#define      PERF_GLOBAL_CTRL_FIXED0_ENABLE  (1ull << 32)
> > +#define      PERF_GLOBAL_CTRL_FIXED1_ENABLE  (1ull << 33)
> > +#define      PERF_GLOBAL_CTRL_FIXED2_ENABLE  (1ull << 34)
> > +
> >  /* Geode defined MSRs */
> >  #define MSR_GEODE_BUSCONT_CONF0              0x00001900
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 02c8e095a239..ecddb8baaa7f 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -89,8 +89,19 @@ EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
> >  #ifdef CONFIG_X86_64
> >  static
> >  u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
> > +static
> > +u64 __read_mostly perf_global_ctrl_reserved_bits =
> > +                             ~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE    |
> > +                                     PERF_GLOBAL_CTRL_PMC1_ENABLE    |
> > +                                     PERF_GLOBAL_CTRL_FIXED0_ENABLE  |
> > +                                     PERF_GLOBAL_CTRL_FIXED1_ENABLE  |
> > +                                     PERF_GLOBAL_CTRL_FIXED2_ENABLE));
> >  #else
> >  static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
> > +static
>
> Why is static on a different line?
>
> > +u64 __read_mostly perf_global_ctrl_reserved_bits =
> > +                             ~((u64)(PERF_GLOBAL_CTRL_PMC0_ENABLE    |
> > +                                     PERF_GLOBAL_CTRL_PMC1_ENABLE));
>
> Why are the fixed bits reserved on a 32-bit build?
>
> >  #endif
> >
> >  #define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
> > @@ -1255,6 +1266,15 @@ static int do_get_msr_feature(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
> >       return 0;
> >  }
> >
> > +bool kvm_valid_perf_global_ctrl(u64 perf_global)
> > +{
> > +     if (perf_global & perf_global_ctrl_reserved_bits)
>
> If the check were correct, this could be:
>
>         return !(perf_blobal & perf_global_ctrl_reserved_bits);
>
> But the check isn't correct, the number of counters is variable, i.e. the
> helper should query the guest's CPUID 0xA (ignoring for the moment the
> fact that this bypasses the PMU handling of guest vs. host ownership).

Aren't the reserved bits already easily calculated as
~pmu->global_ctrl_mask? Well, there's also bit 48, potentially, I
guess, but I don't think kvm virtualizes it.

Once we have this concept, shouldn't intel_pmu_set_msr() return
non-zero if an attempt is made to set a reserved bit of this MSR?

> > +             return false;
> > +
> > +     return true;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_valid_perf_global_ctrl);
> > +
> >  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  {
> >       if (efer & efer_reserved_bits)
> > --
> > 2.17.2
> >

[PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Host Control Registers and MSRs" in Intel
SDM vol 3C, the following check is performed on vmentry of nested guests:

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

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.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 83cd887638cb..d2067370e288 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
 	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
 		return -EINVAL;
+
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	   !kvm_valid_perf_global_ctrl(vmcs12->host_ia32_perf_global_ctrl))
+		return -EINVAL;
+
 	/*
 	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
 	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
-- 
2.17.2

Re: [PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:20PM -0400, Krish Sadhukhan wrote:
> According to section "Checks on Host Control Registers and MSRs" in Intel
> SDM vol 3C, the following check is performed on vmentry of nested guests:
> 
>     "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."
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.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 83cd887638cb..d2067370e288 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>  	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
>  	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
>  		return -EINVAL;
> +
> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
> +	   !kvm_valid_perf_global_ctrl(vmcs12->host_ia32_perf_global_ctrl))

If vmcs12->host_ia32_perf_global_ctrl were ever actually consumed, this
needs to ensure L1 isn't able to take control of counters that are owned
by the host.

> +		return -EINVAL;
> +
>  	/*
>  	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
>  	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
> -- 
> 2.17.2
> 

Re: [PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests

[Krish Sadhukhan]

On 05/13/2019 12:00 PM, Sean Christopherson wrote:
> On Wed, Apr 24, 2019 at 07:17:20PM -0400, Krish Sadhukhan wrote:
>> According to section "Checks on Host Control Registers and MSRs" in Intel
>> SDM vol 3C, the following check is performed on vmentry of nested guests:
>>
>>      "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."
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.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 83cd887638cb..d2067370e288 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>>   	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
>>   	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
>>   		return -EINVAL;
>> +
>> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
>> +	   !kvm_valid_perf_global_ctrl(vmcs12->host_ia32_perf_global_ctrl))
> If vmcs12->host_ia32_perf_global_ctrl were ever actually consumed, this
> needs to ensure L1 isn't able to take control of counters that are owned
> by the host.

Sorry, I didn't understand your concern. Could you please explain how L1 
can control L0's counters ?

>
>> +		return -EINVAL;
>> +
>>   	/*
>>   	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
>>   	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
>> -- 
>> 2.17.2
>>

Re: [PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests

[Sean Christopherson]

On Thu, May 16, 2019 at 03:07:48PM -0700, Krish Sadhukhan wrote:
> 
> 
> On 05/13/2019 12:00 PM, Sean Christopherson wrote:
> >On Wed, Apr 24, 2019 at 07:17:20PM -0400, Krish Sadhukhan wrote:
> >>According to section "Checks on Host Control Registers and MSRs" in Intel
> >>SDM vol 3C, the following check is performed on vmentry of nested guests:
> >>
> >>     "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."
> >>
> >>Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >>Reviewed-by: Karl Heubaum <karl.heubaum@oracle.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 83cd887638cb..d2067370e288 100644
> >>--- a/arch/x86/kvm/vmx/nested.c
> >>+++ b/arch/x86/kvm/vmx/nested.c
> >>@@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> >>  	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> >>  	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
> >>  		return -EINVAL;
> >>+
> >>+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
> >>+	   !kvm_valid_perf_global_ctrl(vmcs12->host_ia32_perf_global_ctrl))
> >If vmcs12->host_ia32_perf_global_ctrl were ever actually consumed, this
> >needs to ensure L1 isn't able to take control of counters that are owned
> >by the host.
> 
> Sorry, I didn't understand your concern. Could you please explain how L1 can
> control L0's counters ?

MSR_CORE_PERF_GLOBAL_CTRL isn't virtualized in the sense that there is
only one MSR in hardware (per logical CPU).  Loading an arbitrary value
into hardware on a nested VM-Exit via vmcs12->host_ia32_perf_global_ctrl
means L1 could toggle counters on/off without L0's knowledge.  See
intel_guest_get_msrs() and intel_pmu_set_msr().

Except that your patches don't actually do anything functional with
vmcs12->host_ia32_perf_global_ctrl, hence my confusion over what you're
trying to accomplish.  If KVM advertises VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL
to L1, then L1 will think it can use the VMCS field to handle
MSR_CORE_PERF_GLOBAL_CTRL when running L2, e.g. instead of adding
MSR_CORE_PERF_GLOBAL_CTRL to the MSR load lists, which will break L1
(assuming nested PMU works today).

Re: [PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests

[Jim Mattson]

On Fri, May 17, 2019 at 1:34 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, May 16, 2019 at 03:07:48PM -0700, Krish Sadhukhan wrote:
> >
> >
> > On 05/13/2019 12:00 PM, Sean Christopherson wrote:
> > >On Wed, Apr 24, 2019 at 07:17:20PM -0400, Krish Sadhukhan wrote:
> > >>According to section "Checks on Host Control Registers and MSRs" in Intel
> > >>SDM vol 3C, the following check is performed on vmentry of nested guests:
> > >>
> > >>     "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."
> > >>
> > >>Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > >>Reviewed-by: Karl Heubaum <karl.heubaum@oracle.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 83cd887638cb..d2067370e288 100644
> > >>--- a/arch/x86/kvm/vmx/nested.c
> > >>+++ b/arch/x86/kvm/vmx/nested.c
> > >>@@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> > >>        !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> > >>        !nested_cr3_valid(vcpu, vmcs12->host_cr3))
> > >>            return -EINVAL;
> > >>+
> > >>+   if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL &&
> > >>+      !kvm_valid_perf_global_ctrl(vmcs12->host_ia32_perf_global_ctrl))
> > >If vmcs12->host_ia32_perf_global_ctrl were ever actually consumed, this
> > >needs to ensure L1 isn't able to take control of counters that are owned
> > >by the host.
> >
> > Sorry, I didn't understand your concern. Could you please explain how L1 can
> > control L0's counters ?
>
> MSR_CORE_PERF_GLOBAL_CTRL isn't virtualized in the sense that there is
> only one MSR in hardware (per logical CPU).  Loading an arbitrary value
> into hardware on a nested VM-Exit via vmcs12->host_ia32_perf_global_ctrl
> means L1 could toggle counters on/off without L0's knowledge.  See
> intel_guest_get_msrs() and intel_pmu_set_msr().
>
> Except that your patches don't actually do anything functional with
> vmcs12->host_ia32_perf_global_ctrl, hence my confusion over what you're
> trying to accomplish.  If KVM advertises VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL
> to L1, then L1 will think it can use the VMCS field to handle
> MSR_CORE_PERF_GLOBAL_CTRL when running L2, e.g. instead of adding
> MSR_CORE_PERF_GLOBAL_CTRL to the MSR load lists, which will break L1
> (assuming nested PMU works today).

I think nested PMU should work today if the vmcs12 VM-entry and
VM-exit MSR-load lists are used to swap IA32_PERF_GLOBAL_CTRL values
between L1 and L2. Note that atomic_switch_perf_msrs() is called on
every vmx_vcpu_run(), for vmcs02 as well as vmcs01.

This change set should aim to provide equivalent functionality with
the new(er) VM-entry and VM-exit controls for "load
IA32_PERF_GLOBAL_CTRL," so that L1 doesn't have to resort to the
MSR-load lists.

[PATCH 5/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control on vmentry of nested guests

[Krish Sadhukhan]

According to section "CHECKING AND LOADING GUEST STATE" in Intel SDM vol 3C,
the following checks are performed on vmentry of nested guests:

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

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d2067370e288..a7bf19eaa70b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2691,6 +2691,10 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
 		return 1;
 	}
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
+	    !kvm_valid_perf_global_ctrl(vmcs12->guest_ia32_perf_global_ctrl))
+		return 1;
+
 	/*
 	 * If the load IA32_EFER VM-entry control is 1, the following checks
 	 * are performed on the field for the IA32_EFER MSR:
-- 
2.17.2

Re: [PATCH 5/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control on vmentry of nested guests

[Jim Mattson]

On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> According to section "CHECKING AND LOADING GUEST STATE" in Intel SDM vol 3C,
> the following checks are performed on vmentry of nested guests:
>
>     "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."
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index d2067370e288..a7bf19eaa70b 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2691,6 +2691,10 @@ static int nested_vmx_check_vmentry_postreqs(struct kvm_vcpu *vcpu,
>                 return 1;
>         }
>
> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL &&
> +           !kvm_valid_perf_global_ctrl(vmcs12->guest_ia32_perf_global_ctrl))
> +               return 1;
> +

I'd rather see this built on an interface like:

bool kvm_valid_msr_value(u32 msr_index, u64 value);

But as long as we don't end up with a plethora of
kvm_valid_MSR_NAME(u64 value) functions, this looks fine.

Reviewed-by: Jim Mattson <jmattson@google.com>

[PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Krish Sadhukhan]

According to section "Loading Guest State" in Intel SDM vol 3C, the
IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:

    "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
     IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
     field."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index a7bf19eaa70b..8177374886a9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
 	vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
 
+	if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
+		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
+			     vmcs12->guest_ia32_perf_global_ctrl);
+
 	if (vmx->nested.nested_run_pending &&
 	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
 		vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
-- 
2.17.2

Re: [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Jim Mattson]

On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> According to section "Loading Guest State" in Intel SDM vol 3C, the
> IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:
>
>     "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
>      IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
>      field."
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index a7bf19eaa70b..8177374886a9 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>         vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
>         vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
>
> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
> +               vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> +                            vmcs12->guest_ia32_perf_global_ctrl);
> +
>         if (vmx->nested.nested_run_pending &&
>             (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
>                 vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
> --
> 2.17.2
>

This isn't quite right. The GUEST_IA32_PERF_GLOBAL_CTRL value is just
going to get overwritten later by atomic_switch_perf_msrs().

Instead of writing the vmcs12 value directly into the vmcs02, you
should call kvm_set_msr(), exactly as it would have been called if
MSR_CORE_PERF_GLOBAL_CTRL had been in the vmcs12
VM-entry MSR-load list. Then, atomic_switch_perf_msrs() will
automatically do the right thing.

Re: [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Krish Sadhukhan]

On 8/15/19 3:44 PM, Jim Mattson wrote:
> On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> According to section "Loading Guest State" in Intel SDM vol 3C, the
>> IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:
>>
>>      "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
>>       IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
>>       field."
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Suggested-by: Jim Mattson <jmattson@google.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   arch/x86/kvm/vmx/nested.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index a7bf19eaa70b..8177374886a9 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>>          vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
>>          vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
>>
>> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
>> +               vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
>> +                            vmcs12->guest_ia32_perf_global_ctrl);
>> +
>>          if (vmx->nested.nested_run_pending &&
>>              (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
>>                  vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
>> --
>> 2.17.2
>>
> This isn't quite right. The GUEST_IA32_PERF_GLOBAL_CTRL value is just
> going to get overwritten later by atomic_switch_perf_msrs().


atomic_switch_perf_msrs() gest called from vmx_vcpu_run() which I 
thought was executing before handle_vmlaunch() stuff that lead to 
prepare_vmcs02(). Did I miss something in the call-chain ?


> Instead of writing the vmcs12 value directly into the vmcs02, you
> should call kvm_set_msr(), exactly as it would have been called if
> MSR_CORE_PERF_GLOBAL_CTRL had been in the vmcs12
> VM-entry MSR-load list. Then, atomic_switch_perf_msrs() will
> automatically do the right thing.

Re: [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Jim Mattson]

On Wed, Aug 21, 2019 at 4:05 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 8/15/19 3:44 PM, Jim Mattson wrote:
> > On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >> According to section "Loading Guest State" in Intel SDM vol 3C, the
> >> IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:
> >>
> >>      "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
> >>       IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
> >>       field."
> >>
> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >> Suggested-by: Jim Mattson <jmattson@google.com>
> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> >> ---
> >>   arch/x86/kvm/vmx/nested.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >> index a7bf19eaa70b..8177374886a9 100644
> >> --- a/arch/x86/kvm/vmx/nested.c
> >> +++ b/arch/x86/kvm/vmx/nested.c
> >> @@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> >>          vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
> >>          vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
> >>
> >> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
> >> +               vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> >> +                            vmcs12->guest_ia32_perf_global_ctrl);
> >> +
> >>          if (vmx->nested.nested_run_pending &&
> >>              (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
> >>                  vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
> >> --
> >> 2.17.2
> >>
> > This isn't quite right. The GUEST_IA32_PERF_GLOBAL_CTRL value is just
> > going to get overwritten later by atomic_switch_perf_msrs().
>
>
> atomic_switch_perf_msrs() gest called from vmx_vcpu_run() which I
> thought was executing before handle_vmlaunch() stuff that lead to
> prepare_vmcs02(). Did I miss something in the call-chain ?

Handle_vmlaunch is called on a VM-exit from vmcs01 as the result of a
VMLAUNCH instruction. Atomic_switch_perf_msrs() is called on every
VM-entry (to either vmcs01 or vmcs02).

> > Instead of writing the vmcs12 value directly into the vmcs02, you
> > should call kvm_set_msr(), exactly as it would have been called if
> > MSR_CORE_PERF_GLOBAL_CTRL had been in the vmcs12
> > VM-entry MSR-load list. Then, atomic_switch_perf_msrs() will
> > automatically do the right thing.

Re: [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Krish Sadhukhan]

On 8/15/19 3:44 PM, Jim Mattson wrote:
> On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> According to section "Loading Guest State" in Intel SDM vol 3C, the
>> IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:
>>
>>      "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
>>       IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
>>       field."
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Suggested-by: Jim Mattson <jmattson@google.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   arch/x86/kvm/vmx/nested.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index a7bf19eaa70b..8177374886a9 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>>          vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
>>          vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
>>
>> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
>> +               vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
>> +                            vmcs12->guest_ia32_perf_global_ctrl);
>> +
>>          if (vmx->nested.nested_run_pending &&
>>              (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
>>                  vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
>> --
>> 2.17.2
>>
> This isn't quite right. The GUEST_IA32_PERF_GLOBAL_CTRL value is just
> going to get overwritten later by atomic_switch_perf_msrs().
>
> Instead of writing the vmcs12 value directly into the vmcs02, you
> should call kvm_set_msr(), exactly as it would have been called if
> MSR_CORE_PERF_GLOBAL_CTRL had been in the vmcs12
> VM-entry MSR-load list. Then, atomic_switch_perf_msrs() will
> automatically do the right thing.


I notice that the existing code for VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL 
in load_vmcs12_host_state() doesn't use kvm_set_msr():

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

This should also be changed to use kvm_set_msr() then ?

Re: [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests

[Jim Mattson]

On Thu, Aug 22, 2019 at 10:29 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 8/15/19 3:44 PM, Jim Mattson wrote:
> > On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >> According to section "Loading Guest State" in Intel SDM vol 3C, the
> >> IA32_PERF_GLOBAL_CTRL MSR is loaded on vmentry of nested guests:
> >>
> >>      "If the “load IA32_PERF_GLOBAL_CTRL” VM-entry control is 1, the
> >>       IA32_PERF_GLOBAL_CTRL MSR is loaded from the IA32_PERF_GLOBAL_CTRL
> >>       field."
> >>
> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >> Suggested-by: Jim Mattson <jmattson@google.com>
> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> >> ---
> >>   arch/x86/kvm/vmx/nested.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >> index a7bf19eaa70b..8177374886a9 100644
> >> --- a/arch/x86/kvm/vmx/nested.c
> >> +++ b/arch/x86/kvm/vmx/nested.c
> >> @@ -2300,6 +2300,10 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> >>          vcpu->arch.cr0_guest_owned_bits &= ~vmcs12->cr0_guest_host_mask;
> >>          vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
> >>
> >> +       if (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL)
> >> +               vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> >> +                            vmcs12->guest_ia32_perf_global_ctrl);
> >> +
> >>          if (vmx->nested.nested_run_pending &&
> >>              (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT)) {
> >>                  vmcs_write64(GUEST_IA32_PAT, vmcs12->guest_ia32_pat);
> >> --
> >> 2.17.2
> >>
> > This isn't quite right. The GUEST_IA32_PERF_GLOBAL_CTRL value is just
> > going to get overwritten later by atomic_switch_perf_msrs().
> >
> > Instead of writing the vmcs12 value directly into the vmcs02, you
> > should call kvm_set_msr(), exactly as it would have been called if
> > MSR_CORE_PERF_GLOBAL_CTRL had been in the vmcs12
> > VM-entry MSR-load list. Then, atomic_switch_perf_msrs() will
> > automatically do the right thing.
>
>
> I notice that the existing code for VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL
> in load_vmcs12_host_state() doesn't use kvm_set_msr():
>
>              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);
>
> This should also be changed to use kvm_set_msr() then ?

Yes. The VMWRITE here is incorrect, but fortunately it is also unreachable.

[PATCH 7/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit} controls

[Krish Sadhukhan]

 ...based on whether the guest CPU supports PMU

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Suggested-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4d39f731bc33..fa9c786afcfa 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6964,6 +6964,7 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
 static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	bool pmu_enabled = guest_cpuid_has_pmu(vcpu);
 
 	if (kvm_mpx_supported()) {
 		bool mpx_enabled = guest_cpuid_has(vcpu, X86_FEATURE_MPX);
@@ -6976,6 +6977,17 @@ static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
 			vmx->nested.msrs.exit_ctls_high &= ~VM_EXIT_CLEAR_BNDCFGS;
 		}
 	}
+	if (pmu_enabled) {
+		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;
+	}
 }
 
 static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
-- 
2.17.2

Re: [PATCH 7/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit} controls

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:23PM -0400, Krish Sadhukhan wrote:
>  ...based on whether the guest CPU supports PMU
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 4d39f731bc33..fa9c786afcfa 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6964,6 +6964,7 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
>  static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	bool pmu_enabled = guest_cpuid_has_pmu(vcpu);

A revert has been sent for the patch that added guest_cpuid_has_pmu().

Regardless, checking only the guest's CPUID 0xA is not sufficient, e.g.
at the bare minimum, exposing the controls can be done if and only if
cpu_has_load_perf_global_ctrl() is true.

In general, it's difficult for me to understand exactly what functionality
you intend to introduce.  Proper changelogs would be very helpful.

>  
>  	if (kvm_mpx_supported()) {
>  		bool mpx_enabled = guest_cpuid_has(vcpu, X86_FEATURE_MPX);
> @@ -6976,6 +6977,17 @@ static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
>  			vmx->nested.msrs.exit_ctls_high &= ~VM_EXIT_CLEAR_BNDCFGS;
>  		}
>  	}
> +	if (pmu_enabled) {
> +		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;
> +	}
>  }
>  
>  static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
> -- 
> 2.17.2
> 

Re: [PATCH 7/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit} controls

[Jim Mattson]

On Mon, May 13, 2019 at 12:12 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Apr 24, 2019 at 07:17:23PM -0400, Krish Sadhukhan wrote:
> >  ...based on whether the guest CPU supports PMU
> >
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 4d39f731bc33..fa9c786afcfa 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6964,6 +6964,7 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
> >  static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
> >  {
> >       struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +     bool pmu_enabled = guest_cpuid_has_pmu(vcpu);
>
> A revert has been sent for the patch that added guest_cpuid_has_pmu().
>
> Regardless, checking only the guest's CPUID 0xA is not sufficient, e.g.
> at the bare minimum, exposing the controls can be done if and only if
> cpu_has_load_perf_global_ctrl() is true.

I don't think cpu_has_load_perf_global_ctrl() is required. Support for
the VM-entry and VM-exit controls, "load IA32_PERF_GLOBAL_CTRL," can
be completely emulated by kvm, since add_atomic_switch_msr() is
capable of falling back on the VM-entry and VM-exit MSR-load lists if
!cpu_has_load_perf_global_ctrl().

The only requirement should be kvm_pmu_is_valid_msr(MSR_CORE_PERF_GLOBAL_CTRL).

> In general, it's difficult for me to understand exactly what functionality
> you intend to introduce.  Proper changelogs would be very helpful.

I concur!

[PATCH 8/8][KVM nVMX]: Test "load IA32_PERF_GLOBAL_CTRL" controls on vmentry of nested guests

[Krish Sadhukhan]

 ...to verify KVM performs the appropriate consistency checks for loading
IA32_PERF_GLOBAL_CTRL as part of running a nested guest.

According to section "Checks on Host Control Registers and MSRs" in Intel
SDM vol 3C, the following checks are performed on vmentry of nested guests:

    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.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 66a87f6..85fa8d5 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4995,6 +4995,63 @@ static void test_sysenter_field(u32 field, const char *name)
 	vmcs_write(field, addr_saved);
 }
 
+#define	PERF_GLOBAL_CTRL_VALID_BITS	0x0000000700000003
+
+static void test_perf_global_ctl(u32 field, const char * field_name,
+				 u32 ctrl_field, u64 ctrl_bit)
+{
+	u32 ctrl_saved = vmcs_read(ctrl_field);
+	u64 perf_global_saved = vmcs_read(field);
+	u64 i, val;
+
+	vmcs_write(ctrl_field, ctrl_saved & ~ctrl_bit);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		vmcs_write(field, val);
+		report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		    "VM-exit control is off, HOST_PERF_GLOBAL_CTRL %lx", val);
+		test_vmx_vmlaunch(0, false);
+		report_prefix_pop();
+	}
+
+	vmcs_write(ctrl_field, ctrl_saved | ctrl_bit);
+	for (i = 0; i < 64; i++) {
+		val = 1ull << i;
+		vmcs_write(field, val);
+		report_prefix_pushf("\"load IA32_PERF_GLOBAL_CTRL\" "
+		    "VM-exit control is on, HOST_PERF_GLOBAL_CTRL %lx", val);
+		if (PERF_GLOBAL_CTRL_VALID_BITS & (1ull << i)) {
+			test_vmx_vmlaunch(0, false);
+		} else {
+			test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
+					  false);
+		}
+		report_prefix_pop();
+	}
+
+	vmcs_write(ctrl_field, ctrl_saved);
+	vmcs_write(field, perf_global_saved);
+}
+
+/*
+ * 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.
+ *
+ *  [Intel SDM]
+ */
+static void test_host_perf_global_ctl(void)
+{
+	if (!(ctrl_exit_rev.clr & EXI_LOAD_PERF)) {
+		printf("\"load IA32_PERF_GLOBAL_CTRL\" VM-exit control not "
+		       "supported\n");
+		return;
+	}
+
+	test_perf_global_ctl(HOST_PERF_GLOBAL_CTRL, "HOST_PERF_GLOBAL_CTRL",
+			     EXI_CONTROLS, EXI_LOAD_PERF);
+}
+
 /*
  * Check that the virtual CPU checks the VMX Host State Area as
  * documented in the Intel SDM.
@@ -5010,6 +5067,8 @@ static void vmx_host_state_area_test(void)
 
 	test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP");
 	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
+
+	test_host_perf_global_ctl();
 }
 
 static bool valid_vmcs_for_vmentry(void)
-- 
2.17.2

Re: [KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" on vmentry of nested guests

[Sean Christopherson]

On Wed, Apr 24, 2019 at 07:17:16PM -0400, Krish Sadhukhan wrote:
> Patch 1 through 7 add the necessary KVM code for checking and enabling
> "load IA32_PERF_GLOBAL_CTRL" VM-{exit,entry} controls.
> 
> Patch# 8 adds a unit test for the "load IA32_PERF_GLOBAL_CTRL" VM-exit
> control. I will send a separate patch for the unit test for
> "load IA32_PERF_GLOBAL_CTRL" VM-entry control.
> 
> 
> [PATCH 1/8][KVMnVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control
> [PATCH 2/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry
> [PATCH 3/8][KVM VMX]: Add a function to check reserved bits in
> [PATCH 4/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control
> [PATCH 5/8][KVM nVMX]: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control
> [PATCH 6/8][KVM nVMX]: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested
> [PATCH 7/8][KVM nVMX]: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit}
> [PATCH 8/8][KVM nVMX]: Test "load IA32_PERF_GLOBAL_CTRL" controls on vmentry

These subjects are wrong, should be:

  [PATCH n/8] KVM: nVMX: Blah blah blah

or something along those lines.  Wrapping the scope in square braces causes
the scope to be dropped when the patch is applied.  They're also different
from the diffstat, which is odd.

And nearly all of the patches are missing proper changelogs.

> 
>  arch/x86/include/asm/kvm_host.h  |  1 +
>  arch/x86/include/asm/msr-index.h |  7 +++++++
>  arch/x86/kvm/vmx/nested.c        | 19 +++++++++++++++++--
>  arch/x86/kvm/vmx/vmx.c           | 12 ++++++++++++
>  arch/x86/kvm/x86.c               | 20 ++++++++++++++++++++
>  5 files changed, 57 insertions(+), 2 deletions(-)
> 
> Krish Sadhukhan (7):
>       nVMX: Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests
>       nVMX: Enable "load IA32_PERF_GLOBAL_CTRL" VM-entry control for nested guests
>       VMX: Add a function to check reserved bits in MSR_CORE_PERF_GLOBAL_CTRL
>       nVMX: Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests
>       nVMX: Check "load IA32_PERF_GLOBAL_CTRL" VM-entry control on vmentry of nested guests
>       nVMX: Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests
>       nVMX: Enable "load IA32_PERF_GLOBAL_CTRL VM-{entry,exit} controls
> 
>  x86/vmx_tests.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> Krish Sadhukhan (1):
>       nVMX: Test "load IA32_PERF_GLOBAL_CTRL" controls on vmentry of nested guests
>