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

[Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>]

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