RE: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting

["Zhang, Yang Z" <yang.z.zhang@intel.com>]

Sorry for the late response.

Marcelo Tosatti wrote on 2013-02-04:
> On Thu, Jan 31, 2013 at 03:55:56PM +0200, Gleb Natapov wrote:
>> On Thu, Jan 31, 2013 at 11:44:43AM -0200, Marcelo Tosatti wrote:
>>> On Thu, Jan 31, 2013 at 03:38:37PM +0200, Gleb Natapov wrote:
>>>> On Thu, Jan 31, 2013 at 11:32:45AM -0200, Marcelo Tosatti wrote:
>>>>> On Thu, Jan 31, 2013 at 11:43:48AM +0200, Gleb Natapov wrote:
>>>>>> On Wed, Jan 30, 2013 at 09:03:11PM -0200, Marcelo Tosatti wrote:
>>>>>>> Posted interrupt patch:
>>>>>>> 2) Must move IN_GUEST_MODE assignment after local_irq_disable, in
>>>>>>> vcpu_enter_guest function. Otherwise:
>>>>>>> 
>>>>>>> cpu0			vcpu1<->cpu1
>>>>>>> 
>>>>>>> 			vcpu->mode = IN_GUEST_MODE
>>>>>>> 
>>>>>>> if IN_GUEST_MODE == true
>>>>>>> 	send IPI
>>>>>>> 			local_irq_disable
>>>>>>> 
>>>>>>> PIR not transferred to VIRR, misses interrupt.
>>>>>>> 
>>>>>> cpu0 will set KVM_REQ_EVENT, so vmentry will be aborted after
>>>>>> local_irq_disable() by ->requests check.
>>>>> 
>>>>> Yes, but you don't want KVM_REQ_EVENT+kick. It defeats the purpose
>>>>> of posted interrupts. You want
>>>>> 
>>>>> if vcpu in guest mode
>>>>> 	send posted interrupt IPI
>>>>> else
>>>>> 	KVM_REQ_EVENT+kick
>>>>> 
>>>> I am thinking:
>>>> 
>>>>  set KVM_REQ_EVENT
>>>>  if pi is enabled
>>>>      send posted interrupt IPI else kick
>>> 
>>> KVM_REQ_EVENT must be after sending posted interrupt IPI. Otherwise on
>>> the vcpu entry side
>>> 
>>> test_and_clear(KVM_REQ_EVENT) {
>>> 	No bits set in PIR
>>>    }
>> It should be after updating PIR, but before sending posted interrupt
>> IPI. Otherwise:
>> 
>>  cpu0                             cpu1/vcpu
>>                                   KVM_REQ_EVENT is not set
>> set pir
>> send IPI
>>                                   irq_disable()
>>                                   ->request is empty.
>> set KVM_REQ_EVENT
>> 
>> That's the same sequence as with IRR update, KVM_REQ_EVENT and kick
>> today.
> 
> Can only send IPI if vcpu->mode == IN_GUEST_MODE, which must be set
> after interrupt flag is cleared as noted.
> 
> Also KVM_REQ_EVENT is processed outside of interrupt disabled region today.
> 
> Or maybe i don't get what you say... write a complete description?
This version patch is too old. And I already noticed this problem and the current solution is like this:
+static bool vmx_send_notification_event(struct kvm_vcpu *vcpu,
+               int vector, int *result)
+{
+       struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+       *result = !pi_test_and_set_pir(vector, vmx->pi);
+       if (!pi_test_and_set_on(vmx->pi) && (vcpu->mode == IN_GUEST_MODE)) {
+               kvm_make_request(KVM_REQ_PENDING_PIR, vcpu);
+               apic->send_IPI_mask(get_cpu_mask(vcpu->cpu),
+                                   POSTED_INTR_VECTOR);
+               if (!pi_test_on(vmx->pi))
+                       clear_bit(KVM_REQ_PENDING_PIR, &vcpu->requests) ;
+               return true;
+       }
+       return false;
+}

For detail, please check the v2 patches.

>>> What about item 4 below?
>>> 
>> That's for Yang to answer :)
> 
> "If more than one interrupt is generated with the same vector number,
> the local APIC can set the bit for the vector both in the IRR and ISR.
> This means that for the Pentium 4 and Intel Xeon processors, the IRR
> and ISR can queue two interrupts for each interrupt vector: one in the
> IRR and one in the ISR. Any additional interrupts issued for the same
> interrupt vector are collapsed into the single bit in IRR"
> 
> Which would mean KVM emulation differs... "Eventually 3 interrupts can
> be queued: one in IRR, one in ISR, one in PIR".
>
> Any example how software relies on such two-interrupts-queued-in-IRR/ISR
> behaviour?
Before PI, the interrupt handle logic is IRR->ISR. And now it is : PIR->IRR->ISR.
So, if we can ensure PIR sync to IRR before each access to IRR, I think there should no problem.

Best regards,
Yang