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 <sullivan.jame...@gmail.com>
>> ---
>> +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.
