[Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Thomas Huth]

I've run into a compilation error today with the current version of GCC 8:

In file included from s390-ccw.h:49,
                 from main.c:12:
cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
 } __attribute__ ((packed));
 ^
cc1: all warnings being treated as errors

Since the struct tpi_info contains an element ("struct subchannel_id schid")
which is marked as aligned(4), we've got to mark the struct tpi_info as
aligned(4), too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/cio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index 55eaeee..1a0795f 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -125,7 +125,7 @@ struct tpi_info {
     __u32 reserved3  : 12;
     __u32 int_type   : 3;
     __u32 reserved4  : 12;
-} __attribute__ ((packed));
+} __attribute__ ((packed, aligned(4)));
 
 /* channel command word (type 1) */
 struct ccw1 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Cornelia Huck]

On Tue,  8 May 2018 12:17:52 +0200
Thomas Huth <thuth@redhat.com> wrote:

> I've run into a compilation error today with the current version of GCC 8:
> 
> In file included from s390-ccw.h:49,
>                  from main.c:12:
> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>  } __attribute__ ((packed));
>  ^
> cc1: all warnings being treated as errors
> 
> Since the struct tpi_info contains an element ("struct subchannel_id schid")
> which is marked as aligned(4), we've got to mark the struct tpi_info as
> aligned(4), too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index 55eaeee..1a0795f 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -125,7 +125,7 @@ struct tpi_info {
>      __u32 reserved3  : 12;
>      __u32 int_type   : 3;
>      __u32 reserved4  : 12;
> -} __attribute__ ((packed));
> +} __attribute__ ((packed, aligned(4)));
>  
>  /* channel command word (type 1) */
>  struct ccw1 {

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Alternatively, we could also ditch this struct and the tpi function...
but I have not given up hope yet that we might someday handle channel
I/O more canonically in the bios :)

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Thomas Huth]

On 08.05.2018 12:44, Cornelia Huck wrote:
> On Tue,  8 May 2018 12:17:52 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> I've run into a compilation error today with the current version of GCC 8:
>>
>> In file included from s390-ccw.h:49,
>>                  from main.c:12:
>> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>>  } __attribute__ ((packed));
>>  ^
>> cc1: all warnings being treated as errors
>>
>> Since the struct tpi_info contains an element ("struct subchannel_id schid")
>> which is marked as aligned(4), we've got to mark the struct tpi_info as
>> aligned(4), too.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/cio.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
>> index 55eaeee..1a0795f 100644
>> --- a/pc-bios/s390-ccw/cio.h
>> +++ b/pc-bios/s390-ccw/cio.h
>> @@ -125,7 +125,7 @@ struct tpi_info {
>>      __u32 reserved3  : 12;
>>      __u32 int_type   : 3;
>>      __u32 reserved4  : 12;
>> -} __attribute__ ((packed));
>> +} __attribute__ ((packed, aligned(4)));
>>  
>>  /* channel command word (type 1) */
>>  struct ccw1 {
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Alternatively, we could also ditch this struct and the tpi function...
> but I have not given up hope yet that we might someday handle channel
> I/O more canonically in the bios :)

Yes, it's currently unused (so I think you could also pick up the patch
directly, without the need for recompiling the s390-ccw.img just because
of this) ... I don't mind too much if we fix it or remove it, but since
you've said that it might be useful in the future again, I think we can
simply keep it for now.

 Thomas

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Cornelia Huck]

