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

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

[Farhan Ali]

Hi,

There is an issue in QEMU bios which is exposed by commit

commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
Author: Halil Pasic <pasic@linux.vnet.ibm.com>
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 <pasic@linux.vnet.ibm.com>
     Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
     Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
     Signed-off-by: Cornelia Huck <cohuck@redhat.com>


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.


Thanks
Farhan

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

[Cornelia Huck]

On Thu, 24 Aug 2017 11:05:08 -0400
Farhan Ali <alifm@linux.vnet.ibm.com> wrote:

> Hi,
> 
> There is an issue in QEMU bios which is exposed by commit
> 
> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
> 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 <pasic@linux.vnet.ibm.com>
>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> 
> 
> 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?

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 <alifm@linux.vnet.ibm.com> wrote:
> 
>> Hi,
>>
>> There is an issue in QEMU bios which is exposed by commit
>>
>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>> 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 <pasic@linux.vnet.ibm.com>
>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>
>>
>> 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

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

[Halil Pasic]

On 08/24/2017 05:35 PM, Thomas Huth wrote:
> On 24.08.2017 17:13, Cornelia Huck wrote:
>> On Thu, 24 Aug 2017 11:05:08 -0400
>> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>
>>> Hi,
>>>
>>> There is an issue in QEMU bios which is exposed by commit
>>>
>>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> 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 <pasic@linux.vnet.ibm.com>
>>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>>
>>> 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
> 
> 

I don't think this packed is doing us any good. But even
with the packed removed I not sure we would end up being
8 byte aligned (dobuleword). Wouldn't it be just 4 byte
aligned (according to the ELF ABI supplement for s390)
as the most strictly aligned member is __u32?

Halil

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

[Thomas Huth]

On 24.08.2017 17:47, Halil Pasic wrote:
> 
> 
> On 08/24/2017 05:35 PM, Thomas Huth wrote:
>> On 24.08.2017 17:13, Cornelia Huck wrote:
>>> On Thu, 24 Aug 2017 11:05:08 -0400
>>> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> There is an issue in QEMU bios which is exposed by commit
>>>>
>>>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>>>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> 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 <pasic@linux.vnet.ibm.com>
>>>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>
>>>>
>>>> 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).
> 
> I don't think this packed is doing us any good. But even
> with the packed removed I not sure we would end up being
> 8 byte aligned (dobuleword). Wouldn't it be just 4 byte
> aligned (according to the ELF ABI supplement for s390)
> as the most strictly aligned member is __u32?

True, so that could still be an issue. Looking at the cio.h in the
kernel, they define the struct like this:

struct ccw1 {
        __u8  cmd_code;
        __u8  flags;
        __u16 count;
        __u32 cda;
} __attribute__ ((packed,aligned(8)));

So I guess adding the aligned(8) is the right way to go?

 Thomas

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

[Farhan Ali]

On 08/24/2017 11:50 AM, Thomas Huth wrote:
> On 24.08.2017 17:47, Halil Pasic wrote:
>>
>>
>> On 08/24/2017 05:35 PM, Thomas Huth wrote:
>>> On 24.08.2017 17:13, Cornelia Huck wrote:
>>>> On Thu, 24 Aug 2017 11:05:08 -0400
>>>> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> There is an issue in QEMU bios which is exposed by commit
>>>>>
>>>>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>>>>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>> 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 <pasic@linux.vnet.ibm.com>
>>>>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>>>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>>
>>>>>
>>>>> 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).
>>
>> I don't think this packed is doing us any good. But even
>> with the packed removed I not sure we would end up being
>> 8 byte aligned (dobuleword). Wouldn't it be just 4 byte
>> aligned (according to the ELF ABI supplement for s390)
>> as the most strictly aligned member is __u32?
>
> True, so that could still be an issue. Looking at the cio.h in the
> kernel, they define the struct like this:
>
> struct ccw1 {
>         __u8  cmd_code;
>         __u8  flags;
>         __u16 count;
>         __u32 cda;
> } __attribute__ ((packed,aligned(8)));
>
> So I guess adding the aligned(8) is the right way to go?
>
>  Thomas
>

This was my initial fix and it works on my system. But for some reason 
this fix does not work on my colleague's system. So I am hesitant about 
submitting this fix

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

