| Message ID | 1523372486-13190-3-git-send-email-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Small fixes for s390x QEMU boot menu | expand |
On 10.04.2018 17:01, Collin Walling wrote: > Rename the loadparm char array in main.c to loadparm_str and > increase the size by one byte to account for a null termination > when converting the loadparm string to an int via atoui. Also > allow the boot menu to be enabled when loadparm is set to an > empty string or a series of spaces. > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > Reported-by: Vasily Gorbik <gor@linux.ibm.com> > --- > hw/s390x/ipl.c | 2 ++ > pc-bios/s390-ccw/main.c | 14 +++++++------- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index fdeaec3..23b5b54 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) > loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; > } > > + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ > + > g_free(lp); > return 0; > } > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > index 9d9f8cf..26f9adf 100644 > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -15,11 +15,11 @@ > char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); > static SubChannelId blk_schid = { .one = 1 }; > IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; > +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 }; > QemuIplParameters qipl; > > #define LOADPARM_PROMPT "PROMPT " > -#define LOADPARM_EMPTY "........" > +#define LOADPARM_EMPTY " " Sorry for my ignorance, but why was the old string containing dots? Thomas
On 04/12/2018 02:57 PM, Thomas Huth wrote: > On 10.04.2018 17:01, Collin Walling wrote: >> Rename the loadparm char array in main.c to loadparm_str and >> increase the size by one byte to account for a null termination >> when converting the loadparm string to an int via atoui. Also >> allow the boot menu to be enabled when loadparm is set to an >> empty string or a series of spaces. >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> Reported-by: Vasily Gorbik <gor@linux.ibm.com> >> --- >> hw/s390x/ipl.c | 2 ++ >> pc-bios/s390-ccw/main.c | 14 +++++++------- >> 2 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index fdeaec3..23b5b54 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) >> loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; >> } >> >> + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ >> + >> g_free(lp); >> return 0; >> } >> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >> index 9d9f8cf..26f9adf 100644 >> --- a/pc-bios/s390-ccw/main.c >> +++ b/pc-bios/s390-ccw/main.c >> @@ -15,11 +15,11 @@ >> char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >> static SubChannelId blk_schid = { .one = 1 }; >> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 }; >> QemuIplParameters qipl; >> >> #define LOADPARM_PROMPT "PROMPT " >> -#define LOADPARM_EMPTY "........" >> +#define LOADPARM_EMPTY " " > > Sorry for my ignorance, but why was the old string containing dots? > > Thomas > No need for apologies :) If -machine loadparm is *not* present on the command line, then the loadparm in the sclp will be a series of nulls. For whatever reason, that gets translated into a series of dots. If -machine loadparm="" is present, then loadparm will in the sclp will be a series of spaces. We want to enable the boot menu for both of these cases and, to make things easier, this patch replaces any nulls (dots) to spaces when setting loadparm so that we only have to handle one case instead of two within the bios.
On 04/12/2018 04:57 PM, Collin Walling wrote: > On 04/12/2018 02:57 PM, Thomas Huth wrote: >> On 10.04.2018 17:01, Collin Walling wrote: >>> Rename the loadparm char array in main.c to loadparm_str and >>> increase the size by one byte to account for a null termination >>> when converting the loadparm string to an int via atoui. Also >>> allow the boot menu to be enabled when loadparm is set to an >>> empty string or a series of spaces. >>> >>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>> Reported-by: Vasily Gorbik <gor@linux.ibm.com> >>> --- >>> hw/s390x/ipl.c | 2 ++ >>> pc-bios/s390-ccw/main.c | 14 +++++++------- >>> 2 files changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index fdeaec3..23b5b54 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) >>> loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; >>> } >>> >>> + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ >>> + >>> g_free(lp); >>> return 0; >>> } >>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>> index 9d9f8cf..26f9adf 100644 >>> --- a/pc-bios/s390-ccw/main.c >>> +++ b/pc-bios/s390-ccw/main.c >>> @@ -15,11 +15,11 @@ >>> char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >>> static SubChannelId blk_schid = { .one = 1 }; >>> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >>> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >>> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 }; >>> QemuIplParameters qipl; >>> >>> #define LOADPARM_PROMPT "PROMPT " >>> -#define LOADPARM_EMPTY "........" >>> +#define LOADPARM_EMPTY " " >> >> Sorry for my ignorance, but why was the old string containing dots? >> >> Thomas >> > > No need for apologies :) > > If -machine loadparm is *not* present on the command line, then the loadparm in the sclp > will be a series of nulls. For whatever reason, that gets translated into a series of dots. > It's because of the ebc2asc table we use for conversion, which results in the dots when converting from ebcdic_to_ascii. > If -machine loadparm="" is present, then loadparm will in the sclp will be a series of > spaces. > > We want to enable the boot menu for both of these cases and, to make things easier, this > patch replaces any nulls (dots) to spaces when setting loadparm so that we only have to > handle one case instead of two within the bios. > >
On 12.04.2018 23:04, Farhan Ali wrote: > > > On 04/12/2018 04:57 PM, Collin Walling wrote: >> On 04/12/2018 02:57 PM, Thomas Huth wrote: >>> On 10.04.2018 17:01, Collin Walling wrote: >>>> Rename the loadparm char array in main.c to loadparm_str and >>>> increase the size by one byte to account for a null termination >>>> when converting the loadparm string to an int via atoui. Also >>>> allow the boot menu to be enabled when loadparm is set to an >>>> empty string or a series of spaces. >>>> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>> Reported-by: Vasily Gorbik <gor@linux.ibm.com> >>>> --- >>>> hw/s390x/ipl.c | 2 ++ >>>> pc-bios/s390-ccw/main.c | 14 +++++++------- >>>> 2 files changed, 9 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>>> index fdeaec3..23b5b54 100644 >>>> --- a/hw/s390x/ipl.c >>>> +++ b/hw/s390x/ipl.c >>>> @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) >>>> loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; >>>> } >>>> + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC >>>> spaces */ >>>> + >>>> g_free(lp); >>>> return 0; >>>> } >>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>>> index 9d9f8cf..26f9adf 100644 >>>> --- a/pc-bios/s390-ccw/main.c >>>> +++ b/pc-bios/s390-ccw/main.c >>>> @@ -15,11 +15,11 @@ >>>> char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); >>>> static SubChannelId blk_schid = { .one = 1 }; >>>> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >>>> -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; >>>> +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 }; >>>> QemuIplParameters qipl; >>>> #define LOADPARM_PROMPT "PROMPT " >>>> -#define LOADPARM_EMPTY "........" >>>> +#define LOADPARM_EMPTY " " >>> >>> Sorry for my ignorance, but why was the old string containing dots? >>> >>> Thomas >>> >> >> No need for apologies :) >> >> If -machine loadparm is *not* present on the command line, then the >> loadparm in the sclp >> will be a series of nulls. For whatever reason, that gets translated >> into a series of dots. >> > > It's because of the ebc2asc table we use for conversion, which results > in the dots when converting from ebcdic_to_ascii. Ah, great, thanks to both of you for the explanation, that was the part that I was missing. I was only looking at the tables in include/hw/s390x/ebcdic.h (since I thought that the dots were already created on the QEMU side), and did not expect that the pc-bios could create them. The patch now makes sense to me: Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index fdeaec3..23b5b54 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -352,6 +352,8 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; } + memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ + g_free(lp); return 0; } diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index 9d9f8cf..26f9adf 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -15,11 +15,11 @@ char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE))); static SubChannelId blk_schid = { .one = 1 }; IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); -static char loadparm[8] = { 0, 0, 0, 0, 0, 0, 0, 0 }; +static char loadparm_str[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 }; QemuIplParameters qipl; #define LOADPARM_PROMPT "PROMPT " -#define LOADPARM_EMPTY "........" +#define LOADPARM_EMPTY " " #define BOOT_MENU_FLAG_MASK (QIPL_FLAG_BM_OPTS_CMD | QIPL_FLAG_BM_OPTS_ZIPL) /* @@ -45,7 +45,7 @@ void panic(const char *string) unsigned int get_loadparm_index(void) { - return atoui(loadparm); + return atoui(loadparm_str); } static bool find_dev(Schib *schib, int dev_no) @@ -80,13 +80,13 @@ static bool find_dev(Schib *schib, int dev_no) static void menu_setup(void) { - if (memcmp(loadparm, LOADPARM_PROMPT, 8) == 0) { + if (memcmp(loadparm_str, LOADPARM_PROMPT, 8) == 0) { menu_set_parms(QIPL_FLAG_BM_OPTS_CMD, 0); return; } /* If loadparm was set to any other value, then do not enable menu */ - if (memcmp(loadparm, LOADPARM_EMPTY, 8) != 0) { + if (memcmp(loadparm_str, LOADPARM_EMPTY, 8) != 0) { return; } @@ -116,8 +116,8 @@ static void virtio_setup(void) */ enable_mss_facility(); - sclp_get_loadparm_ascii(loadparm); - memcpy(ldp + 10, loadparm, 8); + sclp_get_loadparm_ascii(loadparm_str); + memcpy(ldp + 10, loadparm_str, 8); sclp_print(ldp); memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
Rename the loadparm char array in main.c to loadparm_str and increase the size by one byte to account for a null termination when converting the loadparm string to an int via atoui. Also allow the boot menu to be enabled when loadparm is set to an empty string or a series of spaces. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reported-by: Vasily Gorbik <gor@linux.ibm.com> --- hw/s390x/ipl.c | 2 ++ pc-bios/s390-ccw/main.c | 14 +++++++------- 2 files changed, 9 insertions(+), 7 deletions(-)