Skip to contentSkip to conversation

x86_64 boot -v2: Add linked list of struct setup_data

PATCH In review Huang, Ying 4 participants 9 messages Mar 28, 2008 – Apr 2, 2008 x86

lore
Share & tools
lore

Huang, Ying

author
#0 lore raw

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

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 <[email protected]>
---
 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(-)
include/asm-x86/bootparam.h +14 -0
@@ -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 {
arch/x86/boot/header.S +5 -1
@@ -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"
arch/x86/kernel/setup_64.c +22 -0
@@ -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
arch/x86/kernel/head64.c +20 -0
@@ -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
#1 lore raw

include/asm-x86/bootparam.h struct setup_header { · line 61

Quoted 2 lines · Huang
include/asm-x86/bootparam.h +2 -0
+	__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

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
#2 lore raw
In reply to Huang, Ying

include/asm-x86/bootparam.h line 13

Quoted 2 lines · Huang
include/asm-x86/bootparam.h +2 -0
+/* 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:

arch/x86/kernel/setup_64.c void __attribute__((weak)) __init memory · line 281

Quoted 4 lines
arch/x86/kernel/setup_64.c +4 -0
+		switch (data->type) {
+		default:
+			break;
+		}

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

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
author
#3 lore raw
Quoted 5 lines · Paul Jackson

include/asm-x86/bootparam.h line 13

Quoted 2 lines · Huang
include/asm-x86/bootparam.h +2 -0
+/* setup data types */
+#define SETUP_NONE			0

This define seems unused?

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

Quoted 5 lines

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:

Quoted 7 lines

arch/x86/kernel/setup_64.c void __attribute__((weak)) __init memory · line 281

Quoted 4 lines
arch/x86/kernel/setup_64.c +4 -0
+		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

#4 lore raw
Quoted 8 lines · Huang
Quoted 5 lines · Paul Jackson

include/asm-x86/bootparam.h line 13

Quoted 2 lines · Huang
include/asm-x86/bootparam.h +2 -0
+/* 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?

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
reviewer
#5 lore raw
Quoted 10 lines · Paul Jackson
Quoted 6 lines · Huang
Quoted 4 lines · Paul Jackson

include/asm-x86/bootparam.h line 13

Quoted 2 lines · Huang
include/asm-x86/bootparam.h +2 -0
+/* 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

#6 lore raw
Quoted 2 lines · hpa

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.

Signature · 3 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214
#7 lore raw
In reply to Huang, Ying
Quoted 14 lines · 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 <[email protected]>

---

  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(-)

...

Quoted 15 lines

+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));
Quoted 1 line

+ }

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

author
#8 lore raw

On Sun, 2008-03-30 at 01:15 -0700, Yinghai Lu wrote:
[...]

Quoted 11 lines

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