From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44550) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlHVl-0006PN-Pv for qemu-devel@nongnu.org; Thu, 23 Apr 2015 09:49:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlHVg-0002jy-LA for qemu-devel@nongnu.org; Thu, 23 Apr 2015 09:49:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59402) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlHVg-0002js-GB for qemu-devel@nongnu.org; Thu, 23 Apr 2015 09:49:44 -0400 Date: Thu, 23 Apr 2015 15:49:39 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Message-ID: <20150423134939.GA1992@potion.brq.redhat.com> References: <1428363937-19003-1-git-send-email-sullivan.james.f@gmail.com> <1428363937-19003-2-git-send-email-sullivan.james.f@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428363937-19003-2-git-send-email-sullivan.james.f@gmail.com> 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: James Sullivan Cc: pbonzini@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, jan.kiszka@siemens.com 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. > + } > + } > + } > + 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.) > @@ -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.) > { > - /* 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;'.)