Re: [RESEND PATCH 1/1] pc-bios: Add support for List-Directed IPL from ECKD DASD

[Thomas Huth <thuth@redhat.com>]

On 21/02/2023 18.45, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Check for a List Directed IPL Boot Record, which would supersede the CCW type
> entries.  If the record is valid, proceed to use the new style pointers
> and perform LD-IPL. Each block pointer is interpreted as either an LD-IPL
> pointer or a legacy CCW pointer depending on the type of IPL initiated.
> 
> In either case CCW- or LD-IPL is transparent to the user and will boot the same
> image regardless of which set of pointers is used. Because the interactive boot
> menu is only written with the old style pointers, the menu will be disabled for
> List Directed IPL from ECKD DASD.
> 
> If the LD-IPL fails, retry the IPL using the CCW type pointers.
> 
> If no LD-IPL boot record is found, simply perform CCW type IPL as usual.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> 
> ---
>   pc-bios/s390-ccw/bootmap.c | 157 ++++++++++++++++++++++++++++---------
>   pc-bios/s390-ccw/bootmap.h |  30 ++++++-
>   2 files changed, 148 insertions(+), 39 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 994e59c0b0..77229f93f3 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -72,42 +72,74 @@ static inline void verify_boot_info(BootInfo *bip)
>                  "Bad block size in zIPL section of the 1st record.");
>   }
>   
> -static block_number_t eckd_block_num(EckdCHS *chs)
> +static void eckd_format_chs(ExtEckdBlockPtr *ptr,  bool ldipl,
> +                            uint64_t *c,
> +                            uint64_t *h,
> +                            uint64_t *s)
> +{
> +    if (ldipl) {
> +        *c = ptr->ldptr.chs.cylinder;
> +        *h = ptr->ldptr.chs.head;
> +        *s = ptr->ldptr.chs.sector;
> +    } else {
> +        *c = ptr->bptr.chs.cylinder;
> +        *h = ptr->bptr.chs.head;
> +        *s = ptr->bptr.chs.sector;
> +    }
> +}
> +
> +static block_number_t eckd_chs_to_block(uint64_t c, uint64_t h, uint64_t s)
>   {
>       const uint64_t sectors = virtio_get_sectors();
>       const uint64_t heads = virtio_get_heads();
> -    const uint64_t cylinder = chs->cylinder
> -                            + ((chs->head & 0xfff0) << 12);
> -    const uint64_t head = chs->head & 0x000f;
> +    const uint64_t cylinder = c + ((h & 0xfff0) << 12);
> +    const uint64_t head = h & 0x000f;
>       const block_number_t block = sectors * heads * cylinder
>                                  + sectors * head
> -                               + chs->sector
> -                               - 1; /* block nr starts with zero */
> +                               + s - 1; /* block nr starts with zero */
>       return block;
>   }
>   
> -static bool eckd_valid_address(BootMapPointer *p)
> +static block_number_t eckd_block_num(EckdCHS *chs)
>   {
> -    const uint64_t head = p->eckd.chs.head & 0x000f;
> +    return eckd_chs_to_block(chs->cylinder, chs->head, chs->sector);
> +}
> +
> +static block_number_t gen_eckd_block_num(ExtEckdBlockPtr *ptr, bool ldipl)
> +{
> +    uint64_t cyl, head, sec;
> +    eckd_format_chs(ptr, ldipl, &cyl, &head, &sec);
> +    return eckd_chs_to_block(cyl, head, sec);
> +}
>   
> +static bool eckd_valid_chs(uint64_t cyl, uint64_t head, uint64_t sector)
> +{
>       if (head >= virtio_get_heads()
> -        ||  p->eckd.chs.sector > virtio_get_sectors()
> -        ||  p->eckd.chs.sector <= 0) {
> +        || sector > virtio_get_sectors()
> +        || sector <= 0) {
>           return false;
>       }
>   
>       if (!virtio_guessed_disk_nature() &&
> -        eckd_block_num(&p->eckd.chs) >= virtio_get_blocks()) {
> +        eckd_chs_to_block(cyl, head, sector) >= virtio_get_blocks()) {
>           return false;
>       }
>   
>       return true;
>   }
>   
> -static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
> +static bool eckd_valid_address(ExtEckdBlockPtr *ptr, bool ldipl)
> +{
> +    uint64_t cyl, head, sec;
> +    eckd_format_chs(ptr, ldipl, &cyl, &head, &sec);
> +    return eckd_valid_chs(cyl, head, sec);
> +}

