[v3,2/8] s390-ccw: ipl structs for eckd cdl/ldl

Commit Message

Collin L. Walling Jan. 15, 2018, 4:44 p.m. UTC

ECKD DASDs have different IPL structures for CDL and LDL
formats. The current Ipl1 and Ipl2 structs follow the CDL
format, so we prepend "EckdCdl" to them. Boot info for LDL
has been moved to a new struct: EckdLdlIpl1.

Also introduce structs for IPL stages 1 and 1b and for
disk geometry.

Signed-off-by: Collin L. Walling <[e-mail]>
Acked-by: Janosch Frank <[e-mail]>
---
 pc-bios/s390-ccw/bootmap.c | 29 +++++++++++++-----------
 pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 56 insertions(+), 28 deletions(-)

Comments

Thomas Huth Jan. 16, 2018, 12:32 p.m. UTC | #1

On 15.01.2018 17:44, Collin L. Walling wrote:
> ECKD DASDs have different IPL structures for CDL and LDL
> formats. The current Ipl1 and Ipl2 structs follow the CDL
> format, so we prepend "EckdCdl" to them. Boot info for LDL
> has been moved to a new struct: EckdLdlIpl1.
> 
> Also introduce structs for IPL stages 1 and 1b and for
> disk geometry.
> 
> Signed-off-by: Collin L. Walling <[e-mail]>
> Acked-by: Janosch Frank <[e-mail]>
> ---
>  pc-bios/s390-ccw/bootmap.c | 29 +++++++++++++-----------
>  pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>  2 files changed, 56 insertions(+), 28 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 6f8e30f..29915e4 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -221,9 +221,9 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>  static void ipl_eckd_cdl(void)
>  {
>      XEckdMbr *mbr;
> -    Ipl2 *ipl2 = (void *)sec;
> +    EckdCdlIpl2 *ipl2 = (void *)sec;
>      IplVolumeLabel *vlbl = (void *)sec;
> -    block_number_t block_nr;
> +    block_number_t mbr_block_nr;
>  
>      /* we have just read the block #0 and recognized it as "IPL1" */
>      sclp_print("CDL\n");
> @@ -231,16 +231,13 @@ static void ipl_eckd_cdl(void)
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>      read_block(1, ipl2, "Cannot read IPL2 record at block 1");
>  
> -    mbr = &ipl2->u.x.mbr;
> +    mbr = &ipl2->mbr;
>      IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
>      IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
>                 "Bad block size in zIPL section of IPL2 record.");
>      IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
>                 "Non-ECKD device type in zIPL section of IPL2 record.");
>  
> -    /* save pointer to Boot Script */
> -    block_nr = eckd_block_num((void *)&(mbr->blockptr));
> -
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>      read_block(2, vlbl, "Cannot read Volume Label at block 2");

Why did you move the block_nr = eckd_block_num(...) behind the
read_block() ? That sounds like you could different values here now. If
that has been done on purpose and is not a mistake, please add a proper
sentence about this to the patch description.

