linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL 0/2] EFI fixes for v5.0-rc
@ 2019-02-15 12:33 Ard Biesheuvel
  2019-02-15 12:33 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
  2019-02-15 12:33 ` [PATCH 2/2] efi/arm: Revert "Defer persistent reservations until after paging_init()" Ard Biesheuvel
  0 siblings, 2 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-15 12:33 UTC (permalink / raw)
  To: linux-arm-kernel, Ingo Molnar, Thomas Gleixner
  Cc: linux-efi, Ard Biesheuvel, Marc Zyngier, Will Deacon,
	linux-kernel, Mike Rapoport

The following changes since commit 1f947a7a011fcceb14cb912f5481a53b18f1879a:

  Merge branch 'akpm' (patches from Andrew) (2019-02-12 17:15:33 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

for you to fetch changes up to 98f19a800ac41cffa72a17b019eaf8160a01c22b:

  efi/arm: Revert "Defer persistent reservations until after paging_init()" (2019-02-15 09:58:04 +0100)

----------------------------------------------------------------
Yet another fix for the EFI memreserve code used by the GICv3 driver on
arm64.

----------------------------------------------------------------
Ard Biesheuvel (2):
      arm64: account for GICv3 LPI tables in static memblock reserve table
      efi/arm: Revert "Defer persistent reservations until after paging_init()"

 arch/arm64/include/asm/memory.h         | 11 +++++++++++
 arch/arm64/kernel/setup.c               |  1 -
 drivers/firmware/efi/efi.c              |  4 ----
 drivers/firmware/efi/libstub/arm-stub.c |  3 ---
 include/linux/efi.h                     |  7 -------
 include/linux/memblock.h                |  3 ---
 mm/memblock.c                           | 10 ++++++++--
 7 files changed, 19 insertions(+), 20 deletions(-)

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-15 12:33 [GIT PULL 0/2] EFI fixes for v5.0-rc Ard Biesheuvel
@ 2019-02-15 12:33 ` Ard Biesheuvel
  2019-02-15 12:33 ` [PATCH 2/2] efi/arm: Revert "Defer persistent reservations until after paging_init()" Ard Biesheuvel
  1 sibling, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-15 12:33 UTC (permalink / raw)
  To: linux-arm-kernel, Ingo Molnar, Thomas Gleixner
  Cc: linux-efi, Ard Biesheuvel, Marc Zyngier, Will Deacon,
	linux-kernel, Mike Rapoport

In the irqchip and EFI code, we have what basically amounts to a quirk
to work around a peculiarity in the GICv3 architecture, which permits
the system memory address of LPI tables to be programmable only once
after a CPU reset. This means kexec kernels must use the same memory
as the first kernel, and thus ensure that this memory has not been
given out for other purposes by the time the ITS init code runs, which
is not very early for secondary CPUs.

On systems with many CPUs, these reservations could overflow the
memblock reservation table, and this was addressed in commit
eff896288872 ("efi/arm: Defer persistent reservations until after
paging_init()"). However, this turns out to have made things worse,
since the allocation of page tables and heap space for the resized
memblock reservation table itself may overwrite the regions we are
attempting to reserve, which may cause all kinds of corruption,
also considering that the ITS will still be poking bits into that
memory in response to incoming MSIs.

So instead, let's grow the static memblock reservation table on such
systems so it can accommodate these reservations at an earlier time.
This will permit us to revert the above commit in a subsequent patch.

Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/memory.h | 11 +++++++++++
 include/linux/memblock.h        |  3 ---
 mm/memblock.c                   | 10 ++++++++--
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index e1ec947e7c0c..ada877f56551 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
 #define virt_addr_valid(kaddr)		\
 	(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
 
+/*
+ * Given that the GIC architecture permits ITS implementations that can only be
+ * configured with a LPI table address once, GICv3 systems with many CPUs may
+ * end up reserving a lot of different regions after a kexec for their LPI
+ * tables (one per CPU), as we are forced to reuse the same memory after kexec
+ * (and thus reserve it persistently with EFI beforehand)
+ */
+#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
+#define INIT_MEMBLOCK_RESERVED_REGIONS	(INIT_MEMBLOCK_REGIONS + NR_CPUS + 1)
+#endif
+
 #include <asm-generic/memory_model.h>
 
 #endif
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 64c41cf45590..859b55b66db2 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -29,9 +29,6 @@ extern unsigned long max_pfn;
  */
 extern unsigned long long max_possible_pfn;
 
-#define INIT_MEMBLOCK_REGIONS	128
-#define INIT_PHYSMEM_REGIONS	4
-
 /**
  * enum memblock_flags - definition of memory region attributes
  * @MEMBLOCK_NONE: no special request
diff --git a/mm/memblock.c b/mm/memblock.c
index 022d4cbb3618..a526c3ab8390 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -26,6 +26,12 @@
 
 #include "internal.h"
 
+#define INIT_MEMBLOCK_REGIONS		128
+#define INIT_PHYSMEM_REGIONS		4
+#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
+#define INIT_MEMBLOCK_RESERVED_REGIONS	INIT_MEMBLOCK_REGIONS
+#endif
+
 /**
  * DOC: memblock overview
  *
@@ -92,7 +98,7 @@ unsigned long max_pfn;
 unsigned long long max_possible_pfn;
 
 static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
-static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
+static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
 static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
 #endif
@@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
 
 	.reserved.regions	= memblock_reserved_init_regions,
 	.reserved.cnt		= 1,	/* empty dummy entry */
-	.reserved.max		= INIT_MEMBLOCK_REGIONS,
+	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
 	.reserved.name		= "reserved",
 
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
-- 
2.20.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] efi/arm: Revert "Defer persistent reservations until after paging_init()"
  2019-02-15 12:33 [GIT PULL 0/2] EFI fixes for v5.0-rc Ard Biesheuvel
  2019-02-15 12:33 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
@ 2019-02-15 12:33 ` Ard Biesheuvel
  1 sibling, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-15 12:33 UTC (permalink / raw)
  To: linux-arm-kernel, Ingo Molnar, Thomas Gleixner
  Cc: linux-efi, Ard Biesheuvel, Marc Zyngier, Will Deacon,
	linux-kernel, Mike Rapoport

