| Message ID | 20190213132738.10294-2-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi/arm/gicv3: implement fix for memory reservation issue | expand |
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 >
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
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. >
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. > > >
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.
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...
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.
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
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(-)