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
>>