This reverts commit eff896288872d687d9662000ec9ae11b6d61766f, which
deferred the processing of persistent memory reservations to a point
where the memory may have already been allocated and overwritten,
defeating the purpose.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/setup.c               | 1 -
 drivers/firmware/efi/efi.c              | 4 ----
 drivers/firmware/efi/libstub/arm-stub.c | 3 ---
 include/linux/efi.h                     | 7 -------
 4 files changed, 15 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 4b0e1231625c..d09ec76f08cf 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -313,7 +313,6 @@ 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 4c46ff6f2242..55b77c576c42 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -592,11 +592,7 @@ 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;
 
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index eee42d5e25ee..c037c6c5d0b7 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -75,9 +75,6 @@ void install_memreserve_table(efi_system_table_t *sys_table_arg)
 	efi_guid_t memreserve_table_guid = LINUX_EFI_MEMRESERVE_TABLE_GUID;
 	efi_status_t status;
 
-	if (IS_ENABLED(CONFIG_ARM))
-		return;
-
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA, sizeof(*rsv),
 				(void **)&rsv);
 	if (status != EFI_SUCCESS) {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 45ff763fba76..28604a8d0aa9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1198,8 +1198,6 @@ 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)
 {
@@ -1218,11 +1216,6 @@ 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.20.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-14 16:55     ` Ard Biesheuvel
@ 2019-02-15 10:34       ` Marc Zyngier
  0 siblings, 0 replies; 11+ messages in thread
From: Marc Zyngier @ 2019-02-15 10:34 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, Catalin Marinas, Will Deacon, Linux-MM, James Morse,
	Andrew Morton, linux-arm-kernel

On Thu, 14 Feb 2019 16:55:28 +0000,
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On Thu, 14 Feb 2019 at 16:48, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> > Hi Ard,
> >
> > On 13/02/2019 13:27, Ard Biesheuvel wrote:
> > > In the irqchip and EFI code, we have what basically amounts to a quirk
> > > to work around a peculiarity in the GICv3 architecture, which permits
> > > the system memory address of LPI tables to be programmable only once
> > > after a CPU reset. This means kexec kernels must use the same memory
> > > as the first kernel, and thus ensure that this memory has not been
> > > given out for other purposes by the time the ITS init code runs, which
> > > is not very early for secondary CPUs.
> > >
> > > On systems with many CPUs, these reservations could overflow the
> > > memblock reservation table, and this was addressed in commit
> > > eff896288872 ("efi/arm: Defer persistent reservations until after
> > > paging_init()"). However, this turns out to have made things worse,
> > > since the allocation of page tables and heap space for the resized
> > > memblock reservation table itself may overwrite the regions we are
> > > attempting to reserve, which may cause all kinds of corruption,
> > > also considering that the ITS will still be poking bits into that
> > > memory in response to incoming MSIs.
> > >
> > > So instead, let's grow the static memblock reservation table on such
> > > systems so it can accommodate these reservations at an earlier time.
> > > This will permit us to revert the above commit in a subsequent patch.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  arch/arm64/include/asm/memory.h | 11 +++++++++++
> > >  include/linux/memblock.h        |  3 ---
> > >  mm/memblock.c                   | 10 ++++++++--
> > >  3 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > index e1ec947e7c0c..7e2b13cdd970 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> > >  #define virt_addr_valid(kaddr)               \
> > >       (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> > >
> > > +/*
> > > + * Given that the GIC architecture permits ITS implementations that can only be
> > > + * configured with a LPI table address once, GICv3 systems with many CPUs may
> > > + * end up reserving a lot of different regions after a kexec for their LPI
> > > + * tables, as we are forced to reuse the same memory after kexec (and thus
> > > + * reserve it persistently with EFI beforehand)
> > > + */
> > > +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
> >
> > Since GICv3 has 1 pending table per CPU, plus one global property table,
> > can we make this 2 * NR_CPUS + 1? Or is that enough already?
> >
> 
> Ah, I misread the code then. That would mean we'll only need 1 extra
> slot per CPU.
> 
> So I will change this to
> 
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + NR_CPUS)
> 
> considering that INIT_MEMBLOCK_REGIONS defaults to 128, so that one
> global table is already accounted for.

Look good to me.

Thanks,

	M.

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-14 15:47   ` Marc Zyngier
@ 2019-02-14 16:55     ` Ard Biesheuvel
  2019-02-15 10:34       ` Marc Zyngier
  0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-14 16:55 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-efi, Catalin Marinas, Will Deacon, Linux-MM, James Morse,
	Andrew Morton, linux-arm-kernel

On Thu, 14 Feb 2019 at 16:48, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 13/02/2019 13:27, Ard Biesheuvel wrote:
> > In the irqchip and EFI code, we have what basically amounts to a quirk
> > to work around a peculiarity in the GICv3 architecture, which permits
> > the system memory address of LPI tables to be programmable only once
> > after a CPU reset. This means kexec kernels must use the same memory
> > as the first kernel, and thus ensure that this memory has not been
> > given out for other purposes by the time the ITS init code runs, which
> > is not very early for secondary CPUs.
> >
> > On systems with many CPUs, these reservations could overflow the
> > memblock reservation table, and this was addressed in commit
> > eff896288872 ("efi/arm: Defer persistent reservations until after
> > paging_init()"). However, this turns out to have made things worse,
> > since the allocation of page tables and heap space for the resized
> > memblock reservation table itself may overwrite the regions we are
> > attempting to reserve, which may cause all kinds of corruption,
> > also considering that the ITS will still be poking bits into that
> > memory in response to incoming MSIs.
> >
> > So instead, let's grow the static memblock reservation table on such
> > systems so it can accommodate these reservations at an earlier time.
> > This will permit us to revert the above commit in a subsequent patch.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/include/asm/memory.h | 11 +++++++++++
> >  include/linux/memblock.h        |  3 ---
> >  mm/memblock.c                   | 10 ++++++++--
> >  3 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index e1ec947e7c0c..7e2b13cdd970 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> >  #define virt_addr_valid(kaddr)               \
> >       (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> >
> > +/*
> > + * Given that the GIC architecture permits ITS implementations that can only be
> > + * configured with a LPI table address once, GICv3 systems with many CPUs may
> > + * end up reserving a lot of different regions after a kexec for their LPI
> > + * tables, as we are forced to reuse the same memory after kexec (and thus
> > + * reserve it persistently with EFI beforehand)
> > + */
> > +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
>
> Since GICv3 has 1 pending table per CPU, plus one global property table,
> can we make this 2 * NR_CPUS + 1? Or is that enough already?
>

Ah, I misread the code then. That would mean we'll only need 1 extra
slot per CPU.

So I will change this to

> > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + NR_CPUS)

considering that INIT_MEMBLOCK_REGIONS defaults to 128, so that one
global table is already accounted for.


> > +#endif
> > +
> >  #include <asm-generic/memory_model.h>
> >
> >  #endif
> > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > index 64c41cf45590..859b55b66db2 100644
> > --- a/include/linux/memblock.h
> > +++ b/include/linux/memblock.h
> > @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
> >   */
> >  extern unsigned long long max_possible_pfn;
> >
> > -#define INIT_MEMBLOCK_REGIONS        128
> > -#define INIT_PHYSMEM_REGIONS 4
> > -
> >  /**
> >   * enum memblock_flags - definition of memory region attributes
> >   * @MEMBLOCK_NONE: no special request
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 022d4cbb3618..a526c3ab8390 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -26,6 +26,12 @@
> >
> >  #include "internal.h"
> >
> > +#define INIT_MEMBLOCK_REGIONS                128
> > +#define INIT_PHYSMEM_REGIONS         4
> > +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> > +#define INIT_MEMBLOCK_RESERVED_REGIONS       INIT_MEMBLOCK_REGIONS
> > +#endif
> > +
> >  /**
> >   * DOC: memblock overview
> >   *
> > @@ -92,7 +98,7 @@ unsigned long max_pfn;
> >  unsigned long long max_possible_pfn;
> >
> >  static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > -static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > +static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
> >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> >  static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
> >  #endif
> > @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
> >
> >       .reserved.regions       = memblock_reserved_init_regions,
> >       .reserved.cnt           = 1,    /* empty dummy entry */
> > -     .reserved.max           = INIT_MEMBLOCK_REGIONS,
> > +     .reserved.max           = INIT_MEMBLOCK_RESERVED_REGIONS,
> >       .reserved.name          = "reserved",
> >
> >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> >
>
> Otherwise:
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>
>         M.
> --
> Jazz is not dead. It just smells funny...

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-13 13:27 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
  2019-02-14  8:33   ` Mike Rapoport
  2019-02-14 14:40   ` Will Deacon
@ 2019-02-14 15:47   ` Marc Zyngier
  2019-02-14 16:55     ` Ard Biesheuvel
  2 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2019-02-14 15:47 UTC (permalink / raw)
  To: Ard Biesheuvel, linux-efi
  Cc: Catalin Marinas, Will Deacon, linux-mm, James Morse,
	Andrew Morton, linux-arm-kernel

Hi Ard,

On 13/02/2019 13:27, Ard Biesheuvel wrote:
> In the irqchip and EFI code, we have what basically amounts to a quirk
> to work around a peculiarity in the GICv3 architecture, which permits
> the system memory address of LPI tables to be programmable only once
> after a CPU reset. This means kexec kernels must use the same memory
> as the first kernel, and thus ensure that this memory has not been
> given out for other purposes by the time the ITS init code runs, which
> is not very early for secondary CPUs.
> 
> On systems with many CPUs, these reservations could overflow the
> memblock reservation table, and this was addressed in commit
> eff896288872 ("efi/arm: Defer persistent reservations until after
> paging_init()"). However, this turns out to have made things worse,
> since the allocation of page tables and heap space for the resized
> memblock reservation table itself may overwrite the regions we are
> attempting to reserve, which may cause all kinds of corruption,
> also considering that the ITS will still be poking bits into that
> memory in response to incoming MSIs.
> 
> So instead, let's grow the static memblock reservation table on such
> systems so it can accommodate these reservations at an earlier time.
> This will permit us to revert the above commit in a subsequent patch.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/memory.h | 11 +++++++++++
>  include/linux/memblock.h        |  3 ---
>  mm/memblock.c                   | 10 ++++++++--
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index e1ec947e7c0c..7e2b13cdd970 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define virt_addr_valid(kaddr)		\
>  	(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
>  
> +/*
> + * Given that the GIC architecture permits ITS implementations that can only be
> + * configured with a LPI table address once, GICv3 systems with many CPUs may
> + * end up reserving a lot of different regions after a kexec for their LPI
> + * tables, as we are forced to reuse the same memory after kexec (and thus
> + * reserve it persistently with EFI beforehand)
> + */
> +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> +#define INIT_MEMBLOCK_RESERVED_REGIONS	(INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)

Since GICv3 has 1 pending table per CPU, plus one global property table,
can we make this 2 * NR_CPUS + 1? Or is that enough already?

> +#endif
> +
>  #include <asm-generic/memory_model.h>
>  
>  #endif
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 64c41cf45590..859b55b66db2 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
>   */
>  extern unsigned long long max_possible_pfn;
>  
> -#define INIT_MEMBLOCK_REGIONS	128
> -#define INIT_PHYSMEM_REGIONS	4
> -
>  /**
>   * enum memblock_flags - definition of memory region attributes
>   * @MEMBLOCK_NONE: no special request
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 022d4cbb3618..a526c3ab8390 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -26,6 +26,12 @@
>  
>  #include "internal.h"
>  
> +#define INIT_MEMBLOCK_REGIONS		128
> +#define INIT_PHYSMEM_REGIONS		4
> +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> +#define INIT_MEMBLOCK_RESERVED_REGIONS	INIT_MEMBLOCK_REGIONS
> +#endif
> +
>  /**
>   * DOC: memblock overview
>   *
> @@ -92,7 +98,7 @@ unsigned long max_pfn;
>  unsigned long long max_possible_pfn;
>  
>  static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> -static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> +static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
>  static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
>  #endif
> @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
>  
>  	.reserved.regions	= memblock_reserved_init_regions,
>  	.reserved.cnt		= 1,	/* empty dummy entry */
> -	.reserved.max		= INIT_MEMBLOCK_REGIONS,
> +	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
>  	.reserved.name		= "reserved",
>  
>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> 

Otherwise:

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

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-14 14:57     ` Ard Biesheuvel
@ 2019-02-14 15:11       ` Mike Rapoport
  0 siblings, 0 replies; 11+ messages in thread
From: Mike Rapoport @ 2019-02-14 15:11 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, Marc Zyngier, Catalin Marinas, Will Deacon, Linux-MM,
	James Morse, Andrew Morton, linux-arm-kernel

On Thu, Feb 14, 2019 at 03:57:35PM +0100, Ard Biesheuvel wrote:
> On Thu, 14 Feb 2019 at 09:34, Mike Rapoport <rppt@linux.ibm.com> wrote:
> >
> > On Wed, Feb 13, 2019 at 02:27:37PM +0100, Ard Biesheuvel wrote:
> > > In the irqchip and EFI code, we have what basically amounts to a quirk
> > > to work around a peculiarity in the GICv3 architecture, which permits
> > > the system memory address of LPI tables to be programmable only once
> > > after a CPU reset. This means kexec kernels must use the same memory
> > > as the first kernel, and thus ensure that this memory has not been
> > > given out for other purposes by the time the ITS init code runs, which
> > > is not very early for secondary CPUs.
> > >
> > > On systems with many CPUs, these reservations could overflow the
> > > memblock reservation table, and this was addressed in commit
> > > eff896288872 ("efi/arm: Defer persistent reservations until after
> > > paging_init()"). However, this turns out to have made things worse,
> > > since the allocation of page tables and heap space for the resized
> > > memblock reservation table itself may overwrite the regions we are
> > > attempting to reserve, which may cause all kinds of corruption,
> > > also considering that the ITS will still be poking bits into that
> > > memory in response to incoming MSIs.
> > >
> > > So instead, let's grow the static memblock reservation table on such
> > > systems so it can accommodate these reservations at an earlier time.
> > > This will permit us to revert the above commit in a subsequent patch.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  arch/arm64/include/asm/memory.h | 11 +++++++++++
> > >  include/linux/memblock.h        |  3 ---
> > >  mm/memblock.c                   | 10 ++++++++--
> > >  3 files changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > index e1ec947e7c0c..7e2b13cdd970 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> > >  #define virt_addr_valid(kaddr)               \
> > >       (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> > >
> > > +/*
> > > + * Given that the GIC architecture permits ITS implementations that can only be
> > > + * configured with a LPI table address once, GICv3 systems with many CPUs may
> > > + * end up reserving a lot of different regions after a kexec for their LPI
> > > + * tables, as we are forced to reuse the same memory after kexec (and thus
> > > + * reserve it persistently with EFI beforehand)
> > > + */
> > > +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
> > > +#endif

Not strictly related, with high NR_CPUS the memblock.reserved becomes quite
large and mostly empty in many cases. Is there a reason arm64 does not set
ARCH_DISCARD_MEMBLOCK?

> > > +
> > >  #include <asm-generic/memory_model.h>
> > >
> > >  #endif
> > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > > index 64c41cf45590..859b55b66db2 100644
> > > --- a/include/linux/memblock.h
> > > +++ b/include/linux/memblock.h
> > > @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
> > >   */
> > >  extern unsigned long long max_possible_pfn;
> > >
> > > -#define INIT_MEMBLOCK_REGIONS        128
> > > -#define INIT_PHYSMEM_REGIONS 4
> > > -
> > >  /**
> > >   * enum memblock_flags - definition of memory region attributes
> > >   * @MEMBLOCK_NONE: no special request
> > > diff --git a/mm/memblock.c b/mm/memblock.c
> > > index 022d4cbb3618..a526c3ab8390 100644
> > > --- a/mm/memblock.c
> > > +++ b/mm/memblock.c
> > > @@ -26,6 +26,12 @@
> > >
> > >  #include "internal.h"
> > >
> > > +#define INIT_MEMBLOCK_REGIONS                128
> > > +#define INIT_PHYSMEM_REGIONS         4
> > > +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> > > +#define INIT_MEMBLOCK_RESERVED_REGIONS       INIT_MEMBLOCK_REGIONS
> > > +#endif
> > > +
> >
> > I'd suggest
> >
> > s/INIT_MEMBLOCK_REGIONS/INIT_MEMORY_REGIONS
> > s/INIT_MEMBLOCK_RESERVED_REGIONS/INIT_RESERVED_REGIONS
> >
> 
> Well, I'd prefer to keep MEMBLOCK in the identifier, given that we are
> setting it from an arch header file as well.
 
I was bothered by lack of consistency, but you're right, namespacing here
is more important.

Care to update /** DOC: */ section as a separate patch?

> > Except that,
> >
> > Acked-by: Mike Rapoport <rppt@linux.ibm.com>
> >
> 
> Thanks
> 
> 
> > >  /**
> > >   * DOC: memblock overview
> > >   *
> > > @@ -92,7 +98,7 @@ unsigned long max_pfn;
> > >  unsigned long long max_possible_pfn;
> > >
> > >  static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > > -static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > > +static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
> > >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> > >  static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
> > >  #endif
> > > @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
> > >
> > >       .reserved.regions       = memblock_reserved_init_regions,
> > >       .reserved.cnt           = 1,    /* empty dummy entry */
> > > -     .reserved.max           = INIT_MEMBLOCK_REGIONS,
> > > +     .reserved.max           = INIT_MEMBLOCK_RESERVED_REGIONS,
> > >       .reserved.name          = "reserved",
> > >
> > >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Sincerely yours,
> > Mike.
> >
> 

-- 
Sincerely yours,
Mike.


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-14  8:33   ` Mike Rapoport
@ 2019-02-14 14:57     ` Ard Biesheuvel
  2019-02-14 15:11       ` Mike Rapoport
  0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-14 14:57 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: linux-efi, Marc Zyngier, Catalin Marinas, Will Deacon, Linux-MM,
	James Morse, Andrew Morton, linux-arm-kernel

On Thu, 14 Feb 2019 at 09:34, Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> On Wed, Feb 13, 2019 at 02:27:37PM +0100, Ard Biesheuvel wrote:
> > In the irqchip and EFI code, we have what basically amounts to a quirk
> > to work around a peculiarity in the GICv3 architecture, which permits
> > the system memory address of LPI tables to be programmable only once
> > after a CPU reset. This means kexec kernels must use the same memory
> > as the first kernel, and thus ensure that this memory has not been
> > given out for other purposes by the time the ITS init code runs, which
> > is not very early for secondary CPUs.
> >
> > On systems with many CPUs, these reservations could overflow the
> > memblock reservation table, and this was addressed in commit
> > eff896288872 ("efi/arm: Defer persistent reservations until after
> > paging_init()"). However, this turns out to have made things worse,
> > since the allocation of page tables and heap space for the resized
> > memblock reservation table itself may overwrite the regions we are
> > attempting to reserve, which may cause all kinds of corruption,
> > also considering that the ITS will still be poking bits into that
> > memory in response to incoming MSIs.
> >
> > So instead, let's grow the static memblock reservation table on such
> > systems so it can accommodate these reservations at an earlier time.
> > This will permit us to revert the above commit in a subsequent patch.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/include/asm/memory.h | 11 +++++++++++
> >  include/linux/memblock.h        |  3 ---
> >  mm/memblock.c                   | 10 ++++++++--
> >  3 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index e1ec947e7c0c..7e2b13cdd970 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
> >  #define virt_addr_valid(kaddr)               \
> >       (_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> >
> > +/*
> > + * Given that the GIC architecture permits ITS implementations that can only be
> > + * configured with a LPI table address once, GICv3 systems with many CPUs may
> > + * end up reserving a lot of different regions after a kexec for their LPI
> > + * tables, as we are forced to reuse the same memory after kexec (and thus
> > + * reserve it persistently with EFI beforehand)
> > + */
> > +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> > +#define INIT_MEMBLOCK_RESERVED_REGIONS       (INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
> > +#endif
> > +
> >  #include <asm-generic/memory_model.h>
> >
> >  #endif
> > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > index 64c41cf45590..859b55b66db2 100644
> > --- a/include/linux/memblock.h
> > +++ b/include/linux/memblock.h
> > @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
> >   */
> >  extern unsigned long long max_possible_pfn;
> >
> > -#define INIT_MEMBLOCK_REGIONS        128
> > -#define INIT_PHYSMEM_REGIONS 4
> > -
> >  /**
> >   * enum memblock_flags - definition of memory region attributes
> >   * @MEMBLOCK_NONE: no special request
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 022d4cbb3618..a526c3ab8390 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -26,6 +26,12 @@
> >
> >  #include "internal.h"
> >
> > +#define INIT_MEMBLOCK_REGIONS                128
> > +#define INIT_PHYSMEM_REGIONS         4
> > +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> > +#define INIT_MEMBLOCK_RESERVED_REGIONS       INIT_MEMBLOCK_REGIONS
> > +#endif
> > +
>
> I'd suggest
>
> s/INIT_MEMBLOCK_REGIONS/INIT_MEMORY_REGIONS
> s/INIT_MEMBLOCK_RESERVED_REGIONS/INIT_RESERVED_REGIONS
>

Well, I'd prefer to keep MEMBLOCK in the identifier, given that we are
setting it from an arch header file as well.

> Except that,
>
> Acked-by: Mike Rapoport <rppt@linux.ibm.com>
>

Thanks


> >  /**
> >   * DOC: memblock overview
> >   *
> > @@ -92,7 +98,7 @@ unsigned long max_pfn;
> >  unsigned long long max_possible_pfn;
> >
> >  static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > -static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> > +static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
> >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> >  static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
> >  #endif
> > @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
> >
> >       .reserved.regions       = memblock_reserved_init_regions,
> >       .reserved.cnt           = 1,    /* empty dummy entry */
> > -     .reserved.max           = INIT_MEMBLOCK_REGIONS,
> > +     .reserved.max           = INIT_MEMBLOCK_RESERVED_REGIONS,
> >       .reserved.name          = "reserved",
> >
> >  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> > --
> > 2.20.1
> >
>
> --
> Sincerely yours,
> Mike.
>

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-13 13:27 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
  2019-02-14  8:33   ` Mike Rapoport
@ 2019-02-14 14:40   ` Will Deacon
  2019-02-14 15:47   ` Marc Zyngier
  2 siblings, 0 replies; 11+ messages in thread
From: Will Deacon @ 2019-02-14 14:40 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, Marc Zyngier, Catalin Marinas, linux-mm, James Morse,
	Andrew Morton, linux-arm-kernel

On Wed, Feb 13, 2019 at 02:27:37PM +0100, Ard Biesheuvel wrote:
> In the irqchip and EFI code, we have what basically amounts to a quirk
> to work around a peculiarity in the GICv3 architecture, which permits
> the system memory address of LPI tables to be programmable only once
> after a CPU reset. This means kexec kernels must use the same memory
> as the first kernel, and thus ensure that this memory has not been
> given out for other purposes by the time the ITS init code runs, which
> is not very early for secondary CPUs.
> 
> On systems with many CPUs, these reservations could overflow the
> memblock reservation table, and this was addressed in commit
> eff896288872 ("efi/arm: Defer persistent reservations until after
> paging_init()"). However, this turns out to have made things worse,
> since the allocation of page tables and heap space for the resized
> memblock reservation table itself may overwrite the regions we are
> attempting to reserve, which may cause all kinds of corruption,
> also considering that the ITS will still be poking bits into that
> memory in response to incoming MSIs.
> 
> So instead, let's grow the static memblock reservation table on such
> systems so it can accommodate these reservations at an earlier time.
> This will permit us to revert the above commit in a subsequent patch.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/memory.h | 11 +++++++++++
>  include/linux/memblock.h        |  3 ---
>  mm/memblock.c                   | 10 ++++++++--
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index e1ec947e7c0c..7e2b13cdd970 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define virt_addr_valid(kaddr)		\
>  	(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
>  
> +/*
> + * Given that the GIC architecture permits ITS implementations that can only be
> + * configured with a LPI table address once, GICv3 systems with many CPUs may
> + * end up reserving a lot of different regions after a kexec for their LPI
> + * tables, as we are forced to reuse the same memory after kexec (and thus
> + * reserve it persistently with EFI beforehand)
> + */
> +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> +#define INIT_MEMBLOCK_RESERVED_REGIONS	(INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
> +#endif

Assuming this "ought to be enough for anybody", then:

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

Will

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-13 13:27 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
@ 2019-02-14  8:33   ` Mike Rapoport
  2019-02-14 14:57     ` Ard Biesheuvel
  2019-02-14 14:40   ` Will Deacon
  2019-02-14 15:47   ` Marc Zyngier
  2 siblings, 1 reply; 11+ messages in thread
From: Mike Rapoport @ 2019-02-14  8:33 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, Marc Zyngier, Catalin Marinas, Will Deacon, linux-mm,
	James Morse, Andrew Morton, linux-arm-kernel

On Wed, Feb 13, 2019 at 02:27:37PM +0100, Ard Biesheuvel wrote:
> In the irqchip and EFI code, we have what basically amounts to a quirk
> to work around a peculiarity in the GICv3 architecture, which permits
> the system memory address of LPI tables to be programmable only once
> after a CPU reset. This means kexec kernels must use the same memory
> as the first kernel, and thus ensure that this memory has not been
> given out for other purposes by the time the ITS init code runs, which
> is not very early for secondary CPUs.
> 
> On systems with many CPUs, these reservations could overflow the
> memblock reservation table, and this was addressed in commit
> eff896288872 ("efi/arm: Defer persistent reservations until after
> paging_init()"). However, this turns out to have made things worse,
> since the allocation of page tables and heap space for the resized
> memblock reservation table itself may overwrite the regions we are
> attempting to reserve, which may cause all kinds of corruption,
> also considering that the ITS will still be poking bits into that
> memory in response to incoming MSIs.
> 
> So instead, let's grow the static memblock reservation table on such
> systems so it can accommodate these reservations at an earlier time.
> This will permit us to revert the above commit in a subsequent patch.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/memory.h | 11 +++++++++++
>  include/linux/memblock.h        |  3 ---
>  mm/memblock.c                   | 10 ++++++++--
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index e1ec947e7c0c..7e2b13cdd970 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #define virt_addr_valid(kaddr)		\
>  	(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
> 
> +/*
> + * Given that the GIC architecture permits ITS implementations that can only be
> + * configured with a LPI table address once, GICv3 systems with many CPUs may
> + * end up reserving a lot of different regions after a kexec for their LPI
> + * tables, as we are forced to reuse the same memory after kexec (and thus
> + * reserve it persistently with EFI beforehand)
> + */
> +#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
> +#define INIT_MEMBLOCK_RESERVED_REGIONS	(INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
> +#endif
> +
>  #include <asm-generic/memory_model.h>
> 
>  #endif
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 64c41cf45590..859b55b66db2 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -29,9 +29,6 @@ extern unsigned long max_pfn;
>   */
>  extern unsigned long long max_possible_pfn;
> 
> -#define INIT_MEMBLOCK_REGIONS	128
> -#define INIT_PHYSMEM_REGIONS	4
> -
>  /**
>   * enum memblock_flags - definition of memory region attributes
>   * @MEMBLOCK_NONE: no special request
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 022d4cbb3618..a526c3ab8390 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -26,6 +26,12 @@
> 
>  #include "internal.h"
> 
> +#define INIT_MEMBLOCK_REGIONS		128
> +#define INIT_PHYSMEM_REGIONS		4
> +#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
> +#define INIT_MEMBLOCK_RESERVED_REGIONS	INIT_MEMBLOCK_REGIONS
> +#endif
> +

I'd suggest

s/INIT_MEMBLOCK_REGIONS/INIT_MEMORY_REGIONS
s/INIT_MEMBLOCK_RESERVED_REGIONS/INIT_RESERVED_REGIONS

Except that,

Acked-by: Mike Rapoport <rppt@linux.ibm.com>

>  /**
>   * DOC: memblock overview
>   *
> @@ -92,7 +98,7 @@ unsigned long max_pfn;
>  unsigned long long max_possible_pfn;
> 
>  static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> -static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
> +static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
>  static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
>  #endif
> @@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
> 
>  	.reserved.regions	= memblock_reserved_init_regions,
>  	.reserved.cnt		= 1,	/* empty dummy entry */
> -	.reserved.max		= INIT_MEMBLOCK_REGIONS,
> +	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
>  	.reserved.name		= "reserved",
> 
>  #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
> -- 
> 2.20.1
> 

-- 
Sincerely yours,
Mike.


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table
  2019-02-13 13:27 [PATCH 0/2] efi/arm/gicv3: implement fix for memory reservation issue Ard Biesheuvel
@ 2019-02-13 13:27 ` Ard Biesheuvel
  2019-02-14  8:33   ` Mike Rapoport
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2019-02-13 13:27 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, Marc Zyngier, Catalin Marinas, Will Deacon,
	linux-mm, James Morse, Andrew Morton, linux-arm-kernel

In the irqchip and EFI code, we have what basically amounts to a quirk
to work around a peculiarity in the GICv3 architecture, which permits
the system memory address of LPI tables to be programmable only once
after a CPU reset. This means kexec kernels must use the same memory
as the first kernel, and thus ensure that this memory has not been
given out for other purposes by the time the ITS init code runs, which
is not very early for secondary CPUs.

On systems with many CPUs, these reservations could overflow the
memblock reservation table, and this was addressed in commit
eff896288872 ("efi/arm: Defer persistent reservations until after
paging_init()"). However, this turns out to have made things worse,
since the allocation of page tables and heap space for the resized
memblock reservation table itself may overwrite the regions we are
attempting to reserve, which may cause all kinds of corruption,
also considering that the ITS will still be poking bits into that
memory in response to incoming MSIs.

So instead, let's grow the static memblock reservation table on such
systems so it can accommodate these reservations at an earlier time.
This will permit us to revert the above commit in a subsequent patch.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/memory.h | 11 +++++++++++
 include/linux/memblock.h        |  3 ---
 mm/memblock.c                   | 10 ++++++++--
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index e1ec947e7c0c..7e2b13cdd970 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -332,6 +332,17 @@ static inline void *phys_to_virt(phys_addr_t x)
 #define virt_addr_valid(kaddr)		\
 	(_virt_addr_is_linear(kaddr) && _virt_addr_valid(kaddr))
 
+/*
+ * Given that the GIC architecture permits ITS implementations that can only be
+ * configured with a LPI table address once, GICv3 systems with many CPUs may
+ * end up reserving a lot of different regions after a kexec for their LPI
+ * tables, as we are forced to reuse the same memory after kexec (and thus
+ * reserve it persistently with EFI beforehand)
+ */
+#if defined(CONFIG_EFI) && defined(CONFIG_ARM_GIC_V3_ITS)
+#define INIT_MEMBLOCK_RESERVED_REGIONS	(INIT_MEMBLOCK_REGIONS + 2 * NR_CPUS)
+#endif
+
 #include <asm-generic/memory_model.h>
 
 #endif
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 64c41cf45590..859b55b66db2 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -29,9 +29,6 @@ extern unsigned long max_pfn;
  */
 extern unsigned long long max_possible_pfn;
 
-#define INIT_MEMBLOCK_REGIONS	128
-#define INIT_PHYSMEM_REGIONS	4
-
 /**
  * enum memblock_flags - definition of memory region attributes
  * @MEMBLOCK_NONE: no special request
diff --git a/mm/memblock.c b/mm/memblock.c
index 022d4cbb3618..a526c3ab8390 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -26,6 +26,12 @@
 
 #include "internal.h"
 
+#define INIT_MEMBLOCK_REGIONS		128
+#define INIT_PHYSMEM_REGIONS		4
+#ifndef INIT_MEMBLOCK_RESERVED_REGIONS
+#define INIT_MEMBLOCK_RESERVED_REGIONS	INIT_MEMBLOCK_REGIONS
+#endif
+
 /**
  * DOC: memblock overview
  *
@@ -92,7 +98,7 @@ unsigned long max_pfn;
 unsigned long long max_possible_pfn;
 
 static struct memblock_region memblock_memory_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
-static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_REGIONS] __initdata_memblock;
+static struct memblock_region memblock_reserved_init_regions[INIT_MEMBLOCK_RESERVED_REGIONS] __initdata_memblock;
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
 static struct memblock_region memblock_physmem_init_regions[INIT_PHYSMEM_REGIONS] __initdata_memblock;
 #endif
@@ -105,7 +111,7 @@ struct memblock memblock __initdata_memblock = {
 
 	.reserved.regions	= memblock_reserved_init_regions,
 	.reserved.cnt		= 1,	/* empty dummy entry */
-	.reserved.max		= INIT_MEMBLOCK_REGIONS,
+	.reserved.max		= INIT_MEMBLOCK_RESERVED_REGIONS,
 	.reserved.name		= "reserved",
 
 #ifdef CONFIG_HAVE_MEMBLOCK_PHYS_MAP
-- 
2.20.1


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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-02-15 12:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-15 12:33 [GIT PULL 0/2] EFI fixes for v5.0-rc Ard Biesheuvel
2019-02-15 12:33 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
2019-02-15 12:33 ` [PATCH 2/2] efi/arm: Revert "Defer persistent reservations until after paging_init()" Ard Biesheuvel
  -- strict thread matches above, loose matches on Subject: below --
2019-02-13 13:27 [PATCH 0/2] efi/arm/gicv3: implement fix for memory reservation issue Ard Biesheuvel
2019-02-13 13:27 ` [PATCH 1/2] arm64: account for GICv3 LPI tables in static memblock reserve table Ard Biesheuvel
2019-02-14  8:33   ` Mike Rapoport
2019-02-14 14:57     ` Ard Biesheuvel
2019-02-14 15:11       ` Mike Rapoport
2019-02-14 14:40   ` Will Deacon
2019-02-14 15:47   ` Marc Zyngier
2019-02-14 16:55     ` Ard Biesheuvel
2019-02-15 10:34       ` Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).