From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34170) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlM08-0000sl-4g for qemu-devel@nongnu.org; Thu, 23 Apr 2015 14:37:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlM04-0000Jc-Rv for qemu-devel@nongnu.org; Thu, 23 Apr 2015 14:37:28 -0400 Received: from mail-pa0-x230.google.com ([2607:f8b0:400e:c03::230]:36138) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlM04-0000JT-KD for qemu-devel@nongnu.org; Thu, 23 Apr 2015 14:37:24 -0400 Received: by pabsx10 with SMTP id sx10so25143944pab.3 for ; Thu, 23 Apr 2015 11:37:23 -0700 (PDT) Message-ID: <55393B52.8030509@gmail.com> Date: Thu, 23 Apr 2015 12:34:58 -0600 From: James Sullivan MIME-Version: 1.0 References: <1428363937-19003-1-git-send-email-sullivan.james.f@gmail.com> <1428363937-19003-2-git-send-email-sullivan.james.f@gmail.com> <20150423134939.GA1992@potion.brq.redhat.com> In-Reply-To: <20150423134939.GA1992@potion.brq.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 RESEND 1/5] apic: Implement LAPIC low priority arbitration functions 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 07:49 AM, Radim Krčmář wrote: > 2015-04-06 17:45-0600, James Sullivan: >> Currently, apic_get_arb_pri() is unimplemented and returns 0. >> >> Implemented apic_get_arb_pri() and added two helper functions >> apic_compare_prio() and apic_lowest_prio() to be used for LAPIC >> arbitration. >> >> Signed-off-by: James Sullivan >> --- >> +static int apic_compare_prio(struct APICCommonState *cpu1, >> + struct APICCommonState *cpu2) >> +{ >> + return apic_get_arb_pri(cpu1) - apic_get_arb_pri(cpu2); >> +} >> + >> +static struct APICCommonState *apic_lowest_prio(const uint32_t >> + *deliver_bitmask) >> +{ >> + APICCommonState *lowest = NULL; >> + int i, d; >> + >> + for (i = 0; i < MAX_APIC_WORDS; i++) { >> + if (deliver_bitmask[i]) { >> + d = i * 32 + apic_ffs_bit(deliver_bitmask[i]); >> + if (!lowest || apic_compare_prio(local_apics[d], lowest) < 0) { >> + lowest = local_apics[d]; > > deliver_bitmask is 8 times u32 to express all 255 possible APICs. > apic_ffs_bit() takes the last set bit, so this loop incorrectly > considers only up to 8 candidates for lowest priority delivery. > foreach_apic() could be used when fixing it, which would also avoid a > 'local_apic[d] == NULL' crash. > Dumb mistake, I'll fix the former point. I shied away from foreach_apic() because I think it's a bit messy to embed a block or an `if` statement into the macro like so: foreach_apic(apic,bmask, if (cond) foo(); ); But if people are okay with that sort of thing we can switch to it. >> + } >> + } >> + } >> + return lowest; > > (For consideration: > APM 2-16.2.2 Lowest Priority Messages and Arbitration > If there is a tie for lowest priority, the local APIC with the highest > APIC ID is selected. > > Intel is undefined in spec and picks the lowest APIC ID in practice.) > Pretty quick change there, set lowest = local_apics[d] when apic_compare_prio(local_apics[d], lowest) <= 0 rather than < 0 >> @@ -336,8 +360,27 @@ static int apic_get_ppr(APICCommonState *s) >> >> static int apic_get_arb_pri(APICCommonState *s) > > (I'd call it apic_get_apr() -- we return the state of that register.) > Good point, more consistent with other functions too. >> { >> - /* XXX: arbitration */ >> - return 0; >> + int tpr, isrv, irrv, apr; >> + >> + tpr = apic_get_tpr(s); >> + isrv = get_highest_priority_int(s->isr); >> + if (isrv < 0) { >> + isrv = 0; >> + } >> + isrv >>= 4; >> + irrv = get_highest_priority_int(s->irr); >> + if (irrv < 0) { >> + irrv = 0; >> + } >> + irrv >>= 4; >> + >> + if ((tpr >= irrv) && (tpr > isrv)) { >> + apr = s->tpr & 0xff; >> + } else { >> + apr = ((tpr & isrv) > irrv) ? (tpr & isrv) : irrv; >> + apr <<= 4; >> + } >> + return apr; >> } > > (This function is called too much IMO. > The trivial optimization is to memorize apic_get_arb_pri() of lowest > priority LAPIC. And you can instantly return if it is 0. > The more complicated one is to use ARB as a real LAPIC register and > update it on TPR/ISR/IRR change, so apic_get_arb_pri() would be just > 'return s->apr;'.) > The latter approach would be smart, I'll look into it.