Re: [qemu-s390x] [PATCH v2 4/5] s390-ccw: interactive boot menu for eckd dasd

[Thomas Huth]

On 11.12.2017 23:19, 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
> 
>     Please choose (default will boot in 10 seconds):
> 
> If the user's input is empty or 0, the default zipl entry will
> be chosen. If the input is within the range presented by the
> menu, then the selection will be booted. Any erroneous input
> will cancel the timeout and prompt the user until correct
> input is given.
> 
> Any value set for loadparm will override all boot menu options.
> If loadparm=PROMPT, then the menu prompt will continuously wait
> until correct user input is given.
> 
> The absence of any boot options on the command line will attempt
> to use the zipl loader values.
> 
> Signed-off-by: Collin L. Walling <address@hidden>
> ---
[...]
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 5546b79..c817cf8 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -13,6 +13,7 @@
>  #include "bootmap.h"
>  #include "virtio.h"
>  #include "bswap.h"
> +#include "menu.h"
>  
>  #ifdef DEBUG
>  /* #define DEBUG_FALLBACK */
> @@ -83,6 +84,7 @@ static void jump_to_IPL_code(uint64_t address)
>  
>  static unsigned char _bprs[8*1024]; /* guessed "max" ECKD sector size */
>  static const int max_bprs_entries = sizeof(_bprs) / sizeof(ExtEckdBlockPtr);
> +static uint8_t stage2[STAGE2_MAX_SIZE] 
> __attribute__((__aligned__(PAGE_SIZE)));
>  
>  static inline void verify_boot_info(BootInfo *bip)
>  {
> @@ -182,7 +184,57 @@ static block_number_t load_eckd_segments(block_number_t 
> blk, uint64_t *address)
>      return block_nr;
>  }
>  
> -static void run_eckd_boot_script(block_number_t mbr_block_nr)
> +static void read_stage2(block_number_t s1b_block_nr)
> +{
> +    block_number_t s2_block_nr;
> +    EckdStage1b *s1b = (void *)sec;
> +    int i;
> +
> +    /* Get Stage1b data */
> +    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> +    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader.");
> +
> +    /* Get Stage2 data */
> +    memset(stage2, FREE_SPACE_FILLER, sizeof(stage2));
> +
> +    for (i = 0; i < STAGE2_MAX_SIZE / MAX_SECTOR_SIZE; i++) {
> +        s2_block_nr = eckd_block_num((void *)&(s1b->seek[i].cyl));

You can omit the parentheses around s1b->seek[i].cyl.

> +        if (!s2_block_nr) {
> +            break;
> +        }
> +
> +        read_block(s2_block_nr, (stage2 + MAX_SECTOR_SIZE * i),

You can omit the parentheses around the second parameter.

> +                   "Error reading Stage2 data");
> +    }
> +}
[...]
> @@ -241,6 +300,9 @@ static void ipl_eckd_cdl(void)
>      /* save pointer to Boot Script */
>      mbr_block_nr = eckd_block_num((void *)&(mbr->blockptr));
>  
> +    /* save pointer to Stage1b Data */
> +    s1b_block_nr = eckd_block_num((void *)&(ipl2->stage1.seek[0].cyl));

You can omit the parentheses around ipl2->stage1.seek[0].cyl.

