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

["Radim Krčmář" <rkrcmar@redhat.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 <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.

> +            }
> +        }
> +    }
> +    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;'.)