[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Ard Biesheuvel]

This series addresses the kexec/kdump crash on arm64 system with many CPUs
that was reported by Bhupesh.

Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
EFI persistent memreserve infrastructure so that fewer memblock reservations
are required.

Ard Biesheuvel (4):
  arm64: memblock: don't permit memblock resizing until linear mapping
    is up
  efi/arm: defer persistent reservations until after paging_init()
  efi: permit multiple entries in persistent memreserve data structure
  efi: reduce the amount of memblock reservations for persistent
    allocations

 arch/arm/kernel/setup.c                 |  1 +
 arch/arm64/kernel/setup.c               |  1 +
 arch/arm64/mm/init.c                    |  2 -
 arch/arm64/mm/mmu.c                     |  2 +
 drivers/firmware/efi/efi.c              | 59 ++++++++++++++------
 drivers/firmware/efi/libstub/arm-stub.c |  2 +-
 include/linux/efi.h                     | 23 +++++++-
 7 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.19.1

[PATCH 1/4] arm64: memblock: don't permit memblock resizing until linear mapping is up

[Ard Biesheuvel]

Bhupesh reports that having numerous memblock reservations at early
boot may result in the following crash:

  Unable to handle kernel paging request at virtual address ffff80003ffe0000
  ...
  Call trace:
   __memcpy+0x110/0x180
   memblock_add_range+0x134/0x2e8
   memblock_reserve+0x70/0xb8
   memblock_alloc_base_nid+0x6c/0x88
   __memblock_alloc_base+0x3c/0x4c
   memblock_alloc_base+0x28/0x4c
   memblock_alloc+0x2c/0x38
   early_pgtable_alloc+0x20/0xb0
   paging_init+0x28/0x7f8

This is caused by the fact that we permit memblock resizing before the
linear mapping is up, and so the memblock_reserved() array is moved
into memory that is not mapped yet.

So let's ensure that this crash can no longer occur, by deferring to
call to memblock_allow_resize() to after the linear mapping has been
created.

Reported-by: Bhupesh Sharma <bhsharma@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/mm/init.c | 2 --
 arch/arm64/mm/mmu.c  | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 9d9582cac6c4..9b432d9fcada 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -483,8 +483,6 @@ void __init arm64_memblock_init(void)
 	high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
 
 	dma_contiguous_reserve(arm64_dma_phys_limit);
-
-	memblock_allow_resize();
 }
 
 void __init bootmem_init(void)
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 394b8d554def..d1d6601b385d 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -659,6 +659,8 @@ void __init paging_init(void)
 
 	memblock_free(__pa_symbol(init_pg_dir),
 		      __pa_symbol(init_pg_end) - __pa_symbol(init_pg_dir));
+
+	memblock_allow_resize();
 }
 
 /*
-- 
2.19.1

[PATCH 1/4] arm64: memblock: don't permit memblock resizing until linear mapping is up

[Will Deacon]

On Tue, Nov 06, 2018 at 12:37:29PM +0100, Ard Biesheuvel wrote:
> Bhupesh reports that having numerous memblock reservations at early
> boot may result in the following crash:
> 
>   Unable to handle kernel paging request at virtual address ffff80003ffe0000
>   ...
>   Call trace:
>    __memcpy+0x110/0x180
>    memblock_add_range+0x134/0x2e8
>    memblock_reserve+0x70/0xb8
>    memblock_alloc_base_nid+0x6c/0x88
>    __memblock_alloc_base+0x3c/0x4c
>    memblock_alloc_base+0x28/0x4c
>    memblock_alloc+0x2c/0x38
>    early_pgtable_alloc+0x20/0xb0
>    paging_init+0x28/0x7f8
> 
> This is caused by the fact that we permit memblock resizing before the
> linear mapping is up, and so the memblock_reserved() array is moved
> into memory that is not mapped yet.
> 
> So let's ensure that this crash can no longer occur, by deferring to
> call to memblock_allow_resize() to after the linear mapping has been
> created.
> 
> Reported-by: Bhupesh Sharma <bhsharma@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/mm/init.c | 2 --
>  arch/arm64/mm/mmu.c  | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)

Thanks for posting this so quickly.

Acked-by: Will Deacon <will.deacon@arm.com>

Bhupesh -- please can you give this series a spin and confirm that it fixes
the problem you were seeing?

Thanks,

Will

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Ard Biesheuvel]

The new memory EFI reservation feature we introduced to allow memory
reservations to persist across kexec may trigger an unbounded number
of calls to memblock_reserve(). The memblock subsystem can deal with
this fine, but not before memblock resizing is enabled, which we can
only do after paging_init(), when the memory we reallocate the array
into is actually mapped.

So break out the memreserve table processing into a separate function
and call if after paging_init() on both arm64 and ARM.

Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/kernel/setup.c    | 1 +
 arch/arm64/kernel/setup.c  | 1 +
 drivers/firmware/efi/efi.c | 8 ++++++--
 include/linux/efi.h        | 7 +++++++
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index ac7e08886863..e99f12eaf390 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -1117,6 +1117,7 @@ void __init setup_arch(char **cmdline_p)
 	early_ioremap_reset();
 
 	paging_init(mdesc);
+	efi_apply_persistent_mem_reservations();
 	request_standard_resources(mdesc);
 
 	if (mdesc->restart)
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 953e316521fc..f4fc1e0544b7 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -313,6 +313,7 @@ void __init setup_arch(char **cmdline_p)
 	arm64_memblock_init();
 
 	paging_init();
+	efi_apply_persistent_mem_reservations();
 
 	acpi_table_upgrade();
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 249eb70691b0..55e4ea20bdc3 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -592,7 +592,11 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 
 		early_memunmap(tbl, sizeof(*tbl));
 	}
+	return 0;
+}
 
+int __init efi_apply_persistent_mem_reservations(void)
+{
 	if (efi.mem_reserve != EFI_INVALID_TABLE_ADDR) {
 		unsigned long prsv = efi.mem_reserve;
 
@@ -602,7 +606,7 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 			/* reserve the entry itself */
 			memblock_reserve(prsv, sizeof(*rsv));
 
