[PATCH] kexec: x86: struct x86_linux_param_header should be packed

[PATCH] kexec: x86: struct x86_linux_param_header should be packed

[Vivek Goyal]

I think struct x86_linux_param_header should be packed. Strange that we
did not do it so far. 

Without packing struct size was 3824 (decimal) on my x86_64 machine. With
packing it is 3820. I think there was a padding of 4 bytes at the end. So
it should be harmless.

I tried to introduce more fields and that introduced padding in the
middle of structure and kexec stopped working and that's how I got to
know that bootparam is not packed.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/x86/x86-linux.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: kexec-tools/include/x86/x86-linux.h
===================================================================
--- kexec-tools.orig/include/x86/x86-linux.h	2013-08-05 13:28:33.999338740 -0400
+++ kexec-tools/include/x86/x86-linux.h	2013-08-05 13:28:46.616475104 -0400
@@ -198,7 +198,7 @@ struct x86_linux_param_header {
 	struct 	edd_info eddbuf[EDDMAXNR];	/* 0xd00 */
 						/* 0xeec */
 #define COMMAND_LINE_SIZE 2048
-};
+} __attribute__ ((packed));
 
 struct x86_linux_faked_param_header {
 	struct x86_linux_param_header hdr;	/* 0x00 */

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec: x86: struct x86_linux_param_header should be packed

[WANG Chao]

On 08/05/13 at 01:35pm, Vivek Goyal wrote:
> I think struct x86_linux_param_header should be packed. Strange that we
> did not do it so far. 
> 
> Without packing struct size was 3824 (decimal) on my x86_64 machine. With
> packing it is 3820. I think there was a padding of 4 bytes at the end. So
> it should be harmless.
> 
> I tried to introduce more fields and that introduced padding in the
> middle of structure and kexec stopped working and that's how I got to
> know that bootparam is not packed.

In this case that's true and x86_linux_param_header should be packed.

One more thing is,
in include/x86/x86-linux.h, we already define PACKED macro:
 #define PACKED __attribute__((packed))
But within x86-linux.h, both PACKED_and __attribute__((packed)) are used.

PACKED isn't used much time and __attribute__((packed)) is quite simple
and straightforward. Maybe it's time we can remove the macro and use
__attribute__((packed)) directly.

I can send another patch to address this if anyone thinks it's a good
idea.

Thanks
WANG Chao

> ---
>  include/x86/x86-linux.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: kexec-tools/include/x86/x86-linux.h
> ===================================================================
> --- kexec-tools.orig/include/x86/x86-linux.h	2013-08-05 13:28:33.999338740 -0400
> +++ kexec-tools/include/x86/x86-linux.h	2013-08-05 13:28:46.616475104 -0400
> @@ -198,7 +198,7 @@ struct x86_linux_param_header {
>  	struct 	edd_info eddbuf[EDDMAXNR];	/* 0xd00 */
>  						/* 0xeec */
>  #define COMMAND_LINE_SIZE 2048
> -};
> +} __attribute__ ((packed));
>  
>  struct x86_linux_faked_param_header {
>  	struct x86_linux_param_header hdr;	/* 0x00 */
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec: x86: struct x86_linux_param_header should be packed

[Zhang Yanfei]

On 09/05/2013 04:48 PM, WANG Chao wrote:
> On 08/05/13 at 01:35pm, Vivek Goyal wrote:
>> I think struct x86_linux_param_header should be packed. Strange that we
>> did not do it so far. 
>>
>> Without packing struct size was 3824 (decimal) on my x86_64 machine. With
>> packing it is 3820. I think there was a padding of 4 bytes at the end. So
>> it should be harmless.
>>
>> I tried to introduce more fields and that introduced padding in the
>> middle of structure and kexec stopped working and that's how I got to
>> know that bootparam is not packed.
> 
> In this case that's true and x86_linux_param_header should be packed.
> 
> One more thing is,
> in include/x86/x86-linux.h, we already define PACKED macro:
>  #define PACKED __attribute__((packed))
> But within x86-linux.h, both PACKED_and __attribute__((packed)) are used.
> 
> PACKED isn't used much time and __attribute__((packed)) is quite simple
> and straightforward. Maybe it's time we can remove the macro and use
> __attribute__((packed)) directly.
> 
> I can send another patch to address this if anyone thinks it's a good
> idea.

Unifying this should be OK, I think.

> 
> Thanks
> WANG Chao
> 
>> ---
>>  include/x86/x86-linux.h |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: kexec-tools/include/x86/x86-linux.h
>> ===================================================================
>> --- kexec-tools.orig/include/x86/x86-linux.h	2013-08-05 13:28:33.999338740 -0400
>> +++ kexec-tools/include/x86/x86-linux.h	2013-08-05 13:28:46.616475104 -0400
>> @@ -198,7 +198,7 @@ struct x86_linux_param_header {
>>  	struct 	edd_info eddbuf[EDDMAXNR];	/* 0xd00 */
>>  						/* 0xeec */
>>  #define COMMAND_LINE_SIZE 2048
>> -};
>> +} __attribute__ ((packed));
>>  
>>  struct x86_linux_faked_param_header {
>>  	struct x86_linux_param_header hdr;	/* 0x00 */
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 


-- 
Thanks.
Zhang Yanfei

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] kexec: x86: struct x86_linux_param_header should be packed

[Vivek Goyal]

On Thu, Sep 05, 2013 at 04:48:20PM +0800, WANG Chao wrote:
> On 08/05/13 at 01:35pm, Vivek Goyal wrote:
> > I think struct x86_linux_param_header should be packed. Strange that we
> > did not do it so far. 
> > 
> > Without packing struct size was 3824 (decimal) on my x86_64 machine. With
> > packing it is 3820. I think there was a padding of 4 bytes at the end. So
> > it should be harmless.
> > 
> > I tried to introduce more fields and that introduced padding in the
> > middle of structure and kexec stopped working and that's how I got to
> > know that bootparam is not packed.
> 
> In this case that's true and x86_linux_param_header should be packed.
> 
> One more thing is,
> in include/x86/x86-linux.h, we already define PACKED macro:
>  #define PACKED __attribute__((packed))
> But within x86-linux.h, both PACKED_and __attribute__((packed)) are used.
> 
> PACKED isn't used much time and __attribute__((packed)) is quite simple
> and straightforward. Maybe it's time we can remove the macro and use
> __attribute__((packed)) directly.
> 
> I can send another patch to address this if anyone thinks it's a good
> idea.

I think there is really no need to use macro PACKED. So making the code
uniform does not hurt.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec