|
| From: | Jason J. Herne |
| Subject: | Re: [qemu-s390x] [PATCH 08/15] s390-bios: Map low core memory |
| Date: | Mon, 18 Feb 2019 10:40:45 -0500 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
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 <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 ?
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 (address@hidden)
| [Prev in Thread] | Current Thread | [Next in Thread] |