[PATCH v2 0/3] KVM: nVMX: Do not expose APICv to L1 if disabled on L0

[PATCH v2 0/3] KVM: nVMX: Do not expose APICv to L1 if disabled on L0

[Arbel Moshe]

Hi,

This series aims to fix issue of exposing APICv features to L1 when
enable_apicv==false in L0.

The first patch is just a refactoring of existing code. It replaces updating of
VMX APICv related secondary exec controls, in generic secondary exec control
re-computation. This is done to avoid code-duplication which is error-prone.

The second patch makes sure that if APICv is disabled dynamically when Hyper-V
SynIC is enabled, then also make sure to not expose APICv features to L1 in VMX
MSRs.

The third patch fix a bug of exposing some APICv related features to L1 even
though APICv is disabled in L0.

Regards,
-Arbel Moshe

[PATCH v2 1/3] KVM: nVMX: Refresh APICv secondary exec controls by re-calculating all of them

[Arbel Moshe]

Handle apicv secondary exec ctrls correctly, without duplicating
logic.
This commit doesn't change semantics.

Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7c3522a989d0..84ccd3b2762c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5237,20 +5237,18 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
 	return pin_based_exec_ctrl;
 }
 
+static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx);
+
 static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
+
 	if (cpu_has_secondary_exec_ctrls()) {
-		if (kvm_vcpu_apicv_active(vcpu))
-			vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
-				      SECONDARY_EXEC_APIC_REGISTER_VIRT |
-				      SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
-		else
-			vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
-					SECONDARY_EXEC_APIC_REGISTER_VIRT |
-					SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
+		vmx_compute_secondary_exec_control(vmx);
+		vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
+				vmx->secondary_exec_control);
 	}
 
 	if (cpu_has_vmx_msr_bitmap())
-- 
2.14.1

[PATCH v2 2/3] KVM: nVMX: Update nested MSRs in case APICv refreshing

[Arbel Moshe]

Fix bug of not updating nested MSRs regarding APICv, when refreshing
apicv exec ctrl.

Before this commit, enabling Hyper-V SynIC would disable APICv controls
in VMCS but still expose APICv controls to nested guest.

Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 84ccd3b2762c..0450fbdb97be 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5253,6 +5253,8 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 
 	if (cpu_has_vmx_msr_bitmap())
 		vmx_set_msr_bitmap(vcpu);
+
+	nested_vmx_setup_ctls_msrs(vmx);
 }
 
 static u32 vmx_exec_control(struct vcpu_vmx *vmx)
-- 
2.14.1

[PATCH v2 3/3] KVM: nVMX: Fix nested APICv Secondary CPU Controls when apicv disabled

[Arbel Moshe]

Implementation of virtual APICv relies on L0 being able to use APICv.
Therefore, if enable_apicv==false, we should not expose APICv to L1.

This commit makes sure to not expose APICv Secondary CPU controls
to L1 when enable_apicv==false.

Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 0450fbdb97be..a2f157e9c33c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2811,10 +2811,14 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 		SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
 		SECONDARY_EXEC_DESC |
 		SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
-		SECONDARY_EXEC_APIC_REGISTER_VIRT |
-		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
 		SECONDARY_EXEC_WBINVD_EXITING;
 
+	if (kvm_vcpu_apicv_active(&vmx->vcpu)) {
+		vmx->nested.nested_vmx_secondary_ctls_high |=
+			(SECONDARY_EXEC_APIC_REGISTER_VIRT |
+			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
+	}
+
 	if (enable_ept) {
 		/* nested EPT: emulate EPT also to L1 */
 		vmx->nested.nested_vmx_secondary_ctls_high |=
-- 
2.14.1

Re: [PATCH v2 1/3] KVM: nVMX: Refresh APICv secondary exec controls by re-calculating all of them

[Jim Mattson]

Does this change play well with the other functions that toggle
secondary control bits without updating vmx->secondary_exec_control?
e.g. vmx_disable_shadow_vmcs, set_current_vmptr,
vmx_set_virtual_x2apic_mode, vmcs_set_secondary_exec_control, and
possibly others?

Perhaps the proposed call to vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
vmx->secondary_exec_control) should instead be a call to
vmcs_set_secondary_exec_control(vmx->secondary_exec_control)?

On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@oracle.com> wrote:
> Handle apicv secondary exec ctrls correctly, without duplicating
> logic.
> This commit doesn't change semantics.
>
> Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7c3522a989d0..84ccd3b2762c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5237,20 +5237,18 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
>         return pin_based_exec_ctrl;
>  }
>
> +static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx);
> +
>  static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  {
>         struct vcpu_vmx *vmx = to_vmx(vcpu);
>
>         vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
> +
>         if (cpu_has_secondary_exec_ctrls()) {
> -               if (kvm_vcpu_apicv_active(vcpu))
> -                       vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> -                                     SECONDARY_EXEC_APIC_REGISTER_VIRT |
> -                                     SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> -               else
> -                       vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> -                                       SECONDARY_EXEC_APIC_REGISTER_VIRT |
> -                                       SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +               vmx_compute_secondary_exec_control(vmx);
> +               vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> +                               vmx->secondary_exec_control);
>         }
>
>         if (cpu_has_vmx_msr_bitmap())
> --
> 2.14.1
>

