[PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Huang, Ying]

This patch add a field of 64-bit physical pointer to NULL terminated
single linked list of struct setup_data to real-mode kernel
header. This is used as a more extensible boot parameters passing
mechanism.

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 arch/x86/boot/header.S      |    6 +++++-
 arch/x86/kernel/head64.c    |   20 ++++++++++++++++++++
 arch/x86/kernel/setup_64.c  |   22 ++++++++++++++++++++++
 include/asm-x86/bootparam.h |   14 ++++++++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)

--- a/include/asm-x86/bootparam.h
+++ b/include/asm-x86/bootparam.h
@@ -9,6 +9,17 @@
 #include <asm/ist.h>
 #include <video/edid.h>
 
+/* setup data types */
+#define SETUP_NONE			0
+
+/* extensible setup data list node */
+struct setup_data {
+	u64 next;
+	u32 type;
+	u32 len;
+	u8 data[0];
+};
+
 struct setup_header {
 	__u8	setup_sects;
 	__u16	root_flags;
@@ -46,6 +57,9 @@ struct setup_header {
 	__u32	cmdline_size;
 	__u32	hardware_subarch;
 	__u64	hardware_subarch_data;
+	__u32	payload_offset;
+	__u32	payload_length;
+	__u64	setup_data;
 } __attribute__((packed));
 
 struct sys_desc_table {
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -120,7 +120,7 @@ _start:
 	# Part 2 of the header, from the old setup.S
 
 		.ascii	"HdrS"		# header signature
-		.word	0x0208		# header version number (>= 0x0105)
+		.word	0x0209		# header version number (>= 0x0105)
 					# or else old loadlin-1.5 will fail)
 		.globl realmode_swtch
 realmode_swtch:	.word	0, 0		# default_switch, SETUPSEG
@@ -227,6 +227,10 @@ hardware_subarch_data:	.quad 0
 payload_offset:		.long input_data
 payload_length:		.long input_data_end-input_data
 
+setup_data:		.quad 0			# 64-bit physical pointer to
+						# single linked list of
+						# struct setup_data
+
 # End of setup header #####################################################
 
 	.section ".inittext", "ax"
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -265,6 +265,26 @@ void __attribute__((weak)) __init memory
        machine_specific_memory_setup();
 }
 
+static void __init parse_setup_data(void)
+{
+	struct setup_data *data;
+	unsigned long pa_data;
+
+	if (boot_params.hdr.version < 0x0209)
+		return;
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, PAGE_SIZE);
+		switch (data->type) {
+		default:
+			break;
+		}
+		free_early(pa_data, pa_data+sizeof(*data)+data->len);
+		pa_data = data->next;
+		early_iounmap(data, PAGE_SIZE);
+	}
+}
+
 /*
  * setup_arch - architecture-specific boot-time initializations
  *
@@ -317,6 +337,8 @@ void __init setup_arch(char **cmdline_p)
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
+	parse_setup_data();
+
 	parse_early_param();
 
 #ifdef CONFIG_PROVIDE_OHCI1394_DMA_INIT
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -11,6 +11,7 @@
 #include <linux/string.h>
 #include <linux/percpu.h>
 #include <linux/start_kernel.h>
+#include <linux/io.h>
 
 #include <asm/processor.h>
 #include <asm/proto.h>
@@ -101,6 +102,24 @@ static void __init reserve_ebda_region(v
 	reserve_early(lowmem, 0x100000, "BIOS reserved");
 }
 
+static void __init reserve_setup_data(void)
+{
+	struct setup_data *data;
+	unsigned long pa_data;
+	char buf[32];
+
+	if (boot_params.hdr.version < 0x0209)
+		return;
+	pa_data = boot_params.hdr.setup_data;
+	while (pa_data) {
+		data = early_ioremap(pa_data, sizeof(*data));
+		sprintf(buf, "setup data %x", data->type);
+		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
+		pa_data = data->next;
+		early_iounmap(data, sizeof(*data));
+	}
+}
+
 void __init x86_64_start_kernel(char * real_mode_data)
 {
 	int i;
@@ -157,6 +176,7 @@ void __init x86_64_start_kernel(char * r
 #endif
 
 	reserve_ebda_region();
+	reserve_setup_data();
 
 	/*
 	 * At this point everything still needed from the boot loader

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Paul Jackson]

Huang wrote:
> +	__u32	payload_offset;
> +	__u32	payload_length;

This seems to be the descriptor for an incoming compressed
kernel image, right?  What is it doing in this patchset?

In your PATCH 4/4 to Documentation/i386/boot.txt, I can see
that there already existed documentation of these two payload
fields:

 0248/4	2.08+	payload_offset	Offset of kernel payload
 024C/4	2.08+	payload_length	Length of kernel payload

However, that documentation is just a useless rephrasing of the
variable names, with the added word "kernel".  Is this a compressed
kernel image?  If so, could that documentation be improved, perhaps as:

 0248/4	2.08+	payload_offset	Offset of compressed kernel payload
 024C/4	2.08+	payload_length	Length of compressed kernel payload

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Paul Jackson]

Huang wrote:
> +/* setup data types */
> +#define SETUP_NONE			0