>      IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
> @@ -249,7 +246,10 @@ static void ipl_eckd_cdl(void)
>                 "Invalid magic of volser block");
>      print_volser(vlbl->f.volser);
>  
> -    run_eckd_boot_script(block_nr);
> +    /* save pointer to Boot Script */
> +    mbr_block_nr = eckd_block_num((void *)&mbr->blockptr);
> +
> +    run_eckd_boot_script(mbr_block_nr);
>      /* no return */
>  }
>  
> @@ -280,8 +280,8 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
>  
>  static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>  {
> -    block_number_t block_nr;
> -    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
> +    block_number_t mbr_block_nr;
> +    EckdLdlIpl1 *ipl1 = (void *)sec;
>  
>      if (mode != ECKD_LDL_UNLABELED) {
>          print_eckd_ldl_msg(mode);
> @@ -292,15 +292,18 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>      read_block(0, sec, "Cannot read block 0 to grab boot info.");
>      if (mode == ECKD_LDL_UNLABELED) {
> -        if (!magic_match(bip->magic, ZIPL_MAGIC)) {
> +        if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
>              return; /* not applicable layout */
>          }
>          sclp_print("unlabeled LDL.\n");
>      }
> -    verify_boot_info(bip);
> +    verify_boot_info(&ipl1->boot_info);
> +
> +    /* save pointer to Boot Script */
> +    mbr_block_nr =
> +        eckd_block_num((void *)&ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr);

Well, the original code fitted nicely in one line ... thus maybe better
keep the original variable name? (or use mbr_blk_nr or something similar
shorter?)

> -    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
> -    run_eckd_boot_script(block_nr);
> +    run_eckd_boot_script(mbr_block_nr);
>      /* no return */
>  }

 Thomas

Collin L. Walling Jan. 16, 2018, 3:21 p.m. UTC | #2

On 01/16/2018 07:32 AM, Thomas Huth wrote:
> On 15.01.2018 17:44, Collin L. Walling wrote:
>> ECKD DASDs have different IPL structures for CDL and LDL
>> formats. The current Ipl1 and Ipl2 structs follow the CDL
>> format, so we prepend "EckdCdl" to them. Boot info for LDL
>> has been moved to a new struct: EckdLdlIpl1.
>>
>> Also introduce structs for IPL stages 1 and 1b and for
>> disk geometry.
>>
>> Signed-off-by: Collin L. Walling <[e-mail]>
>> Acked-by: Janosch Frank <[e-mail]>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 29 +++++++++++++-----------
>>   pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
>>   2 files changed, 56 insertions(+), 28 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 6f8e30f..29915e4 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -221,9 +221,9 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>>   static void ipl_eckd_cdl(void)
>>   {
>>       XEckdMbr *mbr;
>> -    Ipl2 *ipl2 = (void *)sec;
>> +    EckdCdlIpl2 *ipl2 = (void *)sec;
>>       IplVolumeLabel *vlbl = (void *)sec;
>> -    block_number_t block_nr;
>> +    block_number_t mbr_block_nr;
>>   
>>       /* we have just read the block #0 and recognized it as "IPL1" */
>>       sclp_print("CDL\n");
>> @@ -231,16 +231,13 @@ static void ipl_eckd_cdl(void)
>>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>>       read_block(1, ipl2, "Cannot read IPL2 record at block 1");
>>   
>> -    mbr = &ipl2->u.x.mbr;
>> +    mbr = &ipl2->mbr;
>>       IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
>>       IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
>>                  "Bad block size in zIPL section of IPL2 record.");
>>       IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
>>                  "Non-ECKD device type in zIPL section of IPL2 record.");
>>   
>> -    /* save pointer to Boot Script */
>> -    block_nr = eckd_block_num((void *)&(mbr->blockptr));
>> -
>>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>>       read_block(2, vlbl, "Cannot read Volume Label at block 2");
> Why did you move the block_nr = eckd_block_num(...) behind the
> read_block() ? That sounds like you could different values here now. If
> that has been done on purpose and is not a mistake, please add a proper
> sentence about this to the patch description.


Yes you are definitely correct... a guest running cdl formatted dasd should
fail to IPL with this.  I must've made this error somewhere along the way
when cleaning up the commits.  Thank you for catching this.


>
>>       IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
>> @@ -249,7 +246,10 @@ static void ipl_eckd_cdl(void)
>>                  "Invalid magic of volser block");
>>       print_volser(vlbl->f.volser);
>>   
>> -    run_eckd_boot_script(block_nr);
>> +    /* save pointer to Boot Script */
>> +    mbr_block_nr = eckd_block_num((void *)&mbr->blockptr);
>> +
>> +    run_eckd_boot_script(mbr_block_nr);
>>       /* no return */
>>   }
>>   
>> @@ -280,8 +280,8 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
>>   
>>   static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>>   {
>> -    block_number_t block_nr;
>> -    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
>> +    block_number_t mbr_block_nr;
>> +    EckdLdlIpl1 *ipl1 = (void *)sec;
>>   
>>       if (mode != ECKD_LDL_UNLABELED) {
>>           print_eckd_ldl_msg(mode);
>> @@ -292,15 +292,18 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
>>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>>       read_block(0, sec, "Cannot read block 0 to grab boot info.");
>>       if (mode == ECKD_LDL_UNLABELED) {
>> -        if (!magic_match(bip->magic, ZIPL_MAGIC)) {
>> +        if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
>>               return; /* not applicable layout */
>>           }
>>           sclp_print("unlabeled LDL.\n");
>>       }
>> -    verify_boot_info(bip);
>> +    verify_boot_info(&ipl1->boot_info);
>> +
>> +    /* save pointer to Boot Script */
>> +    mbr_block_nr =
>> +        eckd_block_num((void *)&ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr);
> Well, the original code fitted nicely in one line ... thus maybe better
> keep the original variable name? (or use mbr_blk_nr or something similar
> shorter?)


Maybe a voidptr to ipl1->boot_info... and pass that to eckd_block_num?
(or something along those lines?)

or

EckdBlockPtr bptr = ipl1->....bptr

I'll play with the options.


>
>> -    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
>> -    run_eckd_boot_script(block_nr);
>> +    run_eckd_boot_script(mbr_block_nr);
>>       /* no return */
>>   }
>   Thomas
>

Patch

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 6f8e30f..29915e4 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -221,9 +221,9 @@  static void run_eckd_boot_script(block_number_t mbr_block_nr)
 static void ipl_eckd_cdl(void)
 {
     XEckdMbr *mbr;
-    Ipl2 *ipl2 = (void *)sec;
+    EckdCdlIpl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
-    block_number_t block_nr;
+    block_number_t mbr_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -231,16 +231,13 @@  static void ipl_eckd_cdl(void)
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(1, ipl2, "Cannot read IPL2 record at block 1");
 
-    mbr = &ipl2->u.x.mbr;
+    mbr = &ipl2->mbr;
     IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
     IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
                "Bad block size in zIPL section of IPL2 record.");
     IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
                "Non-ECKD device type in zIPL section of IPL2 record.");
 
