On Thu, Aug 22, 2019 at 10:29 PM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
> On 8/15/19 3:44 PM, Jim Mattson wrote:
> > On Wed, Apr 24, 2019 at 4:43 PM Krish Sadhukhan
> > <krish.sadhukhan@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> Suggested-by: Jim Mattson <jmattson@xxxxxxxxxx>
> >> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
> >> ---
> >> 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.
