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

[Collin L. Walling]

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

--
- Collin L Walling