On Tue, 8 May 2018 12:49:59 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 08.05.2018 12:44, Cornelia Huck wrote:
> > On Tue,  8 May 2018 12:17:52 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> I've run into a compilation error today with the current version of GCC 8:
> >>
> >> In file included from s390-ccw.h:49,
> >>                  from main.c:12:
> >> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
> >>  } __attribute__ ((packed));
> >>  ^
> >> cc1: all warnings being treated as errors
> >>
> >> Since the struct tpi_info contains an element ("struct subchannel_id schid")
> >> which is marked as aligned(4), we've got to mark the struct tpi_info as
> >> aligned(4), too.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  pc-bios/s390-ccw/cio.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> >> index 55eaeee..1a0795f 100644
> >> --- a/pc-bios/s390-ccw/cio.h
> >> +++ b/pc-bios/s390-ccw/cio.h
> >> @@ -125,7 +125,7 @@ struct tpi_info {
> >>      __u32 reserved3  : 12;
> >>      __u32 int_type   : 3;
> >>      __u32 reserved4  : 12;
> >> -} __attribute__ ((packed));
> >> +} __attribute__ ((packed, aligned(4)));
> >>  
> >>  /* channel command word (type 1) */
> >>  struct ccw1 {  
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > 
> > Alternatively, we could also ditch this struct and the tpi function...
> > but I have not given up hope yet that we might someday handle channel
> > I/O more canonically in the bios :)  
> 
> Yes, it's currently unused (so I think you could also pick up the patch
> directly, without the need for recompiling the s390-ccw.img just because
> of this) ... I don't mind too much if we fix it or remove it, but since
> you've said that it might be useful in the future again, I think we can
> simply keep it for now.

Related question: Should this be cc:stable (without a rebuild)? The
same logic as for the just-merged e500 patch probably applies.

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Christian Borntraeger]

On 05/08/2018 01:06 PM, Cornelia Huck wrote:
> On Tue, 8 May 2018 12:49:59 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 08.05.2018 12:44, Cornelia Huck wrote:
>>> On Tue,  8 May 2018 12:17:52 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> I've run into a compilation error today with the current version of GCC 8:
>>>>
>>>> In file included from s390-ccw.h:49,
>>>>                  from main.c:12:
>>>> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>>>>  } __attribute__ ((packed));
>>>>  ^
>>>> cc1: all warnings being treated as errors
>>>>
>>>> Since the struct tpi_info contains an element ("struct subchannel_id schid")
>>>> which is marked as aligned(4), we've got to mark the struct tpi_info as
>>>> aligned(4), too.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  pc-bios/s390-ccw/cio.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
>>>> index 55eaeee..1a0795f 100644
>>>> --- a/pc-bios/s390-ccw/cio.h
>>>> +++ b/pc-bios/s390-ccw/cio.h
>>>> @@ -125,7 +125,7 @@ struct tpi_info {
>>>>      __u32 reserved3  : 12;
>>>>      __u32 int_type   : 3;
>>>>      __u32 reserved4  : 12;
>>>> -} __attribute__ ((packed));
>>>> +} __attribute__ ((packed, aligned(4)));
>>>>  
>>>>  /* channel command word (type 1) */
>>>>  struct ccw1 {  
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>> Alternatively, we could also ditch this struct and the tpi function...
>>> but I have not given up hope yet that we might someday handle channel
>>> I/O more canonically in the bios :)  
>>
>> Yes, it's currently unused (so I think you could also pick up the patch
>> directly, without the need for recompiling the s390-ccw.img just because
>> of this) ... I don't mind too much if we fix it or remove it, but since
>> you've said that it might be useful in the future again, I think we can
>> simply keep it for now.
> 
> Related question: Should this be cc:stable (without a rebuild)? The
> same logic as for the just-merged e500 patch probably applies.

Ack. Compile warnings/errors qualify for stable I think.

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Thomas Huth]

