Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220779 > unrolled thread

Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-08 15:20 +0200
    Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-08 15:30 +0200
      Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-08 22:40 +0200
        Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-09 09:40 +0200
          Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Matt Fleming <matt@codeblueprint.co.uk> - 2015-09-09 12:00 +0200
            Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-09-09 12:10 +0200

#1220779 — Re: [PATCH] x86/efi: Map EFI memmap entries in-order at runtime

On Fri, 04 Sep, at 08:53:36PM, Ard Biesheuvel wrote:
> On 4 September 2015 at 20:23, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Fri, 04 Sep, at 03:24:21PM, Ard Biesheuvel wrote:
> >>
> >> Since the UEFI spec does not mandate an enumeration order for
> >> GetMemoryMap(), it seems to me that you still need to sort its output
> >> before laying out the VA space. Since you need to sort it anyway, why
> >> not simply sort it in reverse order and keep all the original code?
> >> Considering that this is meant for stable, that would keep the delta
> >> *much* smaller.
> >
> > Hmm... that'd be a neat trick and while it would save on the diff
> > size, I don't think it would be smaller in terms of change complexity.
> >
> > EDK2 sorts the memory map when EFI_PROPERTIES_TABLE is enabled, so we
> > can be reasonably sure the entry order returned by GetMemoryMap() is
> > compatible with the split regions, even if it's not mandated by the
> > spec.
> >
> 
> EDK2 does sort it, but the spec does not mandate it so another
> implementation may do something different entirely.
 
Yeah, we should get that requirement added to the spec.

> > For the non-EFI_PROPERTIES_TABLE case, things have been working fine
> > without the sorting, so I'm reluctant to introduce it now (it's also
> > much less of an issue there).
> >
> 
> I see. I do wonder, since the VA mapping preserves the modulo 2 MB
> alignment of each region, aren't you using much more VA space when
> mapping in reverse order as you are doing now?

It doesn't enforce a 2MB alignment for every entry, just those that
are actually 2MB aligned. This should be exactly what was done in the
previous version of the code. Do you see a bug?

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220782

On 8 September 2015 at 15:16, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 04 Sep, at 08:53:36PM, Ard Biesheuvel wrote:
>> On 4 September 2015 at 20:23, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> > On Fri, 04 Sep, at 03:24:21PM, Ard Biesheuvel wrote:
>> >>
>> >> Since the UEFI spec does not mandate an enumeration order for
>> >> GetMemoryMap(), it seems to me that you still need to sort its output
>> >> before laying out the VA space. Since you need to sort it anyway, why
>> >> not simply sort it in reverse order and keep all the original code?
>> >> Considering that this is meant for stable, that would keep the delta
>> >> *much* smaller.
>> >
>> > Hmm... that'd be a neat trick and while it would save on the diff
>> > size, I don't think it would be smaller in terms of change complexity.
>> >
>> > EDK2 sorts the memory map when EFI_PROPERTIES_TABLE is enabled, so we
>> > can be reasonably sure the entry order returned by GetMemoryMap() is
>> > compatible with the split regions, even if it's not mandated by the
>> > spec.
>> >
>>
>> EDK2 does sort it, but the spec does not mandate it so another
>> implementation may do something different entirely.
>
> Yeah, we should get that requirement added to the spec.
>
>> > For the non-EFI_PROPERTIES_TABLE case, things have been working fine
>> > without the sorting, so I'm reluctant to introduce it now (it's also
>> > much less of an issue there).
>> >
>>
>> I see. I do wonder, since the VA mapping preserves the modulo 2 MB
>> alignment of each region, aren't you using much more VA space when
>> mapping in reverse order as you are doing now?
>
> It doesn't enforce a 2MB alignment for every entry, just those that
> are actually 2MB aligned. This should be exactly what was done in the
> previous version of the code. Do you see a bug?
>

I noticed that the 64-bit version of efi_map_region() preserves the
relative alignment with respect to a 2 MB boundary for /each/ region.
Since the regions are mapped in reverse order, it is highly unlikely
that each region starts at the same 2 MB relative alignment that the
previous region ended at, so you are likely wasting quite a bit of VA
space.

I don't think it is a bug, though, but it does not seem intentional.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221073

