| Message ID | 1511816136-30068-4-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 27 Nov 2017 15:55:34 -0500 "Collin L. Walling" <walling@linux.vnet.ibm.com> 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. > > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 23 +++++++++++++++++++++++ > hw/s390x/ipl.h | 8 ++++++-- > pc-bios/s390-ccw/iplb.h | 8 ++++++-- > 3 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 0d06fc1..0ca9e37 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -23,6 +23,8 @@ > #include "hw/s390x/ebcdic.h" > #include "ipl.h" > #include "qemu/error-report.h" > +#include "qemu/config-file.h" > +#include "qemu/cutils.h" > > #define KERN_IMAGE_START 0x010000UL > #define KERN_PARM_AREA 0x010480UL > @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled, > + uint16_t *boot_menu_timeout) > +{ > + QemuOptsList *plist = qemu_find_opts("boot-opts"); > + QemuOpts *opts = QTAILQ_FIRST(&plist->head); > + const char *p; > + long result = 0; > + > + *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false); This is already parsed in vl.c, isn't it? Can you simply check boot_menu? > + > + if (*boot_menu_enabled) { > + p = qemu_opt_get(opts, "splash-time"); hw/nvram/fw_cfg.c parses splash-time as well, but I'm not sure if it is worth parsing it in vl.c as well. > + qemu_strtol(p, NULL, 10, &result); > + *boot_menu_timeout = result; > + } > +} > + > static bool s390_gen_initial_iplb(S390IPLState *ipl) > { > DeviceState *dev_st;
On 27.11.2017 21:55, 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. > > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> > --- > hw/s390x/ipl.c | 23 +++++++++++++++++++++++ > hw/s390x/ipl.h | 8 ++++++-- > pc-bios/s390-ccw/iplb.h | 8 ++++++-- > 3 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 0d06fc1..0ca9e37 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -23,6 +23,8 @@ > #include "hw/s390x/ebcdic.h" > #include "ipl.h" > #include "qemu/error-report.h" > +#include "qemu/config-file.h" > +#include "qemu/cutils.h" > > #define KERN_IMAGE_START 0x010000UL > #define KERN_PARM_AREA 0x010480UL > @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled, > + uint16_t *boot_menu_timeout) > +{ > + QemuOptsList *plist = qemu_find_opts("boot-opts"); > + QemuOpts *opts = QTAILQ_FIRST(&plist->head); > + const char *p; > + long result = 0; > + > + *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false); > + > + if (*boot_menu_enabled) { > + p = qemu_opt_get(opts, "splash-time"); > + qemu_strtol(p, NULL, 10, &result); It's maybe better to check the return code of qemu_strtol to see whether the option contained a valid number? An additional check for negative numbers would also be fine, I think. > + *boot_menu_timeout = result; > + } > +} > + > static bool s390_gen_initial_iplb(S390IPLState *ipl) > { > DeviceState *dev_st; > @@ -245,6 +264,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > ipl->iplb.pbt = S390_IPL_TYPE_CCW; > ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); > ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; > + s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_enabled, > + &ipl->iplb.ccw.boot_menu_timeout); > } else if (sd) { > SCSIBus *bus = scsi_bus_from_device(sd); > VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus); > @@ -266,6 +287,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) > ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); > ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); > ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; > + s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_enabled, > + &ipl->iplb.scsi.boot_menu_timeout); > } else { > return false; /* unknown device */ > } > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 8a705e0..7c960b1 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -17,7 +17,9 @@ > > struct IplBlockCcw { > uint64_t netboot_start_addr; > - uint8_t reserved0[77]; > + uint8_t reserved0[74]; > + uint8_t boot_menu_enabled; > + uint16_t boot_menu_timeout; Could you please try to keep the fields aligned to their natural alignment? I.e. swap the two new entries, so that timeout comes before enabled? > uint8_t ssid; > uint16_t devno; > uint8_t vm_flags; > @@ -51,7 +53,9 @@ struct IplBlockQemuScsi { > uint32_t lun; > uint16_t target; > uint16_t channel; > - uint8_t reserved0[77]; > + uint8_t reserved0[74]; > + uint8_t boot_menu_enabled; > + uint16_t boot_menu_timeout; dito > uint8_t ssid; > uint16_t devno; > } QEMU_PACKED; Thomas
On 11/28/2017 06:04 AM, Cornelia Huck wrote: > On Mon, 27 Nov 2017 15:55:34 -0500 > "Collin L. Walling" <walling@linux.vnet.ibm.com> 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. >> >> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> >> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> --- >> hw/s390x/ipl.c | 23 +++++++++++++++++++++++ >> hw/s390x/ipl.h | 8 ++++++-- >> pc-bios/s390-ccw/iplb.h | 8 ++++++-- >> 3 files changed, 35 insertions(+), 4 deletions(-) >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 0d06fc1..0ca9e37 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -23,6 +23,8 @@ >> #include "hw/s390x/ebcdic.h" >> #include "ipl.h" >> #include "qemu/error-report.h" >> +#include "qemu/config-file.h" >> +#include "qemu/cutils.h" >> >> #define KERN_IMAGE_START 0x010000UL >> #define KERN_PARM_AREA 0x010480UL >> @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled, >> + uint16_t *boot_menu_timeout) >> +{ >> + QemuOptsList *plist = qemu_find_opts("boot-opts"); >> + QemuOpts *opts = QTAILQ_FIRST(&plist->head); >> + const char *p; >> + long result = 0; >> + >> + *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false); > This is already parsed in vl.c, isn't it? Can you simply check > boot_menu? Well then... how convenient! :) > >> + >> + if (*boot_menu_enabled) { >> + p = qemu_opt_get(opts, "splash-time"); > hw/nvram/fw_cfg.c parses splash-time as well, but I'm not sure if it is > worth parsing it in vl.c as well. It's probably best we parse it ourselves. I see that fw_cfg.c also does some validation on the splash-time value, so I'll add something similar. > >> + qemu_strtol(p, NULL, 10, &result); >> + *boot_menu_timeout = result; >> + } >> +} >> + >> static bool s390_gen_initial_iplb(S390IPLState *ipl) >> { >> DeviceState *dev_st;
On 11/28/2017 06:45 AM, Thomas Huth wrote: > On 27.11.2017 21:55, Collin L. Walling wrote: >> [...] >> >> +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled, >> + uint16_t *boot_menu_timeout) >> +{ >> + QemuOptsList *plist = qemu_find_opts("boot-opts"); >> + QemuOpts *opts = QTAILQ_FIRST(&plist->head); >> + const char *p; >> + long result = 0; >> + >> + *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false); >> + >> + if (*boot_menu_enabled) { >> + p = qemu_opt_get(opts, "splash-time"); >> + qemu_strtol(p, NULL, 10, &result); > It's maybe better to check the return code of qemu_strtol to see whether > the option contained a valid number? An additional check for negative > numbers would also be fine, I think. Agreed. I'll add some integer overflow checking as well. > >> [...] >> >> struct IplBlockCcw { >> uint64_t netboot_start_addr; >> - uint8_t reserved0[77]; >> + uint8_t reserved0[74]; >> + uint8_t boot_menu_enabled; >> + uint16_t boot_menu_timeout; > Could you please try to keep the fields aligned to their natural > alignment? I.e. swap the two new entries, so that timeout comes before > enabled? Can do. > >> uint8_t ssid; >> uint16_t devno; >> uint8_t vm_flags; >> @@ -51,7 +53,9 @@ struct IplBlockQemuScsi { >> uint32_t lun; >> uint16_t target; >> uint16_t channel; >> - uint8_t reserved0[77]; >> + uint8_t reserved0[74]; >> + uint8_t boot_menu_enabled; >> + uint16_t boot_menu_timeout; > dito > >> uint8_t ssid; >> uint16_t devno; >> } QEMU_PACKED; > Thomas > > >
On 27.11.2017 21:55, 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. Aren't this properties usually contained in the zIPL block? (e.g. written and configured by zipl)
On 11/29/2017 05:28 PM, David Hildenbrand wrote: > On 27.11.2017 21:55, 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. > Aren't this properties usually contained in the zIPL block? (e.g. > written and configured by zipl) > I believe the timeout value is nestled in there somewhere, yes. However it was requested that we control the boot menu timeout value via the Qemu command line instead. :)
On 29.11.2017 23:33, Collin L. Walling wrote: > On 11/29/2017 05:28 PM, David Hildenbrand wrote: >> On 27.11.2017 21:55, 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. >> Aren't this properties usually contained in the zIPL block? (e.g. >> written and configured by zipl) >> > I believe the timeout value is nestled in there somewhere, yes. > However it was requested that we control the boot menu timeout value > via the Qemu command line instead. :) > Wonder if it makes sense to always act like the zIPL loader (display menu/timeout) but allow to overwrite it via cmd line.
On Thu, 30 Nov 2017 16:52:37 +0100 David Hildenbrand <david@redhat.com> wrote: > On 29.11.2017 23:33, Collin L. Walling wrote: > > On 11/29/2017 05:28 PM, David Hildenbrand wrote: > >> On 27.11.2017 21:55, 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. > >> Aren't this properties usually contained in the zIPL block? (e.g. > >> written and configured by zipl) > >> > > I believe the timeout value is nestled in there somewhere, yes. > > However it was requested that we control the boot menu timeout value > > via the Qemu command line instead. :) > > > > Wonder if it makes sense to always act like the zIPL loader (display > menu/timeout) but allow to overwrite it via cmd line. > I think that make quite a bit of sense.
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 0d06fc1..0ca9e37 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -23,6 +23,8 @@ #include "hw/s390x/ebcdic.h" #include "ipl.h" #include "qemu/error-report.h" +#include "qemu/config-file.h" +#include "qemu/cutils.h" #define KERN_IMAGE_START 0x010000UL #define KERN_PARM_AREA 0x010480UL @@ -219,6 +221,23 @@ static Property s390_ipl_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled, + uint16_t *boot_menu_timeout) +{ + QemuOptsList *plist = qemu_find_opts("boot-opts"); + QemuOpts *opts = QTAILQ_FIRST(&plist->head); + const char *p; + long result = 0; + + *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false); + + if (*boot_menu_enabled) { + p = qemu_opt_get(opts, "splash-time"); + qemu_strtol(p, NULL, 10, &result); + *boot_menu_timeout = result; + } +} + static bool s390_gen_initial_iplb(S390IPLState *ipl) { DeviceState *dev_st; @@ -245,6 +264,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) ipl->iplb.pbt = S390_IPL_TYPE_CCW; ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno); ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3; + s390_ipl_set_boot_menu(&ipl->iplb.ccw.boot_menu_enabled, + &ipl->iplb.ccw.boot_menu_timeout); } else if (sd) { SCSIBus *bus = scsi_bus_from_device(sd); VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus); @@ -266,6 +287,8 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl) ipl->iplb.scsi.channel = cpu_to_be16(sd->channel); ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno); ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3; + s390_ipl_set_boot_menu(&ipl->iplb.scsi.boot_menu_enabled, + &ipl->iplb.scsi.boot_menu_timeout); } else { return false; /* unknown device */ } diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 8a705e0..7c960b1 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -17,7 +17,9 @@ struct IplBlockCcw { uint64_t netboot_start_addr; - uint8_t reserved0[77]; + uint8_t reserved0[74]; + uint8_t boot_menu_enabled; + uint16_t boot_menu_timeout; uint8_t ssid; uint16_t devno; uint8_t vm_flags; @@ -51,7 +53,9 @@ struct IplBlockQemuScsi { uint32_t lun; uint16_t target; uint16_t channel; - uint8_t reserved0[77]; + uint8_t reserved0[74]; + uint8_t boot_menu_enabled; + uint16_t boot_menu_timeout; uint8_t ssid; uint16_t devno; } QEMU_PACKED; diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h index 890aed9..658d163 100644 --- a/pc-bios/s390-ccw/iplb.h +++ b/pc-bios/s390-ccw/iplb.h @@ -14,7 +14,9 @@ struct IplBlockCcw { uint64_t netboot_start_addr; - uint8_t reserved0[77]; + uint8_t reserved0[74]; + uint8_t boot_menu_enabled; + uint16_t boot_menu_timeout; uint8_t ssid; uint16_t devno; uint8_t vm_flags; @@ -48,7 +50,9 @@ struct IplBlockQemuScsi { uint32_t lun; uint16_t target; uint16_t channel; - uint8_t reserved0[77]; + uint8_t reserved0[74]; + uint8_t boot_menu_enabled; + uint16_t boot_menu_timeout; uint8_t ssid; uint16_t devno; } __attribute__ ((packed));
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. Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> --- hw/s390x/ipl.c | 23 +++++++++++++++++++++++ hw/s390x/ipl.h | 8 ++++++-- pc-bios/s390-ccw/iplb.h | 8 ++++++-- 3 files changed, 35 insertions(+), 4 deletions(-)