| Message ID | 20230627074703.99608-3-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios/s390-ccw: Fixes and improvements for start.S | expand |
Thomas Huth <thuth@redhat.com> writes: > Providing the space of a stack frame is the duty of the caller, > so we should reserve 160 bytes before jumping into the main function. > Otherwise the main() function might write past the stack array. > > While we're at it, add a proper STACK_SIZE macro for the stack size > instead of using magic numbers (this is also required for the following > patch). > > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > Reviewed-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > pc-bios/s390-ccw/start.S | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index d29de09cc6..29b0a9ece0 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -10,10 +10,12 @@ > * directory. > */ > > +#define STACK_SIZE 0x8000 > + > .globl _start > _start: > > - larl %r15,stack + 0x8000 /* Set up stack */ > + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ ^^^ You can also add a macro for this - e.g. STACK_FRAME_SIZE. Besides that, Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> > > /* clear bss */ > larl %r2,__bss_start > -- > 2.39.3 >
On 27/6/23 10:26, Marc Hartmayer wrote: > Thomas Huth <thuth@redhat.com> writes: > >> Providing the space of a stack frame is the duty of the caller, >> so we should reserve 160 bytes before jumping into the main function. >> Otherwise the main() function might write past the stack array. >> >> While we're at it, add a proper STACK_SIZE macro for the stack size >> instead of using magic numbers (this is also required for the following >> patch). >> >> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> pc-bios/s390-ccw/start.S | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> +#define STACK_SIZE 0x8000 >> + >> .globl _start >> _start: >> >> - larl %r15,stack + 0x8000 /* Set up stack */ >> + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ > ^^^ > You can also add a macro for this > - e.g. STACK_FRAME_SIZE. Yes please :) No need to respin. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Besides that, > Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> =
On Tue, 27 Jun 2023 09:47:01 +0200 Thomas Huth <thuth@redhat.com> wrote: > Providing the space of a stack frame is the duty of the caller, > so we should reserve 160 bytes before jumping into the main function. > Otherwise the main() function might write past the stack array. > > While we're at it, add a proper STACK_SIZE macro for the stack size > instead of using magic numbers (this is also required for the following > patch). > > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > Reviewed-by: Cédric Le Goater <clg@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> with Marc's suggestion applied: Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > pc-bios/s390-ccw/start.S | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index d29de09cc6..29b0a9ece0 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -10,10 +10,12 @@ > * directory. > */ > > +#define STACK_SIZE 0x8000 > + > .globl _start > _start: > > - larl %r15,stack + 0x8000 /* Set up stack */ > + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ > > /* clear bss */ > larl %r2,__bss_start
On Tue, 2023-06-27 at 11:14 +0200, Philippe Mathieu-Daudé wrote: > On 27/6/23 10:26, Marc Hartmayer wrote: > > Thomas Huth <thuth@redhat.com> writes: > > > > > Providing the space of a stack frame is the duty of the caller, > > > so we should reserve 160 bytes before jumping into the main > > > function. > > > Otherwise the main() function might write past the stack array. > > > > > > While we're at it, add a proper STACK_SIZE macro for the stack > > > size > > > instead of using magic numbers (this is also required for the > > > following > > > patch). > > > > > > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > --- > > > pc-bios/s390-ccw/start.S | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > +#define STACK_SIZE 0x8000 > > > + > > > .globl _start > > > _start: > > > > > > - larl %r15,stack + 0x8000 /* Set up stack */ > > > + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ > > ^^^ > > You can also add a macro > > for this > > - e.g. STACK_FRAME_SIZE. > > Yes please :) No need to respin. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > Besides that, > > Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> > = Ditto :) Reviewed-by: Eric Farman <farman@linux.ibm.com>
On 27/06/2023 11.14, Philippe Mathieu-Daudé wrote: > On 27/6/23 10:26, Marc Hartmayer wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> Providing the space of a stack frame is the duty of the caller, >>> so we should reserve 160 bytes before jumping into the main function. >>> Otherwise the main() function might write past the stack array. >>> >>> While we're at it, add a proper STACK_SIZE macro for the stack size >>> instead of using magic numbers (this is also required for the following >>> patch). >>> >>> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> >>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> pc-bios/s390-ccw/start.S | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) > > >>> +#define STACK_SIZE 0x8000 >>> + >>> .globl _start >>> _start: >>> - larl %r15,stack + 0x8000 /* Set up stack */ >>> + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ >> ^^^ >> You can also add a macro for this >> - e.g. STACK_FRAME_SIZE. > > Yes please :) No need to respin. Ok, I'll add: diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -10,12 +10,13 @@ * directory. */ -#define STACK_SIZE 0x8000 +#define STACK_SIZE 0x8000 +#define STACK_FRAME_SIZE 160 .globl _start _start: - larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ + larl %r15,stack + STACK_SIZE - STACK_FRAME_SIZE /* Set up stack */ /* clear bss */ larl %r2,.bss Thanks, Thomas
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index d29de09cc6..29b0a9ece0 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -10,10 +10,12 @@ * directory. */ +#define STACK_SIZE 0x8000 + .globl _start _start: - larl %r15,stack + 0x8000 /* Set up stack */ + larl %r15,stack + STACK_SIZE - 160 /* Set up stack */ /* clear bss */ larl %r2,__bss_start