-    /* save pointer to Boot Script */
-    block_nr = eckd_block_num((void *)&(mbr->blockptr));
-
     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 +246,10 @@  static void ipl_eckd_cdl(void)
                "Invalid magic of volser block");
     print_volser(vlbl->f.volser);
 
-    run_eckd_boot_script(block_nr);
+    /* save pointer to Boot Script */
+    mbr_block_nr = eckd_block_num((void *)&mbr->blockptr);
+
+    run_eckd_boot_script(mbr_block_nr);
     /* no return */
 }
 
@@ -280,8 +280,8 @@  static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
 
 static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
 {
-    block_number_t block_nr;
-    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
+    block_number_t mbr_block_nr;
+    EckdLdlIpl1 *ipl1 = (void *)sec;
 
     if (mode != ECKD_LDL_UNLABELED) {
         print_eckd_ldl_msg(mode);
@@ -292,15 +292,18 @@  static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(0, sec, "Cannot read block 0 to grab boot info.");
     if (mode == ECKD_LDL_UNLABELED) {
-        if (!magic_match(bip->magic, ZIPL_MAGIC)) {
+        if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
             return; /* not applicable layout */
         }
         sclp_print("unlabeled LDL.\n");
     }
-    verify_boot_info(bip);
+    verify_boot_info(&ipl1->boot_info);
+
+    /* save pointer to Boot Script */
+    mbr_block_nr =
+        eckd_block_num((void *)&ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr);
 
-    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
-    run_eckd_boot_script(block_nr);
+    run_eckd_boot_script(mbr_block_nr);
     /* no return */
 }
 
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 4980838..df956f2 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -226,22 +226,47 @@  typedef struct BootInfo {          /* @ 0x70, record #0    */
     } bp;
 } __attribute__ ((packed)) BootInfo; /* see also XEckdMbr   */
 
-typedef struct Ipl1 {
-    unsigned char key[4]; /* == "IPL1" */
-    unsigned char data[24];
-} __attribute__((packed)) Ipl1;
+/*
+ * Structs for IPL
+ */
+#define STAGE2_BLK_CNT_MAX  24 /* Stage 1b can load up to 24 blocks */
 
-typedef struct Ipl2 {
-    unsigned char key[4]; /* == "IPL2" */
-    union {
-        unsigned char data[144];
-        struct {
-            unsigned char reserved1[92-4];
-            XEckdMbr mbr;
-            unsigned char reserved2[144-(92-4)-sizeof(XEckdMbr)];
-        } x;
-    } u;
-} __attribute__((packed)) Ipl2;
+typedef struct EckdCdlIpl1 {
+    uint8_t key[4]; /* == "IPL1" */
+    uint8_t data[24];
+} __attribute__((packed)) EckdCdlIpl1;
+
+typedef struct EckdSeekArg {
+    uint16_t pad;
+    uint16_t cyl;
+    uint16_t head;
+    uint8_t sec;
+    uint8_t pad2;
+} __attribute__ ((packed)) EckdSeekArg;
+
+typedef struct EckdStage1b {
+    uint8_t reserved[32 * STAGE2_BLK_CNT_MAX];
+    struct EckdSeekArg seek[STAGE2_BLK_CNT_MAX];
+    uint8_t unused[64];
+} __attribute__ ((packed)) EckdStage1b;
+
+typedef struct EckdStage1 {
+    uint8_t reserved[72];
+    struct EckdSeekArg seek[2];
+} __attribute__ ((packed)) EckdStage1;
+
+typedef struct EckdCdlIpl2 {
+    uint8_t key[4]; /* == "IPL2" */
+    struct EckdStage1 stage1;
+    XEckdMbr mbr;
+    uint8_t reserved[24];
+} __attribute__((packed)) EckdCdlIpl2;
+
+typedef struct EckdLdlIpl1 {
+    uint8_t reserved[24];
+    struct EckdStage1 stage1;
+    BootInfo boot_info; /* BootInfo is MBR for LDL */
+} __attribute__((packed)) EckdLdlIpl1;
 
 typedef struct IplVolumeLabel {
     unsigned char key[4]; /* == "VOL1" */