Re: [PATCH 3/5] s390x: Build IPLB chain for multiple boot devices

[Thomas Huth]

On 05/06/2024 22.01, Jared Rossi wrote:
> On 6/4/24 2:26 PM, Thomas Huth wrote:
> > On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
> > > From: Jared Rossi <jrossi@linux.ibm.com>
> > >
> > > Write a chain of IPLBs into memory for future use.
> > >
> > > The IPLB chain is placed immediately before the BIOS in memory at the 
> > > highest
> > > unused page boundary providing sufficient space to fit the chain. Because 
> > > this
> > > is not a fixed address, the location of the next IPLB and number of 
> > > remaining
> > > boot devices is stored in the QIPL global variable for later access.
> > >
> > > At this stage the IPLB chain is not accessed by the guest during IPL.
> > >
> > > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> > > ---
> > >   hw/s390x/ipl.h              |   1 +
> > >   include/hw/s390x/ipl/qipl.h |   4 +-
> > >   hw/s390x/ipl.c              | 129 +++++++++++++++++++++++++++---------
> > >   3 files changed, 103 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> > > index 1dcb8984bb..4f098d3a81 100644
> > > --- a/hw/s390x/ipl.h
> > > +++ b/hw/s390x/ipl.h
> > > @@ -20,6 +20,7 @@
> > >   #include "qom/object.h"
> > >     #define DIAG308_FLAGS_LP_VALID 0x80
> > > +#define MAX_IPLB_CHAIN 7
> > >     void s390_ipl_set_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
> > >   void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
> > > diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> > > index a6ce6ddfe3..481c459a53 100644
> > > --- a/include/hw/s390x/ipl/qipl.h
> > > +++ b/include/hw/s390x/ipl/qipl.h
> > > @@ -34,7 +34,9 @@ struct QemuIplParameters {
> > >       uint8_t  reserved1[3];
> > >       uint64_t netboot_start_addr;
> > >       uint32_t boot_menu_timeout;
> > > -    uint8_t  reserved2[12];
> > > +    uint8_t  reserved2[2];
> > > +    uint16_t num_iplbs;
> > > +    uint64_t next_iplb;
> > >   }  QEMU_PACKED;
> > >   typedef struct QemuIplParameters QemuIplParameters;
> > >   diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > > index 2d4f5152b3..79429acabd 100644
> > > --- a/hw/s390x/ipl.c
> > > +++ b/hw/s390x/ipl.c
> > > @@ -55,6 +55,13 @@ static bool iplb_extended_needed(void *opaque)
> > >       return ipl->iplbext_migration;
> > >   }
> > >   +/* Start IPLB chain from the boundary of the first unused page before 
> > > BIOS */
> >
> > I'd maybe say "upper boundary" to make it clear that this is at the end of 
> > the page, not at the beginning?
>
> The chain does start at the beginning of a page.  That being said, the 
> comment still needs to be reworded, I'm just not sure exactly how. "Start 
> the IPLB chain from the nearest page boundary providing sufficient space 
> before BIOS?"  Basically because each IPLB is 4K, the chain will occupy the 
> N unused pages before the start of BIOS, where N is the number of chained 
> IPLBS (assuming 4K pages).

Ah, right, I missed that sizeof(IplParameterBlock) == 4096 (I guess I was 
looking at the old version in pc-bios/s390-ccw/iplb.h that does not seem to 
have the padding), sorry for the confusion! It's really good that you now 
unify the headers in your first patch!

 Thomas