On 08.05.2018 13:06, Cornelia Huck wrote:
> On Tue, 8 May 2018 12:49:59 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 08.05.2018 12:44, Cornelia Huck wrote:
>>> On Tue,  8 May 2018 12:17:52 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> I've run into a compilation error today with the current version of GCC 8:
>>>>
>>>> In file included from s390-ccw.h:49,
>>>>                  from main.c:12:
>>>> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>>>>  } __attribute__ ((packed));
>>>>  ^
>>>> cc1: all warnings being treated as errors
>>>>
>>>> Since the struct tpi_info contains an element ("struct subchannel_id schid")
>>>> which is marked as aligned(4), we've got to mark the struct tpi_info as
>>>> aligned(4), too.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  pc-bios/s390-ccw/cio.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
>>>> index 55eaeee..1a0795f 100644
>>>> --- a/pc-bios/s390-ccw/cio.h
>>>> +++ b/pc-bios/s390-ccw/cio.h
>>>> @@ -125,7 +125,7 @@ struct tpi_info {
>>>>      __u32 reserved3  : 12;
>>>>      __u32 int_type   : 3;
>>>>      __u32 reserved4  : 12;
>>>> -} __attribute__ ((packed));
>>>> +} __attribute__ ((packed, aligned(4)));
>>>>  
>>>>  /* channel command word (type 1) */
>>>>  struct ccw1 {  
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>> Alternatively, we could also ditch this struct and the tpi function...
>>> but I have not given up hope yet that we might someday handle channel
>>> I/O more canonically in the bios :)  
>>
>> Yes, it's currently unused (so I think you could also pick up the patch
>> directly, without the need for recompiling the s390-ccw.img just because
>> of this) ... I don't mind too much if we fix it or remove it, but since
>> you've said that it might be useful in the future again, I think we can
>> simply keep it for now.
> 
> Related question: Should this be cc:stable (without a rebuild)? The
> same logic as for the just-merged e500 patch probably applies.

Since the stable releases are normally not built with -Werror, I think
it's ok to not include it there. OTOH it also does not hurt and silences
an annoying warning, and in case somebody tries to build a stable
release with -Werror, it also fixes the build there. So why not, please
add a "Cc: stable" when you pick up the patch.

 Thomas

Re: [Qemu-devel] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[Cornelia Huck]

On Tue,  8 May 2018 12:17:52 +0200
Thomas Huth <thuth@redhat.com> wrote:

> I've run into a compilation error today with the current version of GCC 8:
> 
> In file included from s390-ccw.h:49,
>                  from main.c:12:
> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>  } __attribute__ ((packed));
>  ^
> cc1: all warnings being treated as errors
> 
> Since the struct tpi_info contains an element ("struct subchannel_id schid")
> which is marked as aligned(4), we've got to mark the struct tpi_info as
> aligned(4), too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index 55eaeee..1a0795f 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -125,7 +125,7 @@ struct tpi_info {
>      __u32 reserved3  : 12;
>      __u32 int_type   : 3;
>      __u32 reserved4  : 12;
> -} __attribute__ ((packed));
> +} __attribute__ ((packed, aligned(4)));
>  
>  /* channel command word (type 1) */
>  struct ccw1 {

Thanks, queued to s390-next with a cc:stable.

Re: [Qemu-devel] [qemu-s390x] [PATCH] pc-bios/s390-ccw: struct tpi_info must be declared as aligned(4)

[David Hildenbrand]

On 08.05.2018 12:17, Thomas Huth wrote:
> I've run into a compilation error today with the current version of GCC 8:
> 
> In file included from s390-ccw.h:49,
>                  from main.c:12:
> cio.h:128:1: error: alignment 1 of 'struct tpi_info' is less than 4 [-Werror=packed-not-aligned]
>  } __attribute__ ((packed));
>  ^
> cc1: all warnings being treated as errors
> 
> Since the struct tpi_info contains an element ("struct subchannel_id schid")
> which is marked as aligned(4), we've got to mark the struct tpi_info as
> aligned(4), too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index 55eaeee..1a0795f 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -125,7 +125,7 @@ struct tpi_info {
>      __u32 reserved3  : 12;
>      __u32 int_type   : 3;
>      __u32 reserved4  : 12;
> -} __attribute__ ((packed));
> +} __attribute__ ((packed, aligned(4)));
>  
>  /* channel command word (type 1) */
>  struct ccw1 {
> 

Late but still
Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb