| Message ID | 1519641757-12396-7-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL-for-s390x,01/14] s390-ccw: refactor boot map table code | expand |
On Mon, 26 Feb 2018 11:42:29 +0100 Thomas Huth <thuth@redhat.com> wrote: > From: "Collin L. Walling" <walling@linux.vnet.ibm.com> > > 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: > > <os> > <bootmenu enable='yes|no' timeout='X'/> > </os> > > 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 <walling@linux.vnet.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > 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(&plist->head); > + uint8_t *flags = &ipl->qipl.qipl_flags; > + uint32_t *timeout = &ipl->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. Also, error_report() should not use trailing punctuation, but we can fix that up with a follow-on patch. > + return; > + } > + > + if (!boot_menu) { > + return; > + } > + > + *flags |= QIPL_FLAG_BM_OPTS_CMD; > + > + tmp = qemu_opt_get(opts, "splash-time"); > + > + if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) { > + error_report("splash-time is invalid, forcing it to 0."); > + *timeout = 0; > + return; > + } > + > + if (splash_time > 0xffffffff) { > + error_report("splash-time is too large, forcing it to max value."); > + *timeout = 0xffffffff; > + return; > + } > + > + *timeout = cpu_to_be32(splash_time); > +} > +
On 02/26/2018 01:48 PM, Cornelia Huck wrote: > On Mon, 26 Feb 2018 11:42:29 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> From: "Collin L. Walling" <walling@linux.vnet.ibm.com> >> >> 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: >> >> <os> >> <bootmenu enable='yes|no' timeout='X'/> >> </os> >> >> 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 <walling@linux.vnet.ibm.com> >> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> 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(&plist->head); >> + uint8_t *flags = &ipl->qipl.qipl_flags; >> + uint32_t *timeout = &ipl->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? > [...]
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 <thuth@redhat.com> 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(&plist->head); >>> + uint8_t *flags = &ipl->qipl.qipl_flags; >>> + uint32_t *timeout = &ipl->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
On Mon, 26 Feb 2018 14:44:45 -0500 "Collin L. Walling" <walling@linux.vnet.ibm.com> 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 <thuth@redhat.com> 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(&plist->head); > >>> + uint8_t *flags = &ipl->qipl.qipl_flags; > >>> + uint32_t *timeout = &ipl->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?
On 27.02.2018 10:12, Cornelia Huck wrote: > On Mon, 26 Feb 2018 14:44:45 -0500 > "Collin L. Walling" <walling@linux.vnet.ibm.com> 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 <thuth@redhat.com> 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
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" <walling@linux.vnet.ibm.com> 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 <thuth@redhat.com> 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.
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 79f5a58..ee2039d 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -23,6 +23,9 @@ #include "hw/s390x/ebcdic.h" #include "ipl.h" #include "qemu/error-report.h" +#include "qemu/config-file.h" +#include "qemu/cutils.h" +#include "qemu/option.h" #define KERN_IMAGE_START 0x010000UL #define KERN_PARM_AREA 0x010480UL @@ -219,6 +222,54 @@ static Property s390_ipl_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static void s390_ipl_set_boot_menu(S390IPLState *ipl) +{ + QemuOptsList *plist = qemu_find_opts("boot-opts"); + QemuOpts *opts = QTAILQ_FIRST(&plist->head); + uint8_t *flags = &ipl->qipl.qipl_flags; + uint32_t *timeout = &ipl->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."); + return; + } + + if (!boot_menu) { + return; + } + + *flags |= QIPL_FLAG_BM_OPTS_CMD; + + tmp = qemu_opt_get(opts, "splash-time"); + + if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) { + error_report("splash-time is invalid, forcing it to 0."); + *timeout = 0; + return; + } + + if (splash_time > 0xffffffff) { + error_report("splash-time is too large, forcing it to max value."); + *timeout = 0xffffffff; + return; + } + + *timeout = cpu_to_be32(splash_time); +} + static bool s390_gen_initial_iplb(S390IPLState *ipl) { DeviceState *dev_st; @@ -435,6 +486,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) } ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); } + s390_ipl_set_boot_menu(ipl); s390_ipl_prepare_qipl(cpu); } diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 5cc3b77..d6c6f75 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -91,6 +91,9 @@ void s390_reipl_request(void); #define QIPL_ADDRESS 0xcc +/* Boot Menu flags */ +#define QIPL_FLAG_BM_OPTS_CMD 0x80 + /* * The QEMU IPL Parameters will be stored at absolute address * 204 (0xcc) which means it is 32-bit word aligned but not @@ -104,9 +107,11 @@ void s390_reipl_request(void); * in pc-bios/s390-ccw/iplb.h. */ struct QemuIplParameters { - uint8_t reserved1[4]; + uint8_t qipl_flags; + uint8_t reserved1[3]; uint64_t netboot_start_addr; - uint8_t reserved2[16]; + uint32_t boot_menu_timeout; + uint8_t reserved2[12]; } QEMU_PACKED; typedef struct QemuIplParameters QemuIplParameters; diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h index 31d2934..832bb94 100644 --- a/pc-bios/s390-ccw/iplb.h +++ b/pc-bios/s390-ccw/iplb.h @@ -74,14 +74,19 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); #define QIPL_ADDRESS 0xcc +/* Boot Menu flags */ +#define QIPL_FLAG_BM_OPTS_CMD 0x80 + /* * This definition must be kept in sync with the defininition * in hw/s390x/ipl.h */ struct QemuIplParameters { - uint8_t reserved1[4]; + uint8_t qipl_flags; + uint8_t reserved1[3]; uint64_t netboot_start_addr; - uint8_t reserved2[16]; + uint32_t boot_menu_timeout; + uint8_t reserved2[12]; } __attribute__ ((packed)); typedef struct QemuIplParameters QemuIplParameters;