-			rsv = early_memremap(prsv, sizeof(*rsv));
+			rsv = memremap(prsv, sizeof(*rsv), MEMREMAP_WB);
 			if (rsv == NULL) {
 				pr_err("Could not map UEFI memreserve entry!\n");
 				return -ENOMEM;
@@ -612,7 +616,7 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 				memblock_reserve(rsv->base, rsv->size);
 
 			prsv = rsv->next;
-			early_memunmap(rsv, sizeof(*rsv));
+			memunmap(rsv);
 		}
 	}
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 845174e113ce..100ce4a4aff6 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1167,6 +1167,8 @@ static inline bool efi_enabled(int feature)
 extern void efi_reboot(enum reboot_mode reboot_mode, const char *__unused);
 
 extern bool efi_is_table_address(unsigned long phys_addr);
+
+extern int efi_apply_persistent_mem_reservations(void);
 #else
 static inline bool efi_enabled(int feature)
 {
@@ -1185,6 +1187,11 @@ static inline bool efi_is_table_address(unsigned long phys_addr)
 {
 	return false;
 }
+
+static inline int efi_apply_persistent_mem_reservations(void)
+{
+	return 0;
+}
 #endif
 
 extern int efi_status_to_err(efi_status_t status);
-- 
2.19.1

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Ard Biesheuvel]

On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The new memory EFI reservation feature we introduced to allow memory
> reservations to persist across kexec may trigger an unbounded number
> of calls to memblock_reserve(). The memblock subsystem can deal with
> this fine, but not before memblock resizing is enabled, which we can
> only do after paging_init(), when the memory we reallocate the array
> into is actually mapped.
>
> So break out the memreserve table processing into a separate function
> and call if after paging_init() on both arm64 and ARM.
>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Russell,

If you are ok with this patch, may I have your ack please? I would
like to send it out before the end of the week.

Thanks,

> ---
>  arch/arm/kernel/setup.c    | 1 +
>  arch/arm64/kernel/setup.c  | 1 +
>  drivers/firmware/efi/efi.c | 8 ++++++--
>  include/linux/efi.h        | 7 +++++++
>  4 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index ac7e08886863..e99f12eaf390 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -1117,6 +1117,7 @@ void __init setup_arch(char **cmdline_p)
>         early_ioremap_reset();
>
>         paging_init(mdesc);
> +       efi_apply_persistent_mem_reservations();
>         request_standard_resources(mdesc);
>
>         if (mdesc->restart)
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 953e316521fc..f4fc1e0544b7 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -313,6 +313,7 @@ void __init setup_arch(char **cmdline_p)
>         arm64_memblock_init();
>
>         paging_init();
> +       efi_apply_persistent_mem_reservations();
>
>         acpi_table_upgrade();
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 249eb70691b0..55e4ea20bdc3 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -592,7 +592,11 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>
>                 early_memunmap(tbl, sizeof(*tbl));
>         }
> +       return 0;
> +}
>
> +int __init efi_apply_persistent_mem_reservations(void)
> +{
>         if (efi.mem_reserve != EFI_INVALID_TABLE_ADDR) {
>                 unsigned long prsv = efi.mem_reserve;
>
> @@ -602,7 +606,7 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>                         /* reserve the entry itself */
>                         memblock_reserve(prsv, sizeof(*rsv));
>
> -                       rsv = early_memremap(prsv, sizeof(*rsv));
> +                       rsv = memremap(prsv, sizeof(*rsv), MEMREMAP_WB);
>                         if (rsv == NULL) {
>                                 pr_err("Could not map UEFI memreserve entry!\n");
>                                 return -ENOMEM;
> @@ -612,7 +616,7 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>                                 memblock_reserve(rsv->base, rsv->size);
>
>                         prsv = rsv->next;
> -                       early_memunmap(rsv, sizeof(*rsv));
> +                       memunmap(rsv);
>                 }
>         }
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 845174e113ce..100ce4a4aff6 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1167,6 +1167,8 @@ static inline bool efi_enabled(int feature)
>  extern void efi_reboot(enum reboot_mode reboot_mode, const char *__unused);
>
>  extern bool efi_is_table_address(unsigned long phys_addr);
> +
> +extern int efi_apply_persistent_mem_reservations(void);
>  #else
>  static inline bool efi_enabled(int feature)
>  {
> @@ -1185,6 +1187,11 @@ static inline bool efi_is_table_address(unsigned long phys_addr)
>  {
>         return false;
>  }
> +
> +static inline int efi_apply_persistent_mem_reservations(void)
> +{
> +       return 0;
> +}
>  #endif
>
>  extern int efi_status_to_err(efi_status_t status);
> --
> 2.19.1
>

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Russell King - ARM Linux]

On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > The new memory EFI reservation feature we introduced to allow memory
> > reservations to persist across kexec may trigger an unbounded number
> > of calls to memblock_reserve(). The memblock subsystem can deal with
> > this fine, but not before memblock resizing is enabled, which we can
> > only do after paging_init(), when the memory we reallocate the array
> > into is actually mapped.
> >
> > So break out the memreserve table processing into a separate function
> > and call if after paging_init() on both arm64 and ARM.
> >
> > Cc: Russell King <linux@armlinux.org.uk>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Russell,
> 
> If you are ok with this patch, may I have your ack please? I would
> like to send it out before the end of the week.

You're not going to get a quick answer to this, because it needs me to
investigate what the effect of this change actually is by code review.
I can't guarantee when I'll get around to that.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Ard Biesheuvel]

