On Wed, Aug 12, 2009 at 12:57:07PM +0300, Avi Kivity wrote:
> On 08/12/2009 12:47 PM, Gleb Natapov wrote:
>>> Ah, the real motivation is msi. Pushing locks down doesn't help if we
>>> keep locking them. But for msi we avoid the lock entirely.
>>>
>>>
>> Yes. MSI is one. Multiple IOAPICs may inject interrupt in parallel too
>> (if we will choose to implement multiple IOAPICs sometime).
>>
>
> Right. Given msi, I don't think we'll have multiple ioapics though.
>
>>>>> Why a spinlock and not a mutex?
>>>>>
>>>>>
>>>>>
>>>> Protected sections are small and we do not sleep there.
>>>>
>>>>
>>> So what? A mutex is better since it allows preemption (and still has
>>> spinlock performance if it isn't preempted).
>>>
>>>
>> This lock will be taken during irq injection from irqfd, so may be leave
>> it as spinlock and take it _irqsave()? Do we want to allow irq injection
>> from interrupt context? Otherwise if you say that performance is the
>> same I don't care one way or the other.
>>
>
> Let's leave _irqsave() until later since that has other implications.
>
So change it to mutex then?
>>>>> Need to explain why this is safe. I'm not sure it is, because we touch
>>>>> state afterwards in pic_intack(). We need to do all vcpu-synchronous
>>>>> operations before dropping the lock.
>>>>>
>>>>>
>>>> Forst pic_intack() calls pic_clear_isr() only in auto eoi mode and this
>>>> mode
>>>> is already broken for assigned devices. Second for level triggered
>>>> interrupts pic_intack() does nothing after calling pic_clear_isr() and
>>>> third I can move pic_clear_isr() call to the end of pic_intack().
>>>>
>>>>
>>> I meant, in a comment.
>>>
>> I you agree with above I'll add it as a comment.
>>
>
> Sure.
>
>>> It should. What if there's a reset with an assigned device? We need to
>>> release the device interrupt (after doing FLR?).
>>>
>> Doing this will just re-enable host interrupt while irq condition is not
>> cleared in the device. The host will hang. So I think we really shouldn't.
>>
>
> Ok. What about timer acks?
>
Interrupt wasn't processed by a guest. Why should it be accounted as
such?
>
>
>
>>>> I don't see why we clear remote_irr before dropping the lock. If, while
>>>> lock was dropped, interrupt was delivered to this entry it will be
>>>> injected when ack notifier returns.
>>>>
>>>>
>>> But we'll clear remote_irr afterward the redelivery, and we should to
>>> that only after the new interrupt is acked.
>>>
>> It depend on whether you consider calling ack notifiers a part of
>> interrupt acknowledgement process.
>
> I don't really care, but the ack process has to be atomic. Since we
> need to drop the lock, it means the notifier is not part of the process.
>
>> If you do then remote_irr should not
>> be cleared before ack notifiers since ack process is not completed yet.
>> With current users functionally it shouldn't matter when we clear
>> remote_irr. I prefer doing it like we do it now since this how it was
>> before my patches and since code is simpler this way.
>>
>
> No, I think it introduces a race if an interrupt is raised while the ack
> notifier is running.
>
>
>
>>> No, I mean keep the for loop in kvm_ioapic_update_eoi().
>>>
>>>
>> Can't do that. __kvm_ioapic_update_eoi() is called from other place with
>> lock held already.
>>
>
> Ok.
>
> --
> error compiling committee.c: too many arguments to function
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
