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

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 26 Feb 2018 14:44:45 -0500
"Collin L. Walling" <walling@linux.vnet.ibm.com> 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 <thuth@redhat.com> 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(&plist->head);
> >>> +    uint8_t *flags = &ipl->qipl.qipl_flags;
> >>> +    uint32_t *timeout = &ipl->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?