Re: [Qemu-devel] [PATCH v2 RESEND 1/5] apic: Implement LAPIC low priority arbitration functions

[James Sullivan <sullivan.james.f@gmail.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 <sullivan.james.f@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.