Re: [Qemu-devel] [PATCH v3 5/8] s390-ccw: interactive boot menu for eckd dasd (read stage2 data)

[Thomas Huth <thuth@redhat.com>]

On 15.01.2018 17:44, Collin L. Walling wrote:
> Read the stage2 boot loader data block-by-block. We scan the
> current block for the string "zIPL" to detect the start of the
> boot menu banner. We then load the adjacent blocks (previous
> block and next block) to account for the possibility of menu
> data spanning multiple blocks.
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 112 +++++++++++++++++++++++++++++++++++++++++++--
>  pc-bios/s390-ccw/bootmap.h |   1 +
>  pc-bios/s390-ccw/menu.h    |  10 ++++
>  3 files changed, 118 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 29915e4..fb8ff80 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,10 @@ 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 _s2[MAX_SECTOR_SIZE * 4] __attribute__((__aligned__(PAGE_SIZE)));

If I get the code right, you only load three sectors (prev, cur and
next), aren't you? So "* 3" instead of "* 4" should be enough?

> +static void *s2_prev_blk = _s2;
> +static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
> +static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
>  
>  static inline void verify_boot_info(BootInfo *bip)
>  {
> @@ -182,7 +187,94 @@ 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 ZiplParms get_zipl_parms(int offset)
> +{
> +    ZiplParms zipl_parms = {
> +        .flag = *(uint16_t *)(s2_cur_blk + offset - ZIPL_FLAG_OFFSET),
> +        .timeout = *(uint16_t *)(s2_cur_blk + offset - ZIPL_TIMEOUT_OFFSET),
> +        .menu_start = offset,
> +    };
> +
> +    return zipl_parms;
> +}

Looks weird that you need a separate function (with 9 lines of code) for
just setting three values in a struct ... and since the function is also
only used once: Why don't you simply set the three values at the place
where you call this function below?

> +static bool find_zipl_boot_menu_banner(int *offset)
> +{
> +    int i;
> +
> +    /* Menu banner starts with "zIPL" */
> +    for (i = 0; i < virtio_get_block_size() - 4; i++) {
> +        if (magic_match(s2_cur_blk + i, ZIPL_MAGIC_EBCDIC)) {
> +            *offset = i;
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
> +{
> +    block_number_t cur_block_nr, prev_block_nr, next_block_nr;
> +    EckdStage1b *s1b = (void *)sec;
> +    ZiplParms zipl_parms;
> +    bool found = false;
> +    int offset;
> +    int i;
> +
> +    /* Get Stage1b data */
> +    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> +    read_block(s1b_block_nr, s1b, "Cannot read stage1b boot loader");
> +
> +    memset(_s2, FREE_SPACE_FILLER, sizeof(_s2));
> +
> +    /* Get Stage2 data */
> +    for (i = 0; i < STAGE2_BLK_CNT_MAX; i++) {
> +        cur_block_nr = eckd_block_num((void *)&s1b->seek[i].cyl);

Casting a subset of EckdSeekArg via a void* into a BootMapPointer is
quite ugly. Maybe you could refactor eckd_block_num() instead, e.g. like
this:

static block_number_t eckd_block_num_by_chs(uint64_t cylinder,
                                            uint64_t head, uint64_t sec)
{
    const uint64_t sectors = virtio_get_sectors();
    const uint64_t heads = virtio_get_heads();
    block_number_t block;

    cylinder = cylinder + ((head & 0xfff0) << 12);
    head = head & 0x000f;
    block = sectors * heads * cylinder
            + sectors * head + sec
            - 1; /* block nr starts with zero */

    return block;
}

static block_number_t eckd_block_num(BootMapPointer *p)
{
    const uint64_t sectors = virtio_get_sectors();
    const uint64_t heads = virtio_get_heads();
    const uint64_t cylinder = p->eckd.cylinder
                            + ((p->eckd.head & 0xfff0) << 12);
    const uint64_t head = p->eckd.head & 0x000f;
    const block_number_t block = sectors * heads * cylinder
                               + sectors * head
                               + p->eckd.sector
                               - 1; /* block nr starts with zero */
    return eckd_block_num_by_chs(p->eckd.cylinder, p->eckd.head,
                                 p->eckd.sector);
}

... and then use eckd_block_num_by_chs() here instead?

> +        if (!cur_block_nr) {
> +            break;
> +        }
> +
> +        read_block(cur_block_nr, s2_cur_blk, "Cannot read stage2 boot loader");
> +
> +        if (find_zipl_boot_menu_banner(&offset)) {
> +            /* Load the adjacent blocks to account for the
> +             * possibility of menu data spanning multiple blocks.
> +             */
> +            if (prev_block_nr) {

You did not pre-initialize prev_block_nr = 0 at the beginning of the
function. Do you feel confident enough that the first block never
contains the zipl banner? If not, please set prev_block_nr = 0 before
entering the for-loop.

> +                read_block(prev_block_nr, s2_prev_blk,
> +                           "Cannot read stage2 boot loader");
> +            }
> +
> +            if (i + 1 < STAGE2_BLK_CNT_MAX) {
> +                next_block_nr = eckd_block_num((void *)&s1b->seek[i + 1].cyl);
> +            }
> +
> +            if (next_block_nr) {

Maybe also better set next_block_nr = 0 at the beginning of this function.

> +                read_block(next_block_nr, s2_next_blk,
> +                           "Cannot read stage2 boot loader");
> +            }
> +
> +            zipl_parms = get_zipl_parms(offset);
> +            found = true;
> +            break;
> +        }
> +
> +        prev_block_nr = cur_block_nr;
> +    }
> +
> +    if (!found) {
> +        sclp_print("No zipl boot menu data found. Booting default entry.");
> +        return 0;
> +    }
> +
> +    zipl_parms.menu_start++; /* make compiler happy -- does nothing vital */

<nit>
It's always nicer to avoid introducing code that you've got to remove
right in the next patch. You could rather add a

int menu_get_zipl_boot_index(const void *stage2, ZiplParms zipl_parms)
{
    /* implemented in the next patch */
}

to menu.c in this patch already, so you can already call it here and
don't have to modify eckd_get_boot_menu_index() again in the next patch.
</nit>

> +    return 0; /* implemented next patch */
> +}
> +
> +static void run_eckd_boot_script(block_number_t mbr_block_nr,
> +                                 block_number_t s1b_block_nr)
>  {
>      int i;
>      unsigned int loadparm = get_loadparm_index();
> @@ -191,6 +283,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>      ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
>      BootMapScript *bms = (void *)sec;
>  
> +    if (menu_check_flags(BOOT_MENU_FLAG_BOOT_OPTS | BOOT_MENU_FLAG_ZIPL_OPTS)) {
> +        loadparm = eckd_get_boot_menu_index(s1b_block_nr);
> +    }
> +
>      debug_print_int("loadparm", loadparm);
>      IPL_assert(loadparm < 31, "loadparm value greater than"
>                 " maximum number of boot entries allowed");
> @@ -223,7 +319,7 @@ static void ipl_eckd_cdl(void)
>      XEckdMbr *mbr;
>      EckdCdlIpl2 *ipl2 = (void *)sec;
>      IplVolumeLabel *vlbl = (void *)sec;
> -    block_number_t mbr_block_nr;
> +    block_number_t mbr_block_nr, s1b_block_nr;
>  
>      /* we have just read the block #0 and recognized it as "IPL1" */
>      sclp_print("CDL\n");
> @@ -249,7 +345,10 @@ static void ipl_eckd_cdl(void)
>      /* save pointer to Boot Script */
>      mbr_block_nr = eckd_block_num((void *)&mbr->blockptr);
>  
> -    run_eckd_boot_script(mbr_block_nr);
> +    /* save pointer to Stage1b Data */
> +    s1b_block_nr = eckd_block_num((void *)&ipl2->stage1.seek[0].cyl);

That could use eckd_block_num_by_chs(), too.

> +    run_eckd_boot_script(mbr_block_nr, s1b_block_nr);
>      /* no return */
>  }
>  
> @@ -280,7 +379,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 mbr_block_nr, s1b_block_nr;
>      EckdLdlIpl1 *ipl1 = (void *)sec;
>  
>      if (mode != ECKD_LDL_UNLABELED) {
> @@ -303,7 +402,10 @@ 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);
> +    /* save pointer to Stage1b Data */
> +    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 df956f2..56445f2 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 */
> diff --git a/pc-bios/s390-ccw/menu.h b/pc-bios/s390-ccw/menu.h
> index 04b1db1..3a8a487 100644
> --- a/pc-bios/s390-ccw/menu.h
> +++ b/pc-bios/s390-ccw/menu.h
> @@ -17,6 +17,16 @@
>  #define BOOT_MENU_FLAG_BOOT_OPTS 0x80
>  #define BOOT_MENU_FLAG_ZIPL_OPTS 0x40
>  
> +/* Offsets from zipl fields to zipl banner start */
> +#define ZIPL_TIMEOUT_OFFSET 138
> +#define ZIPL_FLAG_OFFSET    140
> +
> +typedef struct ZiplParms {
> +    uint16_t flag;
> +    uint16_t timeout;
> +    uint64_t menu_start;
> +} ZiplParms;
> +
>  void menu_set_parms(uint8_t boot_menu_flags, uint16_t boot_menu_timeout);
>  bool menu_check_flags(uint8_t check_flags);

 Thomas