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

[Blue Swirl]

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;
};