From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57661 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OKpbM-0005gC-QC for qemu-devel@nongnu.org; Sat, 05 Jun 2010 05:23:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OKpbL-0003eT-DG for qemu-devel@nongnu.org; Sat, 05 Jun 2010 05:23:36 -0400 Received: from mail-pw0-f45.google.com ([209.85.160.45]:51417) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OKpbL-0003eP-4a for qemu-devel@nongnu.org; Sat, 05 Jun 2010 05:23:35 -0400 Received: by pwj3 with SMTP id 3so964101pwj.4 for ; Sat, 05 Jun 2010 02:23:34 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4C0A0A69.9070405@web.de> References: <20100530200722.GA6243@redhat.com> <20100531051905.GD24302@redhat.com> <20100601183058.GB6191@redhat.com> <4C074A72.3070507@web.de> <20100603063456.GM24302@redhat.com> <4C0752CB.9030701@web.de> <20100603070300.GN24302@redhat.com> <20100603070559.GO24302@redhat.com> <4C099471.3060507@web.de> <4C0A0A69.9070405@web.de> From: Blue Swirl Date: Sat, 5 Jun 2010 09:23:14 +0000 Message-ID: Subject: Re: [Qemu-devel] Re: [RFT][PATCH 07/15] qemu_irq: Add IRQ handlers with delivery feedback Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: qemu-devel@nongnu.org, Gleb Natapov , Juan Quintela On Sat, Jun 5, 2010 at 8:27 AM, Jan Kiszka wrote: > Blue Swirl wrote: >> On Sat, Jun 5, 2010 at 12:04 AM, Jan Kiszka wrote: >>> Blue Swirl wrote: >>>> On Thu, Jun 3, 2010 at 7:06 AM, Gleb Natapov 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 bidirection= al >>>>>>>>>> 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 { >>>>>>>>> =C2=A0void (*delivery_cb)(int result); >>>>>>>>> =C2=A0void *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 poin= ter in >>>>>>>>> their irqchips or wherever they need it. IRQ routers on platforms= that >>>>>>>>> make use of these messages would have to replicate them when forw= arding >>>>>>>>> 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 befo= re >>>>>>> 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 a= s 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 { >>>> =C2=A0DeviceState *src; >>>> =C2=A0void (*delivery_cb)(IRQMsg *msg, int result); >>>> =C2=A0void (*eoi_cb)(IRQMsg *msg, int result); >>>> =C2=A0void *src_opaque; >>>> =C2=A0void *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: >> =C2=A0 void (*delivery_cb)(DeviceState *src, void *src_opaque, int resul= t); >> >> 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.