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

[Thomas Huth <thuth@redhat.com>]

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