diff mbox

[v4,01/10] s390-ccw: refactor boot map table code

Message ID 1516732013-18272-2-git-send-email-walling@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Collin L. Walling Jan. 23, 2018, 6:26 p.m. UTC 
- replace ScsiMbr in ECKD code with BootMapTable
- fix read_block messages to reflect BMT
- reduce ipl_scsi code with BMT struct

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
 pc-bios/s390-ccw/bootmap.h |  9 ++++++-
 2 files changed, 28 insertions(+), 39 deletions(-)

Comments

Thomas Huth Jan. 25, 2018, 10:07 a.m. UTC | #1 
On 23.01.2018 19:26, Collin L. Walling wrote:
> - replace ScsiMbr in ECKD code with BootMapTable
> - fix read_block messages to reflect BMT
> - reduce ipl_scsi code with BMT struct
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
>  pc-bios/s390-ccw/bootmap.h |  9 ++++++-
>  2 files changed, 28 insertions(+), 39 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 67a6123..6b6c915 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -182,13 +182,13 @@ 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 run_eckd_boot_script(block_number_t bmt_block_nr)
>  {
>      int i;
>      unsigned int loadparm = get_loadparm_index();
>      block_number_t block_nr;
>      uint64_t address;
> -    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
> +    BootMapTable *bmt = (void *)sec;
>      BootMapScript *bms = (void *)sec;
>  
>      debug_print_int("loadparm", loadparm);
> @@ -196,10 +196,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>                 " maximum number of boot entries allowed");
>  
>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> -    read_block(mbr_block_nr, sec, "Cannot read MBR");
> +    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>  
> -    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
> -    IPL_assert(block_nr != -1, "No Boot Map");
> +    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));

Ok, I checked that sizeof(ScsiBlockPtr) == sizeof(BootMapPointer), so
this should be fine.

But I think you can now remove the "(void *)". And while you're at it,
please also remove the superfluous parentheses:

    block_nr = eckd_block_num(&bmt->bte[loadparm]);

[...]
> @@ -449,10 +451,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>  static void ipl_scsi(void)
>  {
>      ScsiMbr *mbr = (void *)sec;
> -    uint8_t *ns, *ns_end;
> -    int program_table_entries = 0;
> -    const int pte_len = sizeof(ScsiBlockPtr);
> -    ScsiBlockPtr *prog_table_entry = NULL;
> +    BootMapTable *bmt = (void *)sec;
>      unsigned int loadparm = get_loadparm_index();
>  
>      /* Grab the MBR */
> @@ -467,34 +466,17 @@ static void ipl_scsi(void)
>      debug_print_int("MBR Version", mbr->version_id);
>      IPL_check(mbr->version_id == 1,
>                "Unknown MBR layout version, assuming version 1");
> -    debug_print_int("program table", mbr->blockptr[0].blockno);
> -    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
> +    debug_print_int("program table", mbr->bmt.blockno);
> +    IPL_assert(mbr->bmt.blockno, "No Program Table");
>  
>      /* Parse the program table */
> -    read_block(mbr->blockptr[0].blockno, sec,
> -               "Error reading Program Table");
> +    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
>  
>      IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
>  
>      debug_print_int("loadparm index", loadparm);
> -    ns_end = sec + virtio_get_block_size();
> -    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
> -        prog_table_entry = (ScsiBlockPtr *)ns;
> -        if (!prog_table_entry->blockno) {
> -            break;
> -        }
> -
> -        program_table_entries++;
> -        if (program_table_entries == loadparm + 1) {
> -            break; /* selected entry found */
> -        }
> -    }
> -
> -    debug_print_int("program table entries", program_table_entries);
> -
> -    IPL_assert(program_table_entries != 0, "Empty Program Table");

The new code looks much easier, indeed!

But is it OK that the "Empty Program Table" check is gone now? ... I
assume that zipl_run() will catch errors anyway, right?

 Thomas