On Tue, 08 Sep, at 03:21:17PM, Ard Biesheuvel wrote:
> 
> I noticed that the 64-bit version of efi_map_region() preserves the
> relative alignment with respect to a 2 MB boundary for /each/ region.
> Since the regions are mapped in reverse order, it is highly unlikely
> that each region starts at the same 2 MB relative alignment that the
> previous region ended at, so you are likely wasting quite a bit of VA
> space.
> 
> I don't think it is a bug, though, but it does not seem intentional.

Yeah, that's a very good catch. The existing code, that is, top-down
allocation scheme where we map ealier EFI memmap entries at higher
virtual addresses, does incur quite a bit of wasted address space.

That's not true of this patch, though, and it's also not true if we
map the entries in reverse order of the EFI memmap, that is, mapping
the last memmap entry at the highest virtual address.

So it's a bug in the original code, or rather an unintended feature.

Ard, based on your suggestion I cooked this patch up to show what
iterating the EFI memmap in reverse looks like in terms of code. The
below diff and the original patch from this thread give me identical
virtual address space layouts.

Admittedly the below is missing a whole bunch of comments so makes the
diff look smaller, but something like this could work,

---

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 691b333e0038..a2af35f6093a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -704,6 +704,44 @@ out:
 	return ret;
 }
 
