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

[Thomas Huth]

On 2019-01-29 14:29, 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 | 100 
> +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 102 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 1bc61b5..fa90aa3 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 */
>  
>  #define LOADPARM_PROMPT "PROMPT  "
>  #define LOADPARM_EMPTY  "        "
> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
> new file mode 100644
> index 0000000..47eaa04
> --- /dev/null
> +++ b/pc-bios/s390-ccw/s390-arch.h
> @@ -0,0 +1,100 @@
> +/*
> + * S390 Basic Architecture
> + *
> + * Copyright (c) 2018 Jason J. Herne <address@hidden>

2019 ?

> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef S390_ARCH_H
> +#define S390_ARCH_H
> +
> +typedef struct PSW {
> +    uint64_t mask;
> +    uint64_t addr;
> +} __attribute__ ((packed, aligned(8))) PSW;

We've seen quite some trouble with "packed" in the past... See
3b8afb41bc8e or 55281a2c53b884d0 for example ... This is only the
s390-ccw bios code here, so it is likely ok, but still, since this
structure is "naturally" packed, I'd rather go without that attribute
here (even if it's only to allow the compiler to generate better code in
some cases). You could still _Static_assert(sizeof(struct PSW) == 16)
afterwards, just to be sure.

> +
> +/* Older PSW format used by LPSW instruction */
> +typedef struct PSWLegacy {
> +    uint32_t mask;
> +    uint32_t addr;
> +} __attribute__ ((packed, aligned(8))) PSWLegacy;

dito, I'd drop the packed attribute here.

> +/* s390 psw bit masks */
> +#define PSW_MASK_IOINT      0x0200000000000000ULL
> +#define PSW_MASK_WAIT       0x0002000000000000ULL
> +#define PSW_MASK_EAMODE     0x0000000100000000ULL
> +#define PSW_MASK_BAMODE     0x0000000080000000ULL
> +#define PSW_MASK_ZMODE      (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
> +
> +/* Low core mapping */
> +typedef struct LowCore {
> +    /* prefix area: defined by architecture */
> +    uint64_t        ipl_psw;                  /* 0x000 */

No PSWLegacy here? ;-)

> +    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 */
> +    PSW             return_psw;               /* 0x200 */
> +    uint8_t         irb[64];                  /* 0x210 */
> +    uint64_t        sync_enter_timer;         /* 0x250 */
> +    uint64_t        async_enter_timer;        /* 0x258 */
> +    uint64_t        exit_timer;               /* 0x260 */
> +    uint64_t        last_update_timer;        /* 0x268 */
> +    uint64_t        user_timer;               /* 0x270 */
> +    uint64_t        system_timer;             /* 0x278 */
> +    uint64_t        last_update_clock;        /* 0x280 */
> +    uint64_t        steal_clock;              /* 0x288 */
> +    PSW             return_mcck_psw;          /* 0x290 */
> +    uint8_t         pad9[0xc00 - 0x2a0];      /* 0x2a0 */
> +} __attribute__((packed, aligned(8192))) LowCore;

dito, please avoid packed, use _Static_assert instead.

> +extern const LowCore *lowcore;
> +
> +#endif

 Thomas