On 6 November 2018 at 20:08, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
>> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > The new memory EFI reservation feature we introduced to allow memory
>> > reservations to persist across kexec may trigger an unbounded number
>> > of calls to memblock_reserve(). The memblock subsystem can deal with
>> > this fine, but not before memblock resizing is enabled, which we can
>> > only do after paging_init(), when the memory we reallocate the array
>> > into is actually mapped.
>> >
>> > So break out the memreserve table processing into a separate function
>> > and call if after paging_init() on both arm64 and ARM.
>> >
>> > Cc: Russell King <linux@armlinux.org.uk>
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Russell,
>>
>> If you are ok with this patch, may I have your ack please? I would
>> like to send it out before the end of the week.
>
> You're not going to get a quick answer to this, because it needs me to
> investigate what the effect of this change actually is by code review.
> I can't guarantee when I'll get around to that.
>

Fair enough.

I will drop the ARM hunk for now then, and we'll fix ARM when you have
more time.

Thanks,

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Russell King - ARM Linux]

On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
> On 6 November 2018 at 20:08, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
> >> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> > The new memory EFI reservation feature we introduced to allow memory
> >> > reservations to persist across kexec may trigger an unbounded number
> >> > of calls to memblock_reserve(). The memblock subsystem can deal with
> >> > this fine, but not before memblock resizing is enabled, which we can
> >> > only do after paging_init(), when the memory we reallocate the array
> >> > into is actually mapped.
> >> >
> >> > So break out the memreserve table processing into a separate function
> >> > and call if after paging_init() on both arm64 and ARM.
> >> >
> >> > Cc: Russell King <linux@armlinux.org.uk>
> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> Russell,
> >>
> >> If you are ok with this patch, may I have your ack please? I would
> >> like to send it out before the end of the week.
> >
> > You're not going to get a quick answer to this, because it needs me to
> > investigate what the effect of this change actually is by code review.
> > I can't guarantee when I'll get around to that.
> >
> 
> Fair enough.
> 
> I will drop the ARM hunk for now then, and we'll fix ARM when you have
> more time.

I don't see how you can do that - you're dropping the processing of
reserved areas out of the efi_config_parse_tables() path, so that
won't happen any more on ARM if you don't apply the ARM hunk.

So what I see at the moment is that efi_config_parse_tables() is
called prior to paging_init() - and prior to our memblock
initialisation, and you're proposing to move the processing that
marks blocks as reserved immediately after paging_init(), and
to use the normal memremap() interface to map stuff.

I'm not convinced this will work - the memory allocators have not
been setup at this point, so using memremap() will try to allocate
page tables and potentially fail.  If the normal allocators are
setup, it's way too late to be calling memblock_reserve() - the
memory will have been passed over to the page allocator according
to which memblocks are available but not reserved.

The normal page allocator is setup by mem_init(), which happens
way after setup_arch() has returned.

So, I don't see how your patch can be correct at the moment.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Marc Zyngier]

On 06/11/18 23:49, Russell King - ARM Linux wrote:
> On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
>> On 6 November 2018 at 20:08, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>>> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
>>>> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>> The new memory EFI reservation feature we introduced to allow memory
>>>>> reservations to persist across kexec may trigger an unbounded number
>>>>> of calls to memblock_reserve(). The memblock subsystem can deal with
>>>>> this fine, but not before memblock resizing is enabled, which we can
>>>>> only do after paging_init(), when the memory we reallocate the array
>>>>> into is actually mapped.
>>>>>
>>>>> So break out the memreserve table processing into a separate function
>>>>> and call if after paging_init() on both arm64 and ARM.
>>>>>
>>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>
>>>> Russell,
>>>>
>>>> If you are ok with this patch, may I have your ack please? I would
>>>> like to send it out before the end of the week.
>>>
>>> You're not going to get a quick answer to this, because it needs me to
>>> investigate what the effect of this change actually is by code review.
>>> I can't guarantee when I'll get around to that.
>>>
>>
>> Fair enough.
>>
>> I will drop the ARM hunk for now then, and we'll fix ARM when you have
>> more time.
> 
> I don't see how you can do that - you're dropping the processing of
> reserved areas out of the efi_config_parse_tables() path, so that
> won't happen any more on ARM if you don't apply the ARM hunk.

The only reason for the whole persistent region thing is to support 
GICv3 redistributors memory tables across kexec, for those GIC 
implementations where LPIs cannot be disabled once they have been 
enabled.

There is no HW platform that combines 32bit cores and GICv3 (not to 
mention using EFI). The only existing "platform" is KVM/arm64, which 
doesn't suffer from the above problem (LPIs can be freely disabled in 
emulation). So the whole persistent region feature serves no purpose on 
32bit ARM.

