Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 02/27/2018 04:22 AM, Thomas Huth wrote:

On 27.02.2018 10:12, Cornelia Huck wrote:

On Mon, 26 Feb 2018 14:44:45 -0500
"Collin L. Walling"  wrote:


On 02/26/2018 02:29 PM, Collin L. Walling wrote:

On 02/26/2018 01:48 PM, Cornelia Huck wrote:

On Mon, 26 Feb 2018 11:42:29 +0100
Thomas Huth  wrote:

[...]

+    switch (ipl->iplb.pbt) {
+    case S390_IPL_TYPE_CCW:
+    break;
+    default:
+    error_report("boot menu is not supported for this device
type.");

If I specify both a bootindex for a device and a -kernel parameter, I
get this error message. Looks a tad odd, but not sure how to avoid it.

Hmm... perhaps an additional check if no IPLB, then skip trying to set
any boot menu data?

[...]

How shall we proceed? I'd be willing to pull this now and then apply
fixups on top, or we can have a respin. Thomas?

Since this is about a change in hw/s390x/ and not about
pc-bios/s390-ccw/ (i.e. no need to rebuild the firmware binaries for
this), I think I'd rather prefer a fix-up patch on top instead of a re-spin.

  Thomas


I'll get right on it and post to the qemu lists.

--
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Thomas Huth]

On 27.02.2018 10:12, Cornelia Huck wrote:
> On Mon, 26 Feb 2018 14:44:45 -0500
> "Collin L. Walling"  wrote:
> 
>> On 02/26/2018 02:29 PM, Collin L. Walling wrote:
>>> On 02/26/2018 01:48 PM, Cornelia Huck wrote:  
 On Mon, 26 Feb 2018 11:42:29 +0100
 Thomas Huth  wrote:
[...]
> +    switch (ipl->iplb.pbt) {
> +    case S390_IPL_TYPE_CCW:
> +    break;
> +    default:
> +    error_report("boot menu is not supported for this device 
> type.");  
 If I specify both a bootindex for a device and a -kernel parameter, I
 get this error message. Looks a tad odd, but not sure how to avoid it.  
>>>
>>> Hmm... perhaps an additional check if no IPLB, then skip trying to set
>>> any boot menu data?
[...]
> How shall we proceed? I'd be willing to pull this now and then apply
> fixups on top, or we can have a respin. Thomas?

Since this is about a change in hw/s390x/ and not about
pc-bios/s390-ccw/ (i.e. no need to rebuild the firmware binaries for
this), I think I'd rather prefer a fix-up patch on top instead of a re-spin.

 Thomas

Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Cornelia Huck]

On Mon, 26 Feb 2018 14:44:45 -0500
"Collin L. Walling"  wrote:

> On 02/26/2018 02:29 PM, Collin L. Walling wrote:
> > On 02/26/2018 01:48 PM, Cornelia Huck wrote:  
> >> On Mon, 26 Feb 2018 11:42:29 +0100
> >> Thomas Huth  wrote:
> >>
> >> [...]
> >>  
> >>>   3 files changed, 66 insertions(+), 4 deletions(-)
> >>> +static void s390_ipl_set_boot_menu(S390IPLState *ipl)
> >>> +{
> >>> +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> >>> +    QemuOpts *opts = QTAILQ_FIRST(>head);
> >>> +    uint8_t *flags = >qipl.qipl_flags;
> >>> +    uint32_t *timeout = >qipl.boot_menu_timeout;
> >>> +    const char *tmp;
> >>> +    unsigned long splash_time = 0;
> >>> +
> >>> +    if (!get_boot_device(0)) {
> >>> +    if (boot_menu) {
> >>> +    error_report("boot menu requires a bootindex to be 
> >>> specified for "
> >>> + "the IPL device.");
> >>> +    }
> >>> +    return;
> >>> +    }
> >>> +
> >>> +    switch (ipl->iplb.pbt) {
> >>> +    case S390_IPL_TYPE_CCW:
> >>> +    break;
> >>> +    default:
> >>> +    error_report("boot menu is not supported for this device 
> >>> type.");  
> >> If I specify both a bootindex for a device and a -kernel parameter, I
> >> get this error message. Looks a tad odd, but not sure how to avoid it.  
> >
> > Hmm... perhaps an additional check if no IPLB, then skip trying to set
> > any boot menu data?
> >  
> >> [...]  
> >
> >  
> Something like:
> 
>      if (!ipl->iplb.len) {
>      return;
>      }
> 
> placed just below the if (!get_boot_device(0)) chunk fixed it for me.If 
> no IPLB was set,
> then the IPLB fields should just all be zeros.  Why not just check if 
> the length is 0 to
> determine that we did not set an IPLB at all?


Yes, that makes the message go away for me.

> 
> also:
> 
>      if (!ipl->iplb.len) {
>          if (boot_menu) {
>              error_report("boot menu requires an IPLB to function");
>          }
>      return;
>      }
> 
> if you think an error message is needed (use a better message, mine is 
> not helpful but I
> just wanted to demonstrate that the if (boot_menu) check should be 
> nested first).

I'm not sure we want an error message for a command line that booted
without any message previously. It's not like I'd expect a boot menu
when I pass in a kernel anyway...

> 
> Thanks for reporting this.  Seems to be a few cases that I missed on my end.

The usual result of different people trying things :)

How shall we proceed? I'd be willing to pull this now and then apply
fixups on top, or we can have a respin. Thomas?

Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 02/26/2018 02:29 PM, Collin L. Walling wrote:

On 02/26/2018 01:48 PM, Cornelia Huck wrote:

On Mon, 26 Feb 2018 11:42:29 +0100
Thomas Huth  wrote:

[...]


  3 files changed, 66 insertions(+), 4 deletions(-)
+static void s390_ipl_set_boot_menu(S390IPLState *ipl)
+{
+    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOpts *opts = QTAILQ_FIRST(>head);
+    uint8_t *flags = >qipl.qipl_flags;
+    uint32_t *timeout = >qipl.boot_menu_timeout;
+    const char *tmp;
+    unsigned long splash_time = 0;
+
+    if (!get_boot_device(0)) {
+    if (boot_menu) {
+    error_report("boot menu requires a bootindex to be 
specified for "

+ "the IPL device.");
+    }
+    return;
+    }
+
+    switch (ipl->iplb.pbt) {
+    case S390_IPL_TYPE_CCW:
+    break;
+    default:
+    error_report("boot menu is not supported for this device 
type.");

If I specify both a bootindex for a device and a -kernel parameter, I
get this error message. Looks a tad odd, but not sure how to avoid it.


Hmm... perhaps an additional check if no IPLB, then skip trying to set
any boot menu data?


[...]




Something like:

    if (!ipl->iplb.len) {
    return;
    }

placed just below the if (!get_boot_device(0)) chunk fixed it for me.If 
no IPLB was set,
then the IPLB fields should just all be zeros.  Why not just check if 
the length is 0 to

determine that we did not set an IPLB at all?

also:

    if (!ipl->iplb.len) {
        if (boot_menu) {
            error_report("boot menu requires an IPLB to function");
        }
    return;
    }

if you think an error message is needed (use a better message, mine is 
not helpful but I
just wanted to demonstrate that the if (boot_menu) check should be 
nested first).


Thanks for reporting this.  Seems to be a few cases that I missed on my end.

--
- Collin L Walling

Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 02/26/2018 01:48 PM, Cornelia Huck wrote:

On Mon, 26 Feb 2018 11:42:29 +0100
Thomas Huth  wrote:


From: "Collin L. Walling" 

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:

 
   
 

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.

Signed-off-by: Collin L. Walling 
Reviewed-by: Janosch Frank 
Reviewed-by: Thomas Huth 
Signed-off-by: Thomas Huth 
---
  hw/s390x/ipl.c  | 52 +
  hw/s390x/ipl.h  |  9 +++--
  pc-bios/s390-ccw/iplb.h |  9 +++--

Updating the header, but it is not consumed in the bios?


  3 files changed, 66 insertions(+), 4 deletions(-)
+static void s390_ipl_set_boot_menu(S390IPLState *ipl)
+{
+QemuOptsList *plist = qemu_find_opts("boot-opts");
+QemuOpts *opts = QTAILQ_FIRST(>head);
+uint8_t *flags = >qipl.qipl_flags;
+uint32_t *timeout = >qipl.boot_menu_timeout;
+const char *tmp;
+unsigned long splash_time = 0;
+
+if (!get_boot_device(0)) {
+if (boot_menu) {
+error_report("boot menu requires a bootindex to be specified for "
+ "the IPL device.");
+}
+return;
+}
+
+switch (ipl->iplb.pbt) {
+case S390_IPL_TYPE_CCW:
+break;
+default:
+error_report("boot menu is not supported for this device type.");

If I specify both a bootindex for a device and a -kernel parameter, I
get this error message. Looks a tad odd, but not sure how to avoid it.


Hmm... perhaps an additional check if no IPLB, then skip trying to set
any boot menu data?


[...]



--
- Collin L Walling