[Skiboot] [PATCH 1/4] ipmi: Fix potential infinite loop in sync_msg polling

[Nicholas Piggin]

On Mon May 15, 2023 at 6:29 AM AEST, Stewart Smith wrote:
>
> > On May 13, 2023, at 05:12, Nicholas Piggin <npiggin at gmail.com> wrote:
> > 
> > Current gcc with -Os happens to generate code that re-loads the variable
> > in the loop, but that could change without notice, and with -O2 it does
> > infinite loop if sync_msg is !NULL, because it is not declared volatile
> > and there is no compiler barrier in the loop.
>
> I guess it’s a good thing we only ever had size constraints rather than performance ones :)

Yeah, I only found it by inspection when looking for that IPMI
issue I had with kvm-unit-tests.

> > Add the usual cpu_relax() there to provide the compiler barrier.
>
> I’m trying to remember if we needed a call to go do background work if only one CPU thread but it turns out my memory is getting hazy on the details, and I 100% need more coffee right now.

Interesting point. We have time_wait vs time_wait_nopoll of course,
which get to the issue you're talkingabout. And there *are* a few
cpu_relax loops that also do some polling explicitly. Might be worth
auditing them all (there aren't too many) and maybe rename to
cpu_relax_nopoll() so it doesn't just get used without considering
pollers...

In this case the other CPU that owns the sync_msg that we're waiting
to release is doing polling, so I think that's alright.

> > Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>
> Feel free to have my first skiboot reviewed by in a while though :)
>
> Reviewed-by: Stewart Smith <stewart at flamingspork.com <mailto:stewart at flamingspork.com>>

Thanks,
Nick