| Message ID | 1523657288-6587-3-git-send-email-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Small fixes for s390x QEMU boot menu | expand |
On 14.04.2018 00:08, Collin Walling wrote: > Rename the loadparm char array in main.c to loadparm_str and > increased the size by one byte to account for a null termination > when converting the loadparm string to an int via atoui. We > 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> > Reviewed-by: Thomas Huth <thuth@redhat.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; > } When compiling this code, my GCC (v4.8.5) complains: CC s390x-softmmu/hw/s390x/ipl.o In file included from /usr/include/string.h:638:0, from /home/thuth/devel/qemu/include/qemu/osdep.h:69, from /home/thuth/devel/qemu/hw/s390x/ipl.c:14: In function ‘memset’, inlined from ‘s390_ipl_set_loadparm’ at /home/thuth/devel/qemu/hw/s390x/ipl.c:376:15: /usr/include/bits/string3.h:81:30: error: call to ‘__warn_memset_zero_len’ declared with attribute warning: memset used with constant zero length parameter; this could be due to transposed parameters [-Werror] __warn_memset_zero_len (); I guess this might happen due to some internal loop unrolling of GCC or something similar ... to make sure that we can compile the code also without warnings, could you please add a check around the memset à la: if (i < 8) { memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ } Thanks, Thomas
On 04/16/2018 08:27 AM, Thomas Huth wrote: > On 14.04.2018 00:08, Collin Walling wrote: >> Rename the loadparm char array in main.c to loadparm_str and >> increased the size by one byte to account for a null termination >> when converting the loadparm string to an int via atoui. We >> 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> >> Reviewed-by: Thomas Huth <thuth@redhat.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; >> } > > When compiling this code, my GCC (v4.8.5) complains: > > CC s390x-softmmu/hw/s390x/ipl.o > In file included from /usr/include/string.h:638:0, > from /home/thuth/devel/qemu/include/qemu/osdep.h:69, > from /home/thuth/devel/qemu/hw/s390x/ipl.c:14: > In function ‘memset’, > inlined from ‘s390_ipl_set_loadparm’ at > /home/thuth/devel/qemu/hw/s390x/ipl.c:376:15: > /usr/include/bits/string3.h:81:30: error: call to > ‘__warn_memset_zero_len’ declared with attribute warning: memset used > with constant zero length parameter; this could be due to transposed > parameters [-Werror] > __warn_memset_zero_len (); > > I guess this might happen due to some internal loop unrolling of GCC or > something similar ... to make sure that we can compile the code also > without warnings, could you please add a check around the memset à la: > > if (i < 8) { > memset(loadparm + i, 0x40, 8 - i); /* fill with EBCDIC spaces */ > } > > Thanks, > Thomas > Can do.
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));