[6/8,nVMX] : Load IA32_PERF_GLOBAL_CTRL MSR on vmentry of nested guests
diff mbox series

Message ID 20190424231724.2014-7-krish.sadhukhan@oracle.com
State New
Headers show
Series
  • [1/8,KVMnVMX] : Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests
Related show

Commit Message

Krish Sadhukhan April 24, 2019, 11:17 p.m. UTC
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(+)

Comments

Jim Mattson Aug. 15, 2019, 10:44 p.m. UTC | #1
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.
Krish Sadhukhan Aug. 21, 2019, 11:05 p.m. UTC | #2
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.
Jim Mattson Aug. 21, 2019, 11:10 p.m. UTC | #3
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.
Krish Sadhukhan Aug. 23, 2019, 5:29 a.m. UTC | #4
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 ?
Jim Mattson Aug. 23, 2019, 3:57 p.m. UTC | #5
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
diff mbox series

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