2019-07-04 16:39:48

by Daniel Kiper

[permalink] [raw]
Subject: [PATCH v2 2/3] x86/boot: Introduce the setup_indirect

The setup_data is a bit awkward to use for extremely large data objects,
both because the setup_data header has to be adjacent to the data object
and because it has a 32-bit length field. However, it is important that
intermediate stages of the boot process have a way to identify which
chunks of memory are occupied by kernel data.

Thus we introduce a uniform way to specify such indirect data as
setup_indirect struct and SETUP_INDIRECT type.

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 <[email protected]>
Signed-off-by: Daniel Kiper <[email protected]>
Reviewed-by: Eric Snowberg <[email protected]>
Reviewed-by: Ross Philipson <[email protected]>
---
v2 - suggestions/fixes:
- add setup_indirect usage example
(suggested by Eric Snowberg and Ross Philipson).
---
Documentation/x86/boot.rst | 38 ++++++++++++++++++++++++++++++++++-
arch/x86/include/uapi/asm/bootparam.h | 11 +++++++++-
2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index a934a56f0516..23d3726d54fc 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -73,7 +73,7 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c188
(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.
+Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
============= ============================================================

.. note::
@@ -827,6 +827,42 @@ Protocol: 2.09+
sure to consider the case where the linked list already contains
entries.

+ The setup_data is a bit awkward to use for extremely large data objects,
+ both because the setup_data header has to be adjacent to the data object
+ and because it has a 32-bit length field. However, it is important that
+ intermediate stages of the boot process have a way to identify which
+ chunks of memory are occupied by kernel data.
+
+ Thus setup_indirect struct and SETUP_INDIRECT type were introduced in
+ protocol 2.15.
+
+ struct setup_indirect {
+ __u32 type;
+ __u32 reserved; /* Reserved, must be set to zero. */
+ __u64 len;
+ __u64 addr;
+ };
+
+ The type member is itself simply a SETUP_* type. However, it cannot be
+ SETUP_INDIRECT since making the setup_indirect a tree structure could
+ require a lot of stack space in something that needs to parse it and
+ stack space can be limited in boot contexts.
+
+ Let's give an example how to point to SETUP_E820_EXT data using setup_indirect.
+ In this case setup_data and setup_indirect will look like this:
+
+ struct setup_data {
+ __u64 next = 0 or <addr_of_next_setup_data_struct>;
+ __u32 type = SETUP_INDIRECT;
+ __u32 len = sizeof(setup_data);
+ __u8 data[sizeof(setup_indirect)] = struct setup_indirect {
+ __u32 type = SETUP_E820_EXT;
+ __u32 reserved = 0;
+ __u64 len = <len_of_SETUP_E820_EXT_data>;
+ __u64 addr = <addr_of_SETUP_E820_EXT_data>;
+ }
+ }
+
============ ============
Field name: pref_address
Type: read (reloc)
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b05318112452..aaaa17fa6ad6 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -2,7 +2,7 @@
#ifndef _ASM_X86_BOOTPARAM_H
#define _ASM_X86_BOOTPARAM_H

-/* setup_data types */
+/* setup_data/setup_indirect types */
#define SETUP_NONE 0
#define SETUP_E820_EXT 1
#define SETUP_DTB 2
@@ -10,6 +10,7 @@
#define SETUP_EFI 4
#define SETUP_APPLE_PROPERTIES 5
#define SETUP_JAILHOUSE 6
+#define SETUP_INDIRECT 7

/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
@@ -47,6 +48,14 @@ struct setup_data {
__u8 data[0];
};

+/* extensible setup indirect data node */
+struct setup_indirect {
+ __u32 type;
+ __u32 reserved; /* Reserved, must be set to zero. */
+ __u64 len;
+ __u64 addr;
+};
+
struct setup_header {
__u8 setup_sects;
__u16 root_flags;
--
2.11.0


2019-07-12 15:57:59

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] x86/boot: Introduce the setup_indirect

On July 4, 2019 9:36:11 AM PDT, Daniel Kiper <[email protected]> wrote:
>The setup_data is a bit awkward to use for extremely large data
>objects,
>both because the setup_data header has to be adjacent to the data
>object
>and because it has a 32-bit length field. However, it is important that
>intermediate stages of the boot process have a way to identify which
>chunks of memory are occupied by kernel data.
>
>Thus we introduce a uniform way to specify such indirect data as
>setup_indirect struct and SETUP_INDIRECT type.
>
>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 <[email protected]>
>Signed-off-by: Daniel Kiper <[email protected]>
>Reviewed-by: Eric Snowberg <[email protected]>
>Reviewed-by: Ross Philipson <[email protected]>
>---
>v2 - suggestions/fixes:
> - add setup_indirect usage example
> (suggested by Eric Snowberg and Ross Philipson).
>---
>Documentation/x86/boot.rst | 38
>++++++++++++++++++++++++++++++++++-
> arch/x86/include/uapi/asm/bootparam.h | 11 +++++++++-
> 2 files changed, 47 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
>index a934a56f0516..23d3726d54fc 100644
>--- a/Documentation/x86/boot.rst
>+++ b/Documentation/x86/boot.rst
>@@ -73,7 +73,7 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT
>ae7e1238e68f2a472a125673ab506d49158c188
> (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.
>+Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
>============= ============================================================
>
> .. note::
>@@ -827,6 +827,42 @@ Protocol: 2.09+
> sure to consider the case where the linked list already contains
> entries.
>
>+ The setup_data is a bit awkward to use for extremely large data
>objects,
>+ both because the setup_data header has to be adjacent to the data
>object
>+ and because it has a 32-bit length field. However, it is important
>that
>+ intermediate stages of the boot process have a way to identify which
>+ chunks of memory are occupied by kernel data.
>+
>+ Thus setup_indirect struct and SETUP_INDIRECT type were introduced
>in
>+ protocol 2.15.
>+
>+ struct setup_indirect {
>+ __u32 type;
>+ __u32 reserved; /* Reserved, must be set to zero. */
>+ __u64 len;
>+ __u64 addr;
>+ };
>+
>+ The type member is itself simply a SETUP_* type. However, it cannot
>be
>+ SETUP_INDIRECT since making the setup_indirect a tree structure
>could
>+ require a lot of stack space in something that needs to parse it and
>+ stack space can be limited in boot contexts.
>+
>+ Let's give an example how to point to SETUP_E820_EXT data using
>setup_indirect.
>+ In this case setup_data and setup_indirect will look like this:
>+
>+ struct setup_data {
>+ __u64 next = 0 or <addr_of_next_setup_data_struct>;
>+ __u32 type = SETUP_INDIRECT;
>+ __u32 len = sizeof(setup_data);
>+ __u8 data[sizeof(setup_indirect)] = struct setup_indirect {
>+ __u32 type = SETUP_E820_EXT;
>+ __u32 reserved = 0;
>+ __u64 len = <len_of_SETUP_E820_EXT_data>;
>+ __u64 addr = <addr_of_SETUP_E820_EXT_data>;
>+ }
>+ }
>+
> ============ ============
> Field name: pref_address
> Type: read (reloc)
>diff --git a/arch/x86/include/uapi/asm/bootparam.h
>b/arch/x86/include/uapi/asm/bootparam.h
>index b05318112452..aaaa17fa6ad6 100644
>--- a/arch/x86/include/uapi/asm/bootparam.h
>+++ b/arch/x86/include/uapi/asm/bootparam.h
>@@ -2,7 +2,7 @@
> #ifndef _ASM_X86_BOOTPARAM_H
> #define _ASM_X86_BOOTPARAM_H
>
>-/* setup_data types */
>+/* setup_data/setup_indirect types */
> #define SETUP_NONE 0
> #define SETUP_E820_EXT 1
> #define SETUP_DTB 2
>@@ -10,6 +10,7 @@
> #define SETUP_EFI 4
> #define SETUP_APPLE_PROPERTIES 5
> #define SETUP_JAILHOUSE 6
>+#define SETUP_INDIRECT 7
>
> /* ram_size flags */
> #define RAMDISK_IMAGE_START_MASK 0x07FF
>@@ -47,6 +48,14 @@ struct setup_data {
> __u8 data[0];
> };
>
>+/* extensible setup indirect data node */
>+struct setup_indirect {
>+ __u32 type;
>+ __u32 reserved; /* Reserved, must be set to zero. */
>+ __u64 len;
>+ __u64 addr;
>+};
>+
> struct setup_header {
> __u8 setup_sects;
> __u16 root_flags;

This needs actual implementation; we can't advertise it until the kernel knows how to consume the data! It probably should be moved to after the 3/3 patch.

Implementing this has two parts:

1. The kernel needs to be augmented so it can find current objects via indirection.

2. And this is the main reason for this in the first place: the early code needs to walk the list and map out the memory areas which are occupied so it doesn't clobber anything; this allows this code to be generic as opposed to having to know what is a pointer and what size it might point to.

(The decompressor didn't need this until kaslr entered the picture, but now it does, sadly.)

Optional/future enhancements that might be nice:

3. Add some kind of description (e.g. foo=u64 ; bar=str ; baz=blob) to make it possible to write a bootloader that can load these kinds of objects without specific enabling.

4. Add support for mapping initramfs fragments this way.

5. Add support for passingload-on-boot kernel modules.

6. ... ?

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2019-10-01 14:51:02

by Daniel Kiper

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] x86/boot: Introduce the setup_indirect

On Fri, Jul 12, 2019 at 08:56:44AM -0700, [email protected] wrote:
> On July 4, 2019 9:36:11 AM PDT, Daniel Kiper <[email protected]> wrote:

[...]

> >diff --git a/arch/x86/include/uapi/asm/bootparam.h
> >b/arch/x86/include/uapi/asm/bootparam.h
> >index b05318112452..aaaa17fa6ad6 100644
> >--- a/arch/x86/include/uapi/asm/bootparam.h
> >+++ b/arch/x86/include/uapi/asm/bootparam.h
> >@@ -2,7 +2,7 @@
> > #ifndef _ASM_X86_BOOTPARAM_H
> > #define _ASM_X86_BOOTPARAM_H
> >
> >-/* setup_data types */
> >+/* setup_data/setup_indirect types */
> > #define SETUP_NONE 0
> > #define SETUP_E820_EXT 1
> > #define SETUP_DTB 2
> >@@ -10,6 +10,7 @@
> > #define SETUP_EFI 4
> > #define SETUP_APPLE_PROPERTIES 5
> > #define SETUP_JAILHOUSE 6
> >+#define SETUP_INDIRECT 7
> >
> > /* ram_size flags */
> > #define RAMDISK_IMAGE_START_MASK 0x07FF
> >@@ -47,6 +48,14 @@ struct setup_data {
> > __u8 data[0];
> > };
> >
> >+/* extensible setup indirect data node */
> >+struct setup_indirect {
> >+ __u32 type;
> >+ __u32 reserved; /* Reserved, must be set to zero. */
> >+ __u64 len;
> >+ __u64 addr;
> >+};
> >+
> > struct setup_header {
> > __u8 setup_sects;
> > __u16 root_flags;
>

> This needs actual implementation; we can't advertise it until the
> kernel knows how to consume the data! It probably should be moved to
> after the 3/3 patch.
>
> Implementing this has two parts:
>
> 1. The kernel needs to be augmented so it can find current objects via
> indirection.
>
> 2. And this is the main reason for this in the first place: the early
> code needs to walk the list and map out the memory areas which are
> occupied so it doesn't clobber anything; this allows this code to be
> generic as opposed to having to know what is a pointer and what size
> it might point to.
>
> (The decompressor didn't need this until kaslr entered the picture,
> but now it does, sadly.)

Do you think about arch/x86/boot/compressed/kaslr.c:mem_avoid[]?
But it is static. OK, we can assume that we do not accept more than
something indirect entries. However, this is not nice...

> Optional/future enhancements that might be nice:
>
> 3. Add some kind of description (e.g. foo=u64 ; bar=str ; baz=blob) to
> make it possible to write a bootloader that can load these kinds of
> objects without specific enabling.

This means an extension to command line parser. Am I right?

> 4. Add support for mapping initramfs fragments this way.
>
> 5. Add support for passingload-on-boot kernel modules.

I am not sure what you mean exactly by those two.

Anyway, I would focus only on things which are potentially useful now or
in the near future and not require much code changes. So, IMO #1 and #2
at this point.

Daniel