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

[Jan Kiszka]

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.

Jan

signature.asc
Description: OpenPGP digital signature