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

[Daniel Kiper <daniel.kiper@oracle.com>]

On Fri, Jul 12, 2019 at 08:56:44AM -0700, hpa@zytor.com wrote:
> On July 4, 2019 9:36:11 AM PDT, Daniel Kiper <daniel.kiper@oracle.com> 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