x86/boot/KASLR: Restrict kernel to be randomized in mirror regions

PATCH RESEND Superseded Baoquan He 3 participants 18 messages Jul 20, 2017 – Aug 4, 2017

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Baoquan He

author

2017-07-20 09:16 UTC #0 lore raw

PATCH RESEND v6 1/1 x86/boot/KASLR: Restrict kernel to be randomized in mirror regions

Kernel text may be located in non-mirror regions (movable zone) when both address range mirroring feature and KASLR are enabled.

The address range mirroring feature arranges such mirror region into normal zone and other region into movable zone in order to locate kernel code and data in mirror region. The physical memory region whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are mirrored.

If efi is detected, iterate efi memory map and pick the mirror region to process for adding candidate of randomization slot. If efi is disabled or no mirror region found, still process e820 memory map.

Signed-off-by Baoquan He <[email protected]>

Reviewed-by Kees Cook <[email protected]>

---
v5->v6:
Code style issue fix according to Kees's comment.

This is based on tip/x86/boot, patch 1,2,3/4 in v5 post has been put into tip/x86/boot now.

 arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

arch/x86/boot/compressed/kaslr.c +66 -2

index 99c7194f7ea6..271b9632dd95 100644
@@ -37,7 +37,9 @@
 #include <linux/uts.h>
 #include <linux/utsname.h>
 #include <linux/ctype.h>
+#include <linux/efi.h>
 #include <generated/utsrelease.h>
+#include <asm/efi.h>
 
 /* Macros used by the included decompressor code below. */
 #define STATIC