[Halil Pasic]

On 08/24/2017 05:53 PM, Farhan Ali wrote:
> 
> 
> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>> On 24.08.2017 17:47, Halil Pasic wrote:
>>>
>>>
>>> On 08/24/2017 05:35 PM, Thomas Huth wrote:
>>>> On 24.08.2017 17:13, Cornelia Huck wrote:
>>>>> On Thu, 24 Aug 2017 11:05:08 -0400
>>>>> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> There is an issue in QEMU bios which is exposed by commit
>>>>>>
>>>>>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>>>>>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>> 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 <pasic@linux.vnet.ibm.com>
>>>>>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>>>>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>>>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>>>
>>>>>>
>>>>>> 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).
>>>
>>> I don't think this packed is doing us any good. But even
>>> with the packed removed I not sure we would end up being
>>> 8 byte aligned (dobuleword). Wouldn't it be just 4 byte
>>> aligned (according to the ELF ABI supplement for s390)
>>> as the most strictly aligned member is __u32?
>>
>> True, so that could still be an issue. Looking at the cio.h in the
>> kernel, they define the struct like this:
>>
>> struct ccw1 {
>>         __u8  cmd_code;
>>         __u8  flags;
>>         __u16 count;
>>         __u32 cda;
>> } __attribute__ ((packed,aligned(8)));
>>
>> So I guess adding the aligned(8) is the right way to go?
>>
>>  Thomas
>>
> 
> This was my initial fix and it works on my system. But for some reason this fix does not work on my colleague's system. So I am hesitant about submitting this fix

That sounds mysterious. Could you debug that with your colleague
and verify that the problem is indeed the alignment? If it
is I would have to re-read the gcc doc because I can't think of
anything else than a bug either in my understanding or in their product.

A stupid question is he testing with the current master?

Halil 

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

[Christian Borntraeger]

On 08/24/2017 06:02 PM, Halil Pasic wrote:
> 
> 
> On 08/24/2017 05:53 PM, Farhan Ali wrote:
>>
>>
>> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>>> On 24.08.2017 17:47, Halil Pasic wrote:
>>>>
>>>>
>>>> On 08/24/2017 05:35 PM, Thomas Huth wrote:
>>>>> On 24.08.2017 17:13, Cornelia Huck wrote:
>>>>>> On Thu, 24 Aug 2017 11:05:08 -0400
>>>>>> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> There is an issue in QEMU bios which is exposed by commit
>>>>>>>
>>>>>>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>>>>>>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>>>>> 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 <pasic@linux.vnet.ibm.com>
>>>>>>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>>>>>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>>>>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>>>>
>>>>>>>
>>>>>>> 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).
>>>>
>>>> I don't think this packed is doing us any good. But even
>>>> with the packed removed I not sure we would end up being
>>>> 8 byte aligned (dobuleword). Wouldn't it be just 4 byte
>>>> aligned (according to the ELF ABI supplement for s390)
>>>> as the most strictly aligned member is __u32?
>>>
>>> True, so that could still be an issue. Looking at the cio.h in the
>>> kernel, they define the struct like this:
>>>
>>> struct ccw1 {
>>>         __u8  cmd_code;
>>>         __u8  flags;
>>>         __u16 count;
>>>         __u32 cda;
>>> } __attribute__ ((packed,aligned(8)));
>>>
>>> So I guess adding the aligned(8) is the right way to go?
>>>
>>>  Thomas
>>>
>>
>> This was my initial fix and it works on my system. But for some reason this fix does not work on my colleague's system. So I am hesitant about submitting this fix
> 
> That sounds mysterious. Could you debug that with your colleague
> and verify that the problem is indeed the alignment? If it
> is I would have to re-read the gcc doc because I can't think of
> anything else than a bug either in my understanding or in their product.
> 
> A stupid question is he testing with the current master?

FWIW, The ELF ABI mandates an 8 byte alignment (double word in IBM speak) for every stack frame,
so with aligned(8) this should work.

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

[Peter Maydell]

