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

Groups > linux.kernel > #1551795 > unrolled thread

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

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 v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Dave Young <dyoung@redhat.com> - 2017-01-05 10:20 +0100
    Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-09 12:50 +0100
      Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Mel Gorman <mgorman@techsingularity.net> - 2017-01-09 14:40 +0100
        Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-09 14:50 +0100
      Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Dave Young <dyoung@redhat.com> - 2017-01-10 01:40 +0100
        Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-10 14:00 +0100
          Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Dave Young <dyoung@redhat.com> - 2017-01-11 09:10 +0100

#1551795 — Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On 12/22/16 at 11:23am, Nicolai Stange wrote:
> Before invoking the arch specific handler, efi_mem_reserve() reserves
> the given memory region through memblock.
> 
> efi_mem_reserve() can get called after mm_init() though -- through
> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> not be used anymore.

It did not fail during previous test so we did not catch this bug, if memblock
can not be used after mm_init(), IMHO it should fail instead of silently succeed.

Matt, can we move the efi_mem_reserve to earlier code for example in
efi_memblock_x86_reserve_range just after reserving the memmap?

> 
> Let efi_mem_reserve() check whether memblock is dead and not do the
> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
> in this case: if the architecture doesn't provide any other means of
> registering the region as reserved, the operation would be a nop.
> 
> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
>  Changes to v1:
>  Change the if condition from slab_is_available() to !slab_is_available
>  as pointed out by Mika Penttilä at
>  http://lkml.kernel.org/r/c7bf34ba-56f0-8346-36d1-7069f2115dcf@nextfour.com
> 
>  drivers/firmware/efi/efi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 92914801e388..158a8df2f4af 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>  	return end;
>  }
>  
> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> +{
> +	WARN(slab_is_available(), "efi_mem_reserve() has no effect");
> +}
>  
>  /**
>   * efi_mem_reserve - Reserve an EFI memory region
> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>   */
>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>  {
> -	if (!memblock_is_region_reserved(addr, size))
> +	if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>  		memblock_reserve(addr, size);
>  
>  	/*
> -- 
> 2.11.0
> 
> --
> 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

Thanks
Dave

[toc] | [next] | [standalone]

#1554245

On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > the given memory region through memblock.
> > 
> > efi_mem_reserve() can get called after mm_init() though -- through
> > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > not be used anymore.
> 
> It did not fail during previous test so we did not catch this bug, if memblock
> can not be used after mm_init(), IMHO it should fail instead of silently succeed.
 
This must literally be the fifth time or so that I've been caught out
by this over the years because there's no hard error if you call the
memblock code after slab and co. are up.

MM folks, is there some way to catch these errors without requiring
the sprinkling of slab_is_available() everywhere?

> Matt, can we move the efi_mem_reserve to earlier code for example in
> efi_memblock_x86_reserve_range just after reserving the memmap?
 
No, it *needs* to be callable from efi_bgrt_init(), because you only
want to reserve those regions if you have the BGRT driver available.

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

#1554314

On Mon, Jan 09, 2017 at 11:44:00AM +0000, Matt Fleming wrote:
> On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> > On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > > the given memory region through memblock.
> > > 
> > > efi_mem_reserve() can get called after mm_init() though -- through
> > > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > > not be used anymore.
> > 
> > It did not fail during previous test so we did not catch this bug, if memblock
> > can not be used after mm_init(), IMHO it should fail instead of silently succeed.
>  
> This must literally be the fifth time or so that I've been caught out
> by this over the years because there's no hard error if you call the
> memblock code after slab and co. are up.
> 
> MM folks, is there some way to catch these errors without requiring
> the sprinkling of slab_is_available() everywhere?
> 

Well, you could put in a __init global variable about availability into
mm/memblock.c and then check it in memblock APIs like memblock_reserve()
to BUG_ON? I know BUG_ON is frowned upon but this is not likely to be a
situation that can be sensibly recovered.

-- 
Mel Gorman
SUSE Labs

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

#1554351

On Mon, 09 Jan, at 01:31:52PM, Mel Gorman wrote:
> 
> Well, you could put in a __init global variable about availability into
> mm/memblock.c and then check it in memblock APIs like memblock_reserve()
> to BUG_ON? I know BUG_ON is frowned upon but this is not likely to be a
> situation that can be sensibly recovered.

Indeed. I've only ever seen this situation lead to silent memory
corruption and bitter tears.

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

#1554826

On 01/09/17 at 11:44am, Matt Fleming wrote:
> On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> > On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > > the given memory region through memblock.
> > > 
> > > efi_mem_reserve() can get called after mm_init() though -- through
> > > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > > not be used anymore.
> > 
> > It did not fail during previous test so we did not catch this bug, if memblock
> > can not be used after mm_init(), IMHO it should fail instead of silently succeed.
>  
> This must literally be the fifth time or so that I've been caught out
> by this over the years because there's no hard error if you call the
> memblock code after slab and co. are up.
> 
> MM folks, is there some way to catch these errors without requiring
> the sprinkling of slab_is_available() everywhere?
> 
> > Matt, can we move the efi_mem_reserve to earlier code for example in
> > efi_memblock_x86_reserve_range just after reserving the memmap?
>  
> No, it *needs* to be callable from efi_bgrt_init(), because you only
> want to reserve those regions if you have the BGRT driver available.

It is true that it depends on acpi init, I was wondering if bgrt parsing can
be moved to early acpi code. But anyway I'm not sure it is doable and
worth.

Thanks
Dave 

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

#1555234

On Tue, 10 Jan, at 08:37:35AM, Dave Young wrote:
> 
> It is true that it depends on acpi init, I was wondering if bgrt parsing can
> be moved to early acpi code. But anyway I'm not sure it is doable and
> worth.

That's a good question. I think I gave up last time I tried to move
the BGRT code to early boot because of the dependencies involved with
having the ACPI table parsing code initialised.

But if you want to take a crack at it, I'd be happy to review the
patches.

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

#1556260

On 01/10/17 at 12:51pm, Matt Fleming wrote:
> On Tue, 10 Jan, at 08:37:35AM, Dave Young wrote:
> > 
> > It is true that it depends on acpi init, I was wondering if bgrt parsing can
> > be moved to early acpi code. But anyway I'm not sure it is doable and
> > worth.
> 
> That's a good question. I think I gave up last time I tried to move
> the BGRT code to early boot because of the dependencies involved with
> having the ACPI table parsing code initialised.
> 
> But if you want to take a crack at it, I'd be happy to review the
> patches.

Ok, I will have a try. 

Thanks
Dave

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web