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.

I'm currently trying to wrap my brain aroundendian conversion with bit 
fields.
I'll investigate the best way to handle this in the mean time, but we 
could also
consider the following:

If neighboring related fields is important, how about moving the fields 
below netboot?

struct QemuIplParameters {
    uint8_t  reserved1[4];
    uint64_t netboot_start_addr;
    uint32_t boot_menu_timeout;
    uint8_t  boot_menu_flags;
    uint8_t  reserved2[11];
  } QEMU_PACKED;


If we're concerned about space, we could retreat to timeout as a 16-bit 
field
(and also bring back the ms -> seconds conversion business)

struct QemuIplParameters {
    uint8_t  boot_menu_flags;
    uint8_t  reserved;
    uint16_t boot_menu_timeout;
    uint64_t netboot_start_addr;
    uint8_t  reserved2[16];
  } QEMU_PACKED;

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