Re: [qemu-s390x] [PATCH v4 08/10] s390-ccw: print zipl boot menu

[Collin L. Walling]

On 01/29/2018 05:15 AM, David Hildenbrand wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
> > When the boot menu options are present and the guest's
> > disk has been configured by the zipl tool, then the user
> > will be presented with an interactive boot menu with
> > labeled entries. An example of what the menu might look
> > like:
> >
> > zIPL v1.37.1-build-20170714 interactive boot menu.
> >
> >    0. default (linux-4.13.0)
> >
> >    1. linux-4.13.0
> >    2. performance
> >    3. kvm
> >
> > Signed-off-by: Collin L. Walling <address@hidden>
> > ---
> >   pc-bios/s390-ccw/menu.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> > index de12c73..174285e 100644
> > --- a/pc-bios/s390-ccw/menu.c
> > +++ b/pc-bios/s390-ccw/menu.c
> > @@ -10,13 +10,62 @@
> >    */
> >   
> >   #include "menu.h"
> > +#include "s390-ccw.h"
> >   
> >   static uint8_t flags;
> >   static uint64_t timeout;
> >   
> > +/* Offsets from zipl fields to zipl banner start */
> > +#define ZIPL_TIMEOUT_OFFSET 138
> > +#define ZIPL_FLAG_OFFSET    140
> > +
> > +static int get_boot_index(int entries)
> > +{
> > +    return 0; /* Implemented next patch */
> > +}
> > +
> > +static void zipl_println(const char *data, size_t len)
> > +{
> > +    char buf[len + 2];
> > +
> > +    ebcdic_to_ascii(data, buf, len);
> > +    buf[len] = '\n';
> > +    buf[len + 1] = '\0';
> > +
> > +    sclp_print(buf);
> > +}
> > +
> >   int menu_get_zipl_boot_index(const void *stage2, int offset)
> >   {
> > -    return 0; /* implemented next patch */
> > +    const char *data = stage2 + offset;
> > +    uint16_t flag;
> > +    size_t len;
> > +    int ct;
> > +
> > +    flag = *(uint16_t *)(data - ZIPL_FLAG_OFFSET);
> You could initialize this directly above or after you verified (flags &
> BOOT_MENU_FLAG_ZIPL_OPTS)
>
> > +
> > +    if (flags & BOOT_MENU_FLAG_ZIPL_OPTS) {
> > +        if (flag) {
> > +            timeout = *(uint16_t *)(data - ZIPL_TIMEOUT_OFFSET);
> > +        } else {
> You can drop the else and return directly.


This would read a lot better if s/flag/zipl_flag.

It will read as "if (!zipl_flag) return".


> > +            return 0; /* Boot default */
> > +        }
> > +    }
> > +
> > +    /* Print and count all menu items, including the banner */
> > +    for (ct = 0; *data; ct++) {
> > +        len = strlen(data);
> > +        zipl_println(data, len);
> > +        data += len + 1;
> > +
> > +        if (ct < 2) {
> > +            sclp_print("\n");
> > +        }
> > +    }
> > +
> > +    sclp_print("\n");
> > +
> > +    return get_boot_index(ct - 1);
> >   }
> >   
> >   void menu_set_parms(uint8_t boot_menu_flag, uint16_t boot_menu_timeout)
> Looks good to me!

Thanks!

--
- Collin L Walling