On Tue, 2020-03-03 at 16:57 -0500, Sean Anderson wrote: > On 3/3/20 4:53 PM, Lukas Auer wrote: > > On Mon, 2020-03-02 at 18:43 -0500, Sean Anderson wrote: > > > On 3/2/20 6:17 PM, Lukas Auer wrote: > > > > Don't move this. It is intended to be run before the IPI is cleared. > > > > > > Hm, ok. I think I moved it to after because of the 'if (!smp_function)' > > > check, but those two don't really need to be done together. > > > > > > > Thanks! We had problems with code corruption in some situations, > > because some secondary harts entered OpenSBI after the main hart while > > OpenSBI expected all harts to be running OpenSBI by that time. Moving > > this code block was part of the fix for this situation, see [1]. > > > > [1]: > > https://gitlab.denx.de/u-boot/u-boot/commit/90ae28143700bae4edd23930a7772899ad259058 > > Ah, this makes a lot more sense why it was located where it was. > > > > > > /* > > > > > * Clear the IPI to acknowledge the request before jumping to > > > > > the > > > > > * requested function. > > > > > */ > > > > > ret = riscv_clear_ipi(hart); > > > > > if (ret) { > > > > > - pr_err("Cannot clear IPI of hart %ld\n", hart); > > > > > + pr_err("Cannot clear IPI of hart %ld (error %d)\n", > > > > > hart, ret); > > > > > return; > > > > > } > > > > > > > > > > + __smp_mb(); > > > > > + > > > > > + smp_function = (void (*)(ulong, ulong, > > > > > ulong))gd->arch.ipi[hart].addr; > > > > > + /* > > > > > + * There may be an IPI raised before u-boot begins execution, > > > > > so check > > > > > + * to ensure we actually have a function to call. > > > > > + */ > > > > > + if (!smp_function) > > > > > + return; > > > > > + log_debug("hart = %lu func = %p\n", hart, smp_function); > > > > > > > > The log messages might be corrupted if multiple harts are calling the > > > > log function here. I have not looked into the details so this might not > > > > be an issue. In that case it is fine to keep, otherwise please remove > > > > it. > > > > > > I ran into this problem a lot when debugging. I ended up implementing a > > > spinlock around puts/putc. I agree it's probably better to remove this, > > > but I worry that concurrency bugs will become much harder to track down > > > without some kind of feedback. (This same criticism applies to the log > > > message above as well). > > > > > > > Especially with your changes, I hope we already have or will soon reach > > a code robustness level where we won't have too many concurrency bugs > > in the future. :) > > Let's remove it for now until the logging backend can handle this > > cleanly. > > Ok. Should the error message above ("Cannot clear IPI of hart...") also > be removed? I found it tended to corrupt the log output if it was ever > triggered. >
Even though it's not ideal, we should keep it for now. Otherwise we don't have a way to get notified about the error. Thanks, Lukas