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

[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 <address@hidden>
> ---
>  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 <address@hidden>