Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3

[Thomas Huth]

On 24.08.2017 17:13, Cornelia Huck wrote:
> On Thu, 24 Aug 2017 11:05:08 -0400
> Farhan Ali <address@hidden> wrote:
> 
>> Hi,
>>
>> There is an issue in QEMU bios which is exposed by commit
>>
>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>> Author: Halil Pasic <address@hidden>
>> Date:   Thu Jul 27 17:48:42 2017 +0200
>>
>>      s390x/css: check ccw address validity
>>
>>      According to the PoP channel command words (CCW) must be doubleword
>>      aligned and 31 bit addressable for format 1 and 24 bit addressable for
>>      format 0 CCWs.
>>
>>      If the channel subsystem encounters a ccw address which does not 
>> satisfy
>>      this alignment requirement a program-check condition is recognised.
>>
>>      The situation with 31 bit addressable is a bit more complicated: 
>> both the
>>      ORB and a format 1 CCW TIC hold the address of (the rest of) the 
>> channel
>>      program, that is the address of the next CCW in a word, and the PoP
>>      mandates that bit 0 of that word shall be zero -- or a program-check
>>      condition is to be recognized -- and does not belong to the field 
>> holding
>>      the ccw address.
>>
>>      Since in code the corresponding fields span across the whole word 
>> (unlike
>>      in PoP where these are defined as 31 bit wide) we can check this by
>>      applying a mask. The 24 addressable case isn't affecting TIC 
>> because the
>>      address is composed of a halfword and a byte portion (no additional 
>> zero
>>      bit requirements) and just slightly complicates the ORB case where also
>>      bits 1-7 need to be zero.
>>
>>      The same requirements (especially n-bit addressability) apply to the
>>      ccw addresses generated while chaining.
>>
>>      Let's make our CSS implementation follow the AR more closely.
>>
>>      Signed-off-by: Halil Pasic <address@hidden>
>>      Message-Id: <address@hidden>
>>      Reviewed-by: Dong Jia Shi <address@hidden>
>>      Signed-off-by: Cornelia Huck <address@hidden>
>>
>>
>> It looks like the bios does not create a double word aligned CCW. 
>> Looking at the bios code we the CCW1 struct is not aligned
>>
>> /* channel command word (type 1) */
>> struct ccw1 {
>>      __u8 cmd_code;
>>      __u8 flags;
>>      __u16 count;
>>      __u32 cda;
>> } __attribute__ ((packed));
>>
>> and it looks like the compiler does not guarantee a doubleword alignment.
> 
> :(
> 
>>
>> The weird thing about it is I see it break in one of my system and works 
>> fine in another system. Trying a simple fix of aligning the struct also 
>> doesn't seem to work all the time.
> 
> I have not seen this problem on any of the systems I tested on (well, I
> would not have merged this if I did...) - RHEL 7 and F26. Do we need a
> dynamic allocation to guarantee alignment?

I guess the problem is the __attribute__((packed)) here - AFAIK GCC then
sometimes assumes that these structs can be byte-aligned. Does it work
if you remove the __attribute__((packed)) here? If yes, I think that
would be a valid fix, since there should not be any padding in this
struct at all (and if you're afraid, you could add an
assert(sizeof(struct ccw1) == 8) somewhere).

 Thomas