On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>
>
> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>> True, so that could still be an issue. Looking at the cio.h in the
>> kernel, they define the struct like this:
>>
>> struct ccw1 {
>>         __u8  cmd_code;
>>         __u8  flags;
>>         __u16 count;
>>         __u32 cda;
>> } __attribute__ ((packed,aligned(8)));
>>
>> So I guess adding the aligned(8) is the right way to go?

> This was my initial fix and it works on my system. But for some reason this
> fix does not work on my colleague's system. So I am hesitant about
> submitting this fix

It seems like it ought to be the obvious fix, so I would double
check that on your colleague's system the change really did
get recompiled and it's actually using the new version (that
sort of mistake can be easy to make and very confusing...)

thanks
-- PMM

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

[Farhan Ali]

On 08/24/2017 12:07 PM, Peter Maydell wrote:
> On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>
>>
>> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>>> True, so that could still be an issue. Looking at the cio.h in the
>>> kernel, they define the struct like this:
>>>
>>> struct ccw1 {
>>>         __u8  cmd_code;
>>>         __u8  flags;
>>>         __u16 count;
>>>         __u32 cda;
>>> } __attribute__ ((packed,aligned(8)));
>>>
>>> So I guess adding the aligned(8) is the right way to go?
>
>> This was my initial fix and it works on my system. But for some reason this
>> fix does not work on my colleague's system. So I am hesitant about
>> submitting this fix
>
> It seems like it ought to be the obvious fix, so I would double
> check that on your colleague's system the change really did
> get recompiled and it's actually using the new version (that
> sort of mistake can be easy to make and very confusing...)
>
> thanks
> -- PMM
>

So after trying again with the fix, it seems to work on my colleague's 
system for most cases. It fails for LDL DASD boot case.... we are still 
investigating it.

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

[Christian Borntraeger]

On 08/24/2017 07:38 PM, Farhan Ali wrote:
> 
> 
> On 08/24/2017 12:07 PM, Peter Maydell wrote:
>> On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>
>>>
>>> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>>>> True, so that could still be an issue. Looking at the cio.h in the
>>>> kernel, they define the struct like this:
>>>>
>>>> struct ccw1 {
>>>>         __u8  cmd_code;
>>>>         __u8  flags;
>>>>         __u16 count;
>>>>         __u32 cda;
>>>> } __attribute__ ((packed,aligned(8)));
>>>>
>>>> So I guess adding the aligned(8) is the right way to go?
>>
>>> This was my initial fix and it works on my system. But for some reason this
>>> fix does not work on my colleague's system. So I am hesitant about
>>> submitting this fix
>>
>> It seems like it ought to be the obvious fix, so I would double
>> check that on your colleague's system the change really did
>> get recompiled and it's actually using the new version (that
>> sort of mistake can be easy to make and very confusing...)

>>
>> thanks
>> -- PMM
>>
> 
> So after trying again with the fix, it seems to work on my colleague's system for most cases. It fails for LDL DASD boot case.... we are still investigating it.

So chances are that this is an independent problem, I guess?

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

[Cornelia Huck]

On Thu, 24 Aug 2017 20:14:06 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 08/24/2017 07:38 PM, Farhan Ali wrote:
> > 
> > 
> > On 08/24/2017 12:07 PM, Peter Maydell wrote:  
> >> On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:  
> >>>
> >>>
> >>> On 08/24/2017 11:50 AM, Thomas Huth wrote:  
> >>>> True, so that could still be an issue. Looking at the cio.h in the
> >>>> kernel, they define the struct like this:
> >>>>
> >>>> struct ccw1 {
> >>>>         __u8  cmd_code;
> >>>>         __u8  flags;
> >>>>         __u16 count;
> >>>>         __u32 cda;
> >>>> } __attribute__ ((packed,aligned(8)));
> >>>>
> >>>> So I guess adding the aligned(8) is the right way to go?  
> >>  
> >>> This was my initial fix and it works on my system. But for some reason this
> >>> fix does not work on my colleague's system. So I am hesitant about
> >>> submitting this fix  
> >>
> >> It seems like it ought to be the obvious fix, so I would double
> >> check that on your colleague's system the change really did
> >> get recompiled and it's actually using the new version (that
> >> sort of mistake can be easy to make and very confusing...)  
> 
> >>
> >> thanks
> >> -- PMM
> >>  
> > 
> > So after trying again with the fix, it seems to work on my colleague's system for most cases. It fails for LDL DASD boot case.... we are still investigating it.  
> 
> So chances are that this is an independent problem, I guess?
> 

