Date:   Fri, 12 Jul 2019 09:04:23 -0700
From:   hpa@...or.com
To:     Daniel Kiper <daniel.kiper@...cle.com>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, x86@...nel.org
CC:     bp@...en8.de, corbet@....net, dpsmith@...rtussolutions.com,
        eric.snowberg@...cle.com, kanth.ghatraju@...cle.com,
        konrad.wilk@...cle.com, mingo@...hat.com,
        ross.philipson@...cle.com, tglx@...utronix.de
Subject: Re: [PATCH v2 1/3] x86/boot: Introduce the kernel_info

On July 4, 2019 9:36:10 AM PDT, Daniel Kiper <daniel.kiper@...cle.com> wrote:
>The relationships between the headers are analogous to the various data
>sections:
>
>  setup_header = .data
>  boot_params/setup_data = .bss
>
>What is missing from the above list? That's right:
>
>  kernel_info = .rodata
>
>We have been (ab)using .data for things that could go into .rodata or
>.bss for
>a long time, for lack of alternatives and -- especially early on --
>inertia.
>Also, the BIOS stub is responsible for creating boot_params, so it
>isn't
>available to a BIOS-based loader (setup_data is, though).
>
>setup_header is permanently limited to 144 bytes due to the reach of
>the
>2-byte jump field, which doubles as a length field for the structure,
>combined
>with the size of the "hole" in struct boot_params that a protected-mode
>loader
>or the BIOS stub has to copy it into. It is currently 119 bytes long,
>which
>leaves us with 25 very precious bytes. This isn't something that can be
>fixed
>without revising the boot protocol entirely, breaking backwards
>compatibility.
>
>boot_params proper is limited to 4096 bytes, but can be arbitrarily
>extended
>by adding setup_data entries. It cannot be used to communicate
>properties of
>the kernel image, because it is .bss and has no image-provided content.
>
>kernel_info solves this by providing an extensible place for
>information about
>the kernel image. It is readonly, because the kernel cannot rely on a
>bootloader copying its contents anywhere, but that is OK; if it becomes
>necessary it can still contain data items that an enabled bootloader
>would be
>expected to copy into a setup_data chunk.
>
>This patch does not bump setup_header version in arch/x86/boot/header.S
>because it will be followed by additional changes coming into the
>Linux/x86 boot protocol.
>
>Suggested-by: H. Peter Anvin <hpa@...or.com>
>Signed-off-by: Daniel Kiper <daniel.kiper@...cle.com>
>Reviewed-by: Eric Snowberg <eric.snowberg@...cle.com>
>Reviewed-by: Ross Philipson <ross.philipson@...cle.com>
>---
>v2 - suggestions/fixes:
>   - rename setup_header2 to kernel_info,
>     (suggested by H. Peter Anvin),
>   - change kernel_info.header value to "InfO" (0x4f666e49),
>   - new kernel_info description in Documentation/x86/boot.rst,
>     (suggested by H. Peter Anvin),
>   - drop kernel_info_offset_update() as an overkill and
>     update kernel_info offset directly from main(),
>     (suggested by Eric Snowberg),
>   - new commit message
>     (suggested by H. Peter Anvin),
>   - fix some commit message misspellings
>     (suggested by Eric Snowberg).
>---
>Documentation/x86/boot.rst             | 89
>++++++++++++++++++++++++++++++++++
> arch/x86/boot/Makefile                 |  2 +-
> arch/x86/boot/compressed/Makefile      |  4 +-
> arch/x86/boot/compressed/kernel_info.S | 12 +++++
> arch/x86/boot/header.S                 |  1 +
> arch/x86/boot/tools/build.c            |  5 ++
> arch/x86/include/uapi/asm/bootparam.h  |  1 +
> 7 files changed, 111 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/boot/compressed/kernel_info.S
>
>diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>index 08a2f100c0e6..a934a56f0516 100644
>--- a/Documentation/x86/boot.rst
>+++ b/Documentation/x86/boot.rst
>@@ -68,8 +68,25 @@ Protocol 2.12	(Kernel 3.8) Added the xloadflags
>field and extension fields
> Protocol 2.13	(Kernel 3.14) Support 32- and 64-bit flags being set in
> 		xloadflags to support booting a 64-bit kernel from 32-bit
> 		EFI
>+
>+Protocol 2.14:	BURNT BY INCORRECT COMMIT
>ae7e1238e68f2a472a125673ab506d49158c1889
>+		(x86/boot: Add ACPI RSDP address to setup_header)
>+		DO NOT USE!!! ASSUME SAME AS 2.13.
>+
>+Protocol 2.15:	(Kernel 5.3) Added the kernel_info.
>=============	============================================================
> 
>+.. note::
>+     The protocol version number should be changed only if the setup
>header
>+     is changed. There is no need to update the version number if
>boot_params
>+     or kernel_info are changed. Additionally, it is recommended to
>use
>+     xloadflags (in this case the protocol version number should not
>be
>+     updated either) or kernel_info to communicate supported Linux
>kernel
>+     features to the boot loader. Due to very limited space available
>in
>+     the original setup header every update to it should be considered
>+     with great care. Starting from the protocol 2.15 the primary way
>to
>+     communicate things to the boot loader is the kernel_info.
>+
> 
> Memory Layout
> =============
>@@ -207,6 +224,7 @@ Offset/Size	Proto		Name			Meaning
> 0258/8		2.10+		pref_address		Preferred loading address
> 0260/4		2.10+		init_size		Linear memory required during initialization
> 0264/4		2.11+		handover_offset		Offset of handover entry point
>+0268/4		2.15+		kernel_info_offset	Offset of the kernel_info
>===========	========	=====================	============================================
> 
> .. note::
>@@ -855,6 +873,77 @@ Offset/size:	0x264/4
> 
>   See EFI HANDOVER PROTOCOL below for more details.
> 
>+============	==================
>+Field name:	kernel_info_offset
>+Type:		read
>+Offset/size:	0x268/4
>+Protocol:	2.15+
>+============	==================
>+
>+  This field is the offset from the beginning of the kernel image to
>the
>+  kernel_info. It is embedded in the Linux image in the uncompressed
>+  protected mode region.
>+
>+
>+The kernel_info
>+===============
>+
>+The relationships between the headers are analogous to the various
>data
>+sections:
>+
>+  setup_header = .data
>+  boot_params/setup_data = .bss
>+
>+What is missing from the above list? That's right:
>+
>+  kernel_info = .rodata
>+
>+We have been (ab)using .data for things that could go into .rodata or
>.bss for
>+a long time, for lack of alternatives and -- especially early on --
>inertia.
>+Also, the BIOS stub is responsible for creating boot_params, so it
>isn't
>+available to a BIOS-based loader (setup_data is, though).
>+
>+setup_header is permanently limited to 144 bytes due to the reach of
>the
>+2-byte jump field, which doubles as a length field for the structure,
>combined
>+with the size of the "hole" in struct boot_params that a
>protected-mode loader
>+or the BIOS stub has to copy it into. It is currently 119 bytes long,
>which
>+leaves us with 25 very precious bytes. This isn't something that can
>be fixed
>+without revising the boot protocol entirely, breaking backwards
>compatibility.
>+
>+boot_params proper is limited to 4096 bytes, but can be arbitrarily
>extended
>+by adding setup_data entries. It cannot be used to communicate
>properties of
>+the kernel image, because it is .bss and has no image-provided
>content.
>+
>+kernel_info solves this by providing an extensible place for
>information about
>+the kernel image. It is readonly, because the kernel cannot rely on a
>+bootloader copying its contents anywhere, but that is OK; if it
>becomes
>+necessary it can still contain data items that an enabled bootloader
>would be
>+expected to copy into a setup_data chunk.
>+
>+It is recommended to not store large data chunks, e.g. strings,
>directly in the
>+kernel_info struct. Such data should be placed outside of it and
>pointed from
>+the kernel_info structure using offsets from the beginning of the
>structure,
>+the kernel_info.header field.
>+
>+
>+Details of the kernel_info Fields
>+=================================
>+
>+============	========
>+Field name:	header
>+Offset/size:	0x0000/4
>+============	========
>+
>+  Contains the magic number "InfO" (0x4f666e49).
>+
>+============	========
>+Field name:	size
>+Offset/size:	0x0004/4
>+============	========
>+
>+  This field contains the size of the kernel_info including
>kernel_info.header.
>+  It should be used by the boot loader to detect supported fields in
>the kernel_info.
>+
> 
> The Image Checksum
> ==================
>diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
>index e2839b5c246c..c30a9b642a86 100644
>--- a/arch/x86/boot/Makefile
>+++ b/arch/x86/boot/Makefile
>@@ -87,7 +87,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
> 
> SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
> 
>-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW]
>\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|_end\|_ehead\|_text\|z_.*\)$$/\#define
>ZO_\2 0x\1/p'
>+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW]
>\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define
>ZO_\2 0x\1/p'
> 
> quiet_cmd_zoffset = ZOFFSET $@
>       cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>diff --git a/arch/x86/boot/compressed/Makefile
>b/arch/x86/boot/compressed/Makefile
>index 6b84afdd7538..fad3b18e2cc3 100644
>--- a/arch/x86/boot/compressed/Makefile
>+++ b/arch/x86/boot/compressed/Makefile
>@@ -72,8 +72,8 @@ $(obj)/../voffset.h: vmlinux FORCE
> 
> $(obj)/misc.o: $(obj)/../voffset.h
> 
>-vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/head_$(BITS).o
>$(obj)/misc.o \
>-	$(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
>+vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/kernel_info.o
>$(obj)/head_$(BITS).o \
>+	$(obj)/misc.o $(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
> 	$(obj)/piggy.o $(obj)/cpuflags.o
> 
> vmlinux-objs-$(CONFIG_EARLY_PRINTK) += $(obj)/early_serial_console.o
>diff --git a/arch/x86/boot/compressed/kernel_info.S
>b/arch/x86/boot/compressed/kernel_info.S
>new file mode 100644
>index 000000000000..3f1cb301b9ff
>--- /dev/null
>+++ b/arch/x86/boot/compressed/kernel_info.S
>@@ -0,0 +1,12 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+
>+	.section ".rodata.kernel_info", "a"
>+
>+	.global kernel_info
>+
>+kernel_info:
>+        /* Header. */
>+	.ascii	"InfO"
>+        /* Size. */
>+	.long	kernel_info_end - kernel_info
>+kernel_info_end:
>diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
>index 850b8762e889..ec6a25a43148 100644
>--- a/arch/x86/boot/header.S
>+++ b/arch/x86/boot/header.S
>@@ -557,6 +557,7 @@ pref_address:		.quad LOAD_PHYSICAL_ADDR	# preferred
>load addr
> 
> init_size:		.long INIT_SIZE		# kernel initialization size
> handover_offset:	.long 0			# Filled in by build.c
>+kernel_info_offset:	.long 0			# Filled in by build.c
> 
># End of setup header
>#####################################################
> 
>diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
>index a93d44e58f9c..55e669d29e54 100644
>--- a/arch/x86/boot/tools/build.c
>+++ b/arch/x86/boot/tools/build.c
>@@ -56,6 +56,7 @@ u8 buf[SETUP_SECT_MAX*512];
> unsigned long efi32_stub_entry;
> unsigned long efi64_stub_entry;
> unsigned long efi_pe_entry;
>+unsigned long kernel_info;
> unsigned long startup_64;
> 
>/*----------------------------------------------------------------------*/
>@@ -321,6 +322,7 @@ static void parse_zoffset(char *fname)
> 		PARSE_ZOFS(p, efi32_stub_entry);
> 		PARSE_ZOFS(p, efi64_stub_entry);
> 		PARSE_ZOFS(p, efi_pe_entry);
>+		PARSE_ZOFS(p, kernel_info);
> 		PARSE_ZOFS(p, startup_64);
> 
> 		p = strchr(p, '\n');
>@@ -410,6 +412,9 @@ int main(int argc, char ** argv)
> 
> 	efi_stub_entry_update();
> 
>+	/* Update kernel_info offset. */
>+	put_unaligned_le32(kernel_info, &buf[0x268]);
>+
> 	crc = partial_crc32(buf, i, crc);
> 	if (fwrite(buf, 1, i, dest) != i)
> 		die("Writing setup failed");
>diff --git a/arch/x86/include/uapi/asm/bootparam.h
>b/arch/x86/include/uapi/asm/bootparam.h
>index 60733f137e9a..b05318112452 100644
>--- a/arch/x86/include/uapi/asm/bootparam.h
>+++ b/arch/x86/include/uapi/asm/bootparam.h
>@@ -86,6 +86,7 @@ struct setup_header {
> 	__u64	pref_address;
> 	__u32	init_size;
> 	__u32	handover_offset;
>+	__u32	kernel_info_offset;
> } __attribute__((packed));
> 
> struct sys_desc_table {

I should like to make make things a bit more stringent: additional data should be made offsets from the kernel_info structure *and they should live in the .rodata.kernel_info section*. We should add a size field for the entire .kernel_info section, thus ensuring it is a single self-contained blob.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Powered by blists - more mailing lists