Re: [Qemu-devel] Re: [RFT][PATCH 07/15] qemu_irq: Add IRQ handlers with delivery feedback

[Blue Swirl]

On Sat, Jun 5, 2010 at 8:27 AM, Jan Kiszka <address@hidden> wrote:
> Blue Swirl wrote:
>> On Sat, Jun 5, 2010 at 12:04 AM, Jan Kiszka <address@hidden> wrote:
>>> Blue Swirl wrote:
>>>> On Thu, Jun 3, 2010 at 7:06 AM, Gleb Natapov <address@hidden> wrote:
>>>>> On Thu, Jun 03, 2010 at 10:03:00AM +0300, Gleb Natapov wrote:
>>>>>> On Thu, Jun 03, 2010 at 08:59:23AM +0200, Jan Kiszka wrote:
>>>>>>> Gleb Natapov wrote:
>>>>>>>> On Thu, Jun 03, 2010 at 08:23:46AM +0200, Jan Kiszka wrote:
>>>>>>>>> Blue Swirl wrote:
>>>>>>>>>> But how about if we introduced instead a message based IRQ? Then the
>>>>>>>>>> message could specify the originator device, maybe ACK/coalesce/NACK
>>>>>>>>>> callbacks and a bigger payload than just 1 bit of level. I think that
>>>>>>>>>> could achieve the same coalescing effect as what the bidirectional
>>>>>>>>>> IRQ. The payload could be useful for other purposes, for example
>>>>>>>>>> Sparc64 IRQ messages contain three 64 bit words.
>>>>>>>>> If there are more users than just IRQ de-coalescing, this indeed 
>>>>>>>>> sounds
>>>>>>>>> superior. We could pass objects like this one around:
>>>>>>>>>
>>>>>>>>> struct qemu_irq_msg {
>>>>>>>>>  void (*delivery_cb)(int result);
>>>>>>>>>  void *payload;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> They would be valid within the scope of the IRQ handlers. Whoever
>>>>>>>>> terminates or actually delivers the IRQ would invoke the callback. And
>>>>>>>>> platforms like sparc64 could evaluate the additional payload pointer 
>>>>>>>>> in
>>>>>>>>> their irqchips or wherever they need it. IRQ routers on platforms that
>>>>>>>>> make use of these messages would have to replicate them when 
>>>>>>>>> forwarding
>>>>>>>>> an event.
>>>>>>>>>
>>>>>>>>> OK?
>>>>>>>>>
>>>>>>>> Let me see if I understand you correctly. qemu_set_irq() will get
>>>>>>>> additional parameter qemu_irq_msg and if irq was not coalesced
>>>>>>>> delivery_cb is called, so there is a guaranty that if delivery_cb is
>>>>>>>> called it is done before qemu_set_irq() returns. Correct?
>>>>>>> If the side that triggers an IRQ passes a message object with a non-NULL
>>>>>>> callback, it is supposed to be called unconditionally, passing the
>>>>>>> result of the delivery (delivered, masked, coalesced). And yes, the
>>>>>>> callback will be invoked in the context of the irq handler, so before
>>>>>>> qemu_set_irq (or rather some new qemu_set_irq_msg) returns.
>>>>>>>
>>>>>> Looks fine to me.
>>>>>>
>>>>> Except that delivery_cb should probably get pointer to qemu_irq_msg as a
>>>>> parameter.
>>>> I'd like to also support EOI handling. When the guest clears the
>>>> interrupt condtion, the EOI callback would be called. This could occur
>>>> much later than the IRQ delivery time. I'm not sure if we need the
>>>> result code in that case.
>>>>
>>>> If any intermediate device (IOAPIC?) needs to be informed about either
>>>> delivery or EOI also, it could create a proxy message with its
>>>> callbacks in place. But we need then a separate opaque field (in
>>>> addition to payload) to store the original message.
>>>>
>>>> struct IRQMsg {
>>>>  DeviceState *src;
>>>>  void (*delivery_cb)(IRQMsg *msg, int result);
>>>>  void (*eoi_cb)(IRQMsg *msg, int result);
>>>>  void *src_opaque;
>>>>  void *payload;
>>>> };
>>> Extending the lifetime of IRQMsg objects beyond the delivery call stack
>>> means qemu_malloc/free for every delivery. I think it takes a _very_
>>> appealing reason to justify this. But so far I do not see any use case
>>> for eio_cb at all.
>>
>> I think it's safer to use allocation model anyway because this will be
>> generic code. For example, an intermediate device may want to queue
>> the IRQs. Alternatively, the callbacks could use DeviceState and some
>> opaque which can be used as the callback context:
>>   void (*delivery_cb)(DeviceState *src, void *src_opaque, int result);
>>
>> EOI can be added later if needed, QEMU seems to work fine now without
>> it. But based on IOAPIC data sheet, I'd suppose it should be need to
>> pass EOI from LAPIC to IOAPIC. It could be used by coalescing as
>> another opportunity to inject IRQs though I guess the guest will ack
>> the IRQ at the same time for both RTC and APIC.
>
> Let's wait for a real use case for an extended IRQMsg lifetime. For now
> we are fine with stack-allocated messages which are way simpler to
> handle. I'm already drafting a first prototype based on this model.
> Switching to dynamic allocation may still happen later on once the
> urgent need shows up.

Passing around stack allocated objects is asking for trouble. I'd much
rather use the DeviceState/opaque version then, so at least
destination should not need to use IRQMsg for anything.