Re: [PATCH v2 3/3] KVM: nVMX: Fix nested APICv Secondary CPU Controls when apicv disabled

[Jim Mattson]

I'm not convinced that the test, "if
(kvm_vcpu_apicv_active(&vmx->vcpu))," is entirely correct. This is the
same as "if (vcpu->arch.apic && vcpu->arch.apicv_active)." I don't
believe that L1 has to have lapic_in_kernel() for L0 to use the APICv
features of the hardware when running L2. I'm also not sure that
Hyper-V SynIC activation for L1 has any bearing on whether or not L0
can use the APICv features of the hardware when running L2.

Should this test be "if (enable_apicv)" instead? Even that gives me
pause, since it means that L1 cannot use any of the three features
{"process posted interrupts," "APIC-register virtualization,"
"virtual-interrupt delivery"} unless it can use all three of them.
Didn't some Ivy Bridge SKUs implement only two of them? (Of course, it
doesn't matter if you just want to run kvm as the L1 hypervisor, but I
hope we're getting past that. :-)

I think the code should actually implement the following:

1. Set vmx->nested.nested_vmx_secondary_ctls_high based on the
kvm-implemented capabilities reported by MSR_IA32_VMX_PROCBASED_CTLS2.
2. Clear the capability bits for "APIC-register virtualization" and
"virtual-interrupt delivery" if the module parameter, enable_apicv, is
zero.

Doing (2) means being able to access the actual module parameter,
irrespective of the clearing done by hardware_setup if the trio of
capabilities referenced above aren't all present.

On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@oracle.com> wrote:
> Implementation of virtual APICv relies on L0 being able to use APICv.
> Therefore, if enable_apicv==false, we should not expose APICv to L1.
>
> This commit makes sure to not expose APICv Secondary CPU controls
> to L1 when enable_apicv==false.
>
> Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 0450fbdb97be..a2f157e9c33c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2811,10 +2811,14 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>                 SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
>                 SECONDARY_EXEC_DESC |
>                 SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
> -               SECONDARY_EXEC_APIC_REGISTER_VIRT |
> -               SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>                 SECONDARY_EXEC_WBINVD_EXITING;
>
> +       if (kvm_vcpu_apicv_active(&vmx->vcpu)) {
> +               vmx->nested.nested_vmx_secondary_ctls_high |=
> +                       (SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +                       SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> +       }
> +
>         if (enable_ept) {
>                 /* nested EPT: emulate EPT also to L1 */
>                 vmx->nested.nested_vmx_secondary_ctls_high |=
> --
> 2.14.1
>

Re: [PATCH v2 2/3] KVM: nVMX: Update nested MSRs in case APICv refreshing

[Jim Mattson]

I believe this change is incorrect, for a couple of reasons.
(1) The VMX capability MSRs are static. Once they have been observed
by L1, they cannot be changed. Doing so would be a violation of the
architectural specification. IIUC, vmx_refresh_apicv_exec_ctrl() may
be executed after L1 has observed the values of the VMX capability
MSRs, because there are no restrictions on when userspace can make the
KVM_ENABLE_CAP ioctl.
(2) I don't believe that enabling Hyper-V SynIC support for L1
precludes the use of the advanced APIC virtualization features for the
execution of L2.

On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@oracle.com> wrote:
> Fix bug of not updating nested MSRs regarding APICv, when refreshing
> apicv exec ctrl.
>
> Before this commit, enabling Hyper-V SynIC would disable APICv controls
> in VMCS but still expose APICv controls to nested guest.
>
> Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 84ccd3b2762c..0450fbdb97be 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5253,6 +5253,8 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>
>         if (cpu_has_vmx_msr_bitmap())
>                 vmx_set_msr_bitmap(vcpu);
> +
> +       nested_vmx_setup_ctls_msrs(vmx);
>  }
>
>  static u32 vmx_exec_control(struct vcpu_vmx *vmx)
> --
> 2.14.1
>

Re: [PATCH v2 1/3] KVM: nVMX: Refresh APICv secondary exec controls by re-calculating all of them

[Jim Mattson]

I don't actually condone the current situation. I'd really like to see
a changeset that makes vmx->secondary_exec_control an actual cache of
the secondary processor-based VM-execution controls from the vmcs01.

On Wed, Nov 22, 2017 at 9:17 AM, Jim Mattson <jmattson@google.com> wrote:
> Does this change play well with the other functions that toggle
> secondary control bits without updating vmx->secondary_exec_control?
> e.g. vmx_disable_shadow_vmcs, set_current_vmptr,
> vmx_set_virtual_x2apic_mode, vmcs_set_secondary_exec_control, and
> possibly others?
>
> Perhaps the proposed call to vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> vmx->secondary_exec_control) should instead be a call to
> vmcs_set_secondary_exec_control(vmx->secondary_exec_control)?
>
> On Wed, Nov 22, 2017 at 2:23 AM, Arbel Moshe <arbel.moshe@oracle.com> wrote:
>> Handle apicv secondary exec ctrls correctly, without duplicating
>> logic.
>> This commit doesn't change semantics.
>>
>> Signed-off-by: Arbel Moshe <arbel.moshe@oracle.com>
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>  arch/x86/kvm/vmx.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 7c3522a989d0..84ccd3b2762c 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5237,20 +5237,18 @@ static u32 vmx_pin_based_exec_ctrl(struct vcpu_vmx *vmx)
>>         return pin_based_exec_ctrl;
>>  }
>>
>> +static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx);
>> +
>>  static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>>  {
>>         struct vcpu_vmx *vmx = to_vmx(vcpu);
>>
>>         vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
>> +
>>         if (cpu_has_secondary_exec_ctrls()) {
>> -               if (kvm_vcpu_apicv_active(vcpu))
>> -                       vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
>> -                                     SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> -                                     SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>> -               else
>> -                       vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
>> -                                       SECONDARY_EXEC_APIC_REGISTER_VIRT |
>> -                                       SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
>> +               vmx_compute_secondary_exec_control(vmx);
>> +               vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
>> +                               vmx->secondary_exec_control);
>>         }
>>
>>         if (cpu_has_vmx_msr_bitmap())
>> --
>> 2.14.1
>>

