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