[4/8,nVMX] : Check "load IA32_PERF_GLOBAL_CTRL" VM-exit control on vmentry of nested guests
diff mbox series

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

Comments

Sean Christopherson May 13, 2019, 7 p.m. UTC | #1
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
>
Krish Sadhukhan May 16, 2019, 10:07 p.m. UTC | #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
>>
Sean Christopherson May 17, 2019, 8:34 p.m. UTC | #3
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).
Jim Mattson Aug. 15, 2019, 10:54 p.m. UTC | #4
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.

Patch
diff mbox series

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,