diff mbox

[v6,01/12] s390-ccw: refactor boot map table code

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

Commit Message

Collin L. Walling Feb. 15, 2018, 10:54 p.m. UTC
Some ECKD bootmap code was using structs designed for SCSI.
Even though this works, it confuses readability. Add a new
BootMapTable struct to assist with readability in bootmap
entry code. Also:

- replace ScsiMbr in ECKD code with appropriate structs
- fix read_block messages to reflect BootMapTable
- fixup ipl_scsi to use BootMapTable (referred to as Program Table)
- defined value for maximum table entries

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

Comments

Thomas Huth Feb. 16, 2018, 10:42 a.m. UTC | #1
On 15.02.2018 23:54, Collin L. Walling wrote:
> Some ECKD bootmap code was using structs designed for SCSI.
> Even though this works, it confuses readability. Add a new
> BootMapTable struct to assist with readability in bootmap
> entry code. Also:
> 
> - replace ScsiMbr in ECKD code with appropriate structs
> - fix read_block messages to reflect BootMapTable
> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
> - defined value for maximum table entries
> 
> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 60 +++++++++++++++++++++-------------------------
>  pc-bios/s390-ccw/bootmap.h | 14 +++++++++--
>  2 files changed, 39 insertions(+), 35 deletions(-)
[...]
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index cf99a4c..850b655 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>      ExtEckdBlockPtr xeckd;
>  } __attribute__ ((packed)) BootMapPointer;
>  
> +#define MAX_TABLE_ENTRIES  30
> +
> +/* aka Program Table */
> +typedef struct BootMapTable {
> +    uint8_t magic[4];
> +    uint8_t reserved[12];
> +    BootMapPointer entry[];
> +} __attribute__ ((packed)) BootMapTable;
> +
>  typedef struct ComponentEntry {
>      ScsiBlockPtr data;
>      uint8_t pad[7];
> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>  typedef struct ScsiMbr {
>      uint8_t magic[4];
>      uint32_t version_id;
> -    uint8_t reserved[8];
> -    ScsiBlockPtr blockptr[];
> +    uint8_t reserved1[8];
> +    ScsiBlockPtr pt;   /* block pointer to program table */
> +    uint8_t reserved2[120];

Did you want to pad the struct to 512 bytes here? If so, I think that
should have been "uint32_t" instead of "uint8_t" or "480" instead of "120" ?

>  } __attribute__ ((packed)) ScsiMbr;
>  
>  #define ZIPL_MAGIC              "zIPL"
> 

 Thomas
Viktor Mihajlovski Feb. 16, 2018, 12:19 p.m. UTC | #2
On 16.02.2018 11:42, Thomas Huth wrote:
> On 15.02.2018 23:54, Collin L. Walling wrote:
>> Some ECKD bootmap code was using structs designed for SCSI.
>> Even though this works, it confuses readability. Add a new
>> BootMapTable struct to assist with readability in bootmap
>> entry code. Also:
>>
>> - replace ScsiMbr in ECKD code with appropriate structs
>> - fix read_block messages to reflect BootMapTable
>> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
>> - defined value for maximum table entries
>>
>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>> ---
>>  pc-bios/s390-ccw/bootmap.c | 60 +++++++++++++++++++++-------------------------
>>  pc-bios/s390-ccw/bootmap.h | 14 +++++++++--
>>  2 files changed, 39 insertions(+), 35 deletions(-)
> [...]
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index cf99a4c..850b655 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>>      ExtEckdBlockPtr xeckd;
>>  } __attribute__ ((packed)) BootMapPointer;
>>  
>> +#define MAX_TABLE_ENTRIES  30
>> +
>> +/* aka Program Table */
>> +typedef struct BootMapTable {
>> +    uint8_t magic[4];
>> +    uint8_t reserved[12];
>> +    BootMapPointer entry[];
>> +} __attribute__ ((packed)) BootMapTable;
>> +
>>  typedef struct ComponentEntry {
>>      ScsiBlockPtr data;
>>      uint8_t pad[7];
>> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>>  typedef struct ScsiMbr {
>>      uint8_t magic[4];
>>      uint32_t version_id;
>> -    uint8_t reserved[8];
>> -    ScsiBlockPtr blockptr[];
>> +    uint8_t reserved1[8];
>> +    ScsiBlockPtr pt;   /* block pointer to program table */
>> +    uint8_t reserved2[120];
> 
> Did you want to pad the struct to 512 bytes here? If so, I think that
> should have been "uint32_t" instead of "uint8_t" or "480" instead of "120" ?
> 
Should probably be uint8_t[480] then to stay in style with other
reserved field definitions.
>>  } __attribute__ ((packed)) ScsiMbr;
>>  
>>  #define ZIPL_MAGIC              "zIPL"
>>
> 
>  Thomas
>
Collin L. Walling Feb. 16, 2018, 2:57 p.m. UTC | #3
On 02/16/2018 07:19 AM, Viktor Mihajlovski wrote:
> On 16.02.2018 11:42, Thomas Huth wrote:
>> On 15.02.2018 23:54, Collin L. Walling wrote:
>>> Some ECKD bootmap code was using structs designed for SCSI.
>>> Even though this works, it confuses readability. Add a new
>>> BootMapTable struct to assist with readability in bootmap
>>> entry code. Also:
>>>
>>> - replace ScsiMbr in ECKD code with appropriate structs
>>> - fix read_block messages to reflect BootMapTable
>>> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
>>> - defined value for maximum table entries
>>>
>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/bootmap.c | 60 +++++++++++++++++++++-------------------------
>>>   pc-bios/s390-ccw/bootmap.h | 14 +++++++++--
>>>   2 files changed, 39 insertions(+), 35 deletions(-)
>> [...]
>>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>>> index cf99a4c..850b655 100644
>>> --- a/pc-bios/s390-ccw/bootmap.h
>>> +++ b/pc-bios/s390-ccw/bootmap.h
>>> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>>>       ExtEckdBlockPtr xeckd;
>>>   } __attribute__ ((packed)) BootMapPointer;
>>>   
>>> +#define MAX_TABLE_ENTRIES  30
>>> +
>>> +/* aka Program Table */
>>> +typedef struct BootMapTable {
>>> +    uint8_t magic[4];
>>> +    uint8_t reserved[12];
>>> +    BootMapPointer entry[];
>>> +} __attribute__ ((packed)) BootMapTable;
>>> +
>>>   typedef struct ComponentEntry {
>>>       ScsiBlockPtr data;
>>>       uint8_t pad[7];
>>> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>>>   typedef struct ScsiMbr {
>>>       uint8_t magic[4];
>>>       uint32_t version_id;
>>> -    uint8_t reserved[8];
>>> -    ScsiBlockPtr blockptr[];
>>> +    uint8_t reserved1[8];
>>> +    ScsiBlockPtr pt;   /* block pointer to program table */
>>> +    uint8_t reserved2[120];
>> Did you want to pad the struct to 512 bytes here? If so, I think that
>> should have been "uint32_t" instead of "uint8_t" or "480" instead of "120" ?
>>
> Should probably be uint8_t[480] then to stay in style with other
> reserved field definitions.

The idea was to mimic the Scsi MBR struct defined in zipl.  It has
reserved space for what would appear to be for 5 more ScsiBlockPtrs
(which are not used in zipl) and a boot_info struct (which we don't need
in QEMU).

Alternatively, I can just omit the reserved2 field. I placed it there
for "completeness" but it is not at all necessary.

>>>   } __attribute__ ((packed)) ScsiMbr;
>>>   
>>>   #define ZIPL_MAGIC              "zIPL"
>>>
>>   Thomas
>>
>
Thomas Huth Feb. 16, 2018, 3:54 p.m. UTC | #4
On 16.02.2018 15:57, Collin L. Walling wrote:
> On 02/16/2018 07:19 AM, Viktor Mihajlovski wrote:
>> On 16.02.2018 11:42, Thomas Huth wrote:
>>> On 15.02.2018 23:54, Collin L. Walling wrote:
>>>> Some ECKD bootmap code was using structs designed for SCSI.
>>>> Even though this works, it confuses readability. Add a new
>>>> BootMapTable struct to assist with readability in bootmap
>>>> entry code. Also:
>>>>
>>>> - replace ScsiMbr in ECKD code with appropriate structs
>>>> - fix read_block messages to reflect BootMapTable
>>>> - fixup ipl_scsi to use BootMapTable (referred to as Program Table)
>>>> - defined value for maximum table entries
>>>>
>>>> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com>
>>>> ---
>>>>   pc-bios/s390-ccw/bootmap.c | 60
>>>> +++++++++++++++++++++-------------------------
>>>>   pc-bios/s390-ccw/bootmap.h | 14 +++++++++--
>>>>   2 files changed, 39 insertions(+), 35 deletions(-)
>>> [...]
>>>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>>>> index cf99a4c..850b655 100644
>>>> --- a/pc-bios/s390-ccw/bootmap.h
>>>> +++ b/pc-bios/s390-ccw/bootmap.h
>>>> @@ -53,6 +53,15 @@ typedef union BootMapPointer {
>>>>       ExtEckdBlockPtr xeckd;
>>>>   } __attribute__ ((packed)) BootMapPointer;
>>>>   +#define MAX_TABLE_ENTRIES  30
>>>> +
>>>> +/* aka Program Table */
>>>> +typedef struct BootMapTable {
>>>> +    uint8_t magic[4];
>>>> +    uint8_t reserved[12];
>>>> +    BootMapPointer entry[];
>>>> +} __attribute__ ((packed)) BootMapTable;
>>>> +
>>>>   typedef struct ComponentEntry {
>>>>       ScsiBlockPtr data;
>>>>       uint8_t pad[7];
>>>> @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
>>>>   typedef struct ScsiMbr {
>>>>       uint8_t magic[4];
>>>>       uint32_t version_id;
>>>> -    uint8_t reserved[8];
>>>> -    ScsiBlockPtr blockptr[];
>>>> +    uint8_t reserved1[8];
>>>> +    ScsiBlockPtr pt;   /* block pointer to program table */
>>>> +    uint8_t reserved2[120];
>>> Did you want to pad the struct to 512 bytes here? If so, I think that
>>> should have been "uint32_t" instead of "uint8_t" or "480" instead of
>>> "120" ?
>>>
>> Should probably be uint8_t[480] then to stay in style with other
>> reserved field definitions.
> 
> The idea was to mimic the Scsi MBR struct defined in zipl.  It has
> reserved space for what would appear to be for 5 more ScsiBlockPtrs
> (which are not used in zipl) and a boot_info struct (which we don't need
> in QEMU).
> 
> Alternatively, I can just omit the reserved2 field. I placed it there
> for "completeness" but it is not at all necessary.

I'd say just drop it. It looks rather confusing than helpful to me.

 Thomas
diff mbox

Patch

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..286de40 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -182,24 +182,24 @@  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);
-    IPL_assert(loadparm < 31, "loadparm value greater than"
+    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
                " 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->entry[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,8 @@  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 *prog_table = (void *)sec;
     unsigned int loadparm = get_loadparm_index();
 
     /* Grab the MBR */
@@ -467,34 +467,28 @@  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->pt.blockno);
+    IPL_assert(mbr->pt.blockno, "No Program Table");
 
     /* Parse the program table */
-    read_block(mbr->blockptr[0].blockno, sec,
-               "Error reading Program Table");
-
+    read_block(mbr->pt.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) {
+    while (program_table_entries <= MAX_TABLE_ENTRIES) {
+        if (!prog_table->entry[program_table_entries].scsi.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 */
+    debug_print_int("loadparm", loadparm);
+    IPL_assert(loadparm <= MAX_TABLE_ENTRIES, "loadparm value greater than"
+               " maximum number of boot entries allowed");
+
+    zipl_run(&prog_table->entry[loadparm].scsi); /* no return */
 }
 
 /***********************************************************************
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..850b655 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -53,6 +53,15 @@  typedef union BootMapPointer {
     ExtEckdBlockPtr xeckd;
 } __attribute__ ((packed)) BootMapPointer;
 
+#define MAX_TABLE_ENTRIES  30
+
+/* aka Program Table */
+typedef struct BootMapTable {
+    uint8_t magic[4];
+    uint8_t reserved[12];
+    BootMapPointer entry[];
+} __attribute__ ((packed)) BootMapTable;
+
 typedef struct ComponentEntry {
     ScsiBlockPtr data;
     uint8_t pad[7];
@@ -69,8 +78,9 @@  typedef struct ComponentHeader {
 typedef struct ScsiMbr {
     uint8_t magic[4];
     uint32_t version_id;
-    uint8_t reserved[8];
-    ScsiBlockPtr blockptr[];
+    uint8_t reserved1[8];
+    ScsiBlockPtr pt;   /* block pointer to program table */
+    uint8_t reserved2[120];
 } __attribute__ ((packed)) ScsiMbr;
 
 #define ZIPL_MAGIC              "zIPL"