Re: [qemu-s390x] [PATCH v3 2/8] s390-ccw: ipl structs for eckd cdl/ldl

[Collin L. Walling]

On 01/16/2018 07:32 AM, Thomas Huth wrote:
> On 15.01.2018 17:44, Collin L. Walling wrote:
> > ECKD DASDs have different IPL structures for CDL and LDL
> > formats. The current Ipl1 and Ipl2 structs follow the CDL
> > format, so we prepend "EckdCdl" to them. Boot info for LDL
> > has been moved to a new struct: EckdLdlIpl1.
> >
> > Also introduce structs for IPL stages 1 and 1b and for
> > disk geometry.
> >
> > Signed-off-by: Collin L. Walling <address@hidden>
> > Acked-by: Janosch Frank <address@hidden>
> > ---
> >   pc-bios/s390-ccw/bootmap.c | 29 +++++++++++++-----------
> >   pc-bios/s390-ccw/bootmap.h | 55 +++++++++++++++++++++++++++++++++-------------
> >   2 files changed, 56 insertions(+), 28 deletions(-)
> >
> > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> > index 6f8e30f..29915e4 100644
> > --- a/pc-bios/s390-ccw/bootmap.c
> > +++ b/pc-bios/s390-ccw/bootmap.c
> > @@ -221,9 +221,9 @@ static void run_eckd_boot_script(block_number_t 
> > mbr_block_nr)
> >   static void ipl_eckd_cdl(void)
> >   {
> >       XEckdMbr *mbr;
> > -    Ipl2 *ipl2 = (void *)sec;
> > +    EckdCdlIpl2 *ipl2 = (void *)sec;
> >       IplVolumeLabel *vlbl = (void *)sec;
> > -    block_number_t block_nr;
> > +    block_number_t mbr_block_nr;
> >   
> >       /* we have just read the block #0 and recognized it as "IPL1" */
> >       sclp_print("CDL\n");
> > @@ -231,16 +231,13 @@ static void ipl_eckd_cdl(void)
> >       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> >       read_block(1, ipl2, "Cannot read IPL2 record at block 1");
> >   
> > -    mbr = &ipl2->u.x.mbr;
> > +    mbr = &ipl2->mbr;
> >       IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 
> > record.");
> >       IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
> >                  "Bad block size in zIPL section of IPL2 record.");
> >       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));
> > -
> >       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> >       read_block(2, vlbl, "Cannot read Volume Label at block 2");
> Why did you move the block_nr = eckd_block_num(...) behind the
> read_block() ? That sounds like you could different values here now. If
> that has been done on purpose and is not a mistake, please add a proper
> sentence about this to the patch description.


Yes you are definitely correct... a guest running cdl formatted dasd should
fail to IPL with this.  I must've made this error somewhere along the way
when cleaning up the commits.  Thank you for catching this.


> >       IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
> > @@ -249,7 +246,10 @@ static void ipl_eckd_cdl(void)
> >                  "Invalid magic of volser block");
> >       print_volser(vlbl->f.volser);
> >   
> > -    run_eckd_boot_script(block_nr);
> > +    /* save pointer to Boot Script */
> > +    mbr_block_nr = eckd_block_num((void *)&mbr->blockptr);
> > +
> > +    run_eckd_boot_script(mbr_block_nr);
> >       /* no return */
> >   }
> >   
> > @@ -280,8 +280,8 @@ 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;
> > -    BootInfo *bip = (void *)(sec + 0x70); /* BootInfo is MBR for LDL */
> > +    block_number_t mbr_block_nr;
> > +    EckdLdlIpl1 *ipl1 = (void *)sec;
> >   
> >       if (mode != ECKD_LDL_UNLABELED) {
> >           print_eckd_ldl_msg(mode);
> > @@ -292,15 +292,18 @@ static void ipl_eckd_ldl(ECKD_IPL_mode_t mode)
> >       memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> >       read_block(0, sec, "Cannot read block 0 to grab boot info.");
> >       if (mode == ECKD_LDL_UNLABELED) {
> > -        if (!magic_match(bip->magic, ZIPL_MAGIC)) {
> > +        if (!magic_match(ipl1->boot_info.magic, ZIPL_MAGIC)) {
> >               return; /* not applicable layout */
> >           }
> >           sclp_print("unlabeled LDL.\n");
> >       }
> > -    verify_boot_info(bip);
> > +    verify_boot_info(&ipl1->boot_info);
> > +
> > +    /* save pointer to Boot Script */
> > +    mbr_block_nr =
> > +        eckd_block_num((void *)&ipl1->boot_info.bp.ipl.bm_ptr.eckd.bptr);
> Well, the original code fitted nicely in one line ... thus maybe better
> keep the original variable name? (or use mbr_blk_nr or something similar
> shorter?)


Maybe a voidptr to ipl1->boot_info... and pass that to eckd_block_num?
(or something along those lines?)

or

EckdBlockPtr bptr = ipl1->....bptr

I'll play with the options.


> > -    block_nr = eckd_block_num((void *)&(bip->bp.ipl.bm_ptr.eckd.bptr));
> > -    run_eckd_boot_script(block_nr);
> > +    run_eckd_boot_script(mbr_block_nr);
> >       /* no return */
> >   }
>   Thomas


--
- Collin L Walling