On 19.02.2018 21:39, Collin L. Walling wrote: > 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; > I didn't consider the le/be ramifications. They can be dealt with, but simple is definitely better as we could see in the discussion. No concerns from my side regarding space.
Another possibility is having a uint8_t field (qipl_flags?) at the beginning of the struct that could hold the boot menu and other QEMU IPL flags to come (if any). I.e. uint8_t qipl_flags; uint8_t reserved1[3]; uint64_t netboot_start_addr; uint32_t boot_menu_timeout; ... and then use a prefix of QIPL_FLAG_ or so. But that's really only a matter of taste, so whatever you decide is OK for me. But while examining this file I noticed that I've put the QemuIplParameters just before the IplParamenterBlock, since I planned to use it as a member there. As the intention is to use it stand-alone now, it could be moved somewhere else, e.g. trailing the IplParameterBlock, which would improve the readability. > > 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; > [...] -- Regards, Viktor Mihajlovski