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

["Jason J. Herne" <jjherne@linux.ibm.com>]

On 2/12/19 7:47 AM, Thomas Huth wrote:
> 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 <jjherne@linux.ibm.com>
>> ---
>>   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 <jjherne@us.ibm.com>
> 
> 2019 ?
> 

Yep, I will update this,

>> + * 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.
> 

So the problem is that this struct, if baked into another struct, will not be aligned 
properly? Given that this struct is only two 64-bit fields I guess we could get away 
without packed.

>> +
>> +/* 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.
> 

Are we sure the compiler will never insert space here if we drop packed? I don't see why 
it would but I'll admit that I don't fully know all of the rules.

>> +/* 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? ;-)
> 

I could *probably* use 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.
> 

I'm not convinced this is a good thing to do. Lowcore freely mixes fields of size 64, 32, 
and 8 bits. If we drop packed then the compiler will perform all sorts of automatic 
alignment thereby completely messing up offsets.


-- 
-- Jason J. Herne (jjherne@linux.ibm.com)