The only issue with dropping this hunk is that the memory reservation 
feature is enabled on 32bit ARM the first place, which can easily be 
dealt with something along those lines:

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 787d7850e064..8bb263c9b99a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1290,6 +1290,7 @@ config EFI
 	select EFI_RUNTIME_WRAPPERS
 	select EFI_STUB
 	select EFI_ARMSTUB
+	select EFI_PERSISTENT_RESERVATIONS
 	default y
 	help
 	  This option provides support for runtime services provided
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 89110dfc7127..b728dab9e901 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -198,3 +198,6 @@ config EFI_DEV_PATH_PARSER
 	bool
 	depends on ACPI
 	default n
+
+config EFI_PERSISTENT_RESERVATIONS
+       bool
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 249eb70691b0..2a1903ab6d21 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -962,6 +962,7 @@ bool efi_is_table_address(unsigned long phys_addr)
 	return false;
 }
 
+#ifdef CONFIG_EFI_PERSISTENT_RESERVATIONS
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
@@ -993,6 +994,12 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 
 	return 0;
 }
+#else
+int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
+{
+	return 0;
+}
+#endif
 
 #ifdef CONFIG_KEXEC
 static int update_efi_random_seed(struct notifier_block *nb,

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Russell King - ARM Linux]

On Wed, Nov 07, 2018 at 09:51:09AM +0000, Marc Zyngier wrote:
> On 06/11/18 23:49, Russell King - ARM Linux wrote:
> > On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
> >> On 6 November 2018 at 20:08, Russell King - ARM Linux
> >> <linux@armlinux.org.uk> wrote:
> >>> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
> >>>> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>>>> The new memory EFI reservation feature we introduced to allow memory
> >>>>> reservations to persist across kexec may trigger an unbounded number
> >>>>> of calls to memblock_reserve(). The memblock subsystem can deal with
> >>>>> this fine, but not before memblock resizing is enabled, which we can
> >>>>> only do after paging_init(), when the memory we reallocate the array
> >>>>> into is actually mapped.
> >>>>>
> >>>>> So break out the memreserve table processing into a separate function
> >>>>> and call if after paging_init() on both arm64 and ARM.
> >>>>>
> >>>>> Cc: Russell King <linux@armlinux.org.uk>
> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>>
> >>>> Russell,
> >>>>
> >>>> If you are ok with this patch, may I have your ack please? I would
> >>>> like to send it out before the end of the week.
> >>>
> >>> You're not going to get a quick answer to this, because it needs me to
> >>> investigate what the effect of this change actually is by code review.
> >>> I can't guarantee when I'll get around to that.
> >>>
> >>
> >> Fair enough.
> >>
> >> I will drop the ARM hunk for now then, and we'll fix ARM when you have
> >> more time.
> > 
> > I don't see how you can do that - you're dropping the processing of
> > reserved areas out of the efi_config_parse_tables() path, so that
> > won't happen any more on ARM if you don't apply the ARM hunk.
> 
> The only reason for the whole persistent region thing is to support 
> GICv3 redistributors memory tables across kexec, for those GIC 
> implementations where LPIs cannot be disabled once they have been 
> enabled.
> 
> There is no HW platform that combines 32bit cores and GICv3 (not to 
> mention using EFI). The only existing "platform" is KVM/arm64, which 
> doesn't suffer from the above problem (LPIs can be freely disabled in 
> emulation). So the whole persistent region feature serves no purpose on 
> 32bit ARM.
> 
> The only issue with dropping this hunk is that the memory reservation 
> feature is enabled on 32bit ARM the first place, which can easily be 
> dealt with something along those lines:

So, it was a waste of my time trying to understand what's going on here
with Ard's patch because it shouldn't be present on 32-bit ARM... Grr.

> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 787d7850e064..8bb263c9b99a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1290,6 +1290,7 @@ config EFI
>  	select EFI_RUNTIME_WRAPPERS
>  	select EFI_STUB
>  	select EFI_ARMSTUB
> +	select EFI_PERSISTENT_RESERVATIONS
>  	default y
>  	help
>  	  This option provides support for runtime services provided
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 89110dfc7127..b728dab9e901 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -198,3 +198,6 @@ config EFI_DEV_PATH_PARSER
>  	bool
>  	depends on ACPI
>  	default n
> +
> +config EFI_PERSISTENT_RESERVATIONS
> +       bool
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 249eb70691b0..2a1903ab6d21 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -962,6 +962,7 @@ bool efi_is_table_address(unsigned long phys_addr)
>  	return false;
>  }
>  
> +#ifdef CONFIG_EFI_PERSISTENT_RESERVATIONS
>  static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
>  
>  int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> @@ -993,6 +994,12 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
>  
>  	return 0;
>  }
> +#else
> +int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> +{
> +	return 0;
> +}
> +#endif
>  
>  #ifdef CONFIG_KEXEC
>  static int update_efi_random_seed(struct notifier_block *nb,
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Ard Biesheuvel]

