| Message ID | 1518818879-18608-7-git-send-email-walling@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Interactive Boot Menu for DASD and SCSI Guests on s390x | expand |
On 16.02.2018 23:07, Collin L. Walling wrote: > 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. > > The absence of any boot options on the command line will flag > to later use the zipl boot loader values. > > 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> You've managed to add new bugs here. Please drop my Reviewed-by again. > --- > hw/s390x/ipl.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > hw/s390x/ipl.h | 9 +++++++-- > pc-bios/s390-ccw/iplb.h | 6 ++++-- > 3 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 31565ce..c8109f5 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,50 @@ static Property s390_ipl_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void s390_ipl_set_boot_menu(IplParameterBlock *iplb) > +{ > + QemuOptsList *plist = qemu_find_opts("boot-opts"); > + QemuOpts *opts = QTAILQ_FIRST(&plist->head); > + uint8_t *flags; > + uint32_t *timeout; > + const char *tmp; > + unsigned long splash_time = 0; > + > + switch (iplb->pbt) { > + case S390_IPL_TYPE_CCW: > + case S390_IPL_TYPE_QEMU_SCSI: > + flags = &iplb->qipl.boot_menu_flags; > + timeout = &iplb->qipl.boot_menu_timeout; > + break; > + default: > + error_report("boot menu is not supported for this device type."); > + return; > + } > + > + /* In the absence of -boot menu, use zipl parameters */ > + if (!qemu_opt_get(opts, "menu")) { > + *flags = BOOT_MENU_FLAG_ZIPL_OPTS; > + } else if (boot_menu) { > + *flags = BOOT_MENU_FLAG_CMD_OPTS; > + > + 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."); > + splash_time = 0; The earlier version of this patch used "*timeout = 0", which was OK. Now you've changed it to the local variable splash_time, but also kept the return statement below. This is bad. Either change it back to *timeout or drop the return statement. > + return; > + } > + > + if (splash_time > 0xffffffff) { > + error_report("splash-time is too large, forcing it to max value."); > + splash_time = 0xffffffff; > + return; dito. > + } > + > + *timeout = cpu_to_be32(splash_time); > + } > +} > + > static bool s390_gen_initial_iplb(S390IPLState *ipl) > { > DeviceState *dev_st; > @@ -435,6 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) > } > ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); > } > + s390_ipl_set_boot_menu(&ipl->iplb); > s390_ipl_prepare_qipl(cpu); > > } > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 74469b1..f632c59 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define QIPL_ADDRESS 0xcc > > +#define BOOT_MENU_FLAG_CMD_OPTS 0x80 > +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40 > + > /* > * The QEMU IPL Parameters will be stored 32-bit word aligned. > * Placement of data fields in this area must account for > @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > * The entire structure must not be larger than 28 bytes. > */ > struct QemuIplParameters { > - uint8_t reserved1[4]; > + uint8_t boot_menu_flags; > + uint8_t reserved1[3]; > + uint32_t boot_menu_timeout; > uint64_t netboot_start_addr; > - uint8_t reserved2[16]; > + 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 a23237e..0e39aa0 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > * The entire structure must not be larger than 28 bytes. > */ > struct QemuIplParameters { > - uint8_t reserved1[4]; > + uint8_t boot_menu_flags; > + uint8_t reserved1[3]; > + uint32_t boot_menu_timeout; > uint64_t netboot_start_addr; > - uint8_t reserved2[16]; > + uint8_t reserved2[12]; > } __attribute__ ((packed)); > typedef struct QemuIplParameters QemuIplParameters; I think Victor's original intention was to get netboot_start_addr aligned in the lowcore memory. Now it's rather aligned in the host memory. Quite confusing, but I think I'd rather prefer Victor's idea to keep it aligned in the lowcore (since that's the "architected" part). Maybe it's better if we do not declare this as a packed struct at all, and then instead of doing a memcpy of the whole struct, we set the fields manually one by one on the host side into the lowcore, and read the fields manually one by one on the guest side? That's more cumbersome, but avoids future confusion about the alignments here... Thomas
On 17.02.2018 09:26, Thomas Huth wrote: [...] >> struct QemuIplParameters { >> - uint8_t reserved1[4]; >> + uint8_t boot_menu_flags; >> + uint8_t reserved1[3]; >> + uint32_t boot_menu_timeout; >> uint64_t netboot_start_addr; >> - uint8_t reserved2[16]; >> + uint8_t reserved2[12]; >> } __attribute__ ((packed)); >> typedef struct QemuIplParameters QemuIplParameters; > > I think Victor's original intention was to get netboot_start_addr > aligned in the lowcore memory. Now it's rather aligned in the host > memory. Quite confusing, but I think I'd rather prefer Victor's idea to > keep it aligned in the lowcore (since that's the "architected" part). > > Maybe it's better if we do not declare this as a packed struct at all, > and then instead of doing a memcpy of the whole struct, we set the > fields manually one by one on the host side into the lowcore, and read > the fields manually one by one on the guest side? That's more > cumbersome, but avoids future confusion about the alignments here... > > Thomas > Hm ... I would prefer to keep it all together and perhaps come up with better comments (for the fields). BTW: I think it would make sense to reserve the last 8 bytes 'seriously': in case more global configuration data is needed in the future, we should have the possibility to install a pointer to an extension block in there. Anyway, here's the follup squash-in for a qipl-free IPLB. --- diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 3c6a411..fe70008 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -222,7 +222,7 @@ static Property s390_ipl_properties[] = { DEFINE_PROP_END_OF_LIST(), }; -static void s390_ipl_set_boot_menu(IplParameterBlock *iplb) +static void s390_ipl_set_boot_menu(S390IPLState *ipl) { QemuOptsList *plist = qemu_find_opts("boot-opts"); QemuOpts *opts = QTAILQ_FIRST(&plist->head); @@ -231,11 +231,11 @@ static void s390_ipl_set_boot_menu(IplParameterBlock *iplb) const char *tmp; unsigned long splash_time = 0; - switch (iplb->pbt) { + switch (ipl->iplb.pbt) { case S390_IPL_TYPE_CCW: case S390_IPL_TYPE_QEMU_SCSI: - flags = &iplb->qipl.boot_menu_flags; - timeout = &iplb->qipl.boot_menu_timeout; + flags = &ipl->qipl.boot_menu_flags; + timeout = &ipl->qipl.boot_menu_timeout; break; default: error_report("boot menu is not supported for this device type."); @@ -482,7 +482,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->iplb); + s390_ipl_set_boot_menu(ipl); s390_ipl_prepare_qipl(cpu); }
On 16.02.2018 23:07, Collin L. Walling wrote: [...] > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 74469b1..f632c59 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > > #define QIPL_ADDRESS 0xcc > > +#define BOOT_MENU_FLAG_CMD_OPTS 0x80 > +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40 > + > /* > * The QEMU IPL Parameters will be stored 32-bit word aligned. > * Placement of data fields in this area must account for > @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; > * The entire structure must not be larger than 28 bytes. > */ > struct QemuIplParameters { > - uint8_t reserved1[4]; > + uint8_t boot_menu_flags; > + uint8_t reserved1[3]; > + uint32_t boot_menu_timeout; > uint64_t netboot_start_addr; > - uint8_t reserved2[16]; > + uint8_t reserved2[12]; > } QEMU_PACKED;Since this has to be touched anyway to re-establish proper alignment, I could also imagine to define the struct as struct QemuIplParameters { struct { uint32_t flags:8; uint32_t timeout:24; } QEMU_PACKED boot_menu; uint64_t netboot_start_addr; uint8_t reserved2[16]; } QEMU_PACKED; would allow to keep the boot menu stuff together without creating unnecessary holes. It would allow for a timeout value of more than 4 hours. The code to set the boot menu would have to be adapted though to properly deal with the bitfields. > typedef struct QemuIplParameters QemuIplParameters; > > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index a23237e..0e39aa0 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); > * The entire structure must not be larger than 28 bytes. > */ > struct QemuIplParameters { > - uint8_t reserved1[4]; > + uint8_t boot_menu_flags; > + uint8_t reserved1[3]; > + uint32_t boot_menu_timeout; > uint64_t netboot_start_addr; > - uint8_t reserved2[16]; > + uint8_t reserved2[12]; > } __attribute__ ((packed)); > typedef struct QemuIplParameters QemuIplParameters; > same here.
On 02/19/2018 10:52 AM, Viktor Mihajlovski wrote: > On 16.02.2018 23:07, Collin L. Walling wrote: > [...] >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >> index 74469b1..f632c59 100644 >> --- a/hw/s390x/ipl.h >> +++ b/hw/s390x/ipl.h >> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; >> >> #define QIPL_ADDRESS 0xcc >> >> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80 >> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40 >> + >> /* >> * The QEMU IPL Parameters will be stored 32-bit word aligned. >> * Placement of data fields in this area must account for >> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; >> * The entire structure must not be larger than 28 bytes. >> */ >> struct QemuIplParameters { >> - uint8_t reserved1[4]; >> + uint8_t boot_menu_flags; >> + uint8_t reserved1[3]; >> + uint32_t boot_menu_timeout; >> uint64_t netboot_start_addr; >> - uint8_t reserved2[16]; >> + uint8_t reserved2[12]; >> } QEMU_PACKED;Since this has to be touched anyway to re-establish proper alignment, I > could also imagine to define the struct as > struct QemuIplParameters { > struct { > uint32_t flags:8; > uint32_t timeout:24; > } QEMU_PACKED boot_menu; > uint64_t netboot_start_addr; > uint8_t reserved2[16]; > } QEMU_PACKED; > would allow to keep the boot menu stuff together without creating > unnecessary holes. > It would allow for a timeout value of more than 4 hours. The code to set > the boot menu would have to be adapted though to properly deal with the > bitfields. I'm currently trying to wrap my brain aroundendian conversion with bit fields. I'll investigate the best way to handle this in the mean time, but we could also consider the following: If neighboring related fields is important, how about moving the fields below netboot? struct QemuIplParameters { uint8_t reserved1[4]; uint64_t netboot_start_addr; uint32_t boot_menu_timeout; uint8_t boot_menu_flags; uint8_t reserved2[11]; } QEMU_PACKED; If we're concerned about space, we could retreat to timeout as a 16-bit field (and also bring back the ms -> seconds conversion business) struct QemuIplParameters { uint8_t boot_menu_flags; uint8_t reserved; uint16_t boot_menu_timeout; uint64_t netboot_start_addr; uint8_t reserved2[16]; } QEMU_PACKED; >> typedef struct QemuIplParameters QemuIplParameters; >> >> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h >> index a23237e..0e39aa0 100644 >> --- a/pc-bios/s390-ccw/iplb.h >> +++ b/pc-bios/s390-ccw/iplb.h >> @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); >> * The entire structure must not be larger than 28 bytes. >> */ >> struct QemuIplParameters { >> - uint8_t reserved1[4]; >> + uint8_t boot_menu_flags; >> + uint8_t reserved1[3]; >> + uint32_t boot_menu_timeout; >> uint64_t netboot_start_addr; >> - uint8_t reserved2[16]; >> + uint8_t reserved2[12]; >> } __attribute__ ((packed)); >> typedef struct QemuIplParameters QemuIplParameters; >> > same here. >
On 19.02.2018 21:39, Collin L. Walling wrote: > On 02/19/2018 10:52 AM, Viktor Mihajlovski wrote: >> On 16.02.2018 23:07, Collin L. Walling wrote: >> [...] >>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >>> index 74469b1..f632c59 100644 >>> --- a/hw/s390x/ipl.h >>> +++ b/hw/s390x/ipl.h >>> @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; >>> >>> #define QIPL_ADDRESS 0xcc >>> >>> +#define BOOT_MENU_FLAG_CMD_OPTS 0x80 >>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40 >>> + >>> /* >>> * The QEMU IPL Parameters will be stored 32-bit word aligned. >>> * Placement of data fields in this area must account for >>> @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; >>> * The entire structure must not be larger than 28 bytes. >>> */ >>> struct QemuIplParameters { >>> - uint8_t reserved1[4]; >>> + uint8_t boot_menu_flags; >>> + uint8_t reserved1[3]; >>> + uint32_t boot_menu_timeout; >>> uint64_t netboot_start_addr; >>> - uint8_t reserved2[16]; >>> + uint8_t reserved2[12]; >>> } QEMU_PACKED;Since this has to be touched anyway to re-establish >>> proper alignment, I >> could also imagine to define the struct as >> struct QemuIplParameters { >> struct { >> uint32_t flags:8; >> uint32_t timeout:24; >> } QEMU_PACKED boot_menu; >> uint64_t netboot_start_addr; >> uint8_t reserved2[16]; >> } QEMU_PACKED; >> would allow to keep the boot menu stuff together without creating >> unnecessary holes. >> It would allow for a timeout value of more than 4 hours. The code to set >> the boot menu would have to be adapted though to properly deal with the >> bitfields. > > I'm currently trying to wrap my brain aroundendian conversion with bit > fields. > I'll investigate the best way to handle this in the mean time, but we > could also > consider the following: > > If neighboring related fields is important, how about moving the fields > below netboot? > > struct QemuIplParameters { > uint8_t reserved1[4]; > uint64_t netboot_start_addr; > uint32_t boot_menu_timeout; > uint8_t boot_menu_flags; > uint8_t reserved2[11]; > } QEMU_PACKED; > I didn't consider the le/be ramifications. They can be dealt with, but simple is definitely better as we could see in the discussion. No concerns from my side regarding space. Another possibility is having a uint8_t field (qipl_flags?) at the beginning of the struct that could hold the boot menu and other QEMU IPL flags to come (if any). I.e. uint8_t qipl_flags; uint8_t reserved1[3]; uint64_t netboot_start_addr; uint32_t boot_menu_timeout; ... and then use a prefix of QIPL_FLAG_ or so. But that's really only a matter of taste, so whatever you decide is OK for me. But while examining this file I noticed that I've put the QemuIplParameters just before the IplParamenterBlock, since I planned to use it as a member there. As the intention is to use it stand-alone now, it could be moved somewhere else, e.g. trailing the IplParameterBlock, which would improve the readability. > > If we're concerned about space, we could retreat to timeout as a 16-bit > field > (and also bring back the ms -> seconds conversion business) > > struct QemuIplParameters { > uint8_t boot_menu_flags; > uint8_t reserved; > uint16_t boot_menu_timeout; > uint64_t netboot_start_addr; > uint8_t reserved2[16]; > } QEMU_PACKED; > [...]
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 31565ce..c8109f5 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,50 @@ static Property s390_ipl_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static void s390_ipl_set_boot_menu(IplParameterBlock *iplb) +{ + QemuOptsList *plist = qemu_find_opts("boot-opts"); + QemuOpts *opts = QTAILQ_FIRST(&plist->head); + uint8_t *flags; + uint32_t *timeout; + const char *tmp; + unsigned long splash_time = 0; + + switch (iplb->pbt) { + case S390_IPL_TYPE_CCW: + case S390_IPL_TYPE_QEMU_SCSI: + flags = &iplb->qipl.boot_menu_flags; + timeout = &iplb->qipl.boot_menu_timeout; + break; + default: + error_report("boot menu is not supported for this device type."); + return; + } + + /* In the absence of -boot menu, use zipl parameters */ + if (!qemu_opt_get(opts, "menu")) { + *flags = BOOT_MENU_FLAG_ZIPL_OPTS; + } else if (boot_menu) { + *flags = BOOT_MENU_FLAG_CMD_OPTS; + + 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."); + splash_time = 0; + return; + } + + if (splash_time > 0xffffffff) { + error_report("splash-time is too large, forcing it to max value."); + splash_time = 0xffffffff; + return; + } + + *timeout = cpu_to_be32(splash_time); + } +} + static bool s390_gen_initial_iplb(S390IPLState *ipl) { DeviceState *dev_st; @@ -435,6 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu) } ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr); } + s390_ipl_set_boot_menu(&ipl->iplb); s390_ipl_prepare_qipl(cpu); } diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 74469b1..f632c59 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -60,6 +60,9 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; #define QIPL_ADDRESS 0xcc +#define BOOT_MENU_FLAG_CMD_OPTS 0x80 +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40 + /* * The QEMU IPL Parameters will be stored 32-bit word aligned. * Placement of data fields in this area must account for @@ -67,9 +70,11 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi; * The entire structure must not be larger than 28 bytes. */ struct QemuIplParameters { - uint8_t reserved1[4]; + uint8_t boot_menu_flags; + uint8_t reserved1[3]; + uint32_t boot_menu_timeout; uint64_t netboot_start_addr; - uint8_t reserved2[16]; + 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 a23237e..0e39aa0 100644 --- a/pc-bios/s390-ccw/iplb.h +++ b/pc-bios/s390-ccw/iplb.h @@ -81,9 +81,11 @@ extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE))); * The entire structure must not be larger than 28 bytes. */ struct QemuIplParameters { - uint8_t reserved1[4]; + uint8_t boot_menu_flags; + uint8_t reserved1[3]; + uint32_t boot_menu_timeout; uint64_t netboot_start_addr; - uint8_t reserved2[16]; + uint8_t reserved2[12]; } __attribute__ ((packed)); typedef struct QemuIplParameters QemuIplParameters;
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. The absence of any boot options on the command line will flag to later use the zipl boot loader values. 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> --- hw/s390x/ipl.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/s390x/ipl.h | 9 +++++++-- pc-bios/s390-ccw/iplb.h | 6 ++++-- 3 files changed, 59 insertions(+), 4 deletions(-)