Re: [PATCH 1/2] firmware/efi: Tell memblock about EFI reservations

[Marc Zyngier <maz@kernel.org>]

On Thu, 13 May 2021 04:20:21 +0100,
Dave Young <dyoung@redhat.com> wrote:
> 
> On 05/03/21 at 11:56am, Moritz Fischer wrote:
> > Marc,
> > 
> > On Thu, Apr 29, 2021 at 02:35:32PM +0100, Marc Zyngier wrote:
> > > kexec_load_file() relies on the memblock infrastructure to avoid
> > > stamping over regions of memory that are essential to the survival
> > > of the system.
> > > 
> > > However, nobody seems to agree how to flag these regions as reserved,
> > > and (for example) EFI only publishes its reservations in /proc/iomem
> > > for the benefit of the traditional, userspace based kexec tool.
> > > 
> > > On arm64 platforms with GICv3, this can result in the payload being
> > > placed at the location of the LPI tables. Shock, horror!
> > > 
> > > Let's augment the EFI reservation code with a memblock_reserve() call,
> > > protecting our dear tables from the secondary kernel invasion.
> > > 
> > > At some point, someone will have to go and figure out a way to unify
> > > these multiple reservation trees, because sprinkling random reservation
> > > calls is only a temporary workaround.
> > > 
> > 
> > Feel free to add (and/or):
> > 
> > Reported-by: Moritz Fischer <mdf@kernel.org>
> > Tested-by: Moritz Fischer <mdf@kernel.org>
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  drivers/firmware/efi/efi.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 4b7ee3fa9224..026b02f5f7d8 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -896,11 +896,25 @@ static int __init efi_memreserve_map_root(void)
> > >  static int efi_mem_reserve_iomem(phys_addr_t addr, u64 size)
> > >  {
> > >  	struct resource *res, *parent;
> > > +	int ret;
> > >  
> > >  	res = kzalloc(sizeof(struct resource), GFP_ATOMIC);
> > >  	if (!res)
> > >  		return -ENOMEM;
> > >  
> > > +	/*
> > > +	 * Given that efi_mem_reserve_iomem() can be called at any
> > > +	 * time, only call memblock_reserve() if the architecture
> > > +	 * keeps the infrastructure around.
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK)) {
> > > +		ret = memblock_reserve(addr, size);
> > > +		if (ret) {
> > > +			kfree(res);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> 
> If you go with memblock, it would be better to handle it separately from
> the iomem?

Do you mean having a separate helper from efi_mem_reserve_iomem()?
Sure, can do.

> 
> > >  	res->name	= "reserved";
> > >  	res->flags	= IORESOURCE_MEM;
> > >  	res->start	= addr;
> > > @@ -908,7 +922,14 @@ static int efi_mem_reserve_iomem(phys_addr_t addr, u64 size)
> > >  
> > >  	/* we expect a conflict with a 'System RAM' region */
> > >  	parent = request_resource_conflict(&iomem_resource, res);
> > > -	return parent ? request_resource(parent, res) : 0;
> > > +	ret = parent ? request_resource(parent, res) : 0;
> > > +	if (ret) {
> > > +		kfree(res);
> > > +		if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
> > > +			memblock_free(addr, size);
> > > +	}
> > > +
> > > +	return ret;
> 
> It looks odd to free memblock when reqeust resource fails, they are not
> relavant?

I'm trying to keep the two trees in sync so that when the caller finds
out that the reservation has failed, we're not in a half-baked state.

But maybe it doesn't really matter, and if a reservation fails, we're
already screwed.

Ard, what do you think?

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel