* 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* 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-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-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: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 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-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