On 05/22/2018 08:44 AM, Cornelia Huck wrote: > On Tue, 15 May 2018 10:45:02 +0200 > Laszlo Ersek <ler...@redhat.com> wrote: > >> I've come across this patch in downstream review (although I really have >> zero background in s390x), and Cornelia suggested I might want to repeat >> my comments on the upstream list too: >> >> On 05/04/18 09:25, Cornelia Huck wrote: >>> From: Collin Walling <wall...@linux.ibm.com> >>> >>> 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 <wall...@linux.ibm.com> >>> Reported-by: Vasily Gorbik <g...@linux.ibm.com> >>> Reviewed-by: Thomas Huth <th...@redhat.com> >>> Reviewed-by: Janosch Frank <fran...@linux.ibm.com> >>> Signed-off-by: Thomas Huth <th...@redhat.com> >>> --- >>> hw/s390x/ipl.c | 4 ++++ >>> pc-bios/s390-ccw/main.c | 14 +++++++------- >>> 2 files changed, 11 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index fb554ab156..150f6c0582 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -373,6 +373,10 @@ int s390_ipl_set_loadparm(uint8_t *loadparm) >>> loadparm[i] = ascii2ebcdic[(uint8_t) lp[i]]; >>> } >>> >>> + if (i < 8) { >>> + 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 9d9f8cf4d3..26f9adf84a 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)); >>> >> >> So here I said, "There are a bunch of naked 8 constants; those should >> all be (sizeof loadparm_str - 1), I think. Should be a separate patch, >> of course." > > Alternatively, as loadparm size is architectured IIRC and unlikely to > change, do >
Correct. > #define LOADPARM_LEN 8 > char loadparm_str[LOADPARM_LEN + 1]; > > and compare against LOADPARM_LEN... opinions? > I have another (unrelated) QEMU patch to throw on the list, so I can get to this today. -- Respectfully, - Collin Walling