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

[Radim Krčmář]

2015-04-23 12:34-0600, James Sullivan:
> 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 <address@hidden>
>>> ---
>>> +    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.

Yeah, I wouldn't use it either :)
It could use the canonical form (and optimizations for sparse bitmasks)
  foreach(...)
    code;

Moving this logic to the loop in [4/4] would be an elegant solution.

> > (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

local_apics[] aren't indexed by APIC ID, which brings two complications
- QEMU allows writing to APIC ID.
  Luckily, the spec allows us to make it read only, but even then
- local_apics[] might not be ordered like their APIC IDs; (I don't know)
  and future development can change that, so we should also encode the
  expecation somewhere.  (Comments don't work very well.)

Taking a look at the real APIC ID would be safest.
(Silently ignoring it is also a viable option.)