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

[Collin L. Walling]

On 12/12/2017 12:00 PM, David Hildenbrand wrote:
> On 11.12.2017 23:19, 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          | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/s390x/ipl.h          |  8 +++++--
> >   pc-bios/s390-ccw/iplb.h |  8 +++++--
> >   3 files changed, 67 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 0d06fc1..ed5e8d1 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
> > @@ -33,6 +35,9 @@
> >   #define ZIPL_IMAGE_START                0x009000UL
> >   #define IPL_PSW_MASK                    (PSW_MASK_32 | PSW_MASK_64)
> >   
> > +#define BOOT_MENU_FLAG_BOOT_OPTS 0x80
> > +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
> these should go to ipl.h and maybe be renamed to something like
>
> IPL_BLOCK_... (not sure if even CCW/SCSI specific ones make sense)


I agree to move them to the header file.

I do not think we need CCW/SCSI specific ones at this time.

I'd prefer to keep the "BOOT_MENU_FLAG*" scheme, as it mirrors the same 
naming
convention Iuse in the bios.


> > +
> >   static bool iplb_extended_needed(void *opaque)
> >   {
> >       S390IPLState *ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
> > @@ -219,6 +224,51 @@ static Property s390_ipl_properties[] = {
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_flags,
> > +                                   uint16_t *boot_menu_timeout)
> > +{
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +    char *lp = object_property_get_str(OBJECT(machine), "loadparm", NULL);
> > +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> > +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> > +    const char *p = qemu_opt_get(opts, "menu");
> > +    unsigned long timeout = 0;
> > +
> > +    if (memcmp(lp, "PROMPT  ", 8) == 0) {
> > +        *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> > +
> remove this empty line
>
> > +    } else if (*lp) {
> > +        /* If loadparm is set to any value, then discard boot menu */
> I find the connection between -boot and loadparm quite confusing.
> Shouldn't the s390-ccw bios handle this? You should just forward here
> what is given via -boot IMHO.


My idea was to handle the setting of all boot flags and timeout options 
under one
function.  IMO, it reads easier when the boot menu options are handled 
in one place
instead of having them potentially changing in the bios (interpreting 
loadparm).

I'm more-or-less curious to see what the feedback on this will be.  If the
consensus is to interpret loadparm in the bios instead, then I'm okay 
with that
too.


> > +        return;
> > +
> > +    } else if (!p) {
> > +        /* In the absence of -boot menu, use zipl loader parameters */
> > +        *boot_menu_flags = BOOT_MENU_FLAG_ZIPL_OPTS;
> > +
> > +    } else if (strncmp(p, "on", 2) == 0) {
> > +        *boot_menu_flags = BOOT_MENU_FLAG_BOOT_OPTS;
> > +
> > +        p = qemu_opt_get(opts, "splash-time");
> > +
> > +        if (p && qemu_strtoul(p, NULL, 10, &timeout)) {
> > +            error_report("splash-time value is invalid, forcing it to 0.");
> > +            return;
> > +        }
> > +
> > +        /* Store timeout value as seconds */
> > +        timeout /= 1000;
> So we pass ms on the command line? Why?


The QEMU user documentation specifies the splash-time as milliseconds.
I figured it would be best to conform to that.


> > +
> > +        if (timeout > 0xffff) {
> > +            error_report("splash-time value is greater than 65535000,"
> > +                         " forcing it to 65535000.");
> > +            *boot_menu_timeout = 0xffff;
> > +            return;
> > +        }
> > +
> > +        *boot_menu_timeout = timeout;
> > +    }
> > +}
> > +
> >   static bool s390_gen_initial_iplb(S390IPLState *ipl)
> >   {
> >       DeviceState *dev_st;
> > @@ -245,6 +295,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_flags,
> > +                                   &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 +318,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_flags,
> > +                                   &ipl->iplb.scsi.boot_menu_timeout);
> can you call this only once for both cases below and only pass ipl? The
> function can figure out using ipl->iplb.pbt what to set.


Can do.


> >           } else {
> >               return false; /* unknown device */
> >           }
> > @@ -273,6 +327,7 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
> >           if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
> >               ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
> >           }
> > +
> unrelated change
>
> >           return true;
> >       }
> >   
> > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> > index 8a705e0..ff3b397 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];
> > +    uint16_t boot_menu_timeout;
> > +    uint8_t  boot_menu_flags;
> >       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];
> > +    uint16_t boot_menu_timeout;
> > +    uint8_t  boot_menu_flags;
> >       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..fe909d2 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];
> > +    uint16_t boot_menu_timeout;
> > +    uint8_t  boot_menu_flags;
> >       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];
> > +    uint16_t boot_menu_timeout;
> > +    uint8_t  boot_menu_flags;
> >       uint8_t  ssid;
> >       uint16_t devno;
> >   } __attribute__ ((packed));
> These look good to me (but as discussed, I think HW folks have to agree
> on this).


Thanks for your feedback.

--
- Collin L Walling