| Message ID | 20190424231724.2014-7-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8,KVMnVMX] : Enable "load IA32_PERF_GLOBAL_CTRL" VM-exit control for nested guests | expand |
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.
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.
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.
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 ?
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.
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);
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(+)