@@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+#ifdef CONFIG_EFI
+/*
+ * Returns true if mirror region found (and must have been processed
+ * for slots adding)
+ */
+static bool process_efi_entries(unsigned long minimum,
+				unsigned long image_size)
+{
+	struct efi_info *e = &boot_params->efi_info;
+	bool efi_mirror_found = false;
+	struct mem_vector region;
+	efi_memory_desc_t *md;
+	unsigned long pmap;
+	char *signature;
+	u32 nr_desc;
+	int i;
+
+	signature = (char *)&boot_params->efi_info.efi_loader_signature;
+	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
+	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
+		return false;
+
+#ifdef CONFIG_X86_32
+	/* Can't handle data above 4GB at this time */
+	if (e->efi_memmap_hi) {
+		warn("Memory map is above 4GB, EFI should be disabled.\n");
+		return false;
+	}
+	pmap =  e->efi_memmap;
+#else
+	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
+#endif
+
+	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
+	for (i = 0; i < nr_desc; i++) {
+		md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
+		if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
+			region.start = md->phys_addr;
+			region.size = md->num_pages << EFI_PAGE_SHIFT;
+			process_mem_region(&region, minimum, image_size);
+			efi_mirror_found = true;
+
+			if (slot_area_index == MAX_SLOT_AREA) {
+				debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+				break;
+			}
+		}
+	}
+
+	return efi_mirror_found;
+}
+#else
+static inline bool process_efi_entries(unsigned long minimum,
+				       unsigned long image_size)
+{
+	return false;
+}
+#endif
+
 static void process_e820_entries(unsigned long minimum,
 				 unsigned long image_size)
 {

@@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
 {
 	/* Check if we had too many memmaps. */
 	if (memmap_too_large) {
-		debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n");
+		debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
 		return 0;
 	}
 
 	/* Make sure minimum is aligned. */
 	minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
+	if (process_efi_entries(minimum, image_size))
+		return slots_fetch_random();
+
 	process_e820_entries(minimum, image_size);
 	return slots_fetch_random();
 }

@@ -652,7 +716,7 @@ void choose_random_location(unsigned long input,
 	 */
 	min_addr = min(*output, 512UL << 20);
 
-	/* Walk e820 and find a random address. */
+	/* Walk available memory entries to find a random address. */
 	random_addr = find_random_phys_addr(min_addr, output_size);
 	if (!random_addr) {
 		warn("Physical KASLR disabled: no suitable memory region!");

Signature · 1 line

2.5.5

Ingo Molnar

2017-07-21 10:33 UTC #1 lore raw

Quoted 9 lines · Baoquan He

Kernel text may be located in non-mirror regions (movable zone) when both address range mirroring feature and KASLR are enabled.
The address range mirroring feature arranges such mirror region into normal zone and other region into movable zone in order to locate kernel code and data in mirror region. The physical memory region whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are mirrored.

Quoted 3 lines

If efi is detected, iterate efi memory map and pick the mirror region to process for adding candidate of randomization slot. If efi is disabled or no mirror region found, still process e820 memory map.

Please read your own changelogs and capitalize 'EFI' consistently!

Also, what is unclear to me after reading this changelog, what does this patch actually achieve, relative to existing behavior?

It would be helpful if it was structured like this:

Previous behavior was that the kernel would ...

This patch changes the old behavior so that the kernel now ...

Thanks,

Ingo

Baoquan He

author

2017-07-21 12:35 UTC #2 lore raw

Quoted 18 lines · Ingo Molnar

Quoted 9 lines · Baoquan He
Kernel text may be located in non-mirror regions (movable zone) when both address range mirroring feature and KASLR are enabled.
The address range mirroring feature arranges such mirror region into normal zone and other region into movable zone in order to locate kernel code and data in mirror region. The physical memory region whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE attribute (bit: 16) are mirrored.
Quoted 3 lines
If efi is detected, iterate efi memory map and pick the mirror region to process for adding candidate of randomization slot. If efi is disabled or no mirror region found, still process e820 memory map.
Please read your own changelogs and capitalize 'EFI' consistently!

Quoted 8 lines

Also, what is unclear to me after reading this changelog, what does this patch actually achieve, relative to existing behavior?
It would be helpful if it was structured like this:
Previous behavior was that the kernel would ...
This patch changes the old behavior so that the kernel now ...

Sure, I will check the patch log and change all 'efi' to 'EFI'. And rewrite the log according to the suggested format. Thanks a lot!

Thanks
Baoquan

Ingo Molnar

2017-07-21 10:38 UTC #3 lore raw

In reply to Baoquan He

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 569

Quoted 6 lines · Baoquan He

arch/x86/boot/compressed/kaslr.c +6 -0

+/*
+ * Returns true if mirror region found (and must have been processed
+ * for slots adding)
+ */
+static bool process_efi_entries(unsigned long minimum,
+				unsigned long image_size)

Also, please don't break the line in the middle of the prototype.

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 580

Quoted 11 lines

arch/x86/boot/compressed/kaslr.c +11 -0

+{
+	struct efi_info *e = &boot_params->efi_info;
+	bool efi_mirror_found = false;
+	struct mem_vector region;
+	efi_memory_desc_t *md;
+	unsigned long pmap;
+	char *signature;
+	u32 nr_desc;
+	int i;
+
+	signature = (char *)&boot_params->efi_info.efi_loader_signature;

This is sloppy too: we already have '&boot_params->efi_info' in 'e', why do you duplicate it again, why not write 'e->efi_loader_signature'??

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 589

Quoted 9 lines

arch/x86/boot/compressed/kaslr.c +9 -0

+	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
+	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
+		return false;
+
+#ifdef CONFIG_X86_32
+	/* Can't handle data above 4GB at this time */
+	if (e->efi_memmap_hi) {
+		warn("Memory map is above 4GB, EFI should be disabled.\n");
+		return false;

This kernel warning is pretty passive-aggressive: please explain what the problem is and how it can be resolved.

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 598

Quoted 9 lines

arch/x86/boot/compressed/kaslr.c +9 -0

+	}
+	pmap =  e->efi_memmap;
+#else
+	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
+#endif
+
+	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
+	for (i = 0; i < nr_desc; i++) {
+		md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));

This looks unnecessarily obfuscated: why not initialize 'md' to 'pmap' when pmap is calculated and just use md[i]?

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 616

Quoted 2 lines

arch/x86/boot/compressed/kaslr.c +2 -0

+static inline bool process_efi_entries(unsigned long minimum,
+				       unsigned long image_size)

ugly linebreak again ...

Thanks,

Ingo

Baoquan He

author

2017-07-21 13:20 UTC #4 lore raw

Quoted 11 lines · Ingo Molnar

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 569
Quoted 6 lines · Baoquan He
arch/x86/boot/compressed/kaslr.c +6 -0
+/*
+ * Returns true if mirror region found (and must have been processed
+ * for slots adding)
+ */
+static bool process_efi_entries(unsigned long minimum,
+				unsigned long image_size)
Also, please don't break the line in the middle of the prototype.

OK, will change it into oneline. I worry it's a little too long since it's 80 chars wide if not break.

Quoted 15 lines

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 580
Quoted 12 lines
arch/x86/boot/compressed/kaslr.c +11 -0
+{
+	struct efi_info *e = &boot_params->efi_info;
+	bool efi_mirror_found = false;
+	struct mem_vector region;
+	efi_memory_desc_t *md;
+	unsigned long pmap;
+	char *signature;
+	u32 nr_desc;
+	int i;
+
+	signature = (char *)&boot_params->efi_info.efi_loader_signature;
This is sloppy too: we already have '&boot_params->efi_info' in 'e', why do you duplicate it again, why not write 'e->efi_loader_signature'??

Right, will change.

Quoted 13 lines

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 589
Quoted 10 lines
arch/x86/boot/compressed/kaslr.c +9 -0
+	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
+	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
+		return false;
+
+#ifdef CONFIG_X86_32
+	/* Can't handle data above 4GB at this time */
+	if (e->efi_memmap_hi) {
+		warn("Memory map is above 4GB, EFI should be disabled.\n");
+		return false;
This kernel warning is pretty passive-aggressive: please explain what the problem is and how it can be resolved.

Maybe it can be like :

warn("Data above 4G can't be handled now in 32bit system, EFI should be disabled.\n");

Quoted 15 lines

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 598
Quoted 12 lines
arch/x86/boot/compressed/kaslr.c +10 -0
+		return false;
 
 
+	}
+	pmap =  e->efi_memmap;
+#else
+	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
+#endif
+
+	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
+	for (i = 0; i < nr_desc; i++) {
+		md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
This looks unnecessarily obfuscated: why not initialize 'md' to 'pmap' when pmap is calculated and just use md[i]?

There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?

Hi Matt,

Could you help have a look at this?

Quoted 5 lines

arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 616
Quoted 3 lines
arch/x86/boot/compressed/kaslr.c +2 -0
+static inline bool process_efi_entries(unsigned long minimum,
+				       unsigned long image_size)
ugly linebreak again ...

The whole line is more than 80. I break the line and use tab and space to make it align with above 'unsigned long minimum'. Don't know why it becomes messy in patch. Will check and try again.

Thanks
Baoquan

Ingo Molnar

2017-07-21 17:38 UTC #5 lore raw

Quoted 8 lines · Baoquan He

Quoted 4 lines
arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 616
Quoted 2 lines
arch/x86/boot/compressed/kaslr.c +2 -0
+static inline bool process_efi_entries(unsigned long minimum,
+				       unsigned long image_size)
ugly linebreak again ...
The whole line is more than 80. I break the line and use tab and space to make it align with above 'unsigned long minimum'. Don't know why it becomes messy in patch. Will check and try again.

Then make the linebreak less ugly, or ignore the checkpatch warning!

This commonly used pattern:

static inline bool
process_efi_entries(unsigned long minimum, unsigned long image_size)

looks a lot better than the function parameter list broken in the middle.

Thanks,

Ingo

Baoquan He

author

2017-07-21 23:43 UTC #6 lore raw

Quoted 12 lines · Ingo Molnar

Quoted 8 lines · Baoquan He
Quoted 4 lines
arch/x86/boot/compressed/kaslr.c static void process_mem_region(struct mem_vector *entry, · line 616
Quoted 2 lines
arch/x86/boot/compressed/kaslr.c +2 -0
+static inline bool process_efi_entries(unsigned long minimum,
+				       unsigned long image_size)
ugly linebreak again ...
The whole line is more than 80. I break the line and use tab and space to make it align with above 'unsigned long minimum'. Don't know why it becomes messy in patch. Will check and try again.

Quoted 8 lines

Then make the linebreak less ugly, or ignore the checkpatch warning!
This commonly used pattern:
static inline bool
process_efi_entries(unsigned long minimum, unsigned long image_size)
looks a lot better than the function parameter list broken in the middle.

Got it, will use this one. Thanks a lot!

Matt Fleming

2017-07-24 13:34 UTC #7 lore raw

In reply to Baoquan He

Quoted 9 lines · Baoquan He

There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Hi Matt,
Could you help have a look at this?

You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

Baoquan He

author

2017-07-25 00:24 UTC #8 lore raw

Quoted 7 lines · Matt Fleming

Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?

Quoted 7 lines

Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

Thanks a lot, Matt. Then I will keep using the way.

Baoquan He

author

2017-07-28 08:06 UTC #9 lore raw

In reply to Matt Fleming

Hi Ingo,

Quoted 7 lines · Matt Fleming

Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?

Quoted 7 lines

Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

I saw your new comment in v7 post. Matt has helped to confirm it. The EFI code was made to get efi memmap in that way on purpose. There are several sub-threads about this patch, it could be missed.

Thanks
Baoquan

Ingo Molnar

2017-07-28 09:55 UTC #10 lore raw

In reply to Matt Fleming

Quoted 7 lines · Matt Fleming

Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?

Quoted 7 lines

Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

This is not obvious and looks pretty ugly as well, and open coded in several places.

At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).

Thanks,

Ingo

Baoquan He

author

2017-07-28 10:18 UTC #11 lore raw

Quoted 18 lines · Ingo Molnar

Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

Quoted 6 lines

This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).

Seems for_each_efi_memory_desc_in_map() can be used to hide the calculation and type cast each time for better memmap loop.

Baoquan He

author

2017-07-28 10:38 UTC #12 lore raw

Quoted 22 lines · Baoquan He

Quoted 21 lines · Ingo Molnar
Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.
This is not obvious and looks pretty ugly as well, and open coded in several places.

Quoted 6 lines

Quoted 3 lines
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).
Seems for_each_efi_memory_desc_in_map() can be used to hide the calculation and type cast each time for better memmap loop.

Oh, plesse ignore this noise, it need involve struct efi_memory_map which need be built till efi_init() called.

Baoquan He

author

2017-07-28 11:26 UTC #13 lore raw

In reply to Ingo Molnar

Hi Matt,

Quoted 18 lines · Ingo Molnar

Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.

Quoted 6 lines

This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).

I can make a efi_memdesc_ptr(efi, i) wrapper as Ingo suggested and use it here if you agree. Seems it might be not good to add another for_each_efi_memory_desc_xxxx wrapper since there are different memmap data structures in x86 boot and in general efi libstub. Or any other idea?

Thanks
Baoquan

Matt Fleming

2017-08-04 11:23 UTC #14 lore raw

Quoted 28 lines · Baoquan He

Hi Matt,
Quoted 18 lines · Ingo Molnar
Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.
Quoted 6 lines
This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).

Quoted 5 lines

I can make a efi_memdesc_ptr(efi, i) wrapper as Ingo suggested and use it here if you agree. Seems it might be not good to add another for_each_efi_memory_desc_xxxx wrapper since there are different memmap data structures in x86 boot and in general efi libstub. Or any other idea?

I think adding a wrapper is fine, but I'd suggest including the word "early" (or something similar) to explain that it should only be used during bootup -- we want everyone else to use the for_each_efi_memory_*() API.

Baoquan He

author

2017-08-04 11:40 UTC #15 lore raw

Quoted 35 lines · Matt Fleming

Quoted 28 lines · Baoquan He
Hi Matt,
Quoted 18 lines · Ingo Molnar
Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.
Quoted 6 lines
This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).
Quoted 5 lines
I can make a efi_memdesc_ptr(efi, i) wrapper as Ingo suggested and use it here if you agree. Seems it might be not good to add another for_each_efi_memory_desc_xxxx wrapper since there are different memmap data structures in x86 boot and in general efi libstub. Or any other idea?

