| Message ID | 20220818152114.213135-1-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v1,1/1] lib/s390x: fix SMP setup bug | expand |
Quoting Claudio Imbrenda (2022-08-18 17:21:14) > 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@linux.ibm.com> Reviewed-and-tested-by: Nico Boehr <nrb@linux.ibm.com>
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@linux.ibm.com> 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@linux.ibm.com> > --- > 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; .lowcore = (void *)0; .active = true; > + > + /* > + * 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 */ > } > } > spin_unlock(&lock);
On Fri, 19 Aug 2022 10:52:40 +0200 Janosch Frank <frankja@linux.ibm.com> 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@linux.ibm.com> > > 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@linux.ibm.com> > > > --- > > 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); >
On 8/19/22 11:12, Claudio Imbrenda wrote: > On Fri, 19 Aug 2022 10:52:40 +0200 > Janosch Frank <frankja@linux.ibm.com> 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@linux.ibm.com> >> >> 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@linux.ibm.com> >> >>> --- >>> 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 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. > >> >> >>> + >>> + /* >>> + * 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); >> >
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@linux.ibm.com> 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@linux.ibm.com> >>> >>> 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@linux.ibm.com> >>> [...] >> >> 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@linux.ibm.com> Thanks, picked
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 }; + + /* + * 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]; } } spin_unlock(&lock);
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@linux.ibm.com> --- lib/s390x/io.c | 9 +++++++++ lib/s390x/smp.c | 1 + 2 files changed, 10 insertions(+)