On 7 November 2018 at 10:58, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Nov 07, 2018 at 09:51:09AM +0000, Marc Zyngier wrote:
>> On 06/11/18 23:49, Russell King - ARM Linux wrote:
>> > On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
>> >> On 6 November 2018 at 20:08, Russell King - ARM Linux
>> >> <linux@armlinux.org.uk> wrote:
>> >>> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
>> >>>> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> >>>>> The new memory EFI reservation feature we introduced to allow memory
>> >>>>> reservations to persist across kexec may trigger an unbounded number
>> >>>>> of calls to memblock_reserve(). The memblock subsystem can deal with
>> >>>>> this fine, but not before memblock resizing is enabled, which we can
>> >>>>> only do after paging_init(), when the memory we reallocate the array
>> >>>>> into is actually mapped.
>> >>>>>
>> >>>>> So break out the memreserve table processing into a separate function
>> >>>>> and call if after paging_init() on both arm64 and ARM.
>> >>>>>
>> >>>>> Cc: Russell King <linux@armlinux.org.uk>
>> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>>>
>> >>>> Russell,
>> >>>>
>> >>>> If you are ok with this patch, may I have your ack please? I would
>> >>>> like to send it out before the end of the week.
>> >>>
>> >>> You're not going to get a quick answer to this, because it needs me to
>> >>> investigate what the effect of this change actually is by code review.
>> >>> I can't guarantee when I'll get around to that.
>> >>>
>> >>
>> >> Fair enough.
>> >>
>> >> I will drop the ARM hunk for now then, and we'll fix ARM when you have
>> >> more time.
>> >
>> > I don't see how you can do that - you're dropping the processing of
>> > reserved areas out of the efi_config_parse_tables() path, so that
>> > won't happen any more on ARM if you don't apply the ARM hunk.
>>
>> The only reason for the whole persistent region thing is to support
>> GICv3 redistributors memory tables across kexec, for those GIC
>> implementations where LPIs cannot be disabled once they have been
>> enabled.
>>
>> There is no HW platform that combines 32bit cores and GICv3 (not to
>> mention using EFI). The only existing "platform" is KVM/arm64, which
>> doesn't suffer from the above problem (LPIs can be freely disabled in
>> emulation). So the whole persistent region feature serves no purpose on
>> 32bit ARM.
>>
>> The only issue with dropping this hunk is that the memory reservation
>> feature is enabled on 32bit ARM the first place, which can easily be
>> dealt with something along those lines:
>
> So, it was a waste of my time trying to understand what's going on here
> with Ard's patch because it shouldn't be present on 32-bit ARM... Grr.
>

Not really. I am trying really hard to do my part when it comes to
keeping the maintenance level of the ARM architecture up. That means
that all the improvements and enhancements that are implemented for
EFI on arm64 are enabled on ARM as well, unless there are pressing
reasons not to.

And even though this persistent reservation feature is only used by
the GICv3 code at the moment, there may be other future uses as well,
so I'd prefer not to special case ARM in this way.

[PATCH 2/4] efi/arm: defer persistent reservations until after paging_init()

[Russell King - ARM Linux]

On Wed, Nov 07, 2018 at 11:04:53AM +0100, Ard Biesheuvel wrote:
> On 7 November 2018 at 10:58, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Wed, Nov 07, 2018 at 09:51:09AM +0000, Marc Zyngier wrote:
> >> On 06/11/18 23:49, Russell King - ARM Linux wrote:
> >> > On Tue, Nov 06, 2018 at 09:06:56PM +0100, Ard Biesheuvel wrote:
> >> >> On 6 November 2018 at 20:08, Russell King - ARM Linux
> >> >> <linux@armlinux.org.uk> wrote:
> >> >>> On Tue, Nov 06, 2018 at 08:02:58PM +0100, Ard Biesheuvel wrote:
> >> >>>> On 6 November 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> >>>>> The new memory EFI reservation feature we introduced to allow memory
> >> >>>>> reservations to persist across kexec may trigger an unbounded number
> >> >>>>> of calls to memblock_reserve(). The memblock subsystem can deal with
> >> >>>>> this fine, but not before memblock resizing is enabled, which we can
> >> >>>>> only do after paging_init(), when the memory we reallocate the array
> >> >>>>> into is actually mapped.
> >> >>>>>
> >> >>>>> So break out the memreserve table processing into a separate function
> >> >>>>> and call if after paging_init() on both arm64 and ARM.
> >> >>>>>
> >> >>>>> Cc: Russell King <linux@armlinux.org.uk>
> >> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >>>>
> >> >>>> Russell,
> >> >>>>
> >> >>>> If you are ok with this patch, may I have your ack please? I would
> >> >>>> like to send it out before the end of the week.
> >> >>>
> >> >>> You're not going to get a quick answer to this, because it needs me to
> >> >>> investigate what the effect of this change actually is by code review.
> >> >>> I can't guarantee when I'll get around to that.
> >> >>>
> >> >>
> >> >> Fair enough.
> >> >>
> >> >> I will drop the ARM hunk for now then, and we'll fix ARM when you have
> >> >> more time.
> >> >
> >> > I don't see how you can do that - you're dropping the processing of
> >> > reserved areas out of the efi_config_parse_tables() path, so that
> >> > won't happen any more on ARM if you don't apply the ARM hunk.
> >>
> >> The only reason for the whole persistent region thing is to support
> >> GICv3 redistributors memory tables across kexec, for those GIC
> >> implementations where LPIs cannot be disabled once they have been
> >> enabled.
> >>
> >> There is no HW platform that combines 32bit cores and GICv3 (not to
> >> mention using EFI). The only existing "platform" is KVM/arm64, which
> >> doesn't suffer from the above problem (LPIs can be freely disabled in
> >> emulation). So the whole persistent region feature serves no purpose on
> >> 32bit ARM.
> >>
> >> The only issue with dropping this hunk is that the memory reservation
> >> feature is enabled on 32bit ARM the first place, which can easily be
> >> dealt with something along those lines:
> >
> > So, it was a waste of my time trying to understand what's going on here
> > with Ard's patch because it shouldn't be present on 32-bit ARM... Grr.
> >
> 
> Not really. I am trying really hard to do my part when it comes to
> keeping the maintenance level of the ARM architecture up. That means
> that all the improvements and enhancements that are implemented for
> EFI on arm64 are enabled on ARM as well, unless there are pressing
> reasons not to.
> 
> And even though this persistent reservation feature is only used by
> the GICv3 code at the moment, there may be other future uses as well,
> so I'd prefer not to special case ARM in this way.

