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

Groups > linux.kernel > #1602242 > unrolled thread

Re: kexec regression since 4.9 caused by efi

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: kexec regression since 4.9 caused by efi Matt Fleming <matt@codeblueprint.co.uk> - 2017-03-16 13:50 +0100
    Re: kexec regression since 4.9 caused by efi Omar Sandoval <osandov@osandov.com> - 2017-03-16 19:00 +0100
    Re: kexec regression since 4.9 caused by efi Dave Young <dyoung@redhat.com> - 2017-03-17 03:30 +0100
      Re: kexec regression since 4.9 caused by efi Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-17 14:30 +0100
      Re: kexec regression since 4.9 caused by efi Matt Fleming <matt@codeblueprint.co.uk> - 2017-03-17 14:40 +0100
        Re: kexec regression since 4.9 caused by efi Dave Young <dyoung@redhat.com> - 2017-03-20 03:20 +0100
          Re: kexec regression since 4.9 caused by efi Dave Young <dyoung@redhat.com> - 2017-03-21 08:50 +0100
            Re: kexec regression since 4.9 caused by efi Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-03-22 17:20 +0100
              Re: kexec regression since 4.9 caused by efi Dave Young <dyoung@redhat.com> - 2017-03-23 03:50 +0100

#1602242 — Re: kexec regression since 4.9 caused by efi

On Mon, 13 Mar, at 03:37:48PM, Dave Young wrote:
> 
> Omar, could you try below patch? Looking at the efi_mem_desc_lookup, it is not
> correct to be used in efi_arch_mem_reserve, if it passed your test, I
> can rewrite patch log with more background and send it out:
> 
>         for_each_efi_memory_desc(md) {
> 		[snip]
>                 if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>                     md->type != EFI_BOOT_SERVICES_DATA &&
>                     md->type != EFI_RUNTIME_SERVICES_DATA) {
>                         continue;
>                 }
> 
> In above code, it meant to get a md of EFI_MEMORY_RUNTIME of either boot
> data or runtime data, this is wrong for efi_mem_reserve, because we are
> reserving boot data which has no EFI_MEMORY_RUNTIME attribute at the
> running time. Just is happened to work and we did not capture the error.

Wouldn't something like this be simpler?

---

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 30031d5293c4..cdfe8c628959 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -201,6 +201,10 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
 		return;
 	}
 
+	/* No need to reserve regions that will never be freed. */
+	if (md.attribute & EFI_MEMORY_RUNTIME)
+		return;
+
 	size += addr % EFI_PAGE_SIZE;
 	size = round_up(size, EFI_PAGE_SIZE);
 	addr = round_down(addr, EFI_PAGE_SIZE);

[toc] | [next] | [standalone]

#1602721

On Thu, Mar 16, 2017 at 12:41:32PM +0000, Matt Fleming wrote:
> On Mon, 13 Mar, at 03:37:48PM, Dave Young wrote:
> > 
> > Omar, could you try below patch? Looking at the efi_mem_desc_lookup, it is not
> > correct to be used in efi_arch_mem_reserve, if it passed your test, I
> > can rewrite patch log with more background and send it out:
> > 
> >         for_each_efi_memory_desc(md) {
> > 		[snip]
> >                 if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> >                     md->type != EFI_BOOT_SERVICES_DATA &&
> >                     md->type != EFI_RUNTIME_SERVICES_DATA) {
> >                         continue;
> >                 }
> > 
> > In above code, it meant to get a md of EFI_MEMORY_RUNTIME of either boot
> > data or runtime data, this is wrong for efi_mem_reserve, because we are
> > reserving boot data which has no EFI_MEMORY_RUNTIME attribute at the
> > running time. Just is happened to work and we did not capture the error.
> 
> Wouldn't something like this be simpler?
> 
> ---
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 30031d5293c4..cdfe8c628959 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -201,6 +201,10 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>  		return;
>  	}
>  
> +	/* No need to reserve regions that will never be freed. */
> +	if (md.attribute & EFI_MEMORY_RUNTIME)
> +		return;
> +
>  	size += addr % EFI_PAGE_SIZE;
>  	size = round_up(size, EFI_PAGE_SIZE);
>  	addr = round_down(addr, EFI_PAGE_SIZE);

This works for me.

Reported-and-tested-by: Omar Sandoval <osandov@fb.com>

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

#1602996

