On 2017/7/28 12:22, Longpeng (Mike) wrote:
>
>
> On 2017/6/6 18:57, Paolo Bonzini wrote:
>
>> The simplify part: do not touch pi_desc.nv, we can set it when the
>> VCPU is first created. Likewise, pi_desc.sn is only handled by
>> vmx_vcpu_pi_load, do not touch it in __pi_post_block.
>>
>> The fix part: do not check kvm_arch_has_assigned_device, instead
>> check the SN bit to figure out whether vmx_vcpu_pi_put ran before.
>> This matches what the previous patch did in pi_post_block.
>>
>> Cc: Longpeng (Mike) <longpeng2@xxxxxxxxxx>
>> Cc: Huangweidong <weidong.huang@xxxxxxxxxx>
>> Cc: Gonglei <arei.gonglei@xxxxxxxxxx>
>> Cc: wangxin <wangxinxin.wang@xxxxxxxxxx>
>> Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx>
>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/vmx.c | 68 ++++++++++++++++++++++++++++--------------------------
>> 1 file changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 0f4714fe4908..81047f373747 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2184,43 +2184,41 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
>> struct pi_desc old, new;
>> unsigned int dest;
>>
>> - if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
>> - !irq_remapping_cap(IRQ_POSTING_CAP) ||
>> - !kvm_vcpu_apicv_active(vcpu))
>> + /*
>> + * In case of hot-plug or hot-unplug, we may have to undo
>> + * vmx_vcpu_pi_put even if there is no assigned device. And we
>> + * always keep PI.NDST up to date for simplicity: it makes the
>> + * code easier, and CPU migration is not a fast path.
>> + */
>> + if (!pi_test_sn(pi_desc) && vcpu->cpu == cpu)
>> + return;
>
>
> Hi Paolo,
>
> I'm confused with the following scenario:
>
> (suppose the VM has a assigned devices)
> step 1. the running vcpu is be preempted
> --> vmx_vcpu_pi_put [ SET pi.sn ]
> step 2. hot-unplug the assigned devices
> step 3. the vcpu is scheduled in
> --> vmx_vcpu_pi_load [ CLEAR pi.sn ]
> step 4. the running vcpu is be preempted again
> --> vmx_vcpu_pi_put [ direct return ]
> step 5. the vcpu is migrated to another pcpu
> step 6. the vcpu is scheduled in
> --> vmx_vcpu_pi_load [ above check fails and
> continue to execute the follow parts ]
>
> I think vmx_vcpu_pi_load should return direct in step6, because
> vmx_vcpu_pi_put in step4 did nothing.
> So maybe the above check has a potential problem.
Oh! Sorry, please just ignore the above. I made a mistaken.
>
> Please kindly figure out if I misunderstand anything important :)
>
> --
> Regards,
> Longpeng(Mike)
>
>> +
>> + /*
>> + * First handle the simple case where no cmpxchg is necessary; just
>> + * allow posting non-urgent interrupts.
>> + *
>> + * If the 'nv' field is POSTED_INTR_WAKEUP_VECTOR, do not change
>> + * PI.NDST: pi_post_block will do it for us and the wakeup_handler
>> + * expects the VCPU to be on the blocked_vcpu_list that matches
>> + * PI.NDST.
>> + */
>> + if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR ||
>> + vcpu->cpu == cpu) {
>> + pi_clear_sn(pi_desc);
>> return;
>> + }
>>
>> + /* The full case. */
>> do {
>> old.control = new.control = pi_desc->control;
>>
>> - /*
>> - * If 'nv' field is POSTED_INTR_WAKEUP_VECTOR, there
>> - * are two possible cases:
>> - * 1. After running 'pre_block', context switch
>> - * happened. For this case, 'sn' was set in
>> - * vmx_vcpu_put(), so we need to clear it here.
>> - * 2. After running 'pre_block', we were blocked,
>> - * and woken up by some other guy. For this case,
>> - * we don't need to do anything, 'pi_post_block'
>> - * will do everything for us. However, we cannot
>> - * check whether it is case #1 or case #2 here
>> - * (maybe, not needed), so we also clear sn here,
>> - * I think it is not a big deal.
>> - */
>> - if (pi_desc->nv != POSTED_INTR_WAKEUP_VECTOR) {
>> - if (vcpu->cpu != cpu) {
>> - dest = cpu_physical_id(cpu);
>> -
>> - if (x2apic_enabled())
>> - new.ndst = dest;
>> - else
>> - new.ndst = (dest << 8) & 0xFF00;
>> - }
>> + dest = cpu_physical_id(cpu);
>>
>> - /* set 'NV' to 'notification vector' */
>> - new.nv = POSTED_INTR_VECTOR;
>> - }
>> + if (x2apic_enabled())
>> + new.ndst = dest;
>> + else
>> + new.ndst = (dest << 8) & 0xFF00;
>>
>> - /* Allow posting non-urgent interrupts */
>> new.sn = 0;
>> } while (cmpxchg(&pi_desc->control, old.control,
>> new.control) != old.control);
>> @@ -9259,6 +9257,13 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>
>> vmx->msr_ia32_feature_control_valid_bits = FEATURE_CONTROL_LOCKED;
>>
>> + /*
>> + * Enforce invariant: pi_desc.nv is always either POSTED_INTR_VECTOR
>> + * or POSTED_INTR_WAKEUP_VECTOR.
>> + */
>> + vmx->pi_desc.nv = POSTED_INTR_VECTOR;
>> + vmx->pi_desc.sn = 1;
>> +
>> return &vmx->vcpu;
>>
>> free_vmcs:
>> @@ -11249,9 +11254,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>> else
>> new.ndst = (dest << 8) & 0xFF00;
>>
>> - /* Allow posting non-urgent interrupts */
>> - new.sn = 0;
>> -
>> /* set 'NV' to 'notification vector' */
>> new.nv = POSTED_INTR_VECTOR;
>> } while (cmpxchg(&pi_desc->control, old.control,
>
>
--
Regards,
Longpeng(Mike)
