Re: Kexec command line length

From: Vivek Goyal <vgoyal@redhat.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: Neil Horman <nhorman@redhat.com>,
	kexec@lists.infradead.org,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: Kexec command line length
Date: Mon, 28 Jan 2008 14:37:41 -0500	[thread overview]
Message-ID: <20080128193741.GB32765@redhat.com> (raw)
In-Reply-To: <20080128170811.GA1174@hmsreliant.think-freely.org>

On Mon, Jan 28, 2008 at 12:08:11PM -0500, Neil Horman wrote:
> Patch to clean up kexec-tools command line encoding.  It does four things:
> 
> 1) Move the command line out of the zero page, as per Viveks suggestion.  New
> padding scheme places the command line starting at 4096 bytes
> 
> 2) Increase command line length to support maximum size of 2048 bytes
> 
> 3) Pull in new variables from the latest kernels struct setup_header
> 
> 4) Where appropriate (currently only in bzImage_load) check the cmdline_size in
> setup header to ensure that cmdline_size isn't being violated
> 
> Tested by me, with successful results.
> 
> Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 

Hi Neil,

Patch looks good. Some minor nits follow.

> 
>  include/x86/x86-linux.h         |   19 +++++++++++++------
>  kexec/arch/i386/kexec-bzImage.c |    7 +++++++
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/include/x86/x86-linux.h b/include/x86/x86-linux.h
> index afe66bd..0794fe0 100644
> --- a/include/x86/x86-linux.h
> +++ b/include/x86/x86-linux.h
> @@ -144,18 +144,22 @@ struct x86_linux_param_header {
>  	/* 2.04+ */
>  	uint32_t kernel_alignment;		/* 0x230 */
>  	uint8_t  relocatable_kernel;		/* 0x234 */
> -	uint8_t  reserved15[0x2d0 - 0x235];	/* 0x230 */
> +	uint8_t  reserved15[3];			/* 0x237 */
> +	uint32_t cmdline_size;			/* 0x23B */
> +	uint32_t hardware_subarch;		/* 0x23F */
> +	uint64_t hardware_subarch_data;		/* 0x247 */

In general convetion for /* xxx */ seems to be that xxx represents the
offset where that data structure starts. We might want to maintain that.

> +	uint8_t  reserved16[0x2d0 - 0x248];	/* 0x248 */
>  #endif
>  	struct e820entry e820_map[E820MAX];	/* 0x2d0 */
>  						/* 0x550 */
> -#define COMMAND_LINE_SIZE 256
> +#define COMMAND_LINE_SIZE 2048 
>  };
>  
>  struct x86_linux_faked_param_header {
>  	struct x86_linux_param_header hdr;	/* 0x00 */
> -	uint8_t reserved16[688];		/* 0x550 */
> -	uint8_t command_line[COMMAND_LINE_SIZE]; /* 0x800 */
> -	uint8_t reserved17[1792];		/* 0x900 - 0x1000 */
> +	uint8_t reserved16[0xab0];		/* 0x550 */

reserved16 is now already used up in x86_linux_param_header. We might
want to bump up the reservation number here.

Ideally I would have liked to put all the 4K page in
x86_linux_param_header and replace that definition with struct bootparam.
But I think thats' fine for the time being. We might want to do that in 
future as reading and mapping the code to kernel boot protocol becomes
easy.

Thanks
Vivek

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