[RESEND PATCH 0/1] pc-bios: Support List-Directed IPL from ECKD DASD

[RESEND PATCH 0/1] pc-bios: Support List-Directed IPL from ECKD DASD

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Add support for List-Directed IPL (LD-IPL) type pointers.

We check for a boot record indicating LD-IPL and use it if it is valid,
otherwise we use the CCW CDL or LDL format as usual. When a block is accessed
during IPL the block number is first calculated based on the cylinder, head,
and record numbers included in the block pointer; if LD-IPL has been initiated
then each pointer will be interpreted using the new format.

For simplicity, there is no choice presented to forcibly use CCW-IPL if LD-IPL
is available.  Because both sets of pointers ultimately go to the same
kernel/initrd, using CCW- or LD-IPL is transparent to the user.

One aspect of the user experience that does change is the availability of the
interactive boot menu when a loadparm is not specified.  For the existing
CCW-IPL, when the user does not specify a loadparm they are presented with a
list of boot options; however, this list is only written in the old style
pointers.  Therefore, if no loadparm is specified, and LD-IPL is supported, the
default boot option will be used automatically.

Jared Rossi (1):
  pc-bios: Add support for List-Directed IPL from ECKD DASD

 pc-bios/s390-ccw/bootmap.c | 157 ++++++++++++++++++++++++++++---------
 pc-bios/s390-ccw/bootmap.h |  30 ++++++-
 2 files changed, 148 insertions(+), 39 deletions(-)

-- 
2.31.1

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

[jrossi]

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);
+}
+
+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]),
                 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];
+    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 */
     }
 
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 3946aa3f8d..d4690a88c2 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -45,9 +45,23 @@ typedef struct EckdBlockPtr {
                     * it's 0 for TablePtr, ScriptPtr, and SectionPtr */
 } __attribute__ ((packed)) EckdBlockPtr;
 
-typedef struct ExtEckdBlockPtr {
+typedef struct LdEckdCHS {
+    uint32_t cylinder;
+    uint8_t head;
+    uint8_t sector;
+} __attribute__ ((packed)) LdEckdCHS;
+
+typedef struct LdEckdBlockPtr {
+    LdEckdCHS chs; /* cylinder/head/sector is an address of the block */
+    uint8_t reserved[4];
+    uint16_t count;
+    uint32_t pad;
+} __attribute__ ((packed)) LdEckdBlockPtr;
+
+/* bptr is used for CCW type IPL, while ldptr is for list-directed IPL */
+typedef union ExtEckdBlockPtr {
     EckdBlockPtr bptr;
-    uint8_t reserved[8];
+    LdEckdBlockPtr ldptr;
 } __attribute__ ((packed)) ExtEckdBlockPtr;
 
 typedef union BootMapPointer {
@@ -57,6 +71,15 @@ typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
+typedef struct BootRecord {
+    uint8_t magic[4];
+    uint32_t version;
+    uint64_t res1;
+    BootMapPointer pgt;
+    uint8_t reserved[510 - 32];
+    uint16_t os_id;
+} __attribute__ ((packed)) BootRecord;
+
 /* aka Program Table */
 typedef struct BootMapTable {
     uint8_t magic[4];
@@ -292,7 +315,8 @@ typedef struct IplVolumeLabel {
         struct {
             unsigned char key[4]; /* == "VOL1" */
             unsigned char volser[6];
-            unsigned char reserved[6];
+            unsigned char reserved[64];
+            EckdCHS br; /* Location of Boot Record for list-directed IPL */
         } f;
     };
 } __attribute__((packed)) IplVolumeLabel;
-- 
2.36.1

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

[Thomas Huth]

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(-)

  Janosch, Jason, Eric, Christian,

could you please help with reviewing + testing this patch?

  Thanks,
   Thomas

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

[Thomas Huth]

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

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

[Thomas Huth]

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(-)

Ok, the patch survived my local regression testing, and it also looks good 
enough to me for inclusion, so I'll queue it for my
final pull request before the QEMU 8.0 soft freeze tomorrow.

  Thomas

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

[Jared Rossi]

Sounds good.  Thanks Thomas.

Regards,
Jared Rossi

On 3/6/23 7:12 AM, Thomas Huth wrote:
> 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(-)
>
> Ok, the patch survived my local regression testing, and it also looks 
> good enough to me for inclusion, so I'll queue it for my
> final pull request before the QEMU 8.0 soft freeze tomorrow.
>
>  Thomas
>

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

[Eric Farman]

On Fri, 2023-03-03 at 13:38 +0100, Thomas Huth wrote:
> 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(-)
> 
>   Janosch, Jason, Eric, Christian,
> 
> could you please help with reviewing + testing this patch?

Thanks for the heads up, Thomas.

I've been using this on a z16 system with SE guests (with newer zipl to
actually write out LD-IPL data; something that should have been
mentioned here), and it seems to behave fine. I gave a quick glance
through the code, and while nothing jumped out at me this code is
rather opaque. I realize this is already queued by now but I'll still
go through it later this week so it gets another set of eyeballs.

Eric