Quoted 4 lines

I think adding a wrapper is fine, but I'd suggest including the word "early" (or something similar) to explain that it should only be used during bootup -- we want everyone else to use the for_each_efi_memory_*() API.

Thanks, Matt. I can do that. Do you think below helper definition is OK to you? If yes, I can upstate with it and post v9.

#define efi_early_memdesc_ptr(map, desc_size, n)				\
	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))

Matt Fleming

2017-08-04 11:55 UTC #16 lore raw

Quoted 41 lines · Baoquan He

Quoted 35 lines · Matt Fleming
Quoted 28 lines · Baoquan He
Hi Matt,
Quoted 18 lines · Ingo Molnar
Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.
Quoted 6 lines
This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).
Quoted 5 lines
I can make a efi_memdesc_ptr(efi, i) wrapper as Ingo suggested and use it here if you agree. Seems it might be not good to add another for_each_efi_memory_desc_xxxx wrapper since there are different memmap data structures in x86 boot and in general efi libstub. Or any other idea?
Quoted 4 lines
I think adding a wrapper is fine, but I'd suggest including the word "early" (or something similar) to explain that it should only be used during bootup -- we want everyone else to use the for_each_efi_memory_*() API.

Quoted 5 lines

Thanks, Matt. I can do that. Do you think below helper definition is OK to you? If yes, I can upstate with it and post v9.
#define efi_early_memdesc_ptr(map, desc_size, n)				\
	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))

