On 8/19/22 12:31, Janosch Frank wrote:
On 8/19/22 11:12, Claudio Imbrenda wrote:
On Fri, 19 Aug 2022 10:52:40 +0200
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
On 8/18/22 17:21, Claudio Imbrenda wrote:
The lowcore pointer pointing to the current CPU (THIS_CPU) was not
initialized for the boot CPU. The pointer is needed for correct
interrupt handling, which is needed in the setup process before the
struct cpu array is allocated.
The bug went unnoticed because some environments (like qemu/KVM) clear
all memory and don't write anything in the lowcore area before starting
the payload. The pointer thus pointed to 0, an area of memory also not
used. Other environments will write to memory before starting the
payload, causing the unit tests to crash at the first interrupt.
Fix by assigning a temporary struct cpu before the rest of the setup
process, and assigning the pointer to the correct allocated struct
during smp initialization.
Fixes: 4e5dd758 ("lib: s390x: better smp interrupt checks")
Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
I've considered letting the IPL cpu have a static struct cpu and setting
it up in cstart64.S. But that would mean that we would need extra
handling when using smp functions and that'll look even worse.
Reported-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
[...]
this temporary struct is then not accessible from smp_setup, so it
can't be memcpy-ed.
if you really want something meaningful in the temporary struct, it has
to be initialized in smp.c and called in io.c (something like
smp_boot_cpu_tmp_setup(&this_cpu_tmp) ), but then still no memcpy.
in the end the struct cpu is needed only to allow interrupts to happen
without crashes, I don't think we strictly need initialization
Ugh, this feels like a quick fix.
But alright, I've just tried setting it up from cstart64.S and it's way
more ugly code so let's stick with this for now.
Anyway:
Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
Thanks, picked
