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

[Thomas Huth]

On 18/02/2019 16.40, Jason J. Herne wrote:
> 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?

Yeah, stuff like that. Maybe you can get along here because you also
specified aligned(8) ... but from my experience, it's better to avoid
"packed" if possible.

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

That's why I suggested to also use _Static_assert(sizeof(x) == y). That
way you can be sure that the compiler does not add any unwanted padding.

[...]
>>> +} __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.

You should be fine if you also use _Static_assert() afterwards here.

... but since this is the s390-ccw bios and we can be sure that we only
compile this code for s390x and with gcc, I'm also fine if you keep the
attribute here. The problems we've seen so far only occurred on
non-s390x systems (Sparc...) or with Clang, so unlike we want to share
this header with the common QEMU code later, it should not really matter.

 Thomas