OK, to recap:

- the current pre-built bios seems fine
- rebuilding the bios may yield a version that fails on some systems
  (different compiler?)
- adding aligned(8) looks like the right thing to do
- it seems to fix the problem, but on at least one system something
  still seems off (under investigation)

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

[Christian Borntraeger]

On 08/25/2017 09:20 AM, Cornelia Huck wrote:
> On Thu, 24 Aug 2017 20:14:06 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 08/24/2017 07:38 PM, Farhan Ali wrote:
>>>
>>>
>>> On 08/24/2017 12:07 PM, Peter Maydell wrote:  
>>>> On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:  
>>>>>
>>>>>
>>>>> On 08/24/2017 11:50 AM, Thomas Huth wrote:  
>>>>>> True, so that could still be an issue. Looking at the cio.h in the
>>>>>> kernel, they define the struct like this:
>>>>>>
>>>>>> struct ccw1 {
>>>>>>         __u8  cmd_code;
>>>>>>         __u8  flags;
>>>>>>         __u16 count;
>>>>>>         __u32 cda;
>>>>>> } __attribute__ ((packed,aligned(8)));
>>>>>>
>>>>>> So I guess adding the aligned(8) is the right way to go?  
>>>>  
>>>>> This was my initial fix and it works on my system. But for some reason this
>>>>> fix does not work on my colleague's system. So I am hesitant about
>>>>> submitting this fix  
>>>>
>>>> It seems like it ought to be the obvious fix, so I would double
>>>> check that on your colleague's system the change really did
>>>> get recompiled and it's actually using the new version (that
>>>> sort of mistake can be easy to make and very confusing...)  
>>
>>>>
>>>> thanks
>>>> -- PMM
>>>>  
>>>
>>> So after trying again with the fix, it seems to work on my colleague's system for most cases. It fails for LDL DASD boot case.... we are still investigating it.  
>>
>> So chances are that this is an independent problem, I guess?
>>
> 
> OK, to recap:
> 
> - the current pre-built bios seems fine
> - rebuilding the bios may yield a version that fails on some systems
>   (different compiler?)
> - adding aligned(8) looks like the right thing to do
> - it seems to fix the problem, but on at least one system something
>   still seems off (under investigation)

Yes. I am out of office today, so for any aligned(8) patch
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
even for 2.10.

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

[Cornelia Huck]

On Fri, 25 Aug 2017 10:21:58 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 08/25/2017 09:20 AM, Cornelia Huck wrote:

> > OK, to recap:
> > 
> > - the current pre-built bios seems fine
> > - rebuilding the bios may yield a version that fails on some systems
> >   (different compiler?)
> > - adding aligned(8) looks like the right thing to do
> > - it seems to fix the problem, but on at least one system something
> >   still seems off (under investigation)  
> 
> Yes. I am out of office today, so for any aligned(8) patch
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> even for 2.10.

I fear the 2.10 train has already left the station, but any aligned(8)
patch should be cc:stable.

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

[Christian Borntraeger]

On 08/25/2017 10:29 AM, Cornelia Huck wrote:
> On Fri, 25 Aug 2017 10:21:58 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
> 
>>> OK, to recap:
>>>
>>> - the current pre-built bios seems fine
>>> - rebuilding the bios may yield a version that fails on some systems
>>>   (different compiler?)
>>> - adding aligned(8) looks like the right thing to do
>>> - it seems to fix the problem, but on at least one system something
>>>   still seems off (under investigation)  
>>
>> Yes. I am out of office today, so for any aligned(8) patch
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> even for 2.10.
> 
> I fear the 2.10 train has already left the station, but any aligned(8)
> patch should be cc:stable.

I think this could be a topic for QEMU summit. Our process of not allowing
fixes in rcx without requiring an rc(x+1) seems a bit odd. The Linux kernel
style (there are fixes between the last rc and release) seems more balanced
as long as we establish some safety nets.

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

[Thomas Huth]

