Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
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
Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
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
Re: [Qemu-devel] [PATCH v7 06/12] s390-ccw: parse and set boot menu options
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.
> 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.
--
Regards,
Viktor Mihajlovski
