| Message ID | 20190424231724.2014-5-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 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 >
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 >>
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).
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.
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,
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(+)