Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Thomas Huth]

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 <address@hidden>
> Reviewed-by: Janosch Frank <address@hidden>
> ---
>  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