From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlMWJ-00058Y-DO for qemu-devel@nongnu.org; Thu, 23 Apr 2015 15:10:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlMWD-0003LI-Ti for qemu-devel@nongnu.org; Thu, 23 Apr 2015 15:10:43 -0400 Received: from mail-pd0-x234.google.com ([2607:f8b0:400e:c02::234]:33603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlMWD-0003Kw-Lg for qemu-devel@nongnu.org; Thu, 23 Apr 2015 15:10:37 -0400 Received: by pdbnk13 with SMTP id nk13so26284484pdb.0 for ; Thu, 23 Apr 2015 12:10:36 -0700 (PDT) Message-ID: <5539431B.4090405@gmail.com> Date: Thu, 23 Apr 2015 13:08:11 -0600 From: James Sullivan MIME-Version: 1.0 References: <1428363937-19003-1-git-send-email-sullivan.james.f@gmail.com> <1428363937-19003-6-git-send-email-sullivan.james.f@gmail.com> <20150423141407.GA3349@potion.brq.redhat.com> In-Reply-To: <20150423141407.GA3349@potion.brq.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 RESEND 5/5] apic: Implement handling of RH=1 for MSI interrupt delivery List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Cc: pbonzini@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, jan.kiszka@siemens.com On 04/23/2015 08:14 AM, Radim Krčmář wrote: > 2015-04-06 17:45-0600, James Sullivan: >> Added argument to apic_get_delivery_bitmask() for msi_redir_hint, >> and changed calls to the function accordingly (using 0 as a default >> value for non-MSI interrupts). >> >> Modified the implementation of apic_get_delivery_bitmask() to account >> for the RH bit of an MSI IRQ. The RH bit indicates that the message >> should target only the lowest-priority processor among those specified >> by the logical destination of the IRQ. >> >> Signed-off-by: James Sullivan >> --- >> diff --git a/hw/intc/apic.c b/hw/intc/apic.c >> @@ -519,23 +521,27 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, >> } >> } else { >> /* XXX: cluster mode */ >> + int l = -1; >> memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t)); >> for(i = 0; i < MAX_APICS; i++) { >> apic_iter = local_apics[i]; >> + if (!apic_iter) { >> + break; >> + } > > (I wonder if QEMU would allow > 'for(i = 0; i < MAX_APICS && (apic_iter = local_apics[i]); i++) {') > >> + if (apic_match_dest(apic_iter, dest_mode, dest)) { >> + if (msi_redir_hint) { > > You could check for APIC_DM_LOWPRI here as well and save the > apic_lowest_prio() loop in patch [1/4]. > LOWPRI would be delivered like FIXED. > I was considering doing that, saving the loop is a big plus. My concern was that it would change get_delivery_bitmask's semantics in a way that could be confusing (i.e. it is currently expected that the caller is responsible for doing arbitration after the fact, this would flip that responsibility). >> + if (l < 0 || >> + apic_compare_prio(apic_iter, local_apics[l]) < 0) { >> + l = i; > > (Btw. lowest priority has a lot of cases that are forbidden ... > - in combination with physical broadcast > - in combination with clustered logical broadcast > - to invalid/disabled destinations > > These most likely won't work correctly in real hardware. > Lowest priority is a bad concept for large systems, which is why Intel > stopped implementing it.) > Checking for such illegal cases should probably happen in the delivery routines before we set the delivery bitmask (in apic_bus_deliver()?)