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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