Collin L. Walling Jan. 25, 2018, 2:54 p.m. UTC | #2 
On 01/25/2018 05:07 AM, Thomas Huth wrote:
> On 23.01.2018 19:26, Collin L. Walling wrote:
>> - replace ScsiMbr in ECKD code with BootMapTable
>> - fix read_block messages to reflect BMT
>> - reduce ipl_scsi code with BMT struct
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 58 ++++++++++++++++------------------------------
>>   pc-bios/s390-ccw/bootmap.h |  9 ++++++-
>>   2 files changed, 28 insertions(+), 39 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 67a6123..6b6c915 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -182,13 +182,13 @@ 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 run_eckd_boot_script(block_number_t bmt_block_nr)
>>   {
>>       int i;
>>       unsigned int loadparm = get_loadparm_index();
>>       block_number_t block_nr;
>>       uint64_t address;
>> -    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
>> +    BootMapTable *bmt = (void *)sec;
>>       BootMapScript *bms = (void *)sec;
>>   
>>       debug_print_int("loadparm", loadparm);
>> @@ -196,10 +196,10 @@ static void run_eckd_boot_script(block_number_t mbr_block_nr)
>>                  " maximum number of boot entries allowed");
>>   
>>       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
>> -    read_block(mbr_block_nr, sec, "Cannot read MBR");
>> +    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
>>   
>> -    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
>> -    IPL_assert(block_nr != -1, "No Boot Map");
>> +    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));
> Ok, I checked that sizeof(ScsiBlockPtr) == sizeof(BootMapPointer), so
> this should be fine.
>
> But I think you can now remove the "(void *)". And while you're at it,
> please also remove the superfluous parentheses:
>
>      block_nr = eckd_block_num(&bmt->bte[loadparm]);


I fix up the superfluous parens and get rid of the void ptrs in patch #2,
but I suppose it would not hurt to do some cleanup in this patch to
lines I already modify.


>
> [...]
>> @@ -449,10 +451,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>>   static void ipl_scsi(void)
>>   {
>>       ScsiMbr *mbr = (void *)sec;
>> -    uint8_t *ns, *ns_end;
>> -    int program_table_entries = 0;
>> -    const int pte_len = sizeof(ScsiBlockPtr);
>> -    ScsiBlockPtr *prog_table_entry = NULL;
>> +    BootMapTable *bmt = (void *)sec;
>>       unsigned int loadparm = get_loadparm_index();
>>   
>>       /* Grab the MBR */
>> @@ -467,34 +466,17 @@ static void ipl_scsi(void)
>>       debug_print_int("MBR Version", mbr->version_id);
>>       IPL_check(mbr->version_id == 1,
>>                 "Unknown MBR layout version, assuming version 1");
>> -    debug_print_int("program table", mbr->blockptr[0].blockno);
>> -    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
>> +    debug_print_int("program table", mbr->bmt.blockno);
>> +    IPL_assert(mbr->bmt.blockno, "No Program Table");
>>   
>>       /* Parse the program table */
>> -    read_block(mbr->blockptr[0].blockno, sec,
>> -               "Error reading Program Table");
>> +    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
>>   
>>       IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
>>   
>>       debug_print_int("loadparm index", loadparm);
>> -    ns_end = sec + virtio_get_block_size();
>> -    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
>> -        prog_table_entry = (ScsiBlockPtr *)ns;
>> -        if (!prog_table_entry->blockno) {
>> -            break;
>> -        }
>> -
>> -        program_table_entries++;
>> -        if (program_table_entries == loadparm + 1) {
>> -            break; /* selected entry found */
>> -        }
>> -    }
>> -
>> -    debug_print_int("program table entries", program_table_entries);
>> -
>> -    IPL_assert(program_table_entries != 0, "Empty Program Table");
> The new code looks much easier, indeed!
>
> But is it OK that the "Empty Program Table" check is gone now? ... I
> assume that zipl_run() will catch errors anyway, right?
>
>   Thomas
>

zipl_run() will catch any errors.  In the boot menu for SCSI patch, I 
included a simple
counter so that we can pass the number of entries to the menu code.  It 
would probably
do us some good to move that counter to this patch and do the check here.
diff mbox

Patch

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..6b6c915 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -182,13 +182,13 @@  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 run_eckd_boot_script(block_number_t bmt_block_nr)
 {
     int i;
     unsigned int loadparm = get_loadparm_index();
     block_number_t block_nr;
     uint64_t address;
-    ScsiMbr *bte = (void *)sec; /* Eckd bootmap table entry */
+    BootMapTable *bmt = (void *)sec;
     BootMapScript *bms = (void *)sec;
 
     debug_print_int("loadparm", loadparm);
@@ -196,10 +196,10 @@  static void run_eckd_boot_script(block_number_t mbr_block_nr)
                " maximum number of boot entries allowed");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
-    read_block(mbr_block_nr, sec, "Cannot read MBR");
+    read_block(bmt_block_nr, sec, "Cannot read Boot Map Table");
 
-    block_nr = eckd_block_num((void *)&(bte->blockptr[loadparm]));
-    IPL_assert(block_nr != -1, "No Boot Map");
+    block_nr = eckd_block_num((void *)&(bmt->bte[loadparm]));
+    IPL_assert(block_nr != -1, "Cannot find Boot Map Table Entry");
 
     memset(sec, FREE_SPACE_FILLER, sizeof(sec));
     read_block(block_nr, sec, "Cannot read Boot Map Script");
