Re: [qemu-s390x] [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options

[Collin L. Walling]

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.

This is sufficient enough for the boot menu parameters and, honestly, 
makes a bit more
sense (no pun intended). We don't necessarily need a huge field for timeout.

> >   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.


--
- Collin L Walling