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>
>
> > ---
> > lib/s390x/io.c | 9 +++++++++
> > lib/s390x/smp.c | 1 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> > index a4f1b113..fb7b7dda 100644
> > --- a/lib/s390x/io.c
> > +++ b/lib/s390x/io.c
> > @@ -33,6 +33,15 @@ void puts(const char *s)
> >
> > void setup(void)
> > {
> > + struct cpu this_cpu_tmp = { 0 };
>
> We can setup these struct members here and memcpy in smp_setup()
> .addr = stap();
> .stack = stackptr;
stackptr is accessible in io.c (I would need to add an extern
declaration)
> .lowcore = (void *)0;
> .active = true;
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
>
>
> > +
> > + /*
> > + * Set a temporary empty struct cpu for the boot CPU, needed for
> > + * correct interrupt handling in the setup process.
> > + * smp_setup will allocate and set the permanent one.
> > + */
> > + THIS_CPU = &this_cpu_tmp;
> > +
> > setup_args_progname(ipl_args);
> > setup_facilities();
> > sclp_read_info();
> > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> > index 0d98c17d..03d6d2a4 100644
> > --- a/lib/s390x/smp.c
> > +++ b/lib/s390x/smp.c
> > @@ -353,6 +353,7 @@ void smp_setup(void)
> > cpus[0].stack = stackptr;
> > cpus[0].lowcore = (void *)0;
> > cpus[0].active = true;
> > + THIS_CPU = &cpus[0];
>
> /* Override temporary struct cpu address with permanent one */
will be done
>
> > }
> > }
> > spin_unlock(&lock);
>