On 28.08.2017 09:18, Christian Borntraeger wrote:
> 
> 
> On 08/25/2017 10:29 AM, Cornelia Huck wrote:
>> On Fri, 25 Aug 2017 10:21:58 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
>>
>>>> OK, to recap:
>>>>
>>>> - the current pre-built bios seems fine
>>>> - rebuilding the bios may yield a version that fails on some systems
>>>>   (different compiler?)
>>>> - adding aligned(8) looks like the right thing to do
>>>> - it seems to fix the problem, but on at least one system something
>>>>   still seems off (under investigation)  
>>>
>>> Yes. I am out of office today, so for any aligned(8) patch
>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> even for 2.10.
>>
>> I fear the 2.10 train has already left the station, but any aligned(8)
>> patch should be cc:stable.
> 
> I think this could be a topic for QEMU summit. Our process of not allowing
> fixes in rcx without requiring an rc(x+1) seems a bit odd. The Linux kernel
> style (there are fixes between the last rc and release) seems more balanced
> as long as we establish some safety nets.

This sounds like a good idea to me, yes. And maybe we could also ease
the situation a little bit by providing the first stable .1 release
already two or three weeks after the .0 release [*] ? Then these "we are
not 100% sure whether this is a severe blocker or not" patches could
simply be provided to the public with that .1 release instead of
blocking the QEMU master branch in freeze state...

 Thomas


[*] I know that means more additional work for Michael - sorry for that
... but at least we should talk about this, I think. Maybe someone else
could also help with the releases if it's too much work for one person?

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

[Christian Borntraeger]

On 08/29/2017 11:35 AM, Thomas Huth wrote:
> On 28.08.2017 09:18, Christian Borntraeger wrote:
>>
>>
>> On 08/25/2017 10:29 AM, Cornelia Huck wrote:
>>> On Fri, 25 Aug 2017 10:21:58 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>
>>>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
>>>
>>>>> OK, to recap:
>>>>>
>>>>> - the current pre-built bios seems fine
>>>>> - rebuilding the bios may yield a version that fails on some systems
>>>>>   (different compiler?)
>>>>> - adding aligned(8) looks like the right thing to do
>>>>> - it seems to fix the problem, but on at least one system something
>>>>>   still seems off (under investigation)  
>>>>
>>>> Yes. I am out of office today, so for any aligned(8) patch
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> even for 2.10.
>>>
>>> I fear the 2.10 train has already left the station, but any aligned(8)
>>> patch should be cc:stable.
>>
>> I think this could be a topic for QEMU summit. Our process of not allowing
>> fixes in rcx without requiring an rc(x+1) seems a bit odd. The Linux kernel
>> style (there are fixes between the last rc and release) seems more balanced
>> as long as we establish some safety nets.
> 
> This sounds like a good idea to me, yes. And maybe we could also ease
> the situation a little bit by providing the first stable .1 release
> already two or three weeks after the .0 release [*] ?

+1

> Then these "we are
> not 100% sure whether this is a severe blocker or not" patches could
> simply be provided to the public with that .1 release instead of
> blocking the QEMU master branch in freeze state...
> 
>  Thomas
> 
> 
> [*] I know that means more additional work for Michael - sorry for that
> ... but at least we should talk about this, I think. Maybe someone else
> could also help with the releases if it's too much work for one person?
> 

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

[Michael Roth]

Quoting Thomas Huth (2017-08-29 04:35:22)
> On 28.08.2017 09:18, Christian Borntraeger wrote:
> > 
> > 
> > On 08/25/2017 10:29 AM, Cornelia Huck wrote:
> >> On Fri, 25 Aug 2017 10:21:58 +0200
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>
> >>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
> >>
> >>>> OK, to recap:
> >>>>
> >>>> - the current pre-built bios seems fine
> >>>> - rebuilding the bios may yield a version that fails on some systems
> >>>>   (different compiler?)
> >>>> - adding aligned(8) looks like the right thing to do
> >>>> - it seems to fix the problem, but on at least one system something
> >>>>   still seems off (under investigation)  
> >>>
> >>> Yes. I am out of office today, so for any aligned(8) patch
> >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>> even for 2.10.
> >>
> >> I fear the 2.10 train has already left the station, but any aligned(8)
> >> patch should be cc:stable.
> > 
> > I think this could be a topic for QEMU summit. Our process of not allowing
> > fixes in rcx without requiring an rc(x+1) seems a bit odd. The Linux kernel
> > style (there are fixes between the last rc and release) seems more balanced
> > as long as we establish some safety nets.
> 
> This sounds like a good idea to me, yes. And maybe we could also ease
> the situation a little bit by providing the first stable .1 release
> already two or three weeks after the .0 release [*] ? Then these "we are
> not 100% sure whether this is a severe blocker or not" patches could
> simply be provided to the public with that .1 release instead of
> blocking the QEMU master branch in freeze state...

