Hi,
Looking at the working implementations of IPIs I came
across these inconsistencies:
1) APIC: waiting for completion
-------------------------------
APIC has a bit that indicates if an IPI is in progress.
Should we not wait for the dispatch of the previous IPI
to complete before we send another one? Note that I don't
know if l_apic[ICRlo].delivs is CPU local (so a simple
while(l_apic[ICRlo].delivs == DELIVS_PENDING){} would
suffice) or global for the system (then we'd need a lock).
IPIs are sent via l_apic_broadcast_custom_ipi in
arch/ia32/src/smp/apic.c:
int l_apic_broadcast_custom_ipi(uint8_t vector)
{
icr_t icr;
// Wait here for icr.delivs != DELIVS_PENDING ?
icr.lo = l_apic[ICRlo];
icr.delmod = DELMOD_FIXED;
icr.destmod = DESTMOD_LOGIC;
icr.level = LEVEL_ASSERT;
icr.shorthand = SHORTHAND_ALL_EXCL;
icr.trigger_mode = TRIGMOD_LEVEL;
icr.vector = vector;
l_apic[ICRlo] = icr.lo;
icr.lo = l_apic[ICRlo];
if (icr.delivs == DELIVS_PENDING) {
#ifdef CONFIG_DEBUG
printf("IPI is pending.\n");
#endif
}
return apic_poll_errors();
}
2) sparc64/sun4U: while-loop cond
---------------------------------
arch/sparc64/src/smp/sun4u/ipi.c sends IPIs via cross_call().
It has a suspicious while loop end condition. I don't
know enough to decide if it is a bug or not.
static void cross_call(int mid, void (* func)(void))
{
uint64_t status;
bool done;
/* snip */
do {
// snip
if (!done) {
/*
* Prevent deadlock.
*/
(void) interrupts_enable();
delay(20 + (tick_read() & 0xff));
(void) interrupts_disable();
}
} while (done); // <-- instead of "while (!done)" ?!
preemption_enable();
}
3) sparc64/sun4v: cpu->arch.id vs cpu->id
-----------------------------------------
In arch/sparc64/src/smp/sun4v/ipi.c, ipi_broadcast_arch()
seems to be mixing cpu->arch.id with cpu->id as if they
were the same (even if they are guaranteed to match, it
is just not obvious from the code):
void ipi_broadcast_arch(int ipi)
{
void (* func)(void);
/* snip */
unsigned int i;
unsigned idx = 0;
for (i = 0; i < config.cpu_active; i++) {
if (&cpus[i] == CPU) {
continue;
}
// Was this supposed to be CPU->arch.id and
// cpus[i].arch.id ??
ipi_cpu_list[CPU->id][idx] = (uint16_t) cpus[i].id;
idx++;
}
ipi_brodcast_to(func, ipi_cpu_list[CPU->arch.id], idx);
}
Thanks to anyone with the proper knowledge for checking
these inconsistencies.
Adam
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