Once you've come to some agreement on this, and you've looked at the
comment I've made (which leads me to regard your patch as a complete
non-starter for 32-bit ARM), I'll continue to look at it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH 3/4] efi: permit multiple entries in persistent memreserve data structure

[Ard Biesheuvel]

In preparation of updating efi_mem_reserve_persistent() to cause less
fragmentation when dealing with many persistent reservations, update
the struct definition and the code that handles it currently so it
can describe an arbitrary number of reservations using a single linked
list entry. The actual optimization will be implemented in a subsequent
patch.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c              | 30 +++++++++++++-------
 drivers/firmware/efi/libstub/arm-stub.c |  2 +-
 include/linux/efi.h                     | 13 +++++++--
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 55e4ea20bdc3..85d2ec532816 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -602,18 +602,25 @@ int __init efi_apply_persistent_mem_reservations(void)
 
 		while (prsv) {
 			struct linux_efi_memreserve *rsv;
+			u8 *p;
+			int i;
 
-			/* reserve the entry itself */
-			memblock_reserve(prsv, sizeof(*rsv));
-
-			rsv = memremap(prsv, sizeof(*rsv), MEMREMAP_WB);
-			if (rsv == NULL) {
+			p = memremap(ALIGN_DOWN(prsv, PAGE_SIZE), PAGE_SIZE,
+				     MEMREMAP_WB);
+			if (p == NULL) {
 				pr_err("Could not map UEFI memreserve entry!\n");
 				return -ENOMEM;
 			}
 
-			if (rsv->size)
-				memblock_reserve(rsv->base, rsv->size);
+			rsv = (void *)(p + prsv % PAGE_SIZE);
+
+			/* reserve the entry itself */
+			memblock_reserve(prsv, EFI_MEMRESERVE_SIZE(rsv->size));
+
+			for (i = 0; i < atomic_read(&rsv->count); i++) {
+				memblock_reserve(rsv->entry[i].base,
+						 rsv->entry[i].size);
+			}
 
 			prsv = rsv->next;
 			memunmap(rsv);
@@ -971,11 +978,12 @@ static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv, *parent;
+	int rsvsize = EFI_MEMRESERVE_SIZE(1);
 
 	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
 		return -ENODEV;
 
-	rsv = kmalloc(sizeof(*rsv), GFP_KERNEL);
+	rsv = kmalloc(rsvsize, GFP_KERNEL);
 	if (!rsv)
 		return -ENOMEM;
 
@@ -985,8 +993,10 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 		return -ENOMEM;
 	}
 
-	rsv->base = addr;
-	rsv->size = size;
+	rsv->size = 1;
+	atomic_set(&rsv->count, 1);
+	rsv->entry[0].base = addr;
+	rsv->entry[0].size = size;
 
 	spin_lock(&efi_mem_reserve_persistent_lock);
 	rsv->next = parent->next;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 30ac0c975f8a..5bcfa08e8bb1 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -83,8 +83,8 @@ void install_memreserve_table(efi_system_table_t *sys_table_arg)
 	}
 
 	rsv->next = 0;
-	rsv->base = 0;
 	rsv->size = 0;