@@ -223,7 +223,7 @@  static void ipl_eckd_cdl(void)
     XEckdMbr *mbr;
     Ipl2 *ipl2 = (void *)sec;
     IplVolumeLabel *vlbl = (void *)sec;
-    block_number_t block_nr;
+    block_number_t bmt_block_nr;
 
     /* we have just read the block #0 and recognized it as "IPL1" */
     sclp_print("CDL\n");
@@ -238,8 +238,8 @@  static void ipl_eckd_cdl(void)
     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));
+    /* save pointer to Boot Map Table */
+    bmt_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");
@@ -249,7 +249,7 @@  static void ipl_eckd_cdl(void)
                "Invalid magic of volser block");
     print_volser(vlbl->f.volser);
 
-    run_eckd_boot_script(block_nr);
+    run_eckd_boot_script(bmt_block_nr);
     /* no return */
 }
 
@@ -280,7 +280,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 block_nr;
+    block_number_t bmt_block_nr;
     BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
 
     if (mode != ECKD_LDL_UNLABELED) {
@@ -299,8 +299,10 @@  static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
     }
     verify_boot_info(bip);
 
-    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
-    run_eckd_boot_script(block_nr);
+    /* save pointer to Boot Map Table */
+    bmt_block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
+
+    run_eckd_boot_script(bmt_block_nr);
     /* no return */
 }
 
@@ -325,7 +327,7 @@  static void print_eckd_msg(void)
 
 static void ipl_eckd(void)
 {
-    ScsiMbr *mbr = (void *)sec;
+    XEckdMbr *mbr = (void *)sec;
     LDL_VTOC *vlbl = (void *)sec;
 
     print_eckd_msg();
@@ -449,10 +451,7 @@  static void zipl_run(ScsiBlockPtr *pte)
 static void ipl_scsi(void)
 {
     ScsiMbr *mbr = (void *)sec;
-    uint8_t *ns, *ns_end;
-    int program_table_entries = 0;
-    const int pte_len = sizeof(ScsiBlockPtr);
-    ScsiBlockPtr *prog_table_entry = NULL;
+    BootMapTable *bmt = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
 
     /* Grab the MBR */
@@ -467,34 +466,17 @@  static void ipl_scsi(void)
     debug_print_int("MBR Version", mbr->version_id);
     IPL_check(mbr->version_id == 1,
               "Unknown MBR layout version, assuming version 1");
-    debug_print_int("program table", mbr->blockptr[0].blockno);
-    IPL_assert(mbr->blockptr[0].blockno, "No Program Table");
+    debug_print_int("program table", mbr->bmt.blockno);
+    IPL_assert(mbr->bmt.blockno, "No Program Table");
 
     /* Parse the program table */
-    read_block(mbr->blockptr[0].blockno, sec,
-               "Error reading Program Table");
+    read_block(mbr->bmt.blockno, sec, "Error reading Program Table");
 
     IPL_assert(magic_match(sec, ZIPL_MAGIC), "No zIPL magic in PT");
 
     debug_print_int("loadparm index", loadparm);
-    ns_end = sec + virtio_get_block_size();
-    for (ns = (sec + pte_len); (ns + pte_len) < ns_end; ns += pte_len) {
-        prog_table_entry = (ScsiBlockPtr *)ns;
-        if (!prog_table_entry->blockno) {
-            break;
-        }
-
-        program_table_entries++;
-        if (program_table_entries == loadparm + 1) {
-            break; /* selected entry found */
-        }
-    }
-
-    debug_print_int("program table entries", program_table_entries);
-
-    IPL_assert(program_table_entries != 0, "Empty Program Table");
 
-    zipl_run(prog_table_entry); /* no return */
+    zipl_run(&bmt->bte[loadparm].scsi); /* no return */
 }
 
 /***********************************************************************
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..77d56db 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -53,6 +53,13 @@  typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
+/* aka Program Table */
+typedef struct BootMapTable {
+    uint8_t magic[4];
+    uint8_t reserved[12];
+    BootMapPointer bte[];
+} __attribute__ ((packed)) BootMapTable;
+
 typedef struct ComponentEntry {
     ScsiBlockPtr data;
     uint8_t pad[7];
@@ -70,7 +77,7 @@  typedef struct ScsiMbr {
     uint8_t magic[4];
     uint32_t version_id;
     uint8_t reserved[8];
-    ScsiBlockPtr blockptr[];
+    ScsiBlockPtr bmt;
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC              "zIPL"