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

[Collin L. Walling]

On 02/16/2018 07:19 AM, Viktor Mihajlovski wrote:
> On 16.02.2018 11:42, Thomas Huth wrote:
> > On 15.02.2018 23:54, 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 | 14 +++++++++--
> > >   2 files changed, 39 insertions(+), 35 deletions(-)
> > [...]
> > > diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> > > index cf99a4c..850b655 100644
> > > --- a/pc-bios/s390-ccw/bootmap.h
> > > +++ b/pc-bios/s390-ccw/bootmap.h
> > > @@ -53,6 +53,15 @@ typedef union BootMapPointer {
> > >       ExtEckdBlockPtr xeckd;
> > >   } __attribute__ ((packed)) BootMapPointer;
> > >   
> > > +#define MAX_TABLE_ENTRIES  30
> > > +
> > > +/* aka Program Table */
> > > +typedef struct BootMapTable {
> > > +    uint8_t magic[4];
> > > +    uint8_t reserved[12];
> > > +    BootMapPointer entry[];
> > > +} __attribute__ ((packed)) BootMapTable;
> > > +
> > >   typedef struct ComponentEntry {
> > >       ScsiBlockPtr data;
> > >       uint8_t pad[7];
> > > @@ -69,8 +78,9 @@ typedef struct ComponentHeader {
> > >   typedef struct ScsiMbr {
> > >       uint8_t magic[4];
> > >       uint32_t version_id;
> > > -    uint8_t reserved[8];
> > > -    ScsiBlockPtr blockptr[];
> > > +    uint8_t reserved1[8];
> > > +    ScsiBlockPtr pt;   /* block pointer to program table */
> > > +    uint8_t reserved2[120];
> > Did you want to pad the struct to 512 bytes here? If so, I think that
> > should have been "uint32_t" instead of "uint8_t" or "480" instead of "120" ?
> Should probably be uint8_t[480] then to stay in style with other
> reserved field definitions.

The idea was to mimic the Scsi MBR struct defined in zipl.  It has
reserved space for what would appear to be for 5 more ScsiBlockPtrs
(which are not used in zipl) and a boot_info struct (which we don't need
in QEMU).

Alternatively, I can just omit the reserved2 field. I placed it there
for "completeness" but it is not at all necessary.

> > >   } __attribute__ ((packed)) ScsiMbr;
> > >   
> > >   #define ZIPL_MAGIC              "zIPL"
> >   Thomas


--
- Collin L Walling