Re: [PATCH v2 1/3] KVM: nVMX: Refresh APICv secondary exec controls by re-calculating all of them

[Paolo Bonzini]

On 22/11/2017 18:17, Jim Mattson wrote:
> Does this change play well with the other functions that toggle
> secondary control bits without updating vmx->secondary_exec_control?
> e.g. vmx_disable_shadow_vmcs, set_current_vmptr,
> vmx_set_virtual_x2apic_mode, vmcs_set_secondary_exec_control, and
> possibly others?
> 
> Perhaps the proposed call to vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
> vmx->secondary_exec_control) should instead be a call to
> vmcs_set_secondary_exec_control(vmx->secondary_exec_control)?

Yes, this is correct.

Paolo

Re: [PATCH v2 2/3] KVM: nVMX: Update nested MSRs in case APICv refreshing

[Paolo Bonzini]

On 22/11/2017 19:04, Jim Mattson wrote:
> I believe this change is incorrect, for a couple of reasons.
> (1) The VMX capability MSRs are static. Once they have been observed
> by L1, they cannot be changed. Doing so would be a violation of the
> architectural specification. IIUC, vmx_refresh_apicv_exec_ctrl() may
> be executed after L1 has observed the values of the VMX capability
> MSRs, because there are no restrictions on when userspace can make the
> KVM_ENABLE_CAP ioctl.

This is technically true, but I think we can say comfortably that it
makes little sense and userspace Should Not Have Done It.

> (2) I don't believe that enabling Hyper-V SynIC support for L1
> precludes the use of the advanced APIC virtualization features for the
> execution of L2.

This is true.

Paolo

Re: [PATCH v2 3/3] KVM: nVMX: Fix nested APICv Secondary CPU Controls when apicv disabled

[Paolo Bonzini]

On 22/11/2017 18:56, Jim Mattson wrote:
> I don't
> believe that L1 has to have lapic_in_kernel() for L0 to use the APICv
> features of the hardware when running L2.

Without lapic_in_kernel() the guest doesn't have the X2APIC CPUID bit
and x2APIC MSRs (at least on upstream KVM, don't know if Google's
userspace MSR patches can do it).

Therefore it makes no sense to allow the "virtualize APIC accesses"
control for L1, as the control implies the availability of the MSRs.

> I'm also not sure that
> Hyper-V SynIC activation for L1 has any bearing on whether or not L0
> can use the APICv features of the hardware when running L2.

I agree with this.

Paolo

Re: [PATCH v2 3/3] KVM: nVMX: Fix nested APICv Secondary CPU Controls when apicv disabled

[Jim Mattson]

On Thu, Nov 23, 2017 at 3:57 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 22/11/2017 18:56, Jim Mattson wrote:
>> I don't
>> believe that L1 has to have lapic_in_kernel() for L0 to use the APICv
>> features of the hardware when running L2.
>
> Without lapic_in_kernel() the guest doesn't have the X2APIC CPUID bit
> and x2APIC MSRs (at least on upstream KVM, don't know if Google's
> userspace MSR patches can do it).
>
> Therefore it makes no sense to allow the "virtualize APIC accesses"
> control for L1, as the control implies the availability of the MSRs.

The L1 guest doesn't have to have the x2APIC CPUID bit or the x2APIC
MSRs in order for it to present these features to L2 using "virtualize
x2APIC mode" and "APIC-register virtualization." Section 29.5 of the
SDM (volume 3) indicates that the local APIC need not be in x2APIC
mode for x2APIC virtualization to be effective.

>> I'm also not sure that
>> Hyper-V SynIC activation for L1 has any bearing on whether or not L0
>> can use the APICv features of the hardware when running L2.
>
> I agree with this.
>
> Paolo