Re: [Qemu-devel] [PATCH v7 01/12] s390-ccw: refactor boot map table code

2018-02-16 Thread Thomas Huth
On 16.02.2018 23:07, 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 
> ---
>  pc-bios/s390-ccw/bootmap.c | 60 
> +-
>  pc-bios/s390-ccw/bootmap.h | 11 -
>  2 files changed, 37 insertions(+), 34 deletions(-)
> 
> 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]);

I think you could drop the "(void *)" cast here now.

> +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);

dito, (void *) should not be needed here.

But apart from those two nits, the patch looks fine to me now, so:

Reviewed-by: Thomas Huth 



[Qemu-devel] [PATCH v7 01/12] s390-ccw: refactor boot map table code

2018-02-16 Thread Collin L. Walling
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 
---
 pc-bios/s390-ccw/bootmap.c | 60 +-
 pc-bios/s390-ccw/bootmap.h | 11 -
 2 files changed, 37 insertions(+), 34 deletions(-)

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 re