+	atomic_set(&rsv->count, 0);
 
 	status = efi_call_early(install_configuration_table,
 				&memreserve_table_guid,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 100ce4a4aff6..dfce82b2ca8a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1715,9 +1715,16 @@ extern struct efi_runtime_work efi_rts_work;
 extern struct workqueue_struct *efi_rts_wq;
 
 struct linux_efi_memreserve {
-	phys_addr_t	next;
-	phys_addr_t	base;
-	phys_addr_t	size;
+	int		size;			// allocated size of the array
+	atomic_t	count;			// number of entries used
+	phys_addr_t	next;			// pa of next struct instance
+	struct {
+		phys_addr_t	base;
+		phys_addr_t	size;
+	} entry[0];
 };
 
+#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
+	(count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
 #endif /* _LINUX_EFI_H */
-- 
2.19.1

[PATCH 4/4] efi: reduce the amount of memblock reservations for persistent allocations

[Ard Biesheuvel]

The current implementation of efi_mem_reserve_persistent() is rather
naive, in the sense that for each invocation, it creates a separate
linked list entry to describe the reservation. Since the linked list
entries themselves need to persist across subsequent kexec reboots,
every reservation created this way results in two memblock_reserve()
calls at the next boot.

On arm64 systems with 100s of CPUs, this may result in a excessive
number of memblock reservations, and needless fragmentation.

So instead, make use of the newly updated struct linux_efi_memreserve
layout to put multiple reservations into a single linked list entry.
This should get rid of the numerous tiny memblock reservations, and
effectively cut the total number of reservations in half on arm64
systems with many CPUs.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c | 29 ++++++++++++++------
 include/linux/efi.h        |  3 ++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 85d2ec532816..2ca5a59f7568 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -978,22 +978,35 @@ static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv, *parent;
-	int rsvsize = EFI_MEMRESERVE_SIZE(1);
+	unsigned long prsv;
+	int index;
 
 	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
 		return -ENODEV;
 
-	rsv = kmalloc(rsvsize, GFP_KERNEL);
-	if (!rsv)
-		return -ENOMEM;
-
 	parent = memremap(efi.mem_reserve, sizeof(*rsv), MEMREMAP_WB);
-	if (!parent) {
-		kfree(rsv);
+	if (!parent)
 		return -ENOMEM;
+
+	/* first try to find a slot in an existing linked list entry */
+	for (prsv = parent->next; prsv; prsv = rsv->next) {
+		rsv = __va(prsv);
+		index = atomic_fetch_add_unless(&rsv->count, 1, rsv->size);
+		if (index < rsv->size) {
+			rsv->entry[index].base = addr;
+			rsv->entry[index].size = size;
+
+			memunmap(parent);
+			return 0;
+		}
 	}
 
-	rsv->size = 1;
+	/* no slot found - allocate a new linked list entry */
+	rsv = (struct linux_efi_memreserve *)__get_free_page(GFP_KERNEL);
+	if (!rsv)
+		return -ENOMEM;
+
+	rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
 	atomic_set(&rsv->count, 1);
 	rsv->entry[0].base = addr;
 	rsv->entry[0].size = size;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index dfce82b2ca8a..1a1a081f7244 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1727,4 +1727,7 @@ struct linux_efi_memreserve {
 #define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
 	(count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
 
+#define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
+	/ sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
 #endif /* _LINUX_EFI_H */
-- 
2.19.1

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Marc Zyngier]

Hi Ard,

On 06/11/18 11:37, Ard Biesheuvel wrote:
> This series addresses the kexec/kdump crash on arm64 system with many CPUs
> that was reported by Bhupesh.
> 
> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
> EFI persistent memreserve infrastructure so that fewer memblock reservations
> are required.
> 
> Ard Biesheuvel (4):
>   arm64: memblock: don't permit memblock resizing until linear mapping
>     is up
>   efi/arm: defer persistent reservations until after paging_init()
>   efi: permit multiple entries in persistent memreserve data structure
>   efi: reduce the amount of memblock reservations for persistent
>     allocations
> 
>  arch/arm/kernel/setup.c                 |  1 +
>  arch/arm64/kernel/setup.c               |  1 +
>  arch/arm64/mm/init.c                    |  2 -
>  arch/arm64/mm/mmu.c                     |  2 +
>  drivers/firmware/efi/efi.c              | 59 ++++++++++++++------
>  drivers/firmware/efi/libstub/arm-stub.c |  2 +-
>  include/linux/efi.h                     | 23 +++++++-
>  7 files changed, 68 insertions(+), 22 deletions(-)

I've just given these patches a go a a TX2 box (one of the 224 CPU
ones...), and kexec worked just fine (v4.20-rc1 vanilla didn't manage to
kexec on this box).

There seem to be some additional userspace patches that are required for
the ACPI tables not to be corrupted in the secondary kernel, but that's
an orthogonal issue.

Feel free to add

Tested-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Ard Biesheuvel]

On 6 November 2018 at 19:27, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Ard,
>
> On 06/11/18 11:37, Ard Biesheuvel wrote:
>> This series addresses the kexec/kdump crash on arm64 system with many CPUs
>> that was reported by Bhupesh.
>>
>> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
>> EFI persistent memreserve infrastructure so that fewer memblock reservations
>> are required.
>>
>> Ard Biesheuvel (4):
>>   arm64: memblock: don't permit memblock resizing until linear mapping
>>     is up
>>   efi/arm: defer persistent reservations until after paging_init()
>>   efi: permit multiple entries in persistent memreserve data structure
>>   efi: reduce the amount of memblock reservations for persistent
>>     allocations
>>
>>  arch/arm/kernel/setup.c                 |  1 +
>>  arch/arm64/kernel/setup.c               |  1 +
>>  arch/arm64/mm/init.c                    |  2 -
>>  arch/arm64/mm/mmu.c                     |  2 +
>>  drivers/firmware/efi/efi.c              | 59 ++++++++++++++------
>>  drivers/firmware/efi/libstub/arm-stub.c |  2 +-
>>  include/linux/efi.h                     | 23 +++++++-
>>  7 files changed, 68 insertions(+), 22 deletions(-)
>
> I've just given these patches a go a a TX2 box (one of the 224 CPU
> ones...), and kexec worked just fine (v4.20-rc1 vanilla didn't manage to
> kexec on this box).
>
> There seem to be some additional userspace patches that are required for
> the ACPI tables not to be corrupted in the secondary kernel, but that's
> an orthogonal issue.
>
> Feel free to add
>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>

Thanks Marc.

Any thoughts on whether patches #3 and #4 should be included as fixes?
I'm leaning towards yes.

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Marc Zyngier]

