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(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 99c7194f7ea6..271b9632dd95 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -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(®ion, 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!");
--
2.5.5
* Baoquan He <[email protected]> wrote:
> 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.
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 <[email protected]> wrote:
> +/*
> + * 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.
> +{
> + 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'??
> + 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.
> + }
> + 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]?
> +static inline bool process_efi_entries(unsigned long minimum,
> + unsigned long image_size)
ugly linebreak again ...
Thanks,
Ingo
On 07/21/17 at 12:33pm, Ingo Molnar wrote:
>
> * Baoquan He <[email protected]> wrote:
>
> > 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.
>
> 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 ...
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
On 07/21/17 at 12:37pm, Ingo Molnar wrote:
>
> * Baoquan He <[email protected]> wrote:
>
> > +/*
> > + * 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.
>
> > +{
> > + 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.
>
> > + 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");
> > + 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?
>
> > +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
* Baoquan He <[email protected]> wrote:
> > > +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
On 07/21/17 at 07:37pm, Ingo Molnar wrote:
>
> * Baoquan He <[email protected]> wrote:
>
> > > > +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.
Got it, will use this one. Thanks a lot!
On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
>
> 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.
On 07/24/17 at 02:34pm, Matt Fleming wrote:
> On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> >
> > 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.
Thanks a lot, Matt. Then I will keep using the way.
Hi Ingo,
On 07/24/17 at 02:34pm, Matt Fleming wrote:
> On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> >
> > 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.
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
* Matt Fleming <[email protected]> wrote:
> On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> >
> > 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.
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
On 07/28/17 at 11:55am, Ingo Molnar wrote:
>
> * Matt Fleming <[email protected]> wrote:
>
> > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > >
> > > 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.
>
> 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.
On 07/28/17 at 06:18pm, Baoquan He wrote:
> On 07/28/17 at 11:55am, Ingo Molnar wrote:
> >
> > * Matt Fleming <[email protected]> wrote:
> >
> > > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > > >
> > > > 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.
> >
> > 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.
Oh, plesse ignore this noise, it need involve struct efi_memory_map which
need be built till efi_init() called.
Hi Matt,
On 07/28/17 at 11:55am, Ingo Molnar wrote:
>
> * Matt Fleming <[email protected]> wrote:
>
> > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > >
> > > 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.
>
> 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
On Fri, 28 Jul, at 07:26:03PM, Baoquan He wrote:
> Hi Matt,
>
> On 07/28/17 at 11:55am, Ingo Molnar wrote:
> >
> > * Matt Fleming <[email protected]> wrote:
> >
> > > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > > >
> > > > 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.
> >
> > 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?
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.
On 08/04/17 at 12:23pm, Matt Fleming wrote:
> On Fri, 28 Jul, at 07:26:03PM, Baoquan He wrote:
> > Hi Matt,
> >
> > On 07/28/17 at 11:55am, Ingo Molnar wrote:
> > >
> > > * Matt Fleming <[email protected]> wrote:
> > >
> > > > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > > > >
> > > > > 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.
> > >
> > > 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?
>
> 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)))
On Fri, 04 Aug, at 07:40:05PM, Baoquan He wrote:
> On 08/04/17 at 12:23pm, Matt Fleming wrote:
> > On Fri, 28 Jul, at 07:26:03PM, Baoquan He wrote:
> > > Hi Matt,
> > >
> > > On 07/28/17 at 11:55am, Ingo Molnar wrote:
> > > >
> > > > * Matt Fleming <[email protected]> wrote:
> > > >
> > > > > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > > > > >
> > > > > > 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.
> > > >
> > > > 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?
> >
> > 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)))
Looks fine to me, but I'd wait for Ingo's OK before resending.
On 08/04/17 at 12:55pm, Matt Fleming wrote:
> On Fri, 04 Aug, at 07:40:05PM, Baoquan He wrote:
> > On 08/04/17 at 12:23pm, Matt Fleming wrote:
> > > On Fri, 28 Jul, at 07:26:03PM, Baoquan He wrote:
> > > > Hi Matt,
> > > >
> > > > On 07/28/17 at 11:55am, Ingo Molnar wrote:
> > > > >
> > > > > * Matt Fleming <[email protected]> wrote:
> > > > >
> > > > > > On Fri, 21 Jul, at 09:19:56PM, Baoquan He wrote:
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > 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?
> > >
> > > 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 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!