On 03/16/17 at 12:41pm, Matt Fleming wrote:
> On Mon, 13 Mar, at 03:37:48PM, Dave Young wrote:
> > 
> > Omar, could you try below patch? Looking at the efi_mem_desc_lookup, it is not
> > correct to be used in efi_arch_mem_reserve, if it passed your test, I
> > can rewrite patch log with more background and send it out:
> > 
> >         for_each_efi_memory_desc(md) {
> > 		[snip]
> >                 if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> >                     md->type != EFI_BOOT_SERVICES_DATA &&
> >                     md->type != EFI_RUNTIME_SERVICES_DATA) {
> >                         continue;
> >                 }
> > 
> > In above code, it meant to get a md of EFI_MEMORY_RUNTIME of either boot
> > data or runtime data, this is wrong for efi_mem_reserve, because we are
> > reserving boot data which has no EFI_MEMORY_RUNTIME attribute at the
> > running time. Just is happened to work and we did not capture the error.
> 
> Wouldn't something like this be simpler?
> 
> ---
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 30031d5293c4..cdfe8c628959 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -201,6 +201,10 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>  		return;
>  	}
>  
> +	/* No need to reserve regions that will never be freed. */
> +	if (md.attribute & EFI_MEMORY_RUNTIME)
> +		return;
> +

Matt, I think it should be fine although I think the md type checking in
efi_mem_desc_lookup() is causing confusion and not easy to understand..

How about move the if chunk early like below because it seems no need
to sanity check the addr + size any more if the md is still RUNTIME?

--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -196,6 +196,10 @@ void __init efi_arch_mem_reserve(phys_ad
 		return;
 	}
 
+	/* No need to reserve regions that will never be freed. */
+	if (md.attribute & EFI_MEMORY_RUNTIME)
+		return;
+
 	if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
 		pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
 		return;

Thanks
Dave

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

#1603306

On 17 March 2017 at 02:09, Dave Young <dyoung@redhat.com> wrote:
> On 03/16/17 at 12:41pm, Matt Fleming wrote:
>> On Mon, 13 Mar, at 03:37:48PM, Dave Young wrote:
>> >
>> > Omar, could you try below patch? Looking at the efi_mem_desc_lookup, it is not
>> > correct to be used in efi_arch_mem_reserve, if it passed your test, I
>> > can rewrite patch log with more background and send it out:
>> >
>> >         for_each_efi_memory_desc(md) {
>> >             [snip]
>> >                 if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>> >                     md->type != EFI_BOOT_SERVICES_DATA &&
>> >                     md->type != EFI_RUNTIME_SERVICES_DATA) {
>> >                         continue;
>> >                 }
>> >
>> > In above code, it meant to get a md of EFI_MEMORY_RUNTIME of either boot
>> > data or runtime data, this is wrong for efi_mem_reserve, because we are
>> > reserving boot data which has no EFI_MEMORY_RUNTIME attribute at the
>> > running time. Just is happened to work and we did not capture the error.
>>
>> Wouldn't something like this be simpler?
>>
>> ---
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 30031d5293c4..cdfe8c628959 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -201,6 +201,10 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>               return;
>>       }
>>
>> +     /* No need to reserve regions that will never be freed. */
>> +     if (md.attribute & EFI_MEMORY_RUNTIME)
>> +             return;
>> +
>
> Matt, I think it should be fine although I think the md type checking in
> efi_mem_desc_lookup() is causing confusion and not easy to understand..
>
> How about move the if chunk early like below because it seems no need
> to sanity check the addr + size any more if the md is still RUNTIME?
>
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -196,6 +196,10 @@ void __init efi_arch_mem_reserve(phys_ad
>                 return;
>         }
>
> +       /* No need to reserve regions that will never be freed. */
> +       if (md.attribute & EFI_MEMORY_RUNTIME)
> +               return;
> +
>         if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
>                 pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
>                 return;
>

This way, we suppress the error it the region spans multiple
descriptors, and only the first one has the runtime attribute. So the
two patches are not equivalent. I don't have a strong preference
either way, though.

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

#1603313

On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
> 
> Matt, I think it should be fine although I think the md type checking in
> efi_mem_desc_lookup() is causing confusion and not easy to understand..
 
Could you make that a separate patch if you think of improvements
there?

> How about move the if chunk early like below because it seems no need
> to sanity check the addr + size any more if the md is still RUNTIME?

My original version did as you suggest, but I changed it because we
*really* want to know if someone tries to reserve a range that spans
regions. That would be totally unexpected and a warning about a
potential bug/issue.

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

#1604142