On Tue, 06 Nov 2018 19:01:51 +0000,
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On 6 November 2018 at 19:27, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Hi Ard,
> >
> > On 06/11/18 11:37, Ard Biesheuvel wrote:
> >> This series addresses the kexec/kdump crash on arm64 system with many CPUs
> >> that was reported by Bhupesh.
> >>
> >> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
> >> EFI persistent memreserve infrastructure so that fewer memblock reservations
> >> are required.
> >>
> >> Ard Biesheuvel (4):
> >>   arm64: memblock: don't permit memblock resizing until linear mapping
> >>     is up
> >>   efi/arm: defer persistent reservations until after paging_init()
> >>   efi: permit multiple entries in persistent memreserve data structure
> >>   efi: reduce the amount of memblock reservations for persistent
> >>     allocations
> >>
> >>  arch/arm/kernel/setup.c                 |  1 +
> >>  arch/arm64/kernel/setup.c               |  1 +
> >>  arch/arm64/mm/init.c                    |  2 -
> >>  arch/arm64/mm/mmu.c                     |  2 +
> >>  drivers/firmware/efi/efi.c              | 59 ++++++++++++++------
> >>  drivers/firmware/efi/libstub/arm-stub.c |  2 +-
> >>  include/linux/efi.h                     | 23 +++++++-
> >>  7 files changed, 68 insertions(+), 22 deletions(-)
> >
> > I've just given these patches a go a a TX2 box (one of the 224 CPU
> > ones...), and kexec worked just fine (v4.20-rc1 vanilla didn't manage to
> > kexec on this box).
> >
> > There seem to be some additional userspace patches that are required for
> > the ACPI tables not to be corrupted in the secondary kernel, but that's
> > an orthogonal issue.
> >
> > Feel free to add
> >
> > Tested-by: Marc Zyngier <marc.zyngier@arm.com>
> >
> 
> Thanks Marc.
> 
> Any thoughts on whether patches #3 and #4 should be included as fixes?
> I'm leaning towards yes.

They certainly massively reduce the number of list entries, and
probably help reducing the overhead. I'd be inclined to merge them as
well.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Will Deacon]

Hi Ard,

On Tue, Nov 06, 2018 at 12:37:28PM +0100, Ard Biesheuvel wrote:
> This series addresses the kexec/kdump crash on arm64 system with many CPUs
> that was reported by Bhupesh.
> 
> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
> EFI persistent memreserve infrastructure so that fewer memblock reservations
> are required.

I acked the arm64 part and patches 3 and 4 look good afaict, so:

Acked-by: Will Deacon <will.deacon@arm.com>

for those as well.

The only thing I was wondering is whether or not exhausting the page-sized
array in the first list entry is rare enough that we could just realloc the
thing and copy instead of chaining together new pages. That said, without
seeing the code it's hard to tell whether you save much complexity with such
a scheme so I'll leave it up to you.

Will

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Ard Biesheuvel]

On 6 November 2018 at 22:34, Will Deacon <will.deacon@arm.com> wrote:
> Hi Ard,
>
> On Tue, Nov 06, 2018 at 12:37:28PM +0100, Ard Biesheuvel wrote:
>> This series addresses the kexec/kdump crash on arm64 system with many CPUs
>> that was reported by Bhupesh.
>>
>> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
>> EFI persistent memreserve infrastructure so that fewer memblock reservations
>> are required.
>
> I acked the arm64 part and patches 3 and 4 look good afaict, so:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> for those as well.
>
> The only thing I was wondering is whether or not exhausting the page-sized
> array in the first list entry is rare enough that we could just realloc the
> thing and copy instead of chaining together new pages. That said, without
> seeing the code it's hard to tell whether you save much complexity with such
> a scheme so I'll leave it up to you.
>

Well, the problem is that the page-sized array may have been allocated
by a previous kernel, and so it may be memblock_reserve()d already, in
which case reallocating does not actually free up the memory in a
useful way.

Also, in the unlikely event that we race and allocate two new pages at
the same time, the current scheme will not break (and we will
ultimately fill up all the slots in both pages before allocating even
more pages). This will become a lot trickier if we do reallocation.

So if the current approach looks correct to you, then I'd prefer to
stick with it.

Do you agree with applying #3 and #4 as fixes?

[PATCH 0/4] arm/efi: fix memblock reallocation crash due to persistent reservations

[Will Deacon]

On Tue, Nov 06, 2018 at 10:39:47PM +0100, Ard Biesheuvel wrote:
> On 6 November 2018 at 22:34, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Nov 06, 2018 at 12:37:28PM +0100, Ard Biesheuvel wrote:
> >> This series addresses the kexec/kdump crash on arm64 system with many CPUs
> >> that was reported by Bhupesh.
> >>
> >> Patches #1 and #2 fix the actual crash. Patches #3 and #4 optimize the
> >> EFI persistent memreserve infrastructure so that fewer memblock reservations
> >> are required.
> >
> > I acked the arm64 part and patches 3 and 4 look good afaict, so:
> >
> > Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > for those as well.
> >
> > The only thing I was wondering is whether or not exhausting the page-sized
> > array in the first list entry is rare enough that we could just realloc the
> > thing and copy instead of chaining together new pages. That said, without
> > seeing the code it's hard to tell whether you save much complexity with such
> > a scheme so I'll leave it up to you.
> >
> 
> Well, the problem is that the page-sized array may have been allocated
> by a previous kernel, and so it may be memblock_reserve()d already, in
> which case reallocating does not actually free up the memory in a
> useful way.
> 
> Also, in the unlikely event that we race and allocate two new pages at
> the same time, the current scheme will not break (and we will
> ultimately fill up all the slots in both pages before allocating even
> more pages). This will become a lot trickier if we do reallocation.

Ah crap, yeah, the concurrency angle makes that really awful. Thanks.

> So if the current approach looks correct to you, then I'd prefer to
> stick with it.
> 
> Do you agree with applying #3 and #4 as fixes?

If Patch 1 alone is sufficient to solve the issue for Bhupesh, then I don't
think we have a need to treat 3 and 4 as fixes (and therefore we can avoid
having to backport them to stable kernels).

Will