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: 5161 bytes --]

Blue Swirl wrote:
> On Sat, Jun 5, 2010 at 8:27 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Blue Swirl wrote:
>>> On Sat, Jun 5, 2010 at 12:04 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> 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.
>>> 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.

Right, I've hiden the IRQMsg object from the target handler, temporarily
storing it in qemu_irq instead. qemu_irq_handler had to be touched
anyway, so I'm now passing the IRQ object to it so that it can invoke
services to trigger the delivery callback or obtain the payload.

Jan


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