I can give it a try for 2.10.1.

At least initially, I would prefer to not have there be a set expectation
that there will be a quick .1 release though, but rather anything tagged
for-2.10, for instance, that doesn't make it in, will then fall into
the "expedited 2.10.1" bucket, and for that to be the *only* way to get
into that bucket (we'd still do the normal round-up and pull in any
"Cc: qemu-stable@nongnu.org" patches at that point though).

This would help ensure we don't steal focus from the .0 releases and allow
those "is this a blocker?" discussions to happen in the proper context.

> 
>  Thomas
> 
> 
> [*] I know that means more additional work for Michael - sorry for that
> ... but at least we should talk about this, I think. Maybe someone else
> could also help with the releases if it's too much work for one person?
> 

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

[Christian Ehrhardt]

On Thu, Aug 31, 2017 at 7:44 PM, Michael Roth <mdroth@linux.vnet.ibm.com>
wrote:

> Quoting Thomas Huth (2017-08-29 04:35:22)
> > On 28.08.2017 09:18, Christian Borntraeger wrote:
> > >
> > >
> > > On 08/25/2017 10:29 AM, Cornelia Huck wrote:
> > >> On Fri, 25 Aug 2017 10:21:58 +0200
> > >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > >>
> > >>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
> > >>
> > >>>> OK, to recap:
> > >>>>
> > >>>> - the current pre-built bios seems fine
> > >>>> - rebuilding the bios may yield a version that fails on some systems
> > >>>>   (different compiler?)
> > >>>> - adding aligned(8) looks like the right thing to do
> > >>>> - it seems to fix the problem, but on at least one system something
> > >>>>   still seems off (under investigation)
> > >>>
> > >>> Yes. I am out of office today, so for any aligned(8) patch
> > >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > >>> even for 2.10.
> > >>
> > >> I fear the 2.10 train has already left the station, but any aligned(8)
> > >> patch should be cc:stable.
> > >
> > > I think this could be a topic for QEMU summit. Our process of not
> allowing
> > > fixes in rcx without requiring an rc(x+1) seems a bit odd. The Linux
> kernel
> > > style (there are fixes between the last rc and release) seems more
> balanced
> > > as long as we establish some safety nets.
> >
> > This sounds like a good idea to me, yes. And maybe we could also ease
> > the situation a little bit by providing the first stable .1 release
> > already two or three weeks after the .0 release [*] ? Then these "we are
> > not 100% sure whether this is a severe blocker or not" patches could
> > simply be provided to the public with that .1 release instead of
> > blocking the QEMU master branch in freeze state...
>
> I can give it a try for 2.10.1.
>
> At least initially, I would prefer to not have there be a set expectation
> that there will be a quick .1 release though, but rather anything tagged
> for-2.10, for instance, that doesn't make it in, will then fall into
> the "expedited 2.10.1" bucket, and for that to be the *only* way to get
> into that bucket (we'd still do the normal round-up and pull in any
> "Cc: qemu-stable@nongnu.org" patches at that point though).
>

Hi Michael,
To check if I got that correctly:
- a 2.10.1 could happen rather soon (like end of september as suggested) as
several things seem still up in the air?
- but to "get in" you'd still expect all changes that should be considered
to hit "Cc: qemu-stable@nongnu.org" and be bugfix only?

This would help ensure we don't steal focus from the .0 releases and allow
> those "is this a blocker?" discussions to happen in the proper context.
>
> >
> >  Thomas
> >
> >
> > [*] I know that means more additional work for Michael - sorry for that
> > ... but at least we should talk about this, I think. Maybe someone else
> > could also help with the releases if it's too much work for one person?
> >
>
>
>

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