This define seems unused?

Actually, what use would it ever have?  Should not every
struct setup_data on the setup_data linked list have a
valid (not NONE) type?  And perhaps that switch statement
that confused me:

> +		switch (data->type) {
> +		default:
> +			break;
> +		}

should not "break" silently on an unrecognized data->type, but
rather complain bitterly?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Yinghai Lu]

On Thu, Mar 27, 2008 at 7:49 PM, Huang, Ying <ying.huang@intel.com> wrote:
> This patch add a field of 64-bit physical pointer to NULL terminated
>  single linked list of struct setup_data to real-mode kernel
>  header. This is used as a more extensible boot parameters passing
>  mechanism.
>
>  Signed-off-by: Huang Ying <ying.huang@intel.com>
>
>  ---
>   arch/x86/boot/header.S      |    6 +++++-
>   arch/x86/kernel/head64.c    |   20 ++++++++++++++++++++
>   arch/x86/kernel/setup_64.c  |   22 ++++++++++++++++++++++
>   include/asm-x86/bootparam.h |   14 ++++++++++++++
>   4 files changed, 61 insertions(+), 1 deletion(-)
>
...
>  +static void __init reserve_setup_data(void)
>  +{
>  +       struct setup_data *data;
>  +       unsigned long pa_data;
>  +       char buf[32];
>  +
>  +       if (boot_params.hdr.version < 0x0209)
>  +               return;
>  +       pa_data = boot_params.hdr.setup_data;
>  +       while (pa_data) {
>  +               data = early_ioremap(pa_data, sizeof(*data));
>  +               sprintf(buf, "setup data %x", data->type);
>  +               reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
>  +               pa_data = data->next;
>  +               early_iounmap(data, sizeof(*data));
>  +       }

current reserve_early will have limitation with 20 entries.

/*
 * Early reserved memory areas.
 */

#define MAX_EARLY_RES 20

or we need to make reserve_early a little bit smart, so it could merge
continuous region...
and then your free_early need to be updated too.

YH

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Huang, Ying]

On Sun, 2008-03-30 at 01:15 -0700, Yinghai Lu wrote:
[...]
> current reserve_early will have limitation with 20 entries.
> 
> /*
>  * Early reserved memory areas.
>  */
> 
> #define MAX_EARLY_RES 20
> 
> or we need to make reserve_early a little bit smart, so it could merge
> continuous region...
> and then your free_early need to be updated too.

It is planned that the reserve_early code be replaced by e820 based
memory reservation code. So I think we can stay with current
implementation and wait for the new infrastructure.

Best Regards,
Huang Ying

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Huang, Ying]

On Sat, 2008-03-29 at 23:31 -0500, Paul Jackson wrote:
> Huang wrote:
> > +/* setup data types */
> > +#define SETUP_NONE			0
> 
> This define seems unused?

I think this can prevent somebody to use 0 as type id.

> Actually, what use would it ever have?  Should not every
> struct setup_data on the setup_data linked list have a
> valid (not NONE) type?  And perhaps that switch statement
> that confused me:
> 
> > +		switch (data->type) {
> > +		default:
> > +			break;
> > +		}
> 
> should not "break" silently on an unrecognized data->type, but
> rather complain bitterly?

Yes. A warning should be made on unrecognized data->type.

Best Regards,
Huang Ying

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Paul Jackson]

Huang wrote:
> On Sat, 2008-03-29 at 23:31 -0500, Paul Jackson wrote:
> > Huang wrote:
> > > +/* setup data types */
> > > +#define SETUP_NONE			0
> > 
> > This define seems unused?
> 
> I think this can prevent somebody to use 0 as type id.

Why is it a good idea to prevent a 0 type id?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[H. Peter Anvin]

Paul Jackson wrote:
> Huang wrote:
>> On Sat, 2008-03-29 at 23:31 -0500, Paul Jackson wrote:
>>> Huang wrote:
>>>> +/* setup data types */
>>>> +#define SETUP_NONE			0
>>> This define seems unused?
>> I think this can prevent somebody to use 0 as type id.
> 
> Why is it a good idea to prevent a 0 type id?
> 

It is usually handy to have an ID to use as ignore, or not present, and 
for that to be zero.

	-hpa

Re: [PATCH 2/4] x86_64 boot -v2: Add linked list of struct setup_data

[Paul Jackson]

hpa wrote:
> It is usually handy to have an ID to use as ignore, or not present, and 
> for that to be zero.

Sometimes handy - if needed for specific reasons.

But many enumerations (enum or #define constants) in
the kernel have no such NONE/ignore/not-present value.

What's the use here?  I see no use for it.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214