diff mbox

[v7,06/12] s390-ccw: parse and set boot menu options

Message ID 1518818879-18608-7-git-send-email-walling@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Collin L. Walling Feb. 16, 2018, 10:07 p.m. UTC
Set boot menu options for an s390 guest and store them in
the iplb. These options are set via the QEMU command line
option:

    -boot menu=on|off[,splash-time=X]

or via the libvirt domain xml:

    <os>
      <bootmenu enable='yes|no' timeout='X'/>
    </os>

Where X represents some positive integer representing
milliseconds.

Any value set for loadparm will override all boot menu options.
If loadparm=PROMPT, then the menu will be enabled without a
timeout.

The absence of any boot options on the command line will flag
to later use the zipl boot loader values.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/ipl.c          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h          |  9 +++++++--
 pc-bios/s390-ccw/iplb.h |  6 ++++--
 3 files changed, 59 insertions(+), 4 deletions(-)

Comments

Thomas Huth Feb. 17, 2018, 8:26 a.m. UTC | #1
On 16.02.2018 23:07, Collin L. Walling wrote:
> Set boot menu options for an s390 guest and store them in
> the iplb. These options are set via the QEMU command line
> option:
> 
>     -boot menu=on|off[,splash-time=X]
> 
> or via the libvirt domain xml:
> 
>     <os>
>       <bootmenu enable='yes|no' timeout='X'/>
>     </os>
> 
> Where X represents some positive integer representing
> milliseconds.
> 
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu will be enabled without a
> timeout.
> 
> The absence of any boot options on the command line will flag
> to later use the zipl boot loader values.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

You've managed to add new bugs here. Please drop my Reviewed-by again.

> ---
>  hw/s390x/ipl.c          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/s390x/ipl.h          |  9 +++++++--
>  pc-bios/s390-ccw/iplb.h |  6 ++++--
>  3 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 31565ce..c8109f5 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -23,6 +23,9 @@
>  #include "hw/s390x/ebcdic.h"
>  #include "ipl.h"
>  #include "qemu/error-report.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
> +#include "qemu/option.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define KERN_PARM_AREA                  0x010480UL
> @@ -219,6 +222,50 @@ static Property s390_ipl_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
> +{
> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> +    uint8_t *flags;
> +    uint32_t *timeout;
> +    const char *tmp;
> +    unsigned long splash_time = 0;
> +
> +    switch (iplb->pbt) {
> +    case S390_IPL_TYPE_CCW:
> +    case S390_IPL_TYPE_QEMU_SCSI:
> +        flags = &iplb->qipl.boot_menu_flags;
> +        timeout = &iplb->qipl.boot_menu_timeout;
> +        break;
> +    default:
> +        error_report("boot menu is not supported for this device type.");
> +        return;
> +    }
> +
> +    /* In the absence of -boot menu, use zipl parameters */
> +    if (!qemu_opt_get(opts, "menu")) {
> +        *flags = BOOT_MENU_FLAG_ZIPL_OPTS;
> +    } else if (boot_menu) {
> +        *flags = BOOT_MENU_FLAG_CMD_OPTS;
> +
> +        tmp = qemu_opt_get(opts, "splash-time");
> +
> +        if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) {
> +            error_report("splash-time is invalid, forcing it to 0.");
> +            splash_time = 0;

The earlier version of this patch used "*timeout = 0", which was OK. Now
you've changed it to the local variable splash_time, but also kept the
return statement below. This is bad. Either change it back to *timeout
or drop the return statement.

> +            return;
> +        }
> +
> +        if (splash_time > 0xffffffff) {
> +            error_report("splash-time is too large, forcing it to max value.");
> +            splash_time = 0xffffffff;
> +            return;

dito.

> +        }
> +
> +        *timeout = cpu_to_be32(splash_time);
> +    }
> +}
> +
>  static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;
> @@ -435,6 +482,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>          }
>          ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>      }
> +    s390_ipl_set_boot_menu(&ipl->iplb);
>      s390_ipl_prepare_qipl(cpu);
>  
>  }
> 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;
>  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;

I think Victor's original intention was to get netboot_start_addr
aligned in the lowcore memory. Now it's rather aligned in the host
memory. Quite confusing, but I think I'd rather prefer Victor's idea to
keep it aligned in the lowcore (since that's the "architected" part).

Maybe it's better if we do not declare this as a packed struct at all,
and then instead of doing a memcpy of the whole struct, we set the
fields manually one by one on the host side into the lowcore, and read
the fields manually one by one on the guest side? That's more
cumbersome, but avoids future confusion about the alignments here...

 Thomas
Viktor Mihajlovski Feb. 19, 2018, 3:52 p.m. UTC | #2
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.
Collin L. Walling Feb. 19, 2018, 8:39 p.m. UTC | #3
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.
>
Viktor Mihajlovski Feb. 20, 2018, 9:55 a.m. UTC | #4
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;
> [...]
diff mbox

Patch

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 31565ce..c8109f5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -23,6 +23,9 @@ 
 #include "hw/s390x/ebcdic.h"
 #include "ipl.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define KERN_PARM_AREA                  0x010480UL
@@ -219,6 +222,50 @@  static Property s390_ipl_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void s390_ipl_set_boot_menu(IplParameterBlock *iplb)
+{
+    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
+    uint8_t *flags;
+    uint32_t *timeout;
+    const char *tmp;
+    unsigned long splash_time = 0;
+
+    switch (iplb->pbt) {
+    case S390_IPL_TYPE_CCW:
+    case S390_IPL_TYPE_QEMU_SCSI:
+        flags = &iplb->qipl.boot_menu_flags;
+        timeout = &iplb->qipl.boot_menu_timeout;
+        break;
+    default:
+        error_report("boot menu is not supported for this device type.");
+        return;
+    }
+
+    /* In the absence of -boot menu, use zipl parameters */
+    if (!qemu_opt_get(opts, "menu")) {
+        *flags = BOOT_MENU_FLAG_ZIPL_OPTS;
+    } else if (boot_menu) {
+        *flags = BOOT_MENU_FLAG_CMD_OPTS;
+
+        tmp = qemu_opt_get(opts, "splash-time");
+
+        if (tmp && qemu_strtoul(tmp, NULL, 10, &splash_time)) {
+            error_report("splash-time is invalid, forcing it to 0.");
+            splash_time = 0;
+            return;
+        }
+
+        if (splash_time > 0xffffffff) {
+            error_report("splash-time is too large, forcing it to max value.");
+            splash_time = 0xffffffff;
+            return;
+        }
+
+        *timeout = cpu_to_be32(splash_time);
+    }
+}
+
 static bool s390_gen_initial_iplb(S390IPLState *ipl)
 {
     DeviceState *dev_st;
@@ -435,6 +482,7 @@  void s390_ipl_prepare_cpu(S390CPU *cpu)
         }
         ipl->iplb.qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
+    s390_ipl_set_boot_menu(&ipl->iplb);
     s390_ipl_prepare_qipl(cpu);
 
 }
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;
 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;