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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

Blue Swirl wrote:
> On Thu, Jun 3, 2010 at 7:06 AM, Gleb Natapov <gleb@redhat.com> 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]