>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>      read_block(2, vlbl, "Cannot read Volume Label at block 2");
>      IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
> @@ -249,7 +311,7 @@ static void ipl_eckd_cdl(void)
>                 "Invalid magic of volser block");
>      print_volser(vlbl->f.volser);
>  
> -    run_eckd_boot_script(mbr_block_nr);
> +    run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
>      /* no return */
>  }
>  
> @@ -281,6 +343,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
>  static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>  {
>      block_number_t mbr_block_nr;
> +    block_number_t s1b_block_nr;
>      EckdLdlIpl1 *ipl1 = (void *)sec;
>  
>      if (mode != ECKD_LDL_UNLABELED) {
> @@ -302,7 +365,9 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>      mbr_block_nr =
>          eckd_block_num((void *)&(ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr));
>  
> -    run_eckd_boot_script(mbr_block_nr);
> +    s1b_block_nr = eckd_block_num((void *)&(ipl1->stage1.seek[0].cyl));

dito.

> +    run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
>      /* no return */
>  }
>  
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index b700d08..8089402 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -74,6 +74,7 @@ typedef struct ScsiMbr {
>  } __attribute__ ((packed)) ScsiMbr;
>  
>  #define ZIPL_MAGIC              "zIPL"
> +#define ZIPL_MAGIC_EBCDIC       "\xa9\xc9\xd7\xd3"
>  #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
>  #define IPL2_MAGIC "\xc9\xd7\xd3\xf2" /* == "IPL2" in EBCDIC */
>  #define VOL1_MAGIC "\xe5\xd6\xd3\xf1" /* == "VOL1" in EBCDIC */
> @@ -229,6 +230,7 @@ typedef struct BootInfo {          /* @ 0x70, record #0   
>  */
>  /*
>   * Structs for IPL
>   */
> +#define STAGE2_MAX_SIZE     0x3000
>  #define STAGE2_BLK_CNT_MAX  24 /* Stage 1b can load up to 24 blocks */
>  
>  typedef struct EckdCdlIpl1 {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a8ef120..fb0ef92 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -11,6 +11,7 @@
>  #include "libc.h"
>  #include "s390-ccw.h"
>  #include "virtio.h"
> +#include "menu.h"
>  
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  static SubChannelId blk_schid = { .one = 1 };
> @@ -101,6 +102,8 @@ static void virtio_setup(void)
>              blk_schid.ssid = iplb.ccw.ssid & 0x3;
>              debug_print_int("ssid ", blk_schid.ssid);
>              found = find_dev(&schib, dev_no);
> +            menu_set_parms(iplb.ccw.boot_menu_flags,
> +                           iplb.ccw.boot_menu_timeout);
>              break;
>          case S390_IPL_TYPE_QEMU_SCSI:
>              vdev->scsi_device_selected = true;
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> new file mode 100644
> index 0000000..d707afb
> --- /dev/null
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -0,0 +1,223 @@
> +/*
> + * QEMU S390 Interactive Boot Menu
> + *
> + * Copyright 2017 IBM Corp.
> + * Author: Collin L. Walling <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "libc.h"
> +#include "s390-ccw.h"
> +#include "menu.h"
> +
> +#define KEYCODE_NO_INP '\0'
> +#define KEYCODE_ESCAPE '\033'
> +#define KEYCODE_BACKSP '\177'
> +#define KEYCODE_ENTER  '\r'
> +
> +static uint8_t flags;
> +static uint64_t timeout;
> +
> +static inline void enable_clock_int(void)
> +{
> +    uint64_t tmp = 0;
> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "oi         6+%0, 0x8\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp)

Did you check whether we need "memory" in the clobber list here?

> +    );
> +}
> +
> +static inline void disable_clock_int(void)
> +{
> +    uint64_t tmp = 0;
> +
> +    asm volatile(
> +        "stctg      0,0,%0\n"
> +        "ni         6+%0, 0xf7\n"
> +        "lctlg      0,0,%0"
> +        : : "Q" (tmp)
> +    );
> +}
> +
> +static inline void set_clock_comparator(uint64_t time)
> +{
> +    asm volatile("sckc %0" : : "Q" (time));
> +}
> +
> +static inline bool check_clock_int(void)
> +{
> +    uint16_t code = *(uint16_t *)0x86;
> +
> +    consume_sclp_int();

Don't you rather have to read the code from memory *after* calling
consume_sclp_int() ?

> +    return code == 0x1004;
> +}
> +
> +static int read_prompt(char *buf, size_t len)
> +{
> +    char inp[2];
> +    uint8_t idx = 0;
> +    uint64_t time;
> +
> +    if (timeout) {
> +        time = get_clock() + (timeout << 32);
> +        set_clock_comparator(time);
> +        enable_clock_int();
> +    }
> +
> +    inp[1] = '\0';

I think I'd rather declare "char inp[2] = {}", just to make sure that
inp[0] is also correctly pre-initialized - so that in case anything goes
wrong with sclp_read() below, we can be sure that there is not random
data in inp[0].

> +    while (!check_clock_int()) {
> +
> +        /* Process only one character at a time */
> +        sclp_read(inp, 1);
> +
> +        switch (inp[0]) {
> +        case KEYCODE_NO_INP:
> +        case KEYCODE_ESCAPE:
> +            continue;
> +        case KEYCODE_BACKSP:
> +            if (idx > 0) {
> +                /* Remove last character */
> +                buf[idx - 1] = ' ';
> +                sclp_print("\r");
> +                sclp_print(buf);
> +
> +                idx--;
> +
> +                /* Reset cursor */
> +                buf[idx] = 0;
> +                sclp_print("\r");
> +                sclp_print(buf);
> +            }
> +            continue;
> +        case KEYCODE_ENTER:
> +            disable_clock_int();
> +            return idx;
> +        }
> +
> +        /* Echo input and add to buffer */
> +        if (idx < len) {
> +            buf[idx] = inp[0];
> +            sclp_print(inp);
> +            idx++;
> +        }
> +    }
> +
> +    disable_clock_int();
> +    *buf = NULL;

Hmm, I'm used to see NULL only as a pointer, so "*buf = 0" would IMHO be
nicer here?

> +
> +    return 0;
> +}
> +
> +static int get_index(void)
> +{
> +    char buf[10];
> +    int len;
> +    int i;
> +
> +    memset(buf, 0, sizeof(buf));
> +
> +    len = read_prompt(buf, sizeof(buf));

I think you should rather use "sizeof(buf) - 1" here to make sure that
the string is always terminated with a 0?

> +    if (len == 0) {
> +        return 0;
> +    }
> +
> +    for (i = 0; i < len; i++) {
> +        if (!isdigit(buf[i])) {
> +            return -1;
> +        }
> +    }
> +
> +    return atoi(buf);
> +}

 Thomas