| Message ID | 1428363937-19003-2-git-send-email-sullivan.james.f@gmail.com |
|---|---|
| State | New |
| Headers | show |
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;'.)
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.
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 <sullivan.james.f@gmail.com> >>> --- >>> + 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.)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 0f97b47..b372513 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -38,6 +38,7 @@ static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode); static void apic_update_irq(APICCommonState *s); static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask, uint8_t dest, uint8_t dest_mode); +static int apic_get_arb_pri(APICCommonState *s); /* Find first bit starting from msb */ static int apic_fls_bit(uint32_t value) @@ -199,6 +200,29 @@ static void apic_external_nmi(APICCommonState *s) apic_local_deliver(s, APIC_LVT_LINT1); } +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]; + } + } + } + return lowest; +} + #define foreach_apic(apic, deliver_bitmask, code) \ {\ int __i, __j;\ @@ -336,8 +360,27 @@ static int apic_get_ppr(APICCommonState *s) static int apic_get_arb_pri(APICCommonState *s) { - /* 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; }
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> --- hw/intc/apic.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)