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

[Blue Swirl]

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.