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

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

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 <walling@linux.vnet.ibm.com>
>> ---
>>   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