* [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 @ 2017-08-24 15:05 Farhan Ali 2017-08-24 15:13 ` Cornelia Huck 2017-08-24 18:33 ` Christian Borntraeger 0 siblings, 2 replies; 24+ messages in thread From: Farhan Ali @ 2017-08-24 15:05 UTC (permalink / raw) To: qemu-devel, Cornelia Huck Cc: Halil Pasic, Christian Borntraeger, Collin L. Walling 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:05 [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 Farhan Ali @ 2017-08-24 15:13 ` Cornelia Huck 2017-08-24 15:35 ` Thomas Huth 2017-08-24 15:37 ` Halil Pasic 2017-08-24 18:33 ` Christian Borntraeger 1 sibling, 2 replies; 24+ messages in thread From: Cornelia Huck @ 2017-08-24 15:13 UTC (permalink / raw) To: Farhan Ali Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Collin L. Walling, Thomas Huth, David Hildenbrand 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:13 ` Cornelia Huck @ 2017-08-24 15:35 ` Thomas Huth 2017-08-24 15:47 ` Halil Pasic 2017-08-24 15:37 ` Halil Pasic 1 sibling, 1 reply; 24+ messages in thread From: Thomas Huth @ 2017-08-24 15:35 UTC (permalink / raw) To: Cornelia Huck, Farhan Ali Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Collin L. Walling, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:35 ` Thomas Huth @ 2017-08-24 15:47 ` Halil Pasic 2017-08-24 15:50 ` Thomas Huth 0 siblings, 1 reply; 24+ messages in thread From: Halil Pasic @ 2017-08-24 15:47 UTC (permalink / raw) To: Thomas Huth, Cornelia Huck, Farhan Ali Cc: qemu-devel, Christian Borntraeger, Collin L. Walling, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:47 ` Halil Pasic @ 2017-08-24 15:50 ` Thomas Huth 2017-08-24 15:53 ` Farhan Ali 0 siblings, 1 reply; 24+ messages in thread From: Thomas Huth @ 2017-08-24 15:50 UTC (permalink / raw) To: Halil Pasic, Cornelia Huck, Farhan Ali Cc: qemu-devel, Christian Borntraeger, Collin L. Walling, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:50 ` Thomas Huth @ 2017-08-24 15:53 ` Farhan Ali 2017-08-24 16:02 ` Halil Pasic 2017-08-24 16:07 ` Peter Maydell 0 siblings, 2 replies; 24+ messages in thread From: Farhan Ali @ 2017-08-24 15:53 UTC (permalink / raw) To: Thomas Huth, Halil Pasic, Cornelia Huck Cc: qemu-devel, Christian Borntraeger, Collin L. Walling, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:53 ` Farhan Ali @ 2017-08-24 16:02 ` Halil Pasic 2017-08-24 18:15 ` Christian Borntraeger 2017-08-24 16:07 ` Peter Maydell 1 sibling, 1 reply; 24+ messages in thread From: Halil Pasic @ 2017-08-24 16:02 UTC (permalink / raw) To: Farhan Ali, Thomas Huth, Cornelia Huck Cc: Christian Borntraeger, Collin L. Walling, qemu-devel, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 16:02 ` Halil Pasic @ 2017-08-24 18:15 ` Christian Borntraeger 0 siblings, 0 replies; 24+ messages in thread From: Christian Borntraeger @ 2017-08-24 18:15 UTC (permalink / raw) To: Halil Pasic, Farhan Ali, Thomas Huth, Cornelia Huck Cc: Collin L. Walling, qemu-devel, David Hildenbrand 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:53 ` Farhan Ali 2017-08-24 16:02 ` Halil Pasic @ 2017-08-24 16:07 ` Peter Maydell 2017-08-24 17:38 ` Farhan Ali 1 sibling, 1 reply; 24+ messages in thread From: Peter Maydell @ 2017-08-24 16:07 UTC (permalink / raw) To: Farhan Ali Cc: Thomas Huth, Halil Pasic, Cornelia Huck, Christian Borntraeger, Collin L. Walling, QEMU Developers, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 16:07 ` Peter Maydell @ 2017-08-24 17:38 ` Farhan Ali 2017-08-24 18:14 ` Christian Borntraeger 0 siblings, 1 reply; 24+ messages in thread From: Farhan Ali @ 2017-08-24 17:38 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, Halil Pasic, QEMU Developers, Christian Borntraeger, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 17:38 ` Farhan Ali @ 2017-08-24 18:14 ` Christian Borntraeger 2017-08-25 7:20 ` Cornelia Huck 0 siblings, 1 reply; 24+ messages in thread From: Christian Borntraeger @ 2017-08-24 18:14 UTC (permalink / raw) To: Farhan Ali, Peter Maydell Cc: Thomas Huth, David Hildenbrand, Cornelia Huck, Halil Pasic, QEMU Developers, Collin L. Walling 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 18:14 ` Christian Borntraeger @ 2017-08-25 7:20 ` Cornelia Huck 2017-08-25 8:21 ` Christian Borntraeger 2017-08-25 14:38 ` Philippe Mathieu-Daudé 0 siblings, 2 replies; 24+ messages in thread From: Cornelia Huck @ 2017-08-25 7:20 UTC (permalink / raw) To: Christian Borntraeger Cc: Farhan Ali, Peter Maydell, Thomas Huth, David Hildenbrand, Halil Pasic, QEMU Developers, Collin L. Walling 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) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-25 7:20 ` Cornelia Huck @ 2017-08-25 8:21 ` Christian Borntraeger 2017-08-25 8:29 ` Cornelia Huck 2017-08-25 14:38 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 24+ messages in thread From: Christian Borntraeger @ 2017-08-25 8:21 UTC (permalink / raw) To: Cornelia Huck Cc: Farhan Ali, Peter Maydell, Thomas Huth, David Hildenbrand, Halil Pasic, QEMU Developers, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-25 8:21 ` Christian Borntraeger @ 2017-08-25 8:29 ` Cornelia Huck 2017-08-28 7:18 ` Christian Borntraeger 0 siblings, 1 reply; 24+ messages in thread From: Cornelia Huck @ 2017-08-25 8:29 UTC (permalink / raw) To: Christian Borntraeger Cc: Farhan Ali, Peter Maydell, Thomas Huth, David Hildenbrand, Halil Pasic, QEMU Developers, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-25 8:29 ` Cornelia Huck @ 2017-08-28 7:18 ` Christian Borntraeger 2017-08-29 9:35 ` Thomas Huth 0 siblings, 1 reply; 24+ messages in thread From: Christian Borntraeger @ 2017-08-28 7:18 UTC (permalink / raw) To: Cornelia Huck Cc: Farhan Ali, Peter Maydell, Thomas Huth, David Hildenbrand, Halil Pasic, QEMU Developers, Collin L. Walling 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-28 7:18 ` Christian Borntraeger @ 2017-08-29 9:35 ` Thomas Huth 2017-08-29 10:28 ` Christian Borntraeger 2017-08-31 17:44 ` Michael Roth 0 siblings, 2 replies; 24+ messages in thread From: Thomas Huth @ 2017-08-29 9:35 UTC (permalink / raw) To: Christian Borntraeger, Cornelia Huck Cc: Peter Maydell, David Hildenbrand, Farhan Ali, QEMU Developers, Halil Pasic, Collin L. Walling, Michael Roth 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-29 9:35 ` Thomas Huth @ 2017-08-29 10:28 ` Christian Borntraeger 2017-08-31 17:44 ` Michael Roth 1 sibling, 0 replies; 24+ messages in thread From: Christian Borntraeger @ 2017-08-29 10:28 UTC (permalink / raw) To: Thomas Huth, Cornelia Huck Cc: Peter Maydell, David Hildenbrand, Farhan Ali, QEMU Developers, Halil Pasic, Collin L. Walling, Michael Roth 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? > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-29 9:35 ` Thomas Huth 2017-08-29 10:28 ` Christian Borntraeger @ 2017-08-31 17:44 ` Michael Roth 2017-09-01 7:06 ` Christian Ehrhardt 1 sibling, 1 reply; 24+ messages in thread From: Michael Roth @ 2017-08-31 17:44 UTC (permalink / raw) To: Christian Borntraeger, Cornelia Huck, Thomas Huth Cc: Peter Maydell, David Hildenbrand, Farhan Ali, QEMU Developers, Halil Pasic, Collin L. Walling 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? > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-31 17:44 ` Michael Roth @ 2017-09-01 7:06 ` Christian Ehrhardt 2017-09-01 14:03 ` Michael Roth 0 siblings, 1 reply; 24+ messages in thread From: Christian Ehrhardt @ 2017-09-01 7:06 UTC (permalink / raw) To: Michael Roth Cc: Christian Borntraeger, Cornelia Huck, Thomas Huth, Peter Maydell, Halil Pasic, David Hildenbrand, Farhan Ali, QEMU Developers, Collin L. Walling 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? > > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-09-01 7:06 ` Christian Ehrhardt @ 2017-09-01 14:03 ` Michael Roth 0 siblings, 0 replies; 24+ messages in thread From: Michael Roth @ 2017-09-01 14:03 UTC (permalink / raw) To: Christian Ehrhardt Cc: Christian Borntraeger, Cornelia Huck, Thomas Huth, Peter Maydell, Halil Pasic, David Hildenbrand, Farhan Ali, QEMU Developers, Collin L. Walling 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? >>> >> >> >> > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-25 7:20 ` Cornelia Huck 2017-08-25 8:21 ` Christian Borntraeger @ 2017-08-25 14:38 ` Philippe Mathieu-Daudé 1 sibling, 0 replies; 24+ messages in thread From: Philippe Mathieu-Daudé @ 2017-08-25 14:38 UTC (permalink / raw) To: Cornelia Huck, Christian Borntraeger Cc: Peter Maydell, Thomas Huth, David Hildenbrand, Farhan Ali, QEMU Developers, Halil Pasic, Collin L. Walling 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) > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:13 ` Cornelia Huck 2017-08-24 15:35 ` Thomas Huth @ 2017-08-24 15:37 ` Halil Pasic 1 sibling, 0 replies; 24+ messages in thread From: Halil Pasic @ 2017-08-24 15:37 UTC (permalink / raw) To: Cornelia Huck, Farhan Ali Cc: Thomas Huth, David Hildenbrand, qemu-devel, Christian Borntraeger, Collin L. Walling 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 15:05 [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 Farhan Ali 2017-08-24 15:13 ` Cornelia Huck @ 2017-08-24 18:33 ` Christian Borntraeger 2017-08-24 19:56 ` Farhan Ali 1 sibling, 1 reply; 24+ messages in thread From: Christian Borntraeger @ 2017-08-24 18:33 UTC (permalink / raw) To: Farhan Ali, qemu-devel, Cornelia Huck; +Cc: Halil Pasic, Collin L. Walling 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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 2017-08-24 18:33 ` Christian Borntraeger @ 2017-08-24 19:56 ` Farhan Ali 0 siblings, 0 replies; 24+ messages in thread From: Farhan Ali @ 2017-08-24 19:56 UTC (permalink / raw) To: Christian Borntraeger, qemu-devel, Cornelia Huck Cc: Halil Pasic, Collin L. Walling 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 >> ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-09-01 14:03 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-24 15:05 [Qemu-devel] S390 bios breaks in qemu 2.10.rc3 Farhan Ali 2017-08-24 15:13 ` Cornelia Huck 2017-08-24 15:35 ` Thomas Huth 2017-08-24 15:47 ` Halil Pasic 2017-08-24 15:50 ` Thomas Huth 2017-08-24 15:53 ` Farhan Ali 2017-08-24 16:02 ` Halil Pasic 2017-08-24 18:15 ` Christian Borntraeger 2017-08-24 16:07 ` Peter Maydell 2017-08-24 17:38 ` Farhan Ali 2017-08-24 18:14 ` Christian Borntraeger 2017-08-25 7:20 ` Cornelia Huck 2017-08-25 8:21 ` Christian Borntraeger 2017-08-25 8:29 ` Cornelia Huck 2017-08-28 7:18 ` Christian Borntraeger 2017-08-29 9:35 ` Thomas Huth 2017-08-29 10:28 ` Christian Borntraeger 2017-08-31 17:44 ` Michael Roth 2017-09-01 7:06 ` Christian Ehrhardt 2017-09-01 14:03 ` Michael Roth 2017-08-25 14:38 ` Philippe Mathieu-Daudé 2017-08-24 15:37 ` Halil Pasic 2017-08-24 18:33 ` Christian Borntraeger 2017-08-24 19:56 ` Farhan Ali
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.