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

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

On 02/16/2018 11:36 AM, Viktor Mihajlovski wrote:
> On 16.02.2018 17:20, Thomas Huth wrote:
> [...]
>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>> index cab8a97..7c3cab8 100644
>>> --- a/hw/s390x/ipl.h
>>> +++ b/hw/s390x/ipl.h
>>> @@ -60,10 +60,15 @@ typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>>>   
>>>   #define QIPL_ADDRESS  0xcc
>>>   
>>> +#define BOOT_MENU_FLAG_CMD_OPTS  0x80
>>> +#define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>>> +
>>>   struct QemuIplParameters {
>>> -    uint8_t  reserved1[4];
>>> +    uint8_t  boot_menu_flags;
>>> +    uint8_t  reserved1;
>>> +    uint32_t boot_menu_timeout;
>>>       uint64_t netboot_start_addr;
>> The netboot_start_addr field is now never aligned anymore, neither on
>> the host side, nor in guest memory. Not a big problem since the struct
>> is declared with "QEMU_PACKED", but still ... it's always nicer to try
>> to align fields to their natural boundaries. So maybe move
>> boot_menu_flags and reserved1 after netboot_start_addr ?
>>
> Good catch ... we probably should document the alignment needs and state
> that the ipl parameters starts on a word boundary (and that the block
> may not be larger than 28 bytes) in a comment block.

How does this sound?

/* word aligned and cannot exceed 28 bytes */


>>> -    uint8_t  reserved2[16];
>>> +    uint8_t  reserved2[14];
>>>   } QEMU_PACKED;
>>>   typedef struct QemuIplParameters QemuIplParameters;
>>   Thomas
>>
>


-- 
- Collin L Walling