Looks fine to me, but I'd wait for Ingo's OK before resending.

Baoquan He

author

2017-08-04 12:02 UTC #17 lore raw

Quoted 42 lines · Matt Fleming

Quoted 41 lines · Baoquan He
Quoted 39 lines · Matt Fleming
Quoted 28 lines · Baoquan He
Hi Matt,
Quoted 18 lines · Ingo Molnar
Quoted 7 lines · Matt Fleming
Quoted 6 lines · Baoquan He
There are places where the efi map is getting and used like this. E.g in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c. EFI developers worry the size of efi_memory_desc_t could not be the same as e->efi_memdesc_size?
Quoted 7 lines
Quoted 3 lines
Hi Matt,
Could you help have a look at this?
You're exactly right. The code guards against the size of the efi_memory_desc_t struct changing. The UEFI spec says to traverse the memory map this way.
Quoted 6 lines
This is not obvious and looks pretty ugly as well, and open coded in several places.
At minimum we should have an efi_memdesc_ptr(efi, i) wrapper inline (or so) that gives us the entry pointer, plus a comment that points out that ->memdesc_size might not be equal to sizeof(efi_memory_memdesc_t).
Quoted 5 lines
I can make a efi_memdesc_ptr(efi, i) wrapper as Ingo suggested and use it here if you agree. Seems it might be not good to add another for_each_efi_memory_desc_xxxx wrapper since there are different memmap data structures in x86 boot and in general efi libstub. Or any other idea?
I think adding a wrapper is fine, but I'd suggest including the word "early" (or something similar) to explain that it should only be used during bootup -- we want everyone else to use the for_each_efi_memory_*() API.

Quoted 7 lines

Quoted 5 lines
Thanks, Matt. I can do that. Do you think if below helper definition is OK to you? If yes, I can update with it and post v9.
#define efi_early_memdesc_ptr(map, desc_size, n)				\
	(efi_memory_desc_t *)((void *)(map) + ((n) * (desc_size)))
Looks fine to me, but I'd wait for Ingo's OK before resending.

Sure, thanks!