RE: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU is blocked

["Wu, Feng" <feng.wu@intel.com>]

> -----Original Message-----
> From: Marcelo Tosatti [mailto:mtosatti@redhat.com]
> Sent: Saturday, March 28, 2015 3:30 AM
> To: Wu, Feng
> Cc: hpa@zytor.com; tglx@linutronix.de; mingo@redhat.com; x86@kernel.org;
> gleb@kernel.org; pbonzini@redhat.com; dwmw2@infradead.org;
> joro@8bytes.org; alex.williamson@redhat.com; jiang.liu@linux.intel.com;
> eric.auger@linaro.org; linux-kernel@vger.kernel.org;
> iommu@lists.linux-foundation.org; kvm@vger.kernel.org
> Subject: Re: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU
> is blocked
> 
> On Fri, Mar 27, 2015 at 06:34:14AM +0000, Wu, Feng wrote:
> > > > Currently, the following code is executed before local_irq_disable() is
> called,
> > > > so do you mean 1)moving local_irq_disable() to the place before it. 2) after
> > > interrupt
> > > > is disabled, set KVM_REQ_EVENT in case the ON bit is set?
> > >
> > > 2) after interrupt is disabled, set KVM_REQ_EVENT in case the ON bit
> > > is set.
> >
> > Here is my understanding about your comments here:
> > - Disable interrupts
> > - Check 'ON'
> > - Set KVM_REQ_EVENT if 'ON' is set
> >
> > Then we can put the above code inside " if
> (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) "
> > just like it used to be. However, I still have some questions about this
> comment:
> >
> > 1. Where should I set KVM_REQ_EVENT? In function vcpu_enter_guest(), or
> other places?
> 
> See below:
> 
> > If in vcpu_enter_guest(), since currently local_irq_disable() is called after
> 'KVM_REQ_EVENT'
> > is checked, is it helpful to set KVM_REQ_EVENT after local_irq_disable() is
> called?
> 
>         local_irq_disable();
> 
> 	*** add code here ***

So we need add code like the following here, right?

          if ('ON' is set)
              kvm_make_request(KVM_REQ_EVENT, vcpu);

> 
>         if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests
> 						^^^^^^^^^^^^^^
>             || need_resched() || signal_pending(current)) {
>                 vcpu->mode = OUTSIDE_GUEST_MODE;
>                 smp_wmb();
>                 local_irq_enable();
>                 preempt_enable();
>                 vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>                 r = 1;
>                 goto cancel_injection;
>         }
> 
> > 2. 'ON' is set by VT-d hardware, it can be set even when interrupt is disabled
> (the related bit in PIR is also set).
> 
> Yes, we are checking if the HW has set an interrupt in PIR while
> outside VM (which requires PIR->VIRR transfer by software).
> 
> If the interrupt it set by hardware after local_irq_disable(),
> VMX-entry will handle the interrupt and perform the PIR->VIRR
> transfer and reevaluate interrupts, injecting to guest
> if necessary, is that correct ?
> 
> > So does it make sense to check 'ON' and set KVM_REQ_EVENT accordingly
> after interrupt is disabled?
> 
> To replace the costly
> 
> +            */
> +           if (kvm_x86_ops->hwapic_irr_update)
> +                   kvm_x86_ops->hwapic_irr_update(vcpu,
> +                           kvm_lapic_find_highest_irr(vcpu));
> 
> Yes, i think so.

After adding the "checking ON and setting KVM_REQ_EVENT" operations listed in my
comments above, do you mean we still need to keep the costly code above
inside "if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {}" in function
vcpu_enter_guest() as it used to be? If yes, my question is what is the exact purpose
of "checking ON and setting KVM_REQ_EVENT" operations? Here is the code flow in
vcpu_enter_guest():

1. Check KVM_REQ_EVENT, if it is set, sync pir->virr
2. Disable interrupts
3. Check ON and set KVM_REQ_EVENT -- Here, we set KVM_REQ_EVENT, but it is
checked in the step 1, which means, we cannot get any benefits even we set it here,
since the "pir->virr" sync operation was done in step 1, between step 3 and VM-Entry,
we don't synchronize the pir to virr. So even we set KVM_REQ_EVENT here, the interrupts
remaining in PIR cannot be delivered to guest during this VM-Entry, right?

Thanks,
Feng

> 
> > I might miss something in your comments, if so please point out. Thanks a
> lot!
> >
> > Thanks,
> > Feng
> >
> > >
> > > >
> > > > "if (kvm_x86_ops->hwapic_irr_update)
> > > > 	kvm_x86_ops->hwapic_irr_update(vcpu,
> > > > 			kvm_lapic_find_highest_irr(vcpu));
> > > >
> > > > > kvm_lapic_find_highest_irr(vcpu) eats some cache
> > > > > (4 cachelines) versus 1 cacheline for reading ON bit.
> > > > >
> > > > > > > > > Please remove blocked and wakeup_cpu, they should not be
> > > necessary.
> > > > > > > >
> > > > > > > > Why do you think wakeup_cpu is not needed, when vCPU is
> blocked,
> > > > > > > > wakeup_cpu saves the cpu which the vCPU is blocked on, after
> vCPU
> > > > > > > > is woken up, it can run on a different cpu, so we need wakeup_cpu
> to
> > > > > > > > find the right list to wake up the vCPU.
> > > > > > >
> > > > > > > If the vCPU was moved it should have updated IRTE destination field
> > > > > > > to the pCPU which it has moved to?
> > > > > >
> > > > > > Every time a vCPU is scheduled to a new pCPU, the IRTE destination
> filed
> > > > > > would be updated accordingly.
> > > > > >
> > > > > > When vCPU is blocked. To wake up the blocked vCPU, we need to find
> > > which
> > > > > > list the vCPU is blocked on, and this is what wakeup_cpu used for?
> > > > >
> > > > > Right, perhaps prev_vcpu is a better name.
> > > >
> > > > Do you mean "prev_pcpu"?
> > >
> > > Yes.
> > >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html