Re: [qemu-s390x] [PATCH v3 3/8] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 01/16/2018 07:44 AM, Thomas Huth wrote:
> On 15.01.2018 17:44, 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.
> >
> > A loadparm other than 'prompt' will disable the menu and
> > just boot the specified entry.
> >
> > Signed-off-by: Collin L. Walling <address@hidden>
> > Reviewed-by: Janosch Frank <address@hidden>
> > ---
> >   hw/s390x/ipl.c          | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/s390x/ipl.h          | 11 ++++++++--
> >   pc-bios/s390-ccw/iplb.h |  8 ++++++--
> >   3 files changed, 69 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 0d06fc1..81ba9ed 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,55 @@ 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;
> > +    uint16_t *timeout;
> > +    const char *tmp;
> > +    unsigned long result = 0;
> > +
> > +    switch (iplb->pbt) {
> > +    case S390_IPL_TYPE_CCW:
> > +        flags = &iplb->ccw.boot_menu_flags;
> > +        timeout = &iplb->ccw.boot_menu_timeout;
> > +        break;
> > +    case S390_IPL_TYPE_QEMU_SCSI:
> > +        flags = &iplb->scsi.boot_menu_flags;
> > +        timeout = &iplb->scsi.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_BOOT_OPTS;
> > +
> > +        tmp = qemu_opt_get(opts, "splash-time");
> > +
> > +        if (tmp && qemu_strtoul(tmp, NULL, 10, &result)) {
> > +            error_report("splash-time value is invalid, forcing it to 0.");
> It's likely not needed, but an explicit "*timeout = 0" would be fine
> here, I think.


It is not needed, but it might assist with readability.


> > +            return;
> > +        }
> > +
> > +        result /= 1000; /* Store timeout value as seconds */
> Maybe rather do proper rounding:
>
>      result = (result + 500) / 1000;
>
> ?

Makes sense.

> > +        if (result > 0xffff) {
> > +            error_report("splash-time value is greater than 65535000,"
> > +                         " forcing it to 65535000.");
> > +            *timeout = 0xffff;
> > +            return;
> > +        }
> > +
> > +        *timeout = cpu_to_be16(result);
> > +    }
> > +}
> > +
> I've found just some nits ... patch looks fine in general, so feel free
> to add:
>
> Reviewed-by: Thomas Huth <address@hidden>

Thanks for the review, Thomas.

--
- Collin L Walling