Re: [qemu-s390x] [PATCH v3 08/16] s390-bios: Map low core memory

[Jason J. Herne]

On 3/5/19 1:27 AM, Thomas Huth wrote:
> On 01/03/2019 19.59, Jason J. Herne wrote:
> > Create a new header for basic architecture specific definitions and add a
> > mapping of low core memory. This mapping will be used by the real dasd boot
> > process.
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
> > ---
> >   pc-bios/s390-ccw/main.c      |   2 +
> >   pc-bios/s390-ccw/s390-arch.h | 102 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 104 insertions(+)
> >   create mode 100644 pc-bios/s390-ccw/s390-arch.h
> >
> > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> > index 2d912cb..0670c14 100644
> > --- a/pc-bios/s390-ccw/main.c
> > +++ b/pc-bios/s390-ccw/main.c
> > @@ -9,6 +9,7 @@
> >    */
> >   
> >   #include "libc.h"
> > +#include "s390-arch.h"
> >   #include "s390-ccw.h"
> >   #include "cio.h"
> >   #include "virtio.h"
> > @@ -19,6 +20,7 @@ static char loadparm_str[LOADPARM_LEN + 1] = { 0, 0, 0, 0, 0, 
> > 0, 0, 0, 0 };
> >   QemuIplParameters qipl;
> >   IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> >   static bool have_iplb;
> > +const LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>
> Shouldn't that rather be "LowCore const *lowcore" instead?

Yep, will fix this in both places.

...
> > +/* Low core mapping */
> > +typedef struct LowCore {
> > +    /* prefix area: defined by architecture */
> > +    PSWLegacy       ipl_psw;                  /* 0x000 */
>
> Maybe remove some white space between the variable type and name
> everywhere in this struct?

I'd argue to leave this as-is for two reasons. 1) This is how it looks in the original 
(target/s390x/internal.h). 2) I prefer the alignment, I think it makes it easier to find 
what you're looking for, given the size of this struct.

> > +    uint32_t        ccw1[2];                  /* 0x008 */
> > +    uint32_t        ccw2[2];                  /* 0x010 */
> > +    uint8_t         pad1[0x80 - 0x18];        /* 0x018 */
> > +    uint32_t        ext_params;               /* 0x080 */
> > +    uint16_t        cpu_addr;                 /* 0x084 */
> > +    uint16_t        ext_int_code;             /* 0x086 */
> > +    uint16_t        svc_ilen;                 /* 0x088 */
> > +    uint16_t        svc_code;                 /* 0x08a */
> > +    uint16_t        pgm_ilen;                 /* 0x08c */
> > +    uint16_t        pgm_code;                 /* 0x08e */
> > +    uint32_t        data_exc_code;            /* 0x090 */
> > +    uint16_t        mon_class_num;            /* 0x094 */
> > +    uint16_t        per_perc_atmid;           /* 0x096 */
> > +    uint64_t        per_address;              /* 0x098 */
> > +    uint8_t         exc_access_id;            /* 0x0a0 */
> > +    uint8_t         per_access_id;            /* 0x0a1 */
> > +    uint8_t         op_access_id;             /* 0x0a2 */
> > +    uint8_t         ar_access_id;             /* 0x0a3 */
> > +    uint8_t         pad2[0xA8 - 0xA4];        /* 0x0a4 */
> > +    uint64_t        trans_exc_code;           /* 0x0a8 */
> > +    uint64_t        monitor_code;             /* 0x0b0 */
> > +    uint16_t        subchannel_id;            /* 0x0b8 */
> > +    uint16_t        subchannel_nr;            /* 0x0ba */
> > +    uint32_t        io_int_parm;              /* 0x0bc */
> > +    uint32_t        io_int_word;              /* 0x0c0 */
> > +    uint8_t         pad3[0xc8 - 0xc4];        /* 0x0c4 */
> > +    uint32_t        stfl_fac_list;            /* 0x0c8 */
> > +    uint8_t         pad4[0xe8 - 0xcc];        /* 0x0cc */
> > +    uint64_t        mcic;                     /* 0x0e8 */
> > +    uint8_t         pad5[0xf4 - 0xf0];        /* 0x0f0 */
> > +    uint32_t        external_damage_code;     /* 0x0f4 */
> > +    uint64_t        failing_storage_address;  /* 0x0f8 */
> > +    uint8_t         pad6[0x110 - 0x100];      /* 0x100 */
> > +    uint64_t        per_breaking_event_addr;  /* 0x110 */
> > +    uint8_t         pad7[0x120 - 0x118];      /* 0x118 */
> > +    PSW             restart_old_psw;          /* 0x120 */
> > +    PSW             external_old_psw;         /* 0x130 */
> > +    PSW             svc_old_psw;              /* 0x140 */
> > +    PSW             program_old_psw;          /* 0x150 */
> > +    PSW             mcck_old_psw;             /* 0x160 */
> > +    PSW             io_old_psw;               /* 0x170 */
> > +    uint8_t         pad8[0x1a0 - 0x180];      /* 0x180 */
> > +    PSW             restart_new_psw;          /* 0x1a0 */
> > +    PSW             external_new_psw;         /* 0x1b0 */
> > +    PSW             svc_new_psw;              /* 0x1c0 */
> > +    PSW             program_new_psw;          /* 0x1d0 */
> > +    PSW             mcck_new_psw;             /* 0x1e0 */
> > +    PSW             io_new_psw;               /* 0x1f0 */

--
-- Jason J. Herne (address@hidden)