It would have been nice to split the rework of eckd_block_num() and 
eckd_valid_address() into a separate patch first to ease review.

> +static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
> +                                         uint64_t *address)
>   {
>       block_number_t block_nr;
> -    int j, rc;
> +    int j, rc, count;
>       BootMapPointer *bprs = (void *)_bprs;
>       bool more_data;
>   
> @@ -117,7 +149,7 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
>       do {
>           more_data = false;
>           for (j = 0;; j++) {
> -            block_nr = eckd_block_num(&bprs[j].xeckd.bptr.chs);
> +            block_nr = gen_eckd_block_num(&bprs[j].xeckd, ldipl);
>               if (is_null_block_number(block_nr)) { /* end of chunk */
>                   break;
>               }
> @@ -129,11 +161,26 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
>                   break;
>               }
>   
> -            IPL_assert(block_size_ok(bprs[j].xeckd.bptr.size),
> +            /* List directed pointer does not store block size */
> +            IPL_assert(ldipl || block_size_ok(bprs[j].xeckd.bptr.size),
>                          "bad chunk block size");
> -            IPL_assert(eckd_valid_address(&bprs[j]), "bad chunk ECKD addr");
>   
> -            if ((bprs[j].xeckd.bptr.count == 0) && unused_space(&(bprs[j+1]),
> +            if (!eckd_valid_address(&bprs[j].xeckd, ldipl)) {
> +                /*
> +                 * If an invalid address is found during LD-IPL then break and
> +                 * retry as CCW
> +                 */
> +                IPL_assert(ldipl, "bad chunk ECKD addr");
> +                break;
> +            }
> +
> +            if (ldipl) {
> +                count = bprs[j].xeckd.ldptr.count;
> +            } else {
> +                count = bprs[j].xeckd.bptr.count;
> +            }
> +
> +            if ((count == 0) && unused_space(&(bprs[j + 1]),

I know this is pre-existing in the code, but I'd be fine if we'd drop the 
superfluous parantheses here while you're touching this line anyway.

>                   sizeof(EckdBlockPtr))) {
>                   /* This is a "continue" pointer.
>                    * This ptr should be the last one in the current
> @@ -149,11 +196,10 @@ static block_number_t load_eckd_segments(block_number_t blk, uint64_t *address)
>               /* Load (count+1) blocks of code at (block_nr)
>                * to memory (address).
>                */
> -            rc = virtio_read_many(block_nr, (void *)(*address),
> -                                  bprs[j].xeckd.bptr.count+1);
> +            rc = virtio_read_many(block_nr, (void *)(*address), count + 1);
>               IPL_assert(rc == 0, "code chunk read failed");
>   
> -            *address += (bprs[j].xeckd.bptr.count+1) * virtio_get_block_size();
> +            *address += (count + 1) * virtio_get_block_size();
>           }
>       } while (more_data);
>       return block_nr;
> @@ -237,8 +283,10 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
>       uint64_t address;
>       BootMapTable *bmt = (void *)sec;
>       BootMapScript *bms = (void *)sec;
> +    /* The S1B block number is NULL_BLOCK_NR if and only if it's an LD-IPL */
> +    bool ldipl = (s1b_block_nr == NULL_BLOCK_NR);
>   
> -    if (menu_is_enabled_zipl()) {
> +    if (menu_is_enabled_zipl() && !ldipl) {
>           loadparm = eckd_get_boot_menu_index(s1b_block_nr);
>       }
>   
> @@ -249,7 +297,7 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>       read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>   
> -    block_nr = eckd_block_num(&bmt->entry[loadparm].xeckd.bptr.chs);
> +    block_nr = gen_eckd_block_num(&bmt->entry[loadparm].xeckd, ldipl);
>       IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
>   
>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> @@ -264,13 +312,18 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
>           }
>   
>           address = bms->entry[i].address.load_address;
> -        block_nr = eckd_block_num(&bms->entry[i].blkptr.xeckd.bptr.chs);
> +        block_nr = gen_eckd_block_num(&bms->entry[i].blkptr.xeckd, ldipl);
>   
>           do {
> -            block_nr = load_eckd_segments(block_nr, &address);
> +            block_nr = load_eckd_segments(block_nr, ldipl, &address);
>           } while (block_nr != -1);
>       }
>   
> +    if (ldipl && bms->entry[i].type != BOOT_SCRIPT_EXEC) {
> +        /* Abort LD-IPL and retry as CCW-IPL */
> +        return;
> +    }
> +
>       IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
>                  "Unknown script entry type");
>       write_reset_psw(bms->entry[i].address.load_address); /* no return */
> @@ -380,6 +433,23 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>       /* no return */
>   }
>   
> +static block_number_t eckd_find_bmt(ExtEckdBlockPtr *ptr)
> +{
> +    block_number_t blockno;
> +    uint8_t tmp_sec[MAX_SECTOR_SIZE];

Don't we need some alignment for tmp_sec? Would it be possible to use the 
sec[] buffer for this instead?

(Hmm, we're also doing this in the zipl_run() function ... so it likely 
works ... still, it looks fragile to me)

> +    BootRecord *br;
> +
> +    blockno = gen_eckd_block_num(ptr, 0);
> +    read_block(blockno, tmp_sec, "Cannot read boot record");
> +    br = (BootRecord *)tmp_sec;
> +    if (!magic_match(br->magic, ZIPL_MAGIC)) {
> +        /* If the boot record is invalid, return and try CCW-IPL instead */
> +        return NULL_BLOCK_NR;
> +    }
> +
> +    return gen_eckd_block_num(&br->pgt.xeckd, 1);
> +}
> +
>   static void print_eckd_msg(void)
>   {
>       char msg[] = "Using ECKD scheme (block size *****), ";
> @@ -401,28 +471,43 @@ static void print_eckd_msg(void)
>   
>   static void ipl_eckd(void)
>   {
> -    XEckdMbr *mbr = (void *)sec;
> -    LDL_VTOC *vlbl = (void *)sec;
> +    IplVolumeLabel *vlbl = (void *)sec;
> +    LDL_VTOC *vtoc = (void *)sec;
> +    block_number_t ldipl_bmt; /* Boot Map Table for List-Directed IPL */
>   
>       print_eckd_msg();
>   
> -    /* Grab the MBR again */
> +    /* Block 2 can contain either the CDL VOL1 label or the LDL VTOC */
>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> -    read_block(0, mbr, "Cannot read block 0 on DASD");
> +    read_block(2, vlbl, "Cannot read block 2");
>   
> -    if (magic_match(mbr->magic, IPL1_MAGIC)) {
> -        ipl_eckd_cdl();         /* only returns in case of error */
> -        return;
> +    /*
> +     * First check for a list-directed-format pointer which would
> +     * supersede the CCW pointer.
> +     */
> +    if (eckd_valid_address((ExtEckdBlockPtr *)&vlbl->f.br, 0)) {
> +        ldipl_bmt = eckd_find_bmt((ExtEckdBlockPtr *)&vlbl->f.br);
> +        if (ldipl_bmt) {
> +            sclp_print("List-Directed\n");
> +            /* LD-IPL does not use the S1B bock, just make it NULL */
> +            run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR);
> +            /* Only return in error, retry as CCW-IPL */
> +            sclp_print("Retrying IPL ");
> +            print_eckd_msg();
> +        }
> +        memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> +        read_block(2, vtoc, "Cannot read block 2");
>       }
>   
> -    /* LDL/CMS? */
> -    memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> -    read_block(2, vlbl, "Cannot read block 2");
> +    /* Not list-directed */
> +    if (magic_match(vtoc->magic, VOL1_MAGIC)) {
> +        ipl_eckd_cdl(); /* may return in error */
> +    }
>   
> -    if (magic_match(vlbl->magic, CMS1_MAGIC)) {
> +    if (magic_match(vtoc->magic, CMS1_MAGIC)) {
>           ipl_eckd_ldl(ECKD_CMS); /* no return */
>       }
> -    if (magic_match(vlbl->magic, LNX1_MAGIC)) {
> +    if (magic_match(vtoc->magic, LNX1_MAGIC)) {
>           ipl_eckd_ldl(ECKD_LDL); /* no return */
>       }

  Thomas