Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options
On 02/27/2018 04:22 AM, Thomas Huth wrote: On 27.02.2018 10:12, Cornelia Huck wrote: On Mon, 26 Feb 2018 14:44:45 -0500 "Collin L. Walling"wrote: On 02/26/2018 02:29 PM, Collin L. Walling wrote: On 02/26/2018 01:48 PM, Cornelia Huck wrote: On Mon, 26 Feb 2018 11:42:29 +0100 Thomas Huth wrote: [...] + switch (ipl->iplb.pbt) { + case S390_IPL_TYPE_CCW: + break; + default: + error_report("boot menu is not supported for this device type."); If I specify both a bootindex for a device and a -kernel parameter, I get this error message. Looks a tad odd, but not sure how to avoid it. Hmm... perhaps an additional check if no IPLB, then skip trying to set any boot menu data? [...] How shall we proceed? I'd be willing to pull this now and then apply fixups on top, or we can have a respin. Thomas? Since this is about a change in hw/s390x/ and not about pc-bios/s390-ccw/ (i.e. no need to rebuild the firmware binaries for this), I think I'd rather prefer a fix-up patch on top instead of a re-spin. Thomas I'll get right on it and post to the qemu lists. -- - Collin L Walling
Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options
On 27.02.2018 10:12, Cornelia Huck wrote: > On Mon, 26 Feb 2018 14:44:45 -0500 > "Collin L. Walling"wrote: > >> On 02/26/2018 02:29 PM, Collin L. Walling wrote: >>> On 02/26/2018 01:48 PM, Cornelia Huck wrote: On Mon, 26 Feb 2018 11:42:29 +0100 Thomas Huth wrote: [...] > + switch (ipl->iplb.pbt) { > + case S390_IPL_TYPE_CCW: > + break; > + default: > + error_report("boot menu is not supported for this device > type."); If I specify both a bootindex for a device and a -kernel parameter, I get this error message. Looks a tad odd, but not sure how to avoid it. >>> >>> Hmm... perhaps an additional check if no IPLB, then skip trying to set >>> any boot menu data? [...] > How shall we proceed? I'd be willing to pull this now and then apply > fixups on top, or we can have a respin. Thomas? Since this is about a change in hw/s390x/ and not about pc-bios/s390-ccw/ (i.e. no need to rebuild the firmware binaries for this), I think I'd rather prefer a fix-up patch on top instead of a re-spin. Thomas
Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options
On Mon, 26 Feb 2018 14:44:45 -0500 "Collin L. Walling"wrote: > On 02/26/2018 02:29 PM, Collin L. Walling wrote: > > On 02/26/2018 01:48 PM, Cornelia Huck wrote: > >> On Mon, 26 Feb 2018 11:42:29 +0100 > >> Thomas Huth wrote: > >> > >> [...] > >> > >>> 3 files changed, 66 insertions(+), 4 deletions(-) > >>> +static void s390_ipl_set_boot_menu(S390IPLState *ipl) > >>> +{ > >>> + QemuOptsList *plist = qemu_find_opts("boot-opts"); > >>> + QemuOpts *opts = QTAILQ_FIRST(>head); > >>> + uint8_t *flags = >qipl.qipl_flags; > >>> + uint32_t *timeout = >qipl.boot_menu_timeout; > >>> + const char *tmp; > >>> + unsigned long splash_time = 0; > >>> + > >>> + if (!get_boot_device(0)) { > >>> + if (boot_menu) { > >>> + error_report("boot menu requires a bootindex to be > >>> specified for " > >>> + "the IPL device."); > >>> + } > >>> + return; > >>> + } > >>> + > >>> + switch (ipl->iplb.pbt) { > >>> + case S390_IPL_TYPE_CCW: > >>> + break; > >>> + default: > >>> + error_report("boot menu is not supported for this device > >>> type."); > >> If I specify both a bootindex for a device and a -kernel parameter, I > >> get this error message. Looks a tad odd, but not sure how to avoid it. > > > > Hmm... perhaps an additional check if no IPLB, then skip trying to set > > any boot menu data? > > > >> [...] > > > > > Something like: > > if (!ipl->iplb.len) { > return; > } > > placed just below the if (!get_boot_device(0)) chunk fixed it for me.If > no IPLB was set, > then the IPLB fields should just all be zeros. Why not just check if > the length is 0 to > determine that we did not set an IPLB at all? Yes, that makes the message go away for me. > > also: > > if (!ipl->iplb.len) { > if (boot_menu) { > error_report("boot menu requires an IPLB to function"); > } > return; > } > > if you think an error message is needed (use a better message, mine is > not helpful but I > just wanted to demonstrate that the if (boot_menu) check should be > nested first). I'm not sure we want an error message for a command line that booted without any message previously. It's not like I'd expect a boot menu when I pass in a kernel anyway... > > Thanks for reporting this. Seems to be a few cases that I missed on my end. The usual result of different people trying things :) How shall we proceed? I'd be willing to pull this now and then apply fixups on top, or we can have a respin. Thomas?
Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options
On 02/26/2018 02:29 PM, Collin L. Walling wrote: On 02/26/2018 01:48 PM, Cornelia Huck wrote: On Mon, 26 Feb 2018 11:42:29 +0100 Thomas Huthwrote: [...] 3 files changed, 66 insertions(+), 4 deletions(-) +static void s390_ipl_set_boot_menu(S390IPLState *ipl) +{ + QemuOptsList *plist = qemu_find_opts("boot-opts"); + QemuOpts *opts = QTAILQ_FIRST(>head); + uint8_t *flags = >qipl.qipl_flags; + uint32_t *timeout = >qipl.boot_menu_timeout; + const char *tmp; + unsigned long splash_time = 0; + + if (!get_boot_device(0)) { + if (boot_menu) { + error_report("boot menu requires a bootindex to be specified for " + "the IPL device."); + } + return; + } + + switch (ipl->iplb.pbt) { + case S390_IPL_TYPE_CCW: + break; + default: + error_report("boot menu is not supported for this device type."); If I specify both a bootindex for a device and a -kernel parameter, I get this error message. Looks a tad odd, but not sure how to avoid it. Hmm... perhaps an additional check if no IPLB, then skip trying to set any boot menu data? [...] Something like: if (!ipl->iplb.len) { return; } placed just below the if (!get_boot_device(0)) chunk fixed it for me.If no IPLB was set, then the IPLB fields should just all be zeros. Why not just check if the length is 0 to determine that we did not set an IPLB at all? also: if (!ipl->iplb.len) { if (boot_menu) { error_report("boot menu requires an IPLB to function"); } return; } if you think an error message is needed (use a better message, mine is not helpful but I just wanted to demonstrate that the if (boot_menu) check should be nested first). Thanks for reporting this. Seems to be a few cases that I missed on my end. -- - Collin L Walling
Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options
On 02/26/2018 01:48 PM, Cornelia Huck wrote: On Mon, 26 Feb 2018 11:42:29 +0100 Thomas Huthwrote: From: "Collin L. Walling" Set boot menu options for an s390 guest and store them in the iplb. These options are set via the QEMU command line option: -boot menu=on|off[,splash-time=X] or via the libvirt domain xml: Where X represents some positive integer representing milliseconds. Any value set for loadparm will override all boot menu options. If loadparm=PROMPT, then the menu will be enabled without a timeout. Signed-off-by: Collin L. Walling Reviewed-by: Janosch Frank Reviewed-by: Thomas Huth Signed-off-by: Thomas Huth --- hw/s390x/ipl.c | 52 + hw/s390x/ipl.h | 9 +++-- pc-bios/s390-ccw/iplb.h | 9 +++-- Updating the header, but it is not consumed in the bios? 3 files changed, 66 insertions(+), 4 deletions(-) +static void s390_ipl_set_boot_menu(S390IPLState *ipl) +{ +QemuOptsList *plist = qemu_find_opts("boot-opts"); +QemuOpts *opts = QTAILQ_FIRST(>head); +uint8_t *flags = >qipl.qipl_flags; +uint32_t *timeout = >qipl.boot_menu_timeout; +const char *tmp; +unsigned long splash_time = 0; + +if (!get_boot_device(0)) { +if (boot_menu) { +error_report("boot menu requires a bootindex to be specified for " + "the IPL device."); +} +return; +} + +switch (ipl->iplb.pbt) { +case S390_IPL_TYPE_CCW: +break; +default: +error_report("boot menu is not supported for this device type."); If I specify both a bootindex for a device and a -kernel parameter, I get this error message. Looks a tad odd, but not sure how to avoid it. Hmm... perhaps an additional check if no IPLB, then skip trying to set any boot menu data? [...] -- - Collin L Walling