diff mbox

KVM: nVMX: Fix nested APICv Secondary CPU Controls when apicv disabled

Message ID 20171112163102.139877-1-arbel.moshe@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arbel Moshe Nov. 12, 2017, 4:31 p.m. UTC 
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(-)

Comments

Paolo Bonzini Nov. 13, 2017, 8:36 a.m. UTC | #1 
On 12/11/2017 17:31, Arbel Moshe wrote:
>  		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);
> +	}
> +

I think kvm_vcpu_apicv_active may change after
nested_vmx_setup_ctls_msrs is called.  You need to clear the bits in
refresh_apicv_exec_ctrl.

Thanks,

Paolo
Liran Alon Nov. 13, 2017, 8:51 a.m. UTC | #2 
On 13/11/17 10:36, Paolo Bonzini wrote:
> On 12/11/2017 17:31, Arbel Moshe wrote:
>>   		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);
>> +	}
>> +
>
> I think kvm_vcpu_apicv_active may change after
> nested_vmx_setup_ctls_msrs is called.  You need to clear the bits in
> refresh_apicv_exec_ctrl.

Agreed. Seems this is called from kvm_vcpu_deactivate_apicv() which is 
only called from kvm_hv_activate_synic() which enables Hyper-V SynIC.

However, in case Hyper-V SynIC is not enabled, QEMU will never issue 
ioctl that invokes kvm_vcpu_deactivate_apicv() and therefore 
refresh_apicv_exec_ctrl() won't be called.

Therefore, we suggest the following:
1. Keeping the code Arbel added to nested_vmx_setup_ctls_msrs().
2. Adding clearing of relevant bits also to refresh_apicv_exec_ctrl().
3. Fix bug of not also clearing PIN_BASED_POSTED_INTR from the VMCS & 
nested_vmx_pinbased_ctls_high in refresh_apicv_exec_ctrl().

Arbel will fix these in v2 of this series.

Thanks.
-Liran

>
> Thanks,
>
> Paolo
>
Paolo Bonzini Nov. 13, 2017, 10:59 a.m. UTC | #3 
On 13/11/2017 09:51, Liran Alon wrote:
> 
> 
> On 13/11/17 10:36, Paolo Bonzini wrote:
>> On 12/11/2017 17:31, Arbel Moshe wrote:
>>>           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);
>>> +    }
>>> +
>>
>> I think kvm_vcpu_apicv_active may change after
>> nested_vmx_setup_ctls_msrs is called.  You need to clear the bits in
>> refresh_apicv_exec_ctrl.
> 
> Agreed. Seems this is called from kvm_vcpu_deactivate_apicv() which is
> only called from kvm_hv_activate_synic() which enables Hyper-V SynIC.
> 
> However, in case Hyper-V SynIC is not enabled, QEMU will never issue
> ioctl that invokes kvm_vcpu_deactivate_apicv() and therefore
> refresh_apicv_exec_ctrl() won't be called.
> 
> Therefore, we suggest the following:
> 1. Keeping the code Arbel added to nested_vmx_setup_ctls_msrs().
> 2. Adding clearing of relevant bits also to refresh_apicv_exec_ctrl().
> 3. Fix bug of not also clearing PIN_BASED_POSTED_INTR from the VMCS &
> nested_vmx_pinbased_ctls_high in refresh_apicv_exec_ctrl().

Yes, I agree with all points.  Thanks,

Paolo

> Arbel will fix these in v2 of this series.
> 
> Thanks.
> -Liran
> 
>>
>> Thanks,
>>
>> Paolo
>>
diff mbox

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a6f4f095f8f4..7881533280da 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2809,10 +2809,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 |=