[Michael Roth]

On 09/01/2017 02:06 AM, Christian Ehrhardt wrote:
> On Thu, Aug 31, 2017 at 7:44 PM, Michael Roth
> <mdroth@linux.vnet.ibm.com>
> wrote:
> 
>> Quoting Thomas Huth (2017-08-29 04:35:22)
>>> On 28.08.2017 09:18, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 08/25/2017 10:29 AM, Cornelia Huck wrote:
>>>>> On Fri, 25 Aug 2017 10:21:58 +0200
>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>>
>>>>>> On 08/25/2017 09:20 AM, Cornelia Huck wrote:
>>>>>
>>>>>>> OK, to recap:
>>>>>>>
>>>>>>> - the current pre-built bios seems fine
>>>>>>> - rebuilding the bios may yield a version that fails on some
>>>>>>> systems
>>>>>>>    (different compiler?)
>>>>>>> - adding aligned(8) looks like the right thing to do
>>>>>>> - it seems to fix the problem, but on at least one system
>>>>>>> something
>>>>>>>    still seems off (under investigation)
>>>>>>
>>>>>> Yes. I am out of office today, so for any aligned(8) patch
>>>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> even for 2.10.
>>>>>
>>>>> I fear the 2.10 train has already left the station, but any
>>>>> aligned(8)
>>>>> patch should be cc:stable.
>>>>
>>>> I think this could be a topic for QEMU summit. Our process of not
>> allowing
>>>> fixes in rcx without requiring an rc(x+1) seems a bit odd. The
>>>> Linux
>> kernel
>>>> style (there are fixes between the last rc and release) seems more
>> balanced
>>>> as long as we establish some safety nets.
>>>
>>> This sounds like a good idea to me, yes. And maybe we could also
>>> ease
>>> the situation a little bit by providing the first stable .1 release
>>> already two or three weeks after the .0 release [*] ? Then these "we
>>> are
>>> not 100% sure whether this is a severe blocker or not" patches could
>>> simply be provided to the public with that .1 release instead of
>>> blocking the QEMU master branch in freeze state...
>>
>> I can give it a try for 2.10.1.
>>
>> At least initially, I would prefer to not have there be a set
>> expectation
>> that there will be a quick .1 release though, but rather anything
>> tagged
>> for-2.10, for instance, that doesn't make it in, will then fall into
>> the "expedited 2.10.1" bucket, and for that to be the *only* way to
>> get
>> into that bucket (we'd still do the normal round-up and pull in any
>> "Cc: qemu-stable@nongnu.org" patches at that point though).
>>
> 
> Hi Michael,
> To check if I got that correctly:
> - a 2.10.1 could happen rather soon (like end of september as
> suggested) as
> several things seem still up in the air?

Correct.

> - but to "get in" you'd still expect all changes that should be
> considered
> to hit "Cc: qemu-stable@nongnu.org" and be bugfix only?

Well, that's always the case. What I'm suggesting is that for, say,
2.11, we shouldn't plan to have an early 2.11.1, but rather target
everything for 2.11.0 as normal and only do an early 2.11.1 if we
decide to defer anything originally targetted for 2.11.0. Otherwise
2.11.1 would happen toward the end of 2.12 development cycle like we
usually do.

> 
> This would help ensure we don't steal focus from the .0 releases and
> allow
>> those "is this a blocker?" discussions to happen in the proper
>> context.
>>
>>>
>>>   Thomas
>>>
>>>
>>> [*] I know that means more additional work for Michael - sorry for
>>> that
>>> ... but at least we should talk about this, I think. Maybe someone
>>> else
>>> could also help with the releases if it's too much work for one
>>> person?
>>>
>>
>>
>>
> 

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

[Philippe Mathieu-Daudé]

Hi Farhan,

