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

[Cornelia Huck]

On Mon, 18 Feb 2019 10:40:45 -0500
"Jason J. Herne" <address@hidden> 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
> >>

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

I would advise to try to build with clang, but there are other issues
in the ccw bios that prevent that :(

In general, the build assert way seems to be more portable, though.