+static inline void *efi_map_next_entry_reverse(void *entry)
+{
+	if (!entry)
+		return memmap.map_end - memmap.desc_size;
+
+	entry -= memmap.desc_size;
+	if (entry < memmap.map)
+		return NULL;
+
+	return entry;
+}
+
+static void *efi_map_next_entry(void *entry)
+{
+	bool reverse = false;
+
+	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
+		/*
+		 * Iterate the EFI memory map in reverse order because
+		 * the regions will be mapped top-down. The end result
+		 * is the same as if we had mapped things forward, but
+		 * doesn't require us to change the implementation of
+		 * efi_map_region().
+		 */
+		return efi_map_next_entry_reverse(entry);
+	}
+
+	/* Initial call */
+	if (!entry)
+		return memmap.map;
+
+	entry += memmap.desc_size;
+	if (entry >= memmap.map_end)
+		return NULL;
+
+	return entry;
+}
+
 /*
  * Map the efi memory ranges of the runtime services and update new_mmap with
  * virtual addresses.
@@ -718,7 +756,8 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
 	start = -1UL;
 	end = 0;
 
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+	p = NULL;
+	while ((p = efi_map_next_entry(p))) {
 		md = p;
 		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
 #ifdef CONFIG_X86_64

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221284

On 8 September 2015 at 22:37, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 08 Sep, at 03:21:17PM, Ard Biesheuvel wrote:
>>
>> I noticed that the 64-bit version of efi_map_region() preserves the
>> relative alignment with respect to a 2 MB boundary for /each/ region.
>> Since the regions are mapped in reverse order, it is highly unlikely
>> that each region starts at the same 2 MB relative alignment that the
>> previous region ended at, so you are likely wasting quite a bit of VA
>> space.
>>
>> I don't think it is a bug, though, but it does not seem intentional.
>
> Yeah, that's a very good catch. The existing code, that is, top-down
> allocation scheme where we map ealier EFI memmap entries at higher
> virtual addresses, does incur quite a bit of wasted address space.
>
> That's not true of this patch, though, and it's also not true if we
> map the entries in reverse order of the EFI memmap, that is, mapping
> the last memmap entry at the highest virtual address.
>
> So it's a bug in the original code, or rather an unintended feature.
>

Indeed. It does deserve a mention, since the point of this patch is to
prevent reordering and/or rounding up of regions.

> Ard, based on your suggestion I cooked this patch up to show what
> iterating the EFI memmap in reverse looks like in terms of code. The
> below diff and the original patch from this thread give me identical
> virtual address space layouts.
>

Good, as expected.

> Admittedly the below is missing a whole bunch of comments so makes the
> diff look smaller, but something like this could work,
>
> ---
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 691b333e0038..a2af35f6093a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -704,6 +704,44 @@ out:
>         return ret;
>  }
>
> +static inline void *efi_map_next_entry_reverse(void *entry)
> +{
> +       if (!entry)
> +               return memmap.map_end - memmap.desc_size;
> +
> +       entry -= memmap.desc_size;
> +       if (entry < memmap.map)
> +               return NULL;
> +
> +       return entry;
> +}
> +
> +static void *efi_map_next_entry(void *entry)
> +{
> +       bool reverse = false;
> +
> +       if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {

Here, you could also test whether the
EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA bit
(sigh) is set

> +               /*
> +                * Iterate the EFI memory map in reverse order because
> +                * the regions will be mapped top-down. The end result
> +                * is the same as if we had mapped things forward, but
> +                * doesn't require us to change the implementation of
> +                * efi_map_region().
> +                */
> +               return efi_map_next_entry_reverse(entry);
> +       }
> +
> +       /* Initial call */
> +       if (!entry)
> +               return memmap.map;
> +
> +       entry += memmap.desc_size;
> +       if (entry >= memmap.map_end)
> +               return NULL;
> +
> +       return entry;
> +}
> +
>  /*
>   * Map the efi memory ranges of the runtime services and update new_mmap with
>   * virtual addresses.
> @@ -718,7 +756,8 @@ static void * __init efi_map_regions(int *count, int *pg_shift)
>         start = -1UL;
>         end = 0;
>
> -       for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> +       p = NULL;
> +       while ((p = efi_map_next_entry(p))) {
>                 md = p;
>                 if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
>  #ifdef CONFIG_X86_64
>
> --
> Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221349

On Wed, 09 Sep, at 09:37:21AM, Ard Biesheuvel wrote:
> On 8 September 2015 at 22:37, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> >
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 691b333e0038..a2af35f6093a 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -704,6 +704,44 @@ out:
> >         return ret;
> >  }
> >
> > +static inline void *efi_map_next_entry_reverse(void *entry)
> > +{
> > +       if (!entry)
> > +               return memmap.map_end - memmap.desc_size;
> > +
> > +       entry -= memmap.desc_size;
> > +       if (entry < memmap.map)
> > +               return NULL;
> > +
> > +       return entry;
> > +}
> > +
> > +static void *efi_map_next_entry(void *entry)
> > +{
> > +       bool reverse = false;
> > +
> > +       if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
> 
> Here, you could also test whether the
> EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA bit
> (sigh) is set

No, leaving this out was intentional because we're already suffering
from the combinatoral explosion of config options. Introducing more
code paths is very much the wrong thing to do unless absolutely
necessary.

If we can get away with using one mapping scheme here, we should.

When trying to debug this code in the future I do not want to be
thinking "Do you have EFI_PROPERTIES_RUNTIME_OMG_THIS_IS_SILLY bit
set? because that means we're mapping the runtime regions in a
different order".
 
-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221353

On 9 September 2015 at 11:58, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 09 Sep, at 09:37:21AM, Ard Biesheuvel wrote:
>> On 8 September 2015 at 22:37, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> >
>> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> > index 691b333e0038..a2af35f6093a 100644
>> > --- a/arch/x86/platform/efi/efi.c
>> > +++ b/arch/x86/platform/efi/efi.c
>> > @@ -704,6 +704,44 @@ out:
>> >         return ret;
>> >  }
>> >
>> > +static inline void *efi_map_next_entry_reverse(void *entry)
>> > +{
>> > +       if (!entry)
>> > +               return memmap.map_end - memmap.desc_size;
>> > +
>> > +       entry -= memmap.desc_size;
>> > +       if (entry < memmap.map)
>> > +               return NULL;
>> > +
>> > +       return entry;
>> > +}
>> > +
>> > +static void *efi_map_next_entry(void *entry)
>> > +{
>> > +       bool reverse = false;
>> > +
>> > +       if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_64BIT)) {
>>
>> Here, you could also test whether the
>> EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA bit
>> (sigh) is set
>
> No, leaving this out was intentional because we're already suffering
> from the combinatoral explosion of config options. Introducing more
> code paths is very much the wrong thing to do unless absolutely
> necessary.
>
> If we can get away with using one mapping scheme here, we should.
>
> When trying to debug this code in the future I do not want to be
> thinking "Do you have EFI_PROPERTIES_RUNTIME_OMG_THIS_IS_SILLY bit
> set? because that means we're mapping the runtime regions in a
> different order".
>

OK, point taken. I suppose buggy firmware already has the option of
using EFI_OLD_MEMMAP as a fallback.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web