| Message ID | 20190828234134.132704-2-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL | expand |
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...
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?
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 !
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?
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 !
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.]
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 */
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(-)