On 08/25/2017 04:20 AM, Cornelia Huck wrote:
> On Thu, 24 Aug 2017 20:14:06 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 08/24/2017 07:38 PM, Farhan Ali wrote:
>>>
>>>
>>> On 08/24/2017 12:07 PM, Peter Maydell wrote:
>>>> On 24 August 2017 at 16:53, Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>
>>>>> On 08/24/2017 11:50 AM, Thomas Huth wrote:
>>>>>> True, so that could still be an issue. Looking at the cio.h in the
>>>>>> kernel, they define the struct like this:
>>>>>>
>>>>>> struct ccw1 {
>>>>>>          __u8  cmd_code;
>>>>>>          __u8  flags;
>>>>>>          __u16 count;
>>>>>>          __u32 cda;
>>>>>> } __attribute__ ((packed,aligned(8)));
>>>>>>
>>>>>> So I guess adding the aligned(8) is the right way to go?
>>>>   
>>>>> This was my initial fix and it works on my system. But for some reason this
>>>>> fix does not work on my colleague's system. So I am hesitant about
>>>>> submitting this fix
>>>>
>>>> It seems like it ought to be the obvious fix, so I would double
>>>> check that on your colleague's system the change really did
>>>> get recompiled and it's actually using the new version (that
>>>> sort of mistake can be easy to make and very confusing...)
>>
>>>>
>>>> thanks
>>>> -- PMM
>>>>   
>>>
>>> So after trying again with the fix, it seems to work on my colleague's system for most cases. It fails for LDL DASD boot case.... we are still investigating it.

no-packed-stack is GCC default but you can test forcing it with:

   make QEMU_CFLAGS=-mno-packed-stack

Also, can you add LDFLAGS+=-Wl,-verbose in pc-bios/s390-ccw/Makefile and 
send your and your colleague bios build output?

I'm interested in comparing the internal linker scripts.

>>
>> So chances are that this is an independent problem, I guess?
>>
> 
> OK, to recap:
> 
> - the current pre-built bios seems fine
> - rebuilding the bios may yield a version that fails on some systems
>    (different compiler?)
> - adding aligned(8) looks like the right thing to do
> - it seems to fix the problem, but on at least one system something
>    still seems off (under investigation)
> 

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

[Halil Pasic]

On 08/24/2017 05:13 PM, Cornelia Huck wrote:
> On Thu, 24 Aug 2017 11:05:08 -0400
> Farhan Ali <alifm@linux.vnet.ibm.com> wrote:
> 
>> Hi,
>>
>> There is an issue in QEMU bios which is exposed by commit
>>
>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>> 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 <pasic@linux.vnet.ibm.com>
>>      Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>      Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>      Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>
>>
>> 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.

Same here, otherwise I would have mane it a series and posted a fix
first.

> Do we need a
> dynamic allocation to guarantee alignment?
> 

I think a dynamic allocation is one way to get around the
problem but it should be possible to keep the stuff on the
stack. Telling the compiler about the alignment requirement
should do the trick IMHO.

@Farhan:
Since it's your find you have precedence when it comes to
fixing. Are you willing to submit a fix?

Halil

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

[Christian Borntraeger]

Just to understand the urgency. Is the problem happening with the BIOS that is shipped in the
upstream git tree, or a self-built one?


On 08/24/2017 05:05 PM, Farhan Ali wrote:
> Hi,
> 
> There is an issue in QEMU bios which is exposed by commit
> 
> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
> 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 <pasic@linux.vnet.ibm.com>
>     Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>     Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>     Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> 
> 
> 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.
> 
> 
> Thanks
> Farhan
> 

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

[Farhan Ali]

On 08/24/2017 02:33 PM, Christian Borntraeger wrote:
> Just to understand the urgency. Is the problem happening with the BIOS that is shipped in the
> upstream git tree, or a self-built one?
>

The bios image upstream works fine, but if someone tries to recompile 
the bios and use that, it breaks.


>
> On 08/24/2017 05:05 PM, Farhan Ali wrote:
>> Hi,
>>
>> There is an issue in QEMU bios which is exposed by commit
>>
>> commit 198c0d1f9df8c429502cb744fc26b6ba6e71db74
>> Author: Halil Pasic <pasic@linux.vnet.ibm.com>
>> 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 <pasic@linux.vnet.ibm.com>
>>     Message-Id: <20170727154842.23427-1-pasic@linux.vnet.ibm.com>
>>     Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>     Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>
>>
>> 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.
>>
>>
>> Thanks
>> Farhan
>>