On 03/17/17 at 01:32pm, Matt Fleming wrote:
> On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
> > 
> > Matt, I think it should be fine although I think the md type checking in
> > efi_mem_desc_lookup() is causing confusion and not easy to understand..
>  
> Could you make that a separate patch if you think of improvements
> there?

Duplicate the lookup function is indeed a little ugly, will do it when I
have a better idea, we can leave it as is since it works.

> 
> > How about move the if chunk early like below because it seems no need
> > to sanity check the addr + size any more if the md is still RUNTIME?
> 
> My original version did as you suggest, but I changed it because we
> *really* want to know if someone tries to reserve a range that spans
> regions. That would be totally unexpected and a warning about a
> potential bug/issue.

Matt, I'm fine if you prefer to capture the range checking errors.
Would you like me to post it or just you send it out?

Thanks
Dave

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

#1605379

On 03/20/17 at 10:14am, Dave Young wrote:
> On 03/17/17 at 01:32pm, Matt Fleming wrote:
> > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
> > > 
> > > Matt, I think it should be fine although I think the md type checking in
> > > efi_mem_desc_lookup() is causing confusion and not easy to understand..
> >  
> > Could you make that a separate patch if you think of improvements
> > there?
> 
> Duplicate the lookup function is indeed a little ugly, will do it when I
> have a better idea, we can leave it as is since it works.

Matt, rethinking about this, how about doint something below, not
tested, just seeking for idea and opinons, in this way no need duplicate
a function, but there is an assumption that no overlapped mem ranges in
efi memmap.

Also the case Omar reported is the esrt memory range type is
RUNTIME_DATA, that is a little different with the mem attribute of
RUNTIME which also includes BOOT_DATA which has been set the RUNTIME
attribute, like bgrt in kexec reboot. Should we distinguish the two
cases and give out some warnings or debug info?


---
 arch/x86/platform/efi/quirks.c |    5 +++++
 drivers/firmware/efi/efi.c     |    6 ------
 drivers/firmware/efi/esrt.c    |    7 +++++++
 3 files changed, 12 insertions(+), 6 deletions(-)

--- linux-x86.orig/drivers/firmware/efi/efi.c
+++ linux-x86/drivers/firmware/efi/efi.c
@@ -376,12 +376,6 @@ int __init efi_mem_desc_lookup(u64 phys_
 		u64 size;
 		u64 end;
 
-		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
-		    md->type != EFI_BOOT_SERVICES_DATA &&
-		    md->type != EFI_RUNTIME_SERVICES_DATA) {
-			continue;
-		}
-
 		size = md->num_pages << EFI_PAGE_SHIFT;
 		end = md->phys_addr + size;
 		if (phys_addr >= md->phys_addr && phys_addr < end) {
--- linux-x86.orig/drivers/firmware/efi/esrt.c
+++ linux-x86/drivers/firmware/efi/esrt.c
@@ -258,6 +258,13 @@ void __init efi_esrt_init(void)
 		return;
 	}
 
+	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
+	      md->type != EFI_BOOT_SERVICES_DATA &&
+	      md->type != EFI_RUNTIME_SERVICES_DATA) {
+		pr_err("ESRT header memory map type is invalid\n");
+		return;
+	}
+
 	max = efi_mem_desc_end(&md);
 	if (max < efi.esrt) {
 		pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -201,6 +201,11 @@ void __init efi_arch_mem_reserve(phys_ad
 		return;
 	}
 
+	if (md->attribute & EFI_MEMORY_RUNTIME ||
+	      md->type != EFI_BOOT_SERVICES_DATA) {
+		return;
+	}
+
 	size += addr % EFI_PAGE_SIZE;
 	size = round_up(size, EFI_PAGE_SIZE);
 	addr = round_down(addr, EFI_PAGE_SIZE);

> 
> > 
> > > How about move the if chunk early like below because it seems no need
> > > to sanity check the addr + size any more if the md is still RUNTIME?
> > 
> > My original version did as you suggest, but I changed it because we
> > *really* want to know if someone tries to reserve a range that spans
> > regions. That would be totally unexpected and a warning about a
> > potential bug/issue.
> 
> Matt, I'm fine if you prefer to capture the range checking errors.
> Would you like me to post it or just you send it out?
> 
> Thanks
> Dave

Thanks
Dave

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

#1606712

On 21 March 2017 at 07:48, Dave Young <dyoung@redhat.com> wrote:
> On 03/20/17 at 10:14am, Dave Young wrote:
>> On 03/17/17 at 01:32pm, Matt Fleming wrote:
>> > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
>> > >
>> > > Matt, I think it should be fine although I think the md type checking in
>> > > efi_mem_desc_lookup() is causing confusion and not easy to understand..
>> >
>> > Could you make that a separate patch if you think of improvements
>> > there?
>>
>> Duplicate the lookup function is indeed a little ugly, will do it when I
>> have a better idea, we can leave it as is since it works.
>
> Matt, rethinking about this, how about doint something below, not
> tested, just seeking for idea and opinons, in this way no need duplicate
> a function, but there is an assumption that no overlapped mem ranges in
> efi memmap.
>
> Also the case Omar reported is the esrt memory range type is
> RUNTIME_DATA, that is a little different with the mem attribute of
> RUNTIME which also includes BOOT_DATA which has been set the RUNTIME
> attribute, like bgrt in kexec reboot. Should we distinguish the two
> cases and give out some warnings or debug info?
>
>
> ---
>  arch/x86/platform/efi/quirks.c |    5 +++++
>  drivers/firmware/efi/efi.c     |    6 ------
>  drivers/firmware/efi/esrt.c    |    7 +++++++
>  3 files changed, 12 insertions(+), 6 deletions(-)
>
> --- linux-x86.orig/drivers/firmware/efi/efi.c
> +++ linux-x86/drivers/firmware/efi/efi.c
> @@ -376,12 +376,6 @@ int __init efi_mem_desc_lookup(u64 phys_
>                 u64 size;
>                 u64 end;
>
> -               if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> -                   md->type != EFI_BOOT_SERVICES_DATA &&
> -                   md->type != EFI_RUNTIME_SERVICES_DATA) {
> -                       continue;
> -               }
> -
>                 size = md->num_pages << EFI_PAGE_SHIFT;
>                 end = md->phys_addr + size;
>                 if (phys_addr >= md->phys_addr && phys_addr < end) {
> --- linux-x86.orig/drivers/firmware/efi/esrt.c
> +++ linux-x86/drivers/firmware/efi/esrt.c
> @@ -258,6 +258,13 @@ void __init efi_esrt_init(void)
>                 return;
>         }
>
> +       if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> +             md->type != EFI_BOOT_SERVICES_DATA &&
> +             md->type != EFI_RUNTIME_SERVICES_DATA) {
> +               pr_err("ESRT header memory map type is invalid\n");
> +               return;
> +       }
> +

This looks wrong to me. While the meanings get convoluted in practice,
the EFI_MEMORY_RUNTIME attribute only means that the firmware requests
a virtual mapping for the region. It is perfectly legal for a
EFI_RUNTIME_SERVICES_DATA region not to have the EFI_MEMORY_RUNTIME
attribute, if the region is never accessed by the runtime services
themselves, and this is not entirely unlikely for tables that the
firmware exposes to the OS

>         max = efi_mem_desc_end(&md);
>         if (max < efi.esrt) {
>                 pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -201,6 +201,11 @@ void __init efi_arch_mem_reserve(phys_ad
>                 return;
>         }
>
> +       if (md->attribute & EFI_MEMORY_RUNTIME ||
> +             md->type != EFI_BOOT_SERVICES_DATA) {
> +               return;
> +       }
> +
>         size += addr % EFI_PAGE_SIZE;
>         size = round_up(size, EFI_PAGE_SIZE);
>         addr = round_down(addr, EFI_PAGE_SIZE);
>
>>
>> >
>> > > How about move the if chunk early like below because it seems no need
>> > > to sanity check the addr + size any more if the md is still RUNTIME?
>> >
>> > My original version did as you suggest, but I changed it because we
>> > *really* want to know if someone tries to reserve a range that spans
>> > regions. That would be totally unexpected and a warning about a
>> > potential bug/issue.
>>
>> Matt, I'm fine if you prefer to capture the range checking errors.
>> Would you like me to post it or just you send it out?
>>
>> Thanks
>> Dave
>
> Thanks
> Dave
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1607139

On 03/22/17 at 04:10pm, Ard Biesheuvel wrote:
> On 21 March 2017 at 07:48, Dave Young <dyoung@redhat.com> wrote:
> > On 03/20/17 at 10:14am, Dave Young wrote:
> >> On 03/17/17 at 01:32pm, Matt Fleming wrote:
> >> > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote:
> >> > >
> >> > > Matt, I think it should be fine although I think the md type checking in
> >> > > efi_mem_desc_lookup() is causing confusion and not easy to understand..
> >> >
> >> > Could you make that a separate patch if you think of improvements
> >> > there?
> >>
> >> Duplicate the lookup function is indeed a little ugly, will do it when I
> >> have a better idea, we can leave it as is since it works.
> >
> > Matt, rethinking about this, how about doint something below, not
> > tested, just seeking for idea and opinons, in this way no need duplicate
> > a function, but there is an assumption that no overlapped mem ranges in
> > efi memmap.
> >
> > Also the case Omar reported is the esrt memory range type is
> > RUNTIME_DATA, that is a little different with the mem attribute of
> > RUNTIME which also includes BOOT_DATA which has been set the RUNTIME
> > attribute, like bgrt in kexec reboot. Should we distinguish the two
> > cases and give out some warnings or debug info?
> >
> >
> > ---
> >  arch/x86/platform/efi/quirks.c |    5 +++++
> >  drivers/firmware/efi/efi.c     |    6 ------
> >  drivers/firmware/efi/esrt.c    |    7 +++++++
> >  3 files changed, 12 insertions(+), 6 deletions(-)
> >
> > --- linux-x86.orig/drivers/firmware/efi/efi.c
> > +++ linux-x86/drivers/firmware/efi/efi.c
> > @@ -376,12 +376,6 @@ int __init efi_mem_desc_lookup(u64 phys_
> >                 u64 size;
> >                 u64 end;
> >
> > -               if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> > -                   md->type != EFI_BOOT_SERVICES_DATA &&
> > -                   md->type != EFI_RUNTIME_SERVICES_DATA) {
> > -                       continue;
> > -               }
> > -
> >                 size = md->num_pages << EFI_PAGE_SHIFT;
> >                 end = md->phys_addr + size;
> >                 if (phys_addr >= md->phys_addr && phys_addr < end) {
> > --- linux-x86.orig/drivers/firmware/efi/esrt.c
> > +++ linux-x86/drivers/firmware/efi/esrt.c
> > @@ -258,6 +258,13 @@ void __init efi_esrt_init(void)
> >                 return;
> >         }
> >
> > +       if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> > +             md->type != EFI_BOOT_SERVICES_DATA &&
> > +             md->type != EFI_RUNTIME_SERVICES_DATA) {
> > +               pr_err("ESRT header memory map type is invalid\n");
> > +               return;
> > +       }
> > +
> 
> This looks wrong to me. While the meanings get convoluted in practice,
> the EFI_MEMORY_RUNTIME attribute only means that the firmware requests
> a virtual mapping for the region. It is perfectly legal for a
> EFI_RUNTIME_SERVICES_DATA region not to have the EFI_MEMORY_RUNTIME
> attribute, if the region is never accessed by the runtime services
> themselves, and this is not entirely unlikely for tables that the
> firmware exposes to the OS

Thanks for the comment, if so "!(md->attribute & EFI_MEMORY_RUNTIME) &&"
should be dropped.

BTW, md->type should be md.type, bgrt reserving works fine with this
change but I have no esrt machine to test it. I would like to wait for
Matt's opinions about this first before an update. 

Also cc Peter about the esrt piece.
> 
> >         max = efi_mem_desc_end(&md);
> >         if (max < efi.esrt) {
> >                 pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
> > --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> > +++ linux-x86/arch/x86/platform/efi/quirks.c
> > @@ -201,6 +201,11 @@ void __init efi_arch_mem_reserve(phys_ad
> >                 return;
> >         }
> >
> > +       if (md->attribute & EFI_MEMORY_RUNTIME ||
> > +             md->type != EFI_BOOT_SERVICES_DATA) {
> > +               return;
> > +       }
> > +
> >         size += addr % EFI_PAGE_SIZE;
> >         size = round_up(size, EFI_PAGE_SIZE);
> >         addr = round_down(addr, EFI_PAGE_SIZE);
> >
> >>
> >> >
> >> > > How about move the if chunk early like below because it seems no need
> >> > > to sanity check the addr + size any more if the md is still RUNTIME?
> >> >
> >> > My original version did as you suggest, but I changed it because we
> >> > *really* want to know if someone tries to reserve a range that spans
> >> > regions. That would be totally unexpected and a warning about a
> >> > potential bug/issue.
> >>
> >> Matt, I'm fine if you prefer to capture the range checking errors.
> >> Would you like me to post it or just you send it out?
> >>
> >> Thanks
> >> Dave
> >
> > Thanks
> > Dave
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web