| Message ID | 1574944437-31182-1-git-send-email-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] pc-bios/s390-ccw: fix sclp_get_loadparm_ascii | expand |
Ack. Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) as this fixes a regression. Opinions? On 28.11.19 13:33, Claudio Imbrenda wrote: > The existing s390 bios gets the LOADPARM information from the system using > an SCLP call that specifies a buffer length too small to contain all the > output. > > The recent fixes in the SCLP code have exposed this bug, since now the > SCLP call will return an error (as per architecture) instead of > writing partially and completing successfully. > > The solution is simply to specify the full page length as the SCCB > length instead of a smaller size. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > pc-bios/s390-ccw/sclp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index c0223fa..7251f9a 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > ReadInfo *sccb = (void *)_sccb; > > memset((char *)_sccb, 0, sizeof(ReadInfo)); > - sccb->h.length = sizeof(ReadInfo); > + sccb->h.length = SCCB_SIZE; > if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > } >
On 11/28/19 1:33 PM, Claudio Imbrenda wrote: > The existing s390 bios gets the LOADPARM information from the system using > an SCLP call that specifies a buffer length too small to contain all the > output. > > The recent fixes in the SCLP code have exposed this bug, since now the > SCLP call will return an error (as per architecture) instead of > writing partially and completing successfully. > > The solution is simply to specify the full page length as the SCCB > length instead of a smaller size. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") "pc-bios/s390-ccw: fix sclp readinfo buffer length indication"? Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > pc-bios/s390-ccw/sclp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index c0223fa..7251f9a 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > ReadInfo *sccb = (void *)_sccb; > > memset((char *)_sccb, 0, sizeof(ReadInfo)); > - sccb->h.length = sizeof(ReadInfo); > + sccb->h.length = SCCB_SIZE; > if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > } >
On Thu, Nov 28, 2019 at 01:33 PM +0100, Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > The existing s390 bios gets the LOADPARM information from the system using > an SCLP call that specifies a buffer length too small to contain all the > output. > > The recent fixes in the SCLP code have exposed this bug, since now the > SCLP call will return an error (as per architecture) instead of > writing partially and completing successfully. > > The solution is simply to specify the full page length as the SCCB > length instead of a smaller size. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > pc-bios/s390-ccw/sclp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index c0223fa..7251f9a 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > ReadInfo *sccb = (void *)_sccb; > > memset((char *)_sccb, 0, sizeof(ReadInfo)); > - sccb->h.length = sizeof(ReadInfo); > + sccb->h.length = SCCB_SIZE; > if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > } > -- > 2.7.4 Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, 28 Nov 2019 13:33:57 +0100
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
[...]
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
I forgot this:
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
[...]
please add the reported-by when merging :)
On Thu, 28 Nov 2019 13:35:29 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > Ack. > > Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) > as this fixes a regression. Opinions? I fear that this is a bit late for 4.2... but this should get a cc:stable. > > > > On 28.11.19 13:33, Claudio Imbrenda wrote: > > The existing s390 bios gets the LOADPARM information from the system using > > an SCLP call that specifies a buffer length too small to contain all the > > output. > > > > The recent fixes in the SCLP code have exposed this bug, since now the > > SCLP call will return an error (as per architecture) instead of > > writing partially and completing successfully. > > > > The solution is simply to specify the full page length as the SCCB > > length instead of a smaller size. > > > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > > Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > pc-bios/s390-ccw/sclp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > > index c0223fa..7251f9a 100644 > > --- a/pc-bios/s390-ccw/sclp.c > > +++ b/pc-bios/s390-ccw/sclp.c > > @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > > ReadInfo *sccb = (void *)_sccb; > > > > memset((char *)_sccb, 0, sizeof(ReadInfo)); > > - sccb->h.length = sizeof(ReadInfo); > > + sccb->h.length = SCCB_SIZE; > > if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > > ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > > } > > The change seems sane.
On 28.11.19 13:45, Cornelia Huck wrote: > On Thu, 28 Nov 2019 13:35:29 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> Ack. >> >> Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) >> as this fixes a regression. Opinions? > > I fear that this is a bit late for 4.2... but this should get a > cc:stable. So we would rather ship a qemu regression instead of pushing a 1 line fixup? Peter, what is the current state of 4.2? does it look like we will have an rc4 or is everything else silent. > >> >> >> >> On 28.11.19 13:33, Claudio Imbrenda wrote: >>> The existing s390 bios gets the LOADPARM information from the system using >>> an SCLP call that specifies a buffer length too small to contain all the >>> output. >>> >>> The recent fixes in the SCLP code have exposed this bug, since now the >>> SCLP call will return an error (as per architecture) instead of >>> writing partially and completing successfully. >>> >>> The solution is simply to specify the full page length as the SCCB >>> length instead of a smaller size. >>> >>> Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") >>> Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") >>> >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/sclp.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >>> index c0223fa..7251f9a 100644 >>> --- a/pc-bios/s390-ccw/sclp.c >>> +++ b/pc-bios/s390-ccw/sclp.c >>> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) >>> ReadInfo *sccb = (void *)_sccb; >>> >>> memset((char *)_sccb, 0, sizeof(ReadInfo)); >>> - sccb->h.length = sizeof(ReadInfo); >>> + sccb->h.length = SCCB_SIZE; >>> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { >>> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); >>> } >>> > > The change seems sane. >
On 28/11/2019 13.35, Christian Borntraeger wrote: > Ack. > > Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) > as this fixes a regression. Opinions? If we do another rc of 4.2, I think we definitely want this to be included, otherwise quite a bunch of things don't work anymore as expected, e.g. "-boot menu=on"... >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >> index c0223fa..7251f9a 100644 >> --- a/pc-bios/s390-ccw/sclp.c >> +++ b/pc-bios/s390-ccw/sclp.c >> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) >> ReadInfo *sccb = (void *)_sccb; >> >> memset((char *)_sccb, 0, sizeof(ReadInfo)); >> - sccb->h.length = sizeof(ReadInfo); >> + sccb->h.length = SCCB_SIZE; >> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { >> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); >> } I gave it a quick try, and this fixes "-boot menu=on" for me, so: Tested-by: Thomas Huth <thuth@redhat.com>
On Thu, 28 Nov 2019 14:11:38 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 28/11/2019 13.35, Christian Borntraeger wrote: > > Ack. > > > > Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) > > as this fixes a regression. Opinions? > > If we do another rc of 4.2, I think we definitely want this to be > included, otherwise quite a bunch of things don't work anymore as > expected, e.g. "-boot menu=on"... I do agree we want this if possible; the question is really the "possible" part... > > >> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > >> index c0223fa..7251f9a 100644 > >> --- a/pc-bios/s390-ccw/sclp.c > >> +++ b/pc-bios/s390-ccw/sclp.c > >> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > >> ReadInfo *sccb = (void *)_sccb; > >> > >> memset((char *)_sccb, 0, sizeof(ReadInfo)); > >> - sccb->h.length = sizeof(ReadInfo); > >> + sccb->h.length = SCCB_SIZE; > >> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > >> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > >> } > > I gave it a quick try, and this fixes "-boot menu=on" for me, so: > > Tested-by: Thomas Huth <thuth@redhat.com> Thanks. FWIW, I'm currently working to put this + the rebuild on my s390-fixes branch.
On Thu, 28 Nov 2019 13:33:57 +0100 Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > The existing s390 bios gets the LOADPARM information from the system using > an SCLP call that specifies a buffer length too small to contain all the > output. > > The recent fixes in the SCLP code have exposed this bug, since now the > SCLP call will return an error (as per architecture) instead of > writing partially and completing successfully. > > The solution is simply to specify the full page length as the SCCB > length instead of a smaller size. > > Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") > Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > pc-bios/s390-ccw/sclp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c > index c0223fa..7251f9a 100644 > --- a/pc-bios/s390-ccw/sclp.c > +++ b/pc-bios/s390-ccw/sclp.c > @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) > ReadInfo *sccb = (void *)_sccb; > > memset((char *)_sccb, 0, sizeof(ReadInfo)); > - sccb->h.length = sizeof(ReadInfo); > + sccb->h.length = SCCB_SIZE; > if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { > ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); > } Acked-by: Cornelia Huck <cohuck@redhat.com>
On 28.11.19 14:16, Cornelia Huck wrote: > On Thu, 28 Nov 2019 14:11:38 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 28/11/2019 13.35, Christian Borntraeger wrote: >>> Ack. >>> >>> Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) >>> as this fixes a regression. Opinions? >> >> If we do another rc of 4.2, I think we definitely want this to be >> included, otherwise quite a bunch of things don't work anymore as >> expected, e.g. "-boot menu=on"... > > I do agree we want this if possible; the question is really the > "possible" part... Given the issues that we see without that fix, I think this would qualify do an rc4 (or even to apply it on top of rc3 to become 4.2) Maybe just do a pull request? > >> >>>> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c >>>> index c0223fa..7251f9a 100644 >>>> --- a/pc-bios/s390-ccw/sclp.c >>>> +++ b/pc-bios/s390-ccw/sclp.c >>>> @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) >>>> ReadInfo *sccb = (void *)_sccb; >>>> >>>> memset((char *)_sccb, 0, sizeof(ReadInfo)); >>>> - sccb->h.length = sizeof(ReadInfo); >>>> + sccb->h.length = SCCB_SIZE; >>>> if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { >>>> ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); >>>> } >> >> I gave it a quick try, and this fixes "-boot menu=on" for me, so: >> >> Tested-by: Thomas Huth <thuth@redhat.com> > > Thanks. > > FWIW, I'm currently working to put this + the rebuild on my s390-fixes > branch. >
On Thu, 28 Nov 2019 at 12:48, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > > On 28.11.19 13:45, Cornelia Huck wrote: > > On Thu, 28 Nov 2019 13:35:29 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> Ack. > >> > >> Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) > >> as this fixes a regression. Opinions? > > > > I fear that this is a bit late for 4.2... but this should get a > > cc:stable. > > So we would rather ship a qemu regression instead of pushing a 1 line fixup? > Peter, what is the current state of 4.2? does it look like we will have an rc4 > or is everything else silent. There isn't currently anything else that would need an rc4. thanks -- PMM
On 28.11.19 16:05, Peter Maydell wrote: > On Thu, 28 Nov 2019 at 12:48, Christian Borntraeger > <borntraeger@de.ibm.com> wrote: >> >> >> >> On 28.11.19 13:45, Cornelia Huck wrote: >>> On Thu, 28 Nov 2019 13:35:29 +0100 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> Ack. >>>> >>>> Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) >>>> as this fixes a regression. Opinions? >>> >>> I fear that this is a bit late for 4.2... but this should get a >>> cc:stable. >> >> So we would rather ship a qemu regression instead of pushing a 1 line fixup? >> Peter, what is the current state of 4.2? does it look like we will have an rc4 >> or is everything else silent. > > There isn't currently anything else that would need an rc4. I would vote for getting this patch still in. And I think we probably do not need an rc4 for that, the fix seems pretty straight forward.
On 29/11/2019 08.38, Christian Borntraeger wrote: > > > On 28.11.19 16:05, Peter Maydell wrote: >> On Thu, 28 Nov 2019 at 12:48, Christian Borntraeger >> <borntraeger@de.ibm.com> wrote: >>> >>> >>> >>> On 28.11.19 13:45, Cornelia Huck wrote: >>>> On Thu, 28 Nov 2019 13:35:29 +0100 >>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>> >>>>> Ack. >>>>> >>>>> Conny, I think this would be really nice to have for 4.2 (together with a bios rebuild) >>>>> as this fixes a regression. Opinions? >>>> >>>> I fear that this is a bit late for 4.2... but this should get a >>>> cc:stable. >>> >>> So we would rather ship a qemu regression instead of pushing a 1 line fixup? >>> Peter, what is the current state of 4.2? does it look like we will have an rc4 >>> or is everything else silent. >> >> There isn't currently anything else that would need an rc4. > > I would vote for getting this patch still in. And I think we probably do not need an > rc4 for that, the fix seems pretty straight forward. Ok, I'm going to build the binaries and send a pull request today. Thomas
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index c0223fa..7251f9a 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -112,7 +112,7 @@ void sclp_get_loadparm_ascii(char *loadparm) ReadInfo *sccb = (void *)_sccb; memset((char *)_sccb, 0, sizeof(ReadInfo)); - sccb->h.length = sizeof(ReadInfo); + sccb->h.length = SCCB_SIZE; if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) { ebcdic_to_ascii((char *) sccb->loadparm, loadparm, LOADPARM_LEN); }
The existing s390 bios gets the LOADPARM information from the system using an SCLP call that specifies a buffer length too small to contain all the output. The recent fixes in the SCLP code have exposed this bug, since now the SCLP call will return an error (as per architecture) instead of writing partially and completing successfully. The solution is simply to specify the full page length as the SCCB length instead of a smaller size. Fixes: 832be0d8a3bb ("s390x: sclp: Report insufficient SCCB length") Fixes: 9a22473c70f3 ("pc-bios/s390-ccw: get LOADPARM stored in SCP Read Info") Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- pc-bios/s390-ccw/sclp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)