Problem:
=======
On arm64, block and section mapping is supported to build page tables.
However, currently it enforces to take base page mapping for the whole
linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
crashkernel kernel parameter is set. This will cause longer time of the
linear mapping process during bootup and severe performance degradation
during running time.
Root cause:
==========
On arm64, crashkernel reservation relies on knowing the upper limit of
low memory zone because it needs to reserve memory in the zone so that
devices' DMA addressing in kdump kernel can be satisfied. However, the
limit on arm64 is variant. And the upper limit can only be decided late
till bootmem_init() is called.
And we need to map the crashkernel region with base page granularity when
doing linear mapping, because kdump needs to protect the crashkernel region
via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
support well on splitting the built block or section mapping due to some
cpu reststriction [1]. And unfortunately, the linear mapping is done before
bootmem_init().
To resolve the above conflict on arm64, the compromise is enforcing to
take base page mapping for the entire linear mapping if crashkernel is
set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
performance is sacrificed.
Solution:
=========
To fix the problem, we should always take 4G as the crashkernel low
memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
With this, we don't need to defer the crashkernel reservation till
bootmem_init() is called to set the arm64_dma_phys_limit. As long as
memblock init is done, we can conclude what is the upper limit of low
memory zone.
1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
limit = PHYS_ADDR_MAX+1 (Corner cases)
2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
limit = 4G (generic case)
Justification:
==============
In fact, kdump kernel doesn't need to cover all peripherals'
addressing bits. Only device taken as dump target need be taken care of
and its addressing bits need be satified. Currently, there are two kinds
of dumping, dumped to local storage disk or dumped through network card to
remove storage server. It means only storage disk or netowrk card taken
as dump target need be consider if their addressing bits are satisfied.
For saving memory, we usually generate kdump specific initramfs including
necessary kernel modules for dump target devices. All other unnecessary
kernel modules are excluded and their correspondent devices won't be
initialized during kdump kernel bootup.
So far, only Raspberry Pi 4 has some peripherals whcih can only address
30 bits memory range as reported in [2]. Devices on all other arm64 systems
can address 32bits memory range.
So by enforcing to take 4G as the crashkernel low memory end, the only
risk is if RPi4 owns storage disk or network card which can't address
32bits memory range because they could be set as dump target. Even if
RPi4 truly has storage devices or network card which can only address 30
bits memory range, it should be a corner case. We can document it since
crashkernel is more taken as a feature on server. Besides, RPi4 still can
use crashkernel=xM@yM to sepcify a location for 32bits addressing if it
really has that kind of storage device or network card and kdump is expected.
[1]
https://lore.kernel.org/all/[email protected]/T/#u
[2]
[PATCH v6 0/4] Raspberry Pi 4 DMA addressing support
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
======
Question to Nicolas:
Hi Nicolas,
In cover letter of [2] patchset, you told RPi4 has peripherals which
can only address 30bits range. In below sentence, do you mean "the PCIe,
V3D, GENET" can't address 32bit range, or they have wider view of
address space the same as 40-bit DMA channels? I am confused about that.
And the storage device or network card on RPi4 can address 32bit range
or 32bit range, do we have document or do you happen to know that?
"""
The new Raspberry Pi 4 has up to 4GB of memory but most peripherals can
only address the first GB: their DMA address range is
0xc0000000-0xfc000000 which is aliased to the first GB of physical
memory 0x00000000-0x3c000000. Note that only some peripherals have these
limitations: the PCIe, V3D, GENET, and 40-bit DMA channels have a wider
view of the address space by virtue of being hooked up trough a second
interconnect.
"""
Baoquan He (2):
arm64, kdump: enforce to take 4G as the crashkernel low memory end
arm64: remove unneed defer_reserve_crashkernel() and crash_mem_map
arch/arm64/include/asm/memory.h | 5 ----
arch/arm64/mm/init.c | 24 ++++++++-------
arch/arm64/mm/mmu.c | 53 ++++++++++++++-------------------
3 files changed, 36 insertions(+), 46 deletions(-)
base-commit: 10d4879f9ef01cc6190fafe4257d06f375bab92c
--
2.34.1
Now they are not needed any more, clean them up.
Signed-off-by: Baoquan He <[email protected]>
---
arch/arm64/include/asm/memory.h | 5 -----
arch/arm64/mm/mmu.c | 15 ---------------
2 files changed, 20 deletions(-)
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 9dd08cd339c3..b9e71583c9cb 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -363,11 +363,6 @@ static inline void *phys_to_virt(phys_addr_t x)
})
void dump_mem_limit(void);
-
-static inline bool defer_reserve_crashkernel(void)
-{
- return IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32);
-}
#endif /* !ASSEMBLY */
/*
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index cdd338fa2115..c3f8f426c3d8 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -502,21 +502,6 @@ void __init mark_linear_text_alias_ro(void)
PAGE_KERNEL_RO);
}
-static bool crash_mem_map __initdata;
-
-static int __init enable_crash_mem_map(char *arg)
-{
- /*
- * Proper parameter parsing is done by reserve_crashkernel(). We only
- * need to know if the linear map has to avoid block mappings so that
- * the crashkernel reservations can be unmapped later.
- */
- crash_mem_map = true;
-
- return 0;
-}
-early_param("crashkernel", enable_crash_mem_map);
-
static void __init map_mem(pgd_t *pgdp)
{
static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
--
2.34.1
Problem:
=======
On arm64, block and section mapping is supported to build page tables.
However, currently it enforces to take base page mapping for the whole
linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
crashkernel kernel parameter is set. This will cause longer time of the
linear mapping process during bootup and severe performance degradation
during running time.
Root cause:
==========
On arm64, crashkernel reservation relies on knowing the upper limit of
low memory zone because it needs to reserve memory in the zone so that
devices' DMA addressing in kdump kernel can be satisfied. However, the
limit on arm64 is variant. And the upper limit can only be decided late
till bootmem_init() is called.
And we need to map the crashkernel region with base page granularity when
doing linear mapping, because kdump needs to protect the crashkernel region
via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
support well on splitting the built block or section mapping due to some
cpu reststriction [1]. And unfortunately, the linear mapping is done before
bootmem_init().
To resolve the above conflict on arm64, the compromise is enforcing to
take base page mapping for the entire linear mapping if crashkernel is
set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
performance is sacrificed.
Solution:
=========
To fix the problem, we should always take 4G as the crashkernel low
memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
With this, we don't need to defer the crashkernel reservation till
bootmem_init() is called to set the arm64_dma_phys_limit. As long as
memblock init is done, we can conclude what is the upper limit of low
memory zone.
1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
limit = PHYS_ADDR_MAX+1 (Corner cases)
2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
limit = 4G (generic case)
[1]
https://lore.kernel.org/all/[email protected]/T/#u
Signed-off-by: Baoquan He <[email protected]>
---
arch/arm64/mm/init.c | 24 ++++++++++++++----------
arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++----------------
2 files changed, 36 insertions(+), 26 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index b9af30be813e..8ae55afdd11c 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -90,10 +90,22 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
#endif
+static phys_addr_t __init crash_addr_low_max(void)
+{
+ phys_addr_t low_mem_mask = U32_MAX;
+ phys_addr_t phys_start = memblock_start_of_DRAM();
+
+ if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
+ (phys_start > U32_MAX))
+ low_mem_mask = PHYS_ADDR_MAX;
+
+ return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
+}
+
/* Current arm64 boot protocol requires 2MB alignment */
#define CRASH_ALIGN SZ_2M
-#define CRASH_ADDR_LOW_MAX arm64_dma_phys_limit
+#define CRASH_ADDR_LOW_MAX crash_addr_low_max()
#define CRASH_ADDR_HIGH_MAX (PHYS_MASK + 1)
static int __init reserve_crashkernel_low(unsigned long long low_size)
@@ -389,8 +401,7 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
- if (!defer_reserve_crashkernel())
- reserve_crashkernel();
+ reserve_crashkernel();
high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
}
@@ -434,13 +445,6 @@ void __init bootmem_init(void)
*/
dma_contiguous_reserve(arm64_dma_phys_limit);
- /*
- * request_standard_resources() depends on crashkernel's memory being
- * reserved, so do it here.
- */
- if (defer_reserve_crashkernel())
- reserve_crashkernel();
-
memblock_dump_all();
}
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e7ad44585f40..cdd338fa2115 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -547,13 +547,12 @@ static void __init map_mem(pgd_t *pgdp)
memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
#ifdef CONFIG_KEXEC_CORE
- if (crash_mem_map) {
- if (defer_reserve_crashkernel())
- flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
- else if (crashk_res.end)
- memblock_mark_nomap(crashk_res.start,
- resource_size(&crashk_res));
- }
+ if (crashk_res.end)
+ memblock_mark_nomap(crashk_res.start,
+ resource_size(&crashk_res));
+ if (crashk_low_res.end)
+ memblock_mark_nomap(crashk_low_res.start,
+ resource_size(&crashk_low_res));
#endif
/* map all the memory banks */
@@ -589,16 +588,23 @@ static void __init map_mem(pgd_t *pgdp)
* through /sys/kernel/kexec_crash_size interface.
*/
#ifdef CONFIG_KEXEC_CORE
- if (crash_mem_map && !defer_reserve_crashkernel()) {
- if (crashk_res.end) {
- __map_memblock(pgdp, crashk_res.start,
- crashk_res.end + 1,
- PAGE_KERNEL,
- NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
- memblock_clear_nomap(crashk_res.start,
- resource_size(&crashk_res));
- }
+ if (crashk_res.end) {
+ __map_memblock(pgdp, crashk_res.start,
+ crashk_res.end + 1,
+ PAGE_KERNEL,
+ NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+ memblock_clear_nomap(crashk_res.start,
+ resource_size(&crashk_res));
}
+ if (crashk_low_res.end) {
+ __map_memblock(pgdp, crashk_low_res.start,
+ crashk_low_res.end + 1,
+ PAGE_KERNEL,
+ NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+ memblock_clear_nomap(crashk_low_res.start,
+ resource_size(&crashk_low_res));
+ }
+
#endif
}
--
2.34.1
Forgot adding Nicolas when sending patch, add now.
On 08/28/22 at 08:55am, Baoquan He wrote:
> Problem:
> =======
> On arm64, block and section mapping is supported to build page tables.
> However, currently it enforces to take base page mapping for the whole
> linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
> crashkernel kernel parameter is set. This will cause longer time of the
> linear mapping process during bootup and severe performance degradation
> during running time.
>
> Root cause:
> ==========
> On arm64, crashkernel reservation relies on knowing the upper limit of
> low memory zone because it needs to reserve memory in the zone so that
> devices' DMA addressing in kdump kernel can be satisfied. However, the
> limit on arm64 is variant. And the upper limit can only be decided late
> till bootmem_init() is called.
>
> And we need to map the crashkernel region with base page granularity when
> doing linear mapping, because kdump needs to protect the crashkernel region
> via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
> support well on splitting the built block or section mapping due to some
> cpu reststriction [1]. And unfortunately, the linear mapping is done before
> bootmem_init().
>
> To resolve the above conflict on arm64, the compromise is enforcing to
> take base page mapping for the entire linear mapping if crashkernel is
> set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
> performance is sacrificed.
>
> Solution:
> =========
> To fix the problem, we should always take 4G as the crashkernel low
> memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> With this, we don't need to defer the crashkernel reservation till
> bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> memblock init is done, we can conclude what is the upper limit of low
> memory zone.
>
> 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> limit = PHYS_ADDR_MAX+1 (Corner cases)
> 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> limit = 4G (generic case)
>
> Justification:
> ==============
> In fact, kdump kernel doesn't need to cover all peripherals'
> addressing bits. Only device taken as dump target need be taken care of
> and its addressing bits need be satified. Currently, there are two kinds
> of dumping, dumped to local storage disk or dumped through network card to
> remove storage server. It means only storage disk or netowrk card taken
> as dump target need be consider if their addressing bits are satisfied.
> For saving memory, we usually generate kdump specific initramfs including
> necessary kernel modules for dump target devices. All other unnecessary
> kernel modules are excluded and their correspondent devices won't be
> initialized during kdump kernel bootup.
>
> So far, only Raspberry Pi 4 has some peripherals whcih can only address
> 30 bits memory range as reported in [2]. Devices on all other arm64 systems
> can address 32bits memory range.
>
> So by enforcing to take 4G as the crashkernel low memory end, the only
> risk is if RPi4 owns storage disk or network card which can't address
> 32bits memory range because they could be set as dump target. Even if
> RPi4 truly has storage devices or network card which can only address 30
> bits memory range, it should be a corner case. We can document it since
> crashkernel is more taken as a feature on server. Besides, RPi4 still can
> use crashkernel=xM@yM to sepcify a location for 32bits addressing if it
> really has that kind of storage device or network card and kdump is expected.
>
> [1]
> https://lore.kernel.org/all/[email protected]/T/#u
>
> [2]
> [PATCH v6 0/4] Raspberry Pi 4 DMA addressing support
> https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
>
>
> ======
> Question to Nicolas:
>
> Hi Nicolas,
>
> In cover letter of [2] patchset, you told RPi4 has peripherals which
> can only address 30bits range. In below sentence, do you mean "the PCIe,
> V3D, GENET" can't address 32bit range, or they have wider view of
> address space the same as 40-bit DMA channels? I am confused about that.
>
> And the storage device or network card on RPi4 can address 32bit range
> or 32bit range, do we have document or do you happen to know that?
>
> """
> The new Raspberry Pi 4 has up to 4GB of memory but most peripherals can
> only address the first GB: their DMA address range is
> 0xc0000000-0xfc000000 which is aliased to the first GB of physical
> memory 0x00000000-0x3c000000. Note that only some peripherals have these
> limitations: the PCIe, V3D, GENET, and 40-bit DMA channels have a wider
> view of the address space by virtue of being hooked up trough a second
> interconnect.
> """
>
>
> Baoquan He (2):
> arm64, kdump: enforce to take 4G as the crashkernel low memory end
> arm64: remove unneed defer_reserve_crashkernel() and crash_mem_map
>
> arch/arm64/include/asm/memory.h | 5 ----
> arch/arm64/mm/init.c | 24 ++++++++-------
> arch/arm64/mm/mmu.c | 53 ++++++++++++++-------------------
> 3 files changed, 36 insertions(+), 46 deletions(-)
>
>
> base-commit: 10d4879f9ef01cc6190fafe4257d06f375bab92c
> --
> 2.34.1
>
On 2022/8/28 8:55, Baoquan He wrote:
> Now they are not needed any more, clean them up.
Reviewed-by: Zhen Lei <[email protected]>
>
> Signed-off-by: Baoquan He <[email protected]>
> ---
> arch/arm64/include/asm/memory.h | 5 -----
> arch/arm64/mm/mmu.c | 15 ---------------
> 2 files changed, 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 9dd08cd339c3..b9e71583c9cb 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -363,11 +363,6 @@ static inline void *phys_to_virt(phys_addr_t x)
> })
>
> void dump_mem_limit(void);
> -
> -static inline bool defer_reserve_crashkernel(void)
> -{
> - return IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32);
> -}
> #endif /* !ASSEMBLY */
>
> /*
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index cdd338fa2115..c3f8f426c3d8 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -502,21 +502,6 @@ void __init mark_linear_text_alias_ro(void)
> PAGE_KERNEL_RO);
> }
>
> -static bool crash_mem_map __initdata;
> -
> -static int __init enable_crash_mem_map(char *arg)
> -{
> - /*
> - * Proper parameter parsing is done by reserve_crashkernel(). We only
> - * need to know if the linear map has to avoid block mappings so that
> - * the crashkernel reservations can be unmapped later.
> - */
> - crash_mem_map = true;
> -
> - return 0;
> -}
> -early_param("crashkernel", enable_crash_mem_map);
> -
> static void __init map_mem(pgd_t *pgdp)
> {
> static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
>
--
Regards,
Zhen Lei
On 2022/8/28 8:55, Baoquan He wrote:
> Problem:
> =======
> On arm64, block and section mapping is supported to build page tables.
> However, currently it enforces to take base page mapping for the whole
> linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
> crashkernel kernel parameter is set. This will cause longer time of the
> linear mapping process during bootup and severe performance degradation
> during running time.
>
> Root cause:
> ==========
> On arm64, crashkernel reservation relies on knowing the upper limit of
> low memory zone because it needs to reserve memory in the zone so that
> devices' DMA addressing in kdump kernel can be satisfied. However, the
> limit on arm64 is variant. And the upper limit can only be decided late
> till bootmem_init() is called.
>
> And we need to map the crashkernel region with base page granularity when
> doing linear mapping, because kdump needs to protect the crashkernel region
> via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
> support well on splitting the built block or section mapping due to some
> cpu reststriction [1]. And unfortunately, the linear mapping is done before
> bootmem_init().
>
> To resolve the above conflict on arm64, the compromise is enforcing to
> take base page mapping for the entire linear mapping if crashkernel is
> set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
> performance is sacrificed.
>
> Solution:
> =========
> To fix the problem, we should always take 4G as the crashkernel low
> memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> With this, we don't need to defer the crashkernel reservation till
> bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> memblock init is done, we can conclude what is the upper limit of low
> memory zone.
>
> 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> limit = PHYS_ADDR_MAX+1 (Corner cases)
> 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> limit = 4G (generic case)
If Raspberry Pi 4 doesn't need to be considered:
Reviewed-by: Zhen Lei <[email protected]>
In fact, I also think, for the sake of a niche scene, sacrificing the mass scene,
the cart is put before the horse.
>
> [1]
> https://lore.kernel.org/all/[email protected]/T/#u
>
> Signed-off-by: Baoquan He <[email protected]>
> ---
> arch/arm64/mm/init.c | 24 ++++++++++++++----------
> arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++----------------
> 2 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index b9af30be813e..8ae55afdd11c 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -90,10 +90,22 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
> phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
> #endif
>
> +static phys_addr_t __init crash_addr_low_max(void)
> +{
> + phys_addr_t low_mem_mask = U32_MAX;
> + phys_addr_t phys_start = memblock_start_of_DRAM();
> +
> + if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> + (phys_start > U32_MAX))
> + low_mem_mask = PHYS_ADDR_MAX;
> +
> + return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
> +}
> +
> /* Current arm64 boot protocol requires 2MB alignment */
> #define CRASH_ALIGN SZ_2M
>
> -#define CRASH_ADDR_LOW_MAX arm64_dma_phys_limit
> +#define CRASH_ADDR_LOW_MAX crash_addr_low_max()
> #define CRASH_ADDR_HIGH_MAX (PHYS_MASK + 1)
>
> static int __init reserve_crashkernel_low(unsigned long long low_size)
> @@ -389,8 +401,7 @@ void __init arm64_memblock_init(void)
>
> early_init_fdt_scan_reserved_mem();
>
> - if (!defer_reserve_crashkernel())
> - reserve_crashkernel();
> + reserve_crashkernel();
>
> high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
> }
> @@ -434,13 +445,6 @@ void __init bootmem_init(void)
> */
> dma_contiguous_reserve(arm64_dma_phys_limit);
>
> - /*
> - * request_standard_resources() depends on crashkernel's memory being
> - * reserved, so do it here.
> - */
> - if (defer_reserve_crashkernel())
> - reserve_crashkernel();
> -
> memblock_dump_all();
> }
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e7ad44585f40..cdd338fa2115 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -547,13 +547,12 @@ static void __init map_mem(pgd_t *pgdp)
> memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
>
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map) {
> - if (defer_reserve_crashkernel())
> - flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> - else if (crashk_res.end)
> - memblock_mark_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end)
> + memblock_mark_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> + if (crashk_low_res.end)
> + memblock_mark_nomap(crashk_low_res.start,
> + resource_size(&crashk_low_res));
> #endif
>
> /* map all the memory banks */
> @@ -589,16 +588,23 @@ static void __init map_mem(pgd_t *pgdp)
> * through /sys/kernel/kexec_crash_size interface.
> */
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map && !defer_reserve_crashkernel()) {
> - if (crashk_res.end) {
> - __map_memblock(pgdp, crashk_res.start,
> - crashk_res.end + 1,
> - PAGE_KERNEL,
> - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> - memblock_clear_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end) {
> + __map_memblock(pgdp, crashk_res.start,
> + crashk_res.end + 1,
> + PAGE_KERNEL,
> + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> + memblock_clear_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> }
> + if (crashk_low_res.end) {
> + __map_memblock(pgdp, crashk_low_res.start,
> + crashk_low_res.end + 1,
> + PAGE_KERNEL,
> + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> + memblock_clear_nomap(crashk_low_res.start,
> + resource_size(&crashk_low_res));
> + }
> +
> #endif
> }
>
>
--
Regards,
Zhen Lei
On Sun, Aug 28, 2022 at 08:55:44AM +0800, Baoquan He wrote:
> Problem:
> =======
> On arm64, block and section mapping is supported to build page tables.
> However, currently it enforces to take base page mapping for the whole
> linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
> crashkernel kernel parameter is set. This will cause longer time of the
> linear mapping process during bootup and severe performance degradation
> during running time.
>
> Root cause:
> ==========
> On arm64, crashkernel reservation relies on knowing the upper limit of
> low memory zone because it needs to reserve memory in the zone so that
> devices' DMA addressing in kdump kernel can be satisfied. However, the
> limit on arm64 is variant. And the upper limit can only be decided late
> till bootmem_init() is called.
>
> And we need to map the crashkernel region with base page granularity when
> doing linear mapping, because kdump needs to protect the crashkernel region
> via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
> support well on splitting the built block or section mapping due to some
> cpu reststriction [1]. And unfortunately, the linear mapping is done before
> bootmem_init().
>
> To resolve the above conflict on arm64, the compromise is enforcing to
> take base page mapping for the entire linear mapping if crashkernel is
> set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
> performance is sacrificed.
>
> Solution:
> =========
> To fix the problem, we should always take 4G as the crashkernel low
> memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> With this, we don't need to defer the crashkernel reservation till
> bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> memblock init is done, we can conclude what is the upper limit of low
> memory zone.
>
> 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> limit = PHYS_ADDR_MAX+1 (Corner cases)
Why these are corner cases?
The case when CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled is the
simplest one because it does not require the whole dancing around
arm64_dma_phys_limit initialization.
And AFAIK, memblock_start_of_DRAM() > 4G is not uncommon on arm64, but it
does not matter for device DMA addressing.
The actual corner cases are systems with ZONE_DMA/DMA32 and with <32 bits
limit for device DMA addressing (e.g RPi 4). I think the changelog should
mention that to use kdump on these devices user must specify
crashkernel=X@Y
> 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> limit = 4G (generic case)
>
> [1]
> https://lore.kernel.org/all/[email protected]/T/#u
>
> Signed-off-by: Baoquan He <[email protected]>
> ---
> arch/arm64/mm/init.c | 24 ++++++++++++++----------
> arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++----------------
> 2 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index b9af30be813e..8ae55afdd11c 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -90,10 +90,22 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
> phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
> #endif
Please also update the comment above this hunk.
> +static phys_addr_t __init crash_addr_low_max(void)
> +{
> + phys_addr_t low_mem_mask = U32_MAX;
> + phys_addr_t phys_start = memblock_start_of_DRAM();
> +
> + if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> + (phys_start > U32_MAX))
> + low_mem_mask = PHYS_ADDR_MAX;
> +
> + return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
Since RAM frequently starts on non-zero address the limit for systems with
ZONE_DMA/DMA32 should be memblock_start_of_DRAM() + 4G. There is no need to
take into the account the end of DRAM, memblock allocation will take care
of that. I'd suggest to simplify crash_addr_low_max() to be
static phys_addr_t __init crash_addr_low_max(void)
{
if (IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32))
return memblock_start_of_DRAM() + SZ_4G;
return PHYS_ADDR_MAX;
}
> +}
> +
> /* Current arm64 boot protocol requires 2MB alignment */
> #define CRASH_ALIGN SZ_2M
>
> -#define CRASH_ADDR_LOW_MAX arm64_dma_phys_limit
> +#define CRASH_ADDR_LOW_MAX crash_addr_low_max()
With introduction of crash_addr_low_max() I think it's better to get rid of
the CRASH_ADDR_LOW_MAX and use local variables in reserve_crashkernel() and
reserve_crashkernel_low() that would get initialized to
crash_addr_low_max().
Besides, #ifdef around arm64_dma_phys_limit declaration can go away because
this variable will be used only after it is initialized in
zone_sizes_init().
> #define CRASH_ADDR_HIGH_MAX (PHYS_MASK + 1)
>
> static int __init reserve_crashkernel_low(unsigned long long low_size)
> @@ -389,8 +401,7 @@ void __init arm64_memblock_init(void)
>
> early_init_fdt_scan_reserved_mem();
>
> - if (!defer_reserve_crashkernel())
> - reserve_crashkernel();
> + reserve_crashkernel();
>
> high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
> }
> @@ -434,13 +445,6 @@ void __init bootmem_init(void)
> */
> dma_contiguous_reserve(arm64_dma_phys_limit);
>
> - /*
> - * request_standard_resources() depends on crashkernel's memory being
> - * reserved, so do it here.
> - */
> - if (defer_reserve_crashkernel())
> - reserve_crashkernel();
> -
> memblock_dump_all();
> }
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e7ad44585f40..cdd338fa2115 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -547,13 +547,12 @@ static void __init map_mem(pgd_t *pgdp)
> memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
>
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map) {
> - if (defer_reserve_crashkernel())
> - flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> - else if (crashk_res.end)
> - memblock_mark_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end)
> + memblock_mark_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> + if (crashk_low_res.end)
> + memblock_mark_nomap(crashk_low_res.start,
> + resource_size(&crashk_low_res));
> #endif
>
> /* map all the memory banks */
> @@ -589,16 +588,23 @@ static void __init map_mem(pgd_t *pgdp)
> * through /sys/kernel/kexec_crash_size interface.
> */
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map && !defer_reserve_crashkernel()) {
> - if (crashk_res.end) {
> - __map_memblock(pgdp, crashk_res.start,
> - crashk_res.end + 1,
> - PAGE_KERNEL,
> - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> - memblock_clear_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end) {
> + __map_memblock(pgdp, crashk_res.start,
> + crashk_res.end + 1,
> + PAGE_KERNEL,
> + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> + memblock_clear_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> }
> + if (crashk_low_res.end) {
> + __map_memblock(pgdp, crashk_low_res.start,
> + crashk_low_res.end + 1,
> + PAGE_KERNEL,
> + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> + memblock_clear_nomap(crashk_low_res.start,
> + resource_size(&crashk_low_res));
> + }
> +
> #endif
> }
>
> --
> 2.34.1
>
>
--
Sincerely yours,
Mike.
On 08/31/22 at 10:37am, Mike Rapoport wrote:
> On Sun, Aug 28, 2022 at 08:55:44AM +0800, Baoquan He wrote:
> > Problem:
> > =======
> > On arm64, block and section mapping is supported to build page tables.
> > However, currently it enforces to take base page mapping for the whole
> > linear mapping if CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled and
> > crashkernel kernel parameter is set. This will cause longer time of the
> > linear mapping process during bootup and severe performance degradation
> > during running time.
> >
> > Root cause:
> > ==========
> > On arm64, crashkernel reservation relies on knowing the upper limit of
> > low memory zone because it needs to reserve memory in the zone so that
> > devices' DMA addressing in kdump kernel can be satisfied. However, the
> > limit on arm64 is variant. And the upper limit can only be decided late
> > till bootmem_init() is called.
> >
> > And we need to map the crashkernel region with base page granularity when
> > doing linear mapping, because kdump needs to protect the crashkernel region
> > via set_memory_valid(,0) after kdump kernel loading. However, arm64 doesn't
> > support well on splitting the built block or section mapping due to some
> > cpu reststriction [1]. And unfortunately, the linear mapping is done before
> > bootmem_init().
> >
> > To resolve the above conflict on arm64, the compromise is enforcing to
> > take base page mapping for the entire linear mapping if crashkernel is
> > set, and CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabed. Hence
> > performance is sacrificed.
> >
> > Solution:
> > =========
> > To fix the problem, we should always take 4G as the crashkernel low
> > memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> > With this, we don't need to defer the crashkernel reservation till
> > bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> > memblock init is done, we can conclude what is the upper limit of low
> > memory zone.
> >
> > 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> > limit = PHYS_ADDR_MAX+1 (Corner cases)
>
> Why these are corner cases?
> The case when CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled is the
> simplest one because it does not require the whole dancing around
> arm64_dma_phys_limit initialization.
>
> And AFAIK, memblock_start_of_DRAM() > 4G is not uncommon on arm64, but it
> does not matter for device DMA addressing.
Thanks for reviewing.
I could be wrong and have misunderstanding about corner case.
With my understanding, both ZONE_DMA and ZONE_DMA32 are enabled by
default in kernel. And on distros, I believe they are on too. The both
ZONE_DMA and ZONE_DMA32 disabled case should only exist on one specific
product, and the memblock_start_of_DRAM() > 4G case too. At least, I
haven't seen one in our LAB. What I thought the non generic as corner
case could be wrong. I will change that phrasing.
mm/Kconfig:
config ZONE_DMA
bool "Support DMA zone" if ARCH_HAS_ZONE_DMA_SET
default y if ARM64 || X86
config ZONE_DMA32
bool "Support DMA32 zone" if ARCH_HAS_ZONE_DMA_SET
depends on !X86_32
default y if ARM64
>
> The actual corner cases are systems with ZONE_DMA/DMA32 and with <32 bits
> limit for device DMA addressing (e.g RPi 4). I think the changelog should
Right, RPi4's 30bit DMA addressing device is corner case.
> mention that to use kdump on these devices user must specify
> crashkernel=X@Y
Makes sense. I will add words in log, and add sentences to
mention that in code comment or some place of document.
Thanks for advice.
>
> > 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> > limit = 4G (generic case)
> >
> > [1]
> > https://lore.kernel.org/all/[email protected]/T/#u
> >
> > Signed-off-by: Baoquan He <[email protected]>
> > ---
> > arch/arm64/mm/init.c | 24 ++++++++++++++----------
> > arch/arm64/mm/mmu.c | 38 ++++++++++++++++++++++----------------
> > 2 files changed, 36 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index b9af30be813e..8ae55afdd11c 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -90,10 +90,22 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
> > phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
> > #endif
>
> Please also update the comment above this hunk.
Sure, will do.
>
> > +static phys_addr_t __init crash_addr_low_max(void)
> > +{
> > + phys_addr_t low_mem_mask = U32_MAX;
> > + phys_addr_t phys_start = memblock_start_of_DRAM();
> > +
> > + if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > + (phys_start > U32_MAX))
> > + low_mem_mask = PHYS_ADDR_MAX;
> > +
> > + return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
>
> Since RAM frequently starts on non-zero address the limit for systems with
> ZONE_DMA/DMA32 should be memblock_start_of_DRAM() + 4G. There is no need to
It may not be right for memblock_start_of_DRAM(). On most of arm64
servers I ever tested, their memblock usually starts from a higher
address, but not zero which is like x86. E.g below memory ranges printed
on an ampere-mtsnow-altra system, the starting addr is 0x83000000. With
my understanding, DMA addressing bits correspond to the cpu logical
address range devices can address. So memblock_start_of_DRAM() + 4G
seems not right for normal system, and not right for system which
starting physical address is above 4G. I refer to max_zone_phys() of
arch/arm64/mm/init.c when implementing crash_addr_low_max(). Please
correct me if I am wrong.
[ +0.000000] Zone ranges:
[ +0.000000] DMA [mem 0x0000000088300000-0x00000000ffffffff]
[ +0.000000] DMA32 empty
[ +0.000000] Normal [mem 0x0000000100000000-0x00000817ffffffff]
[ +0.000000] Device empty
[ +0.000000] Movable zone start for each node
[ +0.000000] Early memory node ranges
[ +0.000000] node 0: [mem 0x0000000088300000-0x00000000883fffff]
[ +0.000000] node 0: [mem 0x0000000090000000-0x0000000091ffffff]
[ +0.000000] node 0: [mem 0x0000000092000000-0x0000000093ffffff]
[ +0.000000] node 0: [mem 0x0000000094000000-0x00000000ebc18fff]
[ +0.000000] node 0: [mem 0x00000000ebc19000-0x00000000ebfbcfff]
[ +0.000000] node 0: [mem 0x00000000ebfbd000-0x00000000ebfbdfff]
[ +0.000000] node 0: [mem 0x00000000ebfbe000-0x00000000ebfbffff]
[ +0.000000] node 0: [mem 0x00000000ebfc0000-0x00000000ec1dffff]
[ +0.000000] node 0: [mem 0x00000000ec1e0000-0x00000000ec1effff]
[ +0.000000] node 0: [mem 0x00000000ec1f0000-0x00000000ee5effff]
[ +0.000000] node 0: [mem 0x00000000ee5f0000-0x00000000f765ffff]
[ +0.000000] node 0: [mem 0x00000000f7660000-0x00000000f784ffff]
[ +0.000000] node 0: [mem 0x00000000f7850000-0x00000000f7fdffff]
[ +0.000000] node 0: [mem 0x00000000f7fe0000-0x00000000ffc8efff]
[ +0.000000] node 0: [mem 0x00000000ffc8f000-0x00000000ffc8ffff]
[ +0.000000] node 0: [mem 0x00000000ffc90000-0x00000000ffffffff]
[ +0.000000] node 0: [mem 0x0000080000000000-0x000008007fffffff]
[ +0.000000] node 0: [mem 0x0000080100000000-0x00000817ffffffff]
> take into the account the end of DRAM, memblock allocation will take care
> of that. I'd suggest to simplify crash_addr_low_max() to be
>
> static phys_addr_t __init crash_addr_low_max(void)
> {
> if (IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32))
> return memblock_start_of_DRAM() + SZ_4G;
>
> return PHYS_ADDR_MAX;
> }
>
> > +}
> > +
> > /* Current arm64 boot protocol requires 2MB alignment */
> > #define CRASH_ALIGN SZ_2M
> >
> > -#define CRASH_ADDR_LOW_MAX arm64_dma_phys_limit
> > +#define CRASH_ADDR_LOW_MAX crash_addr_low_max()
>
> With introduction of crash_addr_low_max() I think it's better to get rid of
> the CRASH_ADDR_LOW_MAX and use local variables in reserve_crashkernel() and
> reserve_crashkernel_low() that would get initialized to
> crash_addr_low_max().
CRASH_ADDR_LOW_MAX is introduced because we expected to make the
crashkernel reservation code generic and move into kernel/crash_core.c.
The original plan is to deduplicate the x86_64 and arm64 part, seems
it's hard to do now since arm64 has specific handling different than
x86. I think we can remove it now and can add it back if possible.
>
> Besides, #ifdef around arm64_dma_phys_limit declaration can go away because
> this variable will be used only after it is initialized in
> zone_sizes_init().
Right, I will clean it up. Thanks again for careful reviewing.
>
> > #define CRASH_ADDR_HIGH_MAX (PHYS_MASK + 1)
> >
> > static int __init reserve_crashkernel_low(unsigned long long low_size)
> > @@ -389,8 +401,7 @@ void __init arm64_memblock_init(void)
> >
> > early_init_fdt_scan_reserved_mem();
> >
> > - if (!defer_reserve_crashkernel())
> > - reserve_crashkernel();
> > + reserve_crashkernel();
> >
> > high_memory = __va(memblock_end_of_DRAM() - 1) + 1;
> > }
> > @@ -434,13 +445,6 @@ void __init bootmem_init(void)
> > */
> > dma_contiguous_reserve(arm64_dma_phys_limit);
> >
> > - /*
> > - * request_standard_resources() depends on crashkernel's memory being
> > - * reserved, so do it here.
> > - */
> > - if (defer_reserve_crashkernel())
> > - reserve_crashkernel();
> > -
> > memblock_dump_all();
> > }
> >
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index e7ad44585f40..cdd338fa2115 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -547,13 +547,12 @@ static void __init map_mem(pgd_t *pgdp)
> > memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
> >
> > #ifdef CONFIG_KEXEC_CORE
> > - if (crash_mem_map) {
> > - if (defer_reserve_crashkernel())
> > - flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> > - else if (crashk_res.end)
> > - memblock_mark_nomap(crashk_res.start,
> > - resource_size(&crashk_res));
> > - }
> > + if (crashk_res.end)
> > + memblock_mark_nomap(crashk_res.start,
> > + resource_size(&crashk_res));
> > + if (crashk_low_res.end)
> > + memblock_mark_nomap(crashk_low_res.start,
> > + resource_size(&crashk_low_res));
> > #endif
> >
> > /* map all the memory banks */
> > @@ -589,16 +588,23 @@ static void __init map_mem(pgd_t *pgdp)
> > * through /sys/kernel/kexec_crash_size interface.
> > */
> > #ifdef CONFIG_KEXEC_CORE
> > - if (crash_mem_map && !defer_reserve_crashkernel()) {
> > - if (crashk_res.end) {
> > - __map_memblock(pgdp, crashk_res.start,
> > - crashk_res.end + 1,
> > - PAGE_KERNEL,
> > - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> > - memblock_clear_nomap(crashk_res.start,
> > - resource_size(&crashk_res));
> > - }
> > + if (crashk_res.end) {
> > + __map_memblock(pgdp, crashk_res.start,
> > + crashk_res.end + 1,
> > + PAGE_KERNEL,
> > + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> > + memblock_clear_nomap(crashk_res.start,
> > + resource_size(&crashk_res));
> > }
> > +
> > #endif
> > }
> >
> > --
> > 2.34.1
> >
> >
>
> --
> Sincerely yours,
> Mike.
>
On Wed, Aug 31, 2022 at 10:29:39PM +0800, Baoquan He wrote:
> On 08/31/22 at 10:37am, Mike Rapoport wrote:
> > On Sun, Aug 28, 2022 at 08:55:44AM +0800, Baoquan He wrote:
> > >
> > > Solution:
> > > =========
> > > To fix the problem, we should always take 4G as the crashkernel low
> > > memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> > > With this, we don't need to defer the crashkernel reservation till
> > > bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> > > memblock init is done, we can conclude what is the upper limit of low
> > > memory zone.
> > >
> > > 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> > > limit = PHYS_ADDR_MAX+1 (Corner cases)
> >
> > Why these are corner cases?
> > The case when CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled is the
> > simplest one because it does not require the whole dancing around
> > arm64_dma_phys_limit initialization.
> >
> > And AFAIK, memblock_start_of_DRAM() > 4G is not uncommon on arm64, but it
> > does not matter for device DMA addressing.
>
> Thanks for reviewing.
>
> I could be wrong and have misunderstanding about corner case.
>
> With my understanding, both ZONE_DMA and ZONE_DMA32 are enabled by
> default in kernel. And on distros, I believe they are on too. The both
> ZONE_DMA and ZONE_DMA32 disabled case should only exist on one specific
> product, and the memblock_start_of_DRAM() > 4G case too. At least, I
> haven't seen one in our LAB. What I thought the non generic as corner
> case could be wrong. I will change that phrasing.
>
> mm/Kconfig:
> config ZONE_DMA
> bool "Support DMA zone" if ARCH_HAS_ZONE_DMA_SET
> default y if ARM64 || X86
>
> config ZONE_DMA32
> bool "Support DMA32 zone" if ARCH_HAS_ZONE_DMA_SET
> depends on !X86_32
> default y if ARM64
My point was that the cases with ZONE_DMA/DMA32 disabled or with RAM above
4G do not require detection of arm64_dma_phys_limit before reserving the
crash kernel, can use predefined constants and are simple to handle.
> > The actual corner cases are systems with ZONE_DMA/DMA32 and with <32 bits
> > limit for device DMA addressing (e.g RPi 4). I think the changelog should
>
> Right, RPi4's 30bit DMA addressing device is corner case.
>
> > mention that to use kdump on these devices user must specify
> > crashkernel=X@Y
>
> Makes sense. I will add words in log, and add sentences to
> mention that in code comment or some place of document.
> Thanks for advice.
>
> >
> > > 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> > > limit = 4G (generic case)
> > >
...
> > > +static phys_addr_t __init crash_addr_low_max(void)
> > > +{
> > > + phys_addr_t low_mem_mask = U32_MAX;
> > > + phys_addr_t phys_start = memblock_start_of_DRAM();
> > > +
> > > + if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > > + (phys_start > U32_MAX))
> > > + low_mem_mask = PHYS_ADDR_MAX;
> > > +
> > > + return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
> >
> > Since RAM frequently starts on non-zero address the limit for systems with
> > ZONE_DMA/DMA32 should be memblock_start_of_DRAM() + 4G. There is no need to
>
> It may not be right for memblock_start_of_DRAM(). On most of arm64
> servers I ever tested, their memblock usually starts from a higher
> address, but not zero which is like x86. E.g below memory ranges printed
> on an ampere-mtsnow-altra system, the starting addr is 0x83000000. With
> my understanding, DMA addressing bits correspond to the cpu logical
> address range devices can address. So memblock_start_of_DRAM() + 4G
> seems not right for normal system, and not right for system which
> starting physical address is above 4G. I refer to max_zone_phys() of
> arch/arm64/mm/init.c when implementing crash_addr_low_max(). Please
> correct me if I am wrong.
My understanding was that no matter where DRAM starts, the first 4G would
be accessible by 32-bit devices, but I maybe wrong as well :)
I haven't notice you used max_zone_phys() as a reference. Wouldn't it be
simpler to just call it from crash_addr_low_max():
static phys_addr_t __init crash_addr_low_max(void)
{
return max_zone_phys(32);
}
--
Sincerely yours,
Mike.
On 09/01/22 at 10:24am, Mike Rapoport wrote:
> On Wed, Aug 31, 2022 at 10:29:39PM +0800, Baoquan He wrote:
> > On 08/31/22 at 10:37am, Mike Rapoport wrote:
> > > On Sun, Aug 28, 2022 at 08:55:44AM +0800, Baoquan He wrote:
> > > >
> > > > Solution:
> > > > =========
> > > > To fix the problem, we should always take 4G as the crashkernel low
> > > > memory end in case CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 is enabled.
> > > > With this, we don't need to defer the crashkernel reservation till
> > > > bootmem_init() is called to set the arm64_dma_phys_limit. As long as
> > > > memblock init is done, we can conclude what is the upper limit of low
> > > > memory zone.
> > > >
> > > > 1) both CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled or memblock_start_of_DRAM() > 4G
> > > > limit = PHYS_ADDR_MAX+1 (Corner cases)
> > >
> > > Why these are corner cases?
> > > The case when CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are disabled is the
> > > simplest one because it does not require the whole dancing around
> > > arm64_dma_phys_limit initialization.
> > >
> > > And AFAIK, memblock_start_of_DRAM() > 4G is not uncommon on arm64, but it
> > > does not matter for device DMA addressing.
> >
> > Thanks for reviewing.
> >
> > I could be wrong and have misunderstanding about corner case.
> >
> > With my understanding, both ZONE_DMA and ZONE_DMA32 are enabled by
> > default in kernel. And on distros, I believe they are on too. The both
> > ZONE_DMA and ZONE_DMA32 disabled case should only exist on one specific
> > product, and the memblock_start_of_DRAM() > 4G case too. At least, I
> > haven't seen one in our LAB. What I thought the non generic as corner
> > case could be wrong. I will change that phrasing.
> >
> > mm/Kconfig:
> > config ZONE_DMA
> > bool "Support DMA zone" if ARCH_HAS_ZONE_DMA_SET
> > default y if ARM64 || X86
> >
> > config ZONE_DMA32
> > bool "Support DMA32 zone" if ARCH_HAS_ZONE_DMA_SET
> > depends on !X86_32
> > default y if ARM64
>
> My point was that the cases with ZONE_DMA/DMA32 disabled or with RAM above
> 4G do not require detection of arm64_dma_phys_limit before reserving the
> crash kernel, can use predefined constants and are simple to handle.
I see.
>
> > > The actual corner cases are systems with ZONE_DMA/DMA32 and with <32 bits
> > > limit for device DMA addressing (e.g RPi 4). I think the changelog should
> >
> > Right, RPi4's 30bit DMA addressing device is corner case.
> >
> > > mention that to use kdump on these devices user must specify
> > > crashkernel=X@Y
> >
> > Makes sense. I will add words in log, and add sentences to
> > mention that in code comment or some place of document.
> > Thanks for advice.
> >
> > >
> > > > 2) CONFIG_ZONE_DMA or CONFIG_ZONE_DMA32 are enabled:
> > > > limit = 4G (generic case)
> > > >
>
> ...
>
> > > > +static phys_addr_t __init crash_addr_low_max(void)
> > > > +{
> > > > + phys_addr_t low_mem_mask = U32_MAX;
> > > > + phys_addr_t phys_start = memblock_start_of_DRAM();
> > > > +
> > > > + if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > > > + (phys_start > U32_MAX))
> > > > + low_mem_mask = PHYS_ADDR_MAX;
> > > > +
> > > > + return min(low_mem_mask, memblock_end_of_DRAM() - 1) + 1;
> > >
> > > Since RAM frequently starts on non-zero address the limit for systems with
> > > ZONE_DMA/DMA32 should be memblock_start_of_DRAM() + 4G. There is no need to
> >
> > It may not be right for memblock_start_of_DRAM(). On most of arm64
> > servers I ever tested, their memblock usually starts from a higher
> > address, but not zero which is like x86. E.g below memory ranges printed
> > on an ampere-mtsnow-altra system, the starting addr is 0x83000000. With
> > my understanding, DMA addressing bits correspond to the cpu logical
> > address range devices can address. So memblock_start_of_DRAM() + 4G
> > seems not right for normal system, and not right for system which
> > starting physical address is above 4G. I refer to max_zone_phys() of
> > arch/arm64/mm/init.c when implementing crash_addr_low_max(). Please
> > correct me if I am wrong.
>
> My understanding was that no matter where DRAM starts, the first 4G would
> be accessible by 32-bit devices, but I maybe wrong as well :)
>
> I haven't notice you used max_zone_phys() as a reference. Wouldn't it be
> simpler to just call it from crash_addr_low_max():
>
> static phys_addr_t __init crash_addr_low_max(void)
> {
> return max_zone_phys(32);
> }
max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
it like:
static phys_addr_t __init crash_addr_low_max(void)
{
phys_addr_t low_mem_mask = U32_MAX;
phys_addr_t phys_start = memblock_start_of_DRAM();
if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
(phys_start > U32_MAX))
low_mem_mask = PHYS_ADDR_MAX;
return low_mem_mast + 1;
}
or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
as you suggested. Which one do you like better?
static phys_addr_t __init crash_addr_low_max(void)
{
if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
return PHYS_ADDR_MAX + 1;
return max_zone_phys(32);
}
On Thu, Sep 01, 2022 at 08:25:54PM +0800, Baoquan He wrote:
> On 09/01/22 at 10:24am, Mike Rapoport wrote:
>
> max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
> the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
> it like:
>
> static phys_addr_t __init crash_addr_low_max(void)
> {
> phys_addr_t low_mem_mask = U32_MAX;
> phys_addr_t phys_start = memblock_start_of_DRAM();
>
> if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> (phys_start > U32_MAX))
> low_mem_mask = PHYS_ADDR_MAX;
>
> return low_mem_mast + 1;
> }
>
> or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
> as you suggested. Which one do you like better?
>
> static phys_addr_t __init crash_addr_low_max(void)
> {
> if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> return PHYS_ADDR_MAX + 1;
>
> return max_zone_phys(32);
> }
I like the second variant better.
--
Sincerely yours,
Mike.
On 09/05/22 at 01:28pm, Mike Rapoport wrote:
> On Thu, Sep 01, 2022 at 08:25:54PM +0800, Baoquan He wrote:
> > On 09/01/22 at 10:24am, Mike Rapoport wrote:
> >
> > max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
> > the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
> > it like:
> >
> > static phys_addr_t __init crash_addr_low_max(void)
> > {
> > phys_addr_t low_mem_mask = U32_MAX;
> > phys_addr_t phys_start = memblock_start_of_DRAM();
> >
> > if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > (phys_start > U32_MAX))
> > low_mem_mask = PHYS_ADDR_MAX;
> >
> > return low_mem_mast + 1;
> > }
> >
> > or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
> > as you suggested. Which one do you like better?
> >
> > static phys_addr_t __init crash_addr_low_max(void)
> > {
> > if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> > return PHYS_ADDR_MAX + 1;
> >
> > return max_zone_phys(32);
> > }
>
> I like the second variant better.
Sure, will change to use the 2nd one . Thanks.
On Mon, 5 Sept 2022 at 14:08, Baoquan He <[email protected]> wrote:
>
> On 09/05/22 at 01:28pm, Mike Rapoport wrote:
> > On Thu, Sep 01, 2022 at 08:25:54PM +0800, Baoquan He wrote:
> > > On 09/01/22 at 10:24am, Mike Rapoport wrote:
> > >
> > > max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
> > > the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
> > > it like:
> > >
> > > static phys_addr_t __init crash_addr_low_max(void)
> > > {
> > > phys_addr_t low_mem_mask = U32_MAX;
> > > phys_addr_t phys_start = memblock_start_of_DRAM();
> > >
> > > if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > > (phys_start > U32_MAX))
> > > low_mem_mask = PHYS_ADDR_MAX;
> > >
> > > return low_mem_mast + 1;
> > > }
> > >
> > > or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
> > > as you suggested. Which one do you like better?
> > >
> > > static phys_addr_t __init crash_addr_low_max(void)
> > > {
> > > if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> > > return PHYS_ADDR_MAX + 1;
> > >
> > > return max_zone_phys(32);
> > > }
> >
> > I like the second variant better.
>
> Sure, will change to use the 2nd one . Thanks.
>
While I appreciate the effort that has gone into solving this problem,
I don't think there is any consensus that an elaborate fix is required
to ensure that the crash kernel can be unmapped from the linear map at
all cost. In fact, I personally think we shouldn't bother, and IIRC,
Will made a remark along the same lines back when the Huawei engineers
were still driving this effort.
So perhaps we could align on that before doing yet another version of this?
On 09/06/22 at 03:05pm, Ard Biesheuvel wrote:
> On Mon, 5 Sept 2022 at 14:08, Baoquan He <[email protected]> wrote:
> >
> > On 09/05/22 at 01:28pm, Mike Rapoport wrote:
> > > On Thu, Sep 01, 2022 at 08:25:54PM +0800, Baoquan He wrote:
> > > > On 09/01/22 at 10:24am, Mike Rapoport wrote:
> > > >
> > > > max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
> > > > the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
> > > > it like:
> > > >
> > > > static phys_addr_t __init crash_addr_low_max(void)
> > > > {
> > > > phys_addr_t low_mem_mask = U32_MAX;
> > > > phys_addr_t phys_start = memblock_start_of_DRAM();
> > > >
> > > > if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > > > (phys_start > U32_MAX))
> > > > low_mem_mask = PHYS_ADDR_MAX;
> > > >
> > > > return low_mem_mast + 1;
> > > > }
> > > >
> > > > or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
> > > > as you suggested. Which one do you like better?
> > > >
> > > > static phys_addr_t __init crash_addr_low_max(void)
> > > > {
> > > > if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> > > > return PHYS_ADDR_MAX + 1;
> > > >
> > > > return max_zone_phys(32);
> > > > }
> > >
> > > I like the second variant better.
> >
> > Sure, will change to use the 2nd one . Thanks.
> >
>
> While I appreciate the effort that has gone into solving this problem,
> I don't think there is any consensus that an elaborate fix is required
> to ensure that the crash kernel can be unmapped from the linear map at
> all cost. In fact, I personally think we shouldn't bother, and IIRC,
> Will made a remark along the same lines back when the Huawei engineers
> were still driving this effort.
>
> So perhaps we could align on that before doing yet another version of this?
Yes, certainly. That can save everybody's effort if there's different
opinion. Thanks for looking into this and the suggestion.
About Will's remark, I checked those discussing threads, guess you are
mentioning the words in link [1]. I copy them at bottom for better
reference. Pleasae correct me if I am wrong.
With my understanding, Will said so because the patch is too complex,
and there's risk that page table kernel data itself is using could share
the same block/section mapping as crashkernel region. With these
two cons, I agree with Will that we would rather take off the protection
on crashkernel region which is done by mapping or unmapping the region,
even though the protection enhances kdump's ronusness.
Crashkernel reservation needs to know the low meory end so that DMA
buffer can be addressed by the dumping target, e.g storage disk. On the
current arm64, we have facts:
1)Currently, except of Raspberry Pi 4, all arm64 systems can support
32bit DMA addressing. So, except of RPi4, the low memory end can be
decided after memblock init is done, namely at the end of
arm64_memblock_init(). We don't need to defer the crashkernel
reservation until zone_sizes_init() is done. Those cases can be checked
in patch code.
2)For RPi4, if its storage disk is 30bit DMA addressing, then we can
use crashkernel=xM@yM to specify reservation location under 1G to
work around this.
***
Based on above facts, with my patch applied:
pros:
1) Performance issue is resolved;
2) As you can see, the code with this patch applied will much
simpler, more straightforward and clearer;
3) The protection can be kept;
4) Crashkernel reservation can be easier to succeed on small memory
system, e.g virt guest system. The earlier the reservation is done,
it's more likely to get the whole chunk of meomry.
cons:
1) Only RPi4 is put in inconvenience for crashkernel reservation. It
needs to use crashkernel=xM@yM to work around.
***
Take off the protection which is done by mapping or unmapping
crashkernel region as you and Will suggested:
pros:
1) Performance issue is resolved;
2) RPi4 will have the same convenience to set crashkernel;
cons:
1) No protection is taken on crashkernel region;
2) Code logic is twisting. There are two places to separately reserve
crashkernel, one is at the end of arm64_memblock_init(), one is at
the end of bootmem_init().
3) Except of both CONFIG_ZONE_DMA|DMA32 disabled case, crashkernel
reservation is deferred. On small memory system, e.g virt guest system,
it increases risk that the resrevation could fail very possibly caused
by memory fragmentation.
Besides, comparing the above two solutions, I also want to say kdump
is developed for enterprise level of system. We need combine with
reality when considering reasonable solution. E.g on x86_64, it has DMA
zone of 16M and DMA32 zone from 16M to 4G always in normal kernel. For
kdump, we ignore DMA zone directly because it's for ISA style devices.
Kdump doesn't support ISA style device with only 24bit DMA addressing
capability at the beginning, because it doesn't make sense, we never
hear that an enterprise level of x86_64 system needs to arm with kdump.
Hi Ard, Will, Catalin and other reviewers,
Above is my understaning and thinking about the encountered issue,
plesae help check and point out what's missing or incorrect.
Hi Nicolas,
If it's convenient to you, please help make clear if the storage disk or
network card can only address 32bit DMA buffer on RPi4. Really
appreciate that.
***
[1]Will's remark on Huawei's patch
https://lore.kernel.org/all/20220718131005.GA12406@willie-the-truck/T/#u
====quote Will's remark here
I do not think that this complexity is justified. As I have stated on
numerous occasions already, I would prefer that we leave the crashkernel
mapped when rodata is not "full". That fixes your performance issue and
matches what we do for module code, so I do not see a security argument
against it.
I do not plan to merge this patch as-is.
===
On 09/08/22 at 09:33pm, Baoquan He wrote:
> On 09/06/22 at 03:05pm, Ard Biesheuvel wrote:
> > On Mon, 5 Sept 2022 at 14:08, Baoquan He <[email protected]> wrote:
> > >
> > > On 09/05/22 at 01:28pm, Mike Rapoport wrote:
> > > > On Thu, Sep 01, 2022 at 08:25:54PM +0800, Baoquan He wrote:
> > > > > On 09/01/22 at 10:24am, Mike Rapoport wrote:
> > > > >
> > > > > max_zone_phys() only handles cases when CONFIG_ZONE_DMA/DMA32 enabled,
> > > > > the disabledCONFIG_ZONE_DMA/DMA32 case is not included. I can change
> > > > > it like:
> > > > >
> > > > > static phys_addr_t __init crash_addr_low_max(void)
> > > > > {
> > > > > phys_addr_t low_mem_mask = U32_MAX;
> > > > > phys_addr_t phys_start = memblock_start_of_DRAM();
> > > > >
> > > > > if ((!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32)) ||
> > > > > (phys_start > U32_MAX))
> > > > > low_mem_mask = PHYS_ADDR_MAX;
> > > > >
> > > > > return low_mem_mast + 1;
> > > > > }
> > > > >
> > > > > or add the disabled CONFIG_ZONE_DMA/DMA32 case into crash_addr_low_max()
> > > > > as you suggested. Which one do you like better?
> > > > >
> > > > > static phys_addr_t __init crash_addr_low_max(void)
> > > > > {
> > > > > if (!IS_ENABLED(CONFIG_ZONE_DMA) && !IS_ENABLED(CONFIG_ZONE_DMA32))
> > > > > return PHYS_ADDR_MAX + 1;
> > > > >
> > > > > return max_zone_phys(32);
> > > > > }
> > > >
> > > > I like the second variant better.
> > >
> > > Sure, will change to use the 2nd one . Thanks.
> > >
> >
> > While I appreciate the effort that has gone into solving this problem,
> > I don't think there is any consensus that an elaborate fix is required
> > to ensure that the crash kernel can be unmapped from the linear map at
> > all cost. In fact, I personally think we shouldn't bother, and IIRC,
> > Will made a remark along the same lines back when the Huawei engineers
> > were still driving this effort.
> >
> > So perhaps we could align on that before doing yet another version of this?
>
> Yes, certainly. That can save everybody's effort if there's different
> opinion. Thanks for looking into this and the suggestion.
>
> About Will's remark, I checked those discussing threads, guess you are
> mentioning the words in link [1]. I copy them at bottom for better
> reference. Pleasae correct me if I am wrong.
>
> With my understanding, Will said so because the patch is too complex,
> and there's risk that page table kernel data itself is using could share
> the same block/section mapping as crashkernel region. With these
> two cons, I agree with Will that we would rather take off the protection
> on crashkernel region which is done by mapping or unmapping the region,
> even though the protection enhances kdump's ronusness.
>
> Crashkernel reservation needs to know the low meory end so that DMA
> buffer can be addressed by the dumping target, e.g storage disk. On the
> current arm64, we have facts:
> 1)Currently, except of Raspberry Pi 4, all arm64 systems can support
> 32bit DMA addressing. So, except of RPi4, the low memory end can be
> decided after memblock init is done, namely at the end of
> arm64_memblock_init(). We don't need to defer the crashkernel
> reservation until zone_sizes_init() is done. Those cases can be checked
> in patch code.
> 2)For RPi4, if its storage disk is 30bit DMA addressing, then we can
> use crashkernel=xM@yM to specify reservation location under 1G to
> work around this.
>
> ***
> Based on above facts, with my patch applied:
> pros:
> 1) Performance issue is resolved;
> 2) As you can see, the code with this patch applied will much
> simpler, more straightforward and clearer;
> 3) The protection can be kept;
> 4) Crashkernel reservation can be easier to succeed on small memory
> system, e.g virt guest system. The earlier the reservation is done,
> it's more likely to get the whole chunk of meomry.
> cons:
> 1) Only RPi4 is put in inconvenience for crashkernel reservation. It
> needs to use crashkernel=xM@yM to work around.
>
> ***
> Take off the protection which is done by mapping or unmapping
> crashkernel region as you and Will suggested:
> pros:
> 1) Performance issue is resolved;
> 2) RPi4 will have the same convenience to set crashkernel;
>
> cons:
> 1) No protection is taken on crashkernel region;
> 2) Code logic is twisting. There are two places to separately reserve
> crashkernel, one is at the end of arm64_memblock_init(), one is at
> the end of bootmem_init().
> 3) Except of both CONFIG_ZONE_DMA|DMA32 disabled case, crashkernel
> reservation is deferred. On small memory system, e.g virt guest system,
> it increases risk that the resrevation could fail very possibly caused
> by memory fragmentation.
>
> Besides, comparing the above two solutions, I also want to say kdump
> is developed for enterprise level of system. We need combine with
> reality when considering reasonable solution. E.g on x86_64, it has DMA
> zone of 16M and DMA32 zone from 16M to 4G always in normal kernel. For
> kdump, we ignore DMA zone directly because it's for ISA style devices.
> Kdump doesn't support ISA style device with only 24bit DMA addressing
> capability at the beginning, because it doesn't make sense, we never
> hear that an enterprise level of x86_64 system needs to arm with kdump.
Sorry, here I mean we never hear that an enterprise level of x86_64
system owns ISA storage disk and needs to arm with kdump.
>
> Hi Ard, Will, Catalin and other reviewers,
>
> Above is my understaning and thinking about the encountered issue,
> plesae help check and point out what's missing or incorrect.
>
> Hi Nicolas,
>
> If it's convenient to you, please help make clear if the storage disk or
> network card can only address 32bit DMA buffer on RPi4. Really
~~30bit, typo
> appreciate that.
>
> ***
> [1]Will's remark on Huawei's patch
> https://lore.kernel.org/all/20220718131005.GA12406@willie-the-truck/T/#u
>
> ====quote Will's remark here
> I do not think that this complexity is justified. As I have stated on
> numerous occasions already, I would prefer that we leave the crashkernel
> mapped when rodata is not "full". That fixes your performance issue and
> matches what we do for module code, so I do not see a security argument
> against it.
>
> I do not plan to merge this patch as-is.
> ===
>
On Tue, Sep 06, 2022 at 03:05:57PM +0200, Ard Biesheuvel wrote:
>
> While I appreciate the effort that has gone into solving this problem,
> I don't think there is any consensus that an elaborate fix is required
> to ensure that the crash kernel can be unmapped from the linear map at
> all cost. In fact, I personally think we shouldn't bother, and IIRC,
> Will made a remark along the same lines back when the Huawei engineers
> were still driving this effort.
>
> So perhaps we could align on that before doing yet another version of this?
I suggest to start with disabling crash kernel protection when its memory
reservation is deferred and then Baoquan and kdump folks can take it from
here.
From 6430407f784f3571da9b4d79340487f2647a44ab Mon Sep 17 00:00:00 2001
From: Mike Rapoport <[email protected]>
Date: Wed, 21 Sep 2022 10:14:46 +0300
Subject: [PATCH] arm64/mm: don't protect crash kernel memory with
CONFIG_ZONE_DMA/DMA32
Currently, in order to allow protection of crash kernel memory when
CONFIG_ZONE_DMA/DMA32 is enabled, the block mappings in the linear map are
disabled and the entire linear map uses base size pages.
This results in performance degradation because of higher TLB pressure for
kernel memory accesses, so there is a trade off between performance and
ability to protect the crash kernel memory.
Baoquan He said [1]:
In fact, panic is a small probability event, and accidental
corruption on kdump kernel data is a much smaller probability
event.
With this, it makes sense to only protect crash kernel memory only when it
can be reserved before creation of the linear map.
Simplify the logic around crash kernel protection in map_mem() so that it
will use base pages only if crash kernel memory is already reserved and
introduce crashkres_protection_possible variable to ensure that
arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres() won't
try to modify page table if crash kernel is not mapped with base pages.
[1] https://lore.kernel.org/all/Yw2C9ahluhX4Mg3G@MiWiFi-R3L-srv
Suggested-by: Will Deacon <[email protected]>
Signed-off-by: Mike Rapoport <[email protected]>
---
arch/arm64/include/asm/mmu.h | 1 +
arch/arm64/kernel/machine_kexec.c | 6 ++++
arch/arm64/mm/init.c | 30 +++++++++-----------
arch/arm64/mm/mmu.c | 46 ++++++++-----------------------
4 files changed, 32 insertions(+), 51 deletions(-)
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 48f8466a4be9..975607843548 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -71,6 +71,7 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
extern void *fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot);
extern void mark_linear_text_alias_ro(void);
extern bool kaslr_requires_kpti(void);
+extern bool crashkres_protection_possible;
#define INIT_MM_CONTEXT(name) \
.pgd = init_pg_dir,
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 19c2d487cb08..68295403aa40 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -272,6 +272,9 @@ void arch_kexec_protect_crashkres(void)
{
int i;
+ if (!crashkres_protection_possible)
+ return;
+
for (i = 0; i < kexec_crash_image->nr_segments; i++)
set_memory_valid(
__phys_to_virt(kexec_crash_image->segment[i].mem),
@@ -282,6 +285,9 @@ void arch_kexec_unprotect_crashkres(void)
{
int i;
+ if (!crashkres_protection_possible)
+ return;
+
for (i = 0; i < kexec_crash_image->nr_segments; i++)
set_memory_valid(
__phys_to_virt(kexec_crash_image->segment[i].mem),
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index b9af30be813e..220d45655918 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -62,27 +62,21 @@ EXPORT_SYMBOL(memstart_addr);
* In such case, ZONE_DMA32 covers the rest of the 32-bit addressable memory,
* otherwise it is empty.
*
- * Memory reservation for crash kernel either done early or deferred
- * depending on DMA memory zones configs (ZONE_DMA) --
+ * Memory reservation for crash kernel must know the upper limit of low
+ * memory in order to allow DMA access for devices with kdump kernel. When
+ * ZONE_DMA/DMA32 is enabled, this limit is determined after DT/ACPI is
+ * parsed, and crash kernel reservation happens afterwards. In this case,
+ * the crash kernel memory is reserved after linear map is created, there
+ * is no guarantee that crash kernel memory will be mapped with the base
+ * pages in the linear map, and thus the protection if the crash kernel
+ * memory is disabled.
*
* In absence of ZONE_DMA configs arm64_dma_phys_limit initialized
* here instead of max_zone_phys(). This lets early reservation of
* crash kernel memory which has a dependency on arm64_dma_phys_limit.
- * Reserving memory early for crash kernel allows linear creation of block
- * mappings (greater than page-granularity) for all the memory bank rangs.
- * In this scheme a comparatively quicker boot is observed.
- *
- * If ZONE_DMA configs are defined, crash kernel memory reservation
- * is delayed until DMA zone memory range size initialization performed in
- * zone_sizes_init(). The defer is necessary to steer clear of DMA zone
- * memory range to avoid overlap allocation. So crash kernel memory boundaries
- * are not known when mapping all bank memory ranges, which otherwise means
- * not possible to exclude crash kernel range from creating block mappings
- * so page-granularity mappings are created for the entire memory range.
- * Hence a slightly slower boot is observed.
- *
- * Note: Page-granularity mappings are necessary for crash kernel memory
- * range for shrinking its size via /sys/kernel/kexec_crash_size interface.
+ * Reserving crash kernel memory early allows mapping it with base pages in
+ * the linear map so that it can be protected, without preventing usage of
+ * block mappings for creation of the linear map.
*/
#if IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32)
phys_addr_t __ro_after_init arm64_dma_phys_limit;
@@ -90,6 +84,8 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
#endif
+bool __ro_after_init crashkres_protection_possible;
+
/* Current arm64 boot protocol requires 2MB alignment */
#define CRASH_ALIGN SZ_2M
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index c5065abec55a..7b40f38dd3ee 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -502,21 +502,6 @@ void __init mark_linear_text_alias_ro(void)
PAGE_KERNEL_RO);
}
-static bool crash_mem_map __initdata;
-
-static int __init enable_crash_mem_map(char *arg)
-{
- /*
- * Proper parameter parsing is done by reserve_crashkernel(). We only
- * need to know if the linear map has to avoid block mappings so that
- * the crashkernel reservations can be unmapped later.
- */
- crash_mem_map = true;
-
- return 0;
-}
-early_param("crashkernel", enable_crash_mem_map);
-
static void __init map_mem(pgd_t *pgdp)
{
static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
@@ -547,13 +532,9 @@ static void __init map_mem(pgd_t *pgdp)
memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
#ifdef CONFIG_KEXEC_CORE
- if (crash_mem_map) {
- if (defer_reserve_crashkernel())
- flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
- else if (crashk_res.end)
- memblock_mark_nomap(crashk_res.start,
- resource_size(&crashk_res));
- }
+ if (crashk_res.end)
+ memblock_mark_nomap(crashk_res.start,
+ resource_size(&crashk_res));
#endif
/* map all the memory banks */
@@ -584,20 +565,17 @@ static void __init map_mem(pgd_t *pgdp)
memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
/*
- * Use page-level mappings here so that we can shrink the region
- * in page granularity and put back unused memory to buddy system
- * through /sys/kernel/kexec_crash_size interface.
+ * Use page-level mappings here so that we can protect crash kernel
+ * memory to allow post-mortem analysis despite memory errors in
+ * the main kernel.
*/
#ifdef CONFIG_KEXEC_CORE
- if (crash_mem_map && !defer_reserve_crashkernel()) {
- if (crashk_res.end) {
- __map_memblock(pgdp, crashk_res.start,
- crashk_res.end + 1,
- PAGE_KERNEL,
- NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
- memblock_clear_nomap(crashk_res.start,
- resource_size(&crashk_res));
- }
+ if (crashk_res.end) {
+ __map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
+ PAGE_KERNEL, NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+ memblock_clear_nomap(crashk_res.start,
+ resource_size(&crashk_res));
+ crashkres_protection_possible = true;
}
#endif
}
--
2.35.3
--
Sincerely yours,
Mike.
Hi Mike,
On 09/21/22 at 10:45am, Mike Rapoport wrote:
> On Tue, Sep 06, 2022 at 03:05:57PM +0200, Ard Biesheuvel wrote:
> >
> > While I appreciate the effort that has gone into solving this problem,
> > I don't think there is any consensus that an elaborate fix is required
> > to ensure that the crash kernel can be unmapped from the linear map at
> > all cost. In fact, I personally think we shouldn't bother, and IIRC,
> > Will made a remark along the same lines back when the Huawei engineers
> > were still driving this effort.
> >
> > So perhaps we could align on that before doing yet another version of this?
>
> I suggest to start with disabling crash kernel protection when its memory
> reservation is deferred and then Baoquan and kdump folks can take it from
> here.
Thanks for the attempt, really appreciated. We all tried and all see
everybody's effort on this issue.
If disabling protection is chosen, I would suggest disable it at all.
The system w/o having CONFIG_ZONE_DMA|32 is rarely seen, I don't think
we need do the protection for it specifically to make code inconsistent
and could confuse people. We can revert below commit and its later
change do to that.
commit 98d2e1539b84 ("arm64: kdump: protect crash dump kernel memory")
For RPi4, I tried to find one to test and figure out if it can do crash
dumping with buffer above 1G. However, nobody care about kdump when I
asked people around who have one at hand for testing, hobby, or
developping. Since they are not familiar with kdump setting and not so
eager to get to know, and I don't want to take up too much time, finally
I just give up.
So, if solution in this patchset is not accepted, I would like to see
the protection code is reverted. Other opinion, please?
>
> From 6430407f784f3571da9b4d79340487f2647a44ab Mon Sep 17 00:00:00 2001
> From: Mike Rapoport <[email protected]>
> Date: Wed, 21 Sep 2022 10:14:46 +0300
> Subject: [PATCH] arm64/mm: don't protect crash kernel memory with
> CONFIG_ZONE_DMA/DMA32
>
> Currently, in order to allow protection of crash kernel memory when
> CONFIG_ZONE_DMA/DMA32 is enabled, the block mappings in the linear map are
> disabled and the entire linear map uses base size pages.
>
> This results in performance degradation because of higher TLB pressure for
> kernel memory accesses, so there is a trade off between performance and
> ability to protect the crash kernel memory.
>
> Baoquan He said [1]:
>
> In fact, panic is a small probability event, and accidental
> corruption on kdump kernel data is a much smaller probability
> event.
>
> With this, it makes sense to only protect crash kernel memory only when it
> can be reserved before creation of the linear map.
>
> Simplify the logic around crash kernel protection in map_mem() so that it
> will use base pages only if crash kernel memory is already reserved and
> introduce crashkres_protection_possible variable to ensure that
> arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres() won't
> try to modify page table if crash kernel is not mapped with base pages.
>
> [1] https://lore.kernel.org/all/Yw2C9ahluhX4Mg3G@MiWiFi-R3L-srv
>
> Suggested-by: Will Deacon <[email protected]>
> Signed-off-by: Mike Rapoport <[email protected]>
> ---
> arch/arm64/include/asm/mmu.h | 1 +
> arch/arm64/kernel/machine_kexec.c | 6 ++++
> arch/arm64/mm/init.c | 30 +++++++++-----------
> arch/arm64/mm/mmu.c | 46 ++++++++-----------------------
> 4 files changed, 32 insertions(+), 51 deletions(-)
>
> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
> index 48f8466a4be9..975607843548 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -71,6 +71,7 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
> extern void *fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot);
> extern void mark_linear_text_alias_ro(void);
> extern bool kaslr_requires_kpti(void);
> +extern bool crashkres_protection_possible;
>
> #define INIT_MM_CONTEXT(name) \
> .pgd = init_pg_dir,
> diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> index 19c2d487cb08..68295403aa40 100644
> --- a/arch/arm64/kernel/machine_kexec.c
> +++ b/arch/arm64/kernel/machine_kexec.c
> @@ -272,6 +272,9 @@ void arch_kexec_protect_crashkres(void)
> {
> int i;
>
> + if (!crashkres_protection_possible)
> + return;
> +
> for (i = 0; i < kexec_crash_image->nr_segments; i++)
> set_memory_valid(
> __phys_to_virt(kexec_crash_image->segment[i].mem),
> @@ -282,6 +285,9 @@ void arch_kexec_unprotect_crashkres(void)
> {
> int i;
>
> + if (!crashkres_protection_possible)
> + return;
> +
> for (i = 0; i < kexec_crash_image->nr_segments; i++)
> set_memory_valid(
> __phys_to_virt(kexec_crash_image->segment[i].mem),
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index b9af30be813e..220d45655918 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -62,27 +62,21 @@ EXPORT_SYMBOL(memstart_addr);
> * In such case, ZONE_DMA32 covers the rest of the 32-bit addressable memory,
> * otherwise it is empty.
> *
> - * Memory reservation for crash kernel either done early or deferred
> - * depending on DMA memory zones configs (ZONE_DMA) --
> + * Memory reservation for crash kernel must know the upper limit of low
> + * memory in order to allow DMA access for devices with kdump kernel. When
> + * ZONE_DMA/DMA32 is enabled, this limit is determined after DT/ACPI is
> + * parsed, and crash kernel reservation happens afterwards. In this case,
> + * the crash kernel memory is reserved after linear map is created, there
> + * is no guarantee that crash kernel memory will be mapped with the base
> + * pages in the linear map, and thus the protection if the crash kernel
> + * memory is disabled.
> *
> * In absence of ZONE_DMA configs arm64_dma_phys_limit initialized
> * here instead of max_zone_phys(). This lets early reservation of
> * crash kernel memory which has a dependency on arm64_dma_phys_limit.
> - * Reserving memory early for crash kernel allows linear creation of block
> - * mappings (greater than page-granularity) for all the memory bank rangs.
> - * In this scheme a comparatively quicker boot is observed.
> - *
> - * If ZONE_DMA configs are defined, crash kernel memory reservation
> - * is delayed until DMA zone memory range size initialization performed in
> - * zone_sizes_init(). The defer is necessary to steer clear of DMA zone
> - * memory range to avoid overlap allocation. So crash kernel memory boundaries
> - * are not known when mapping all bank memory ranges, which otherwise means
> - * not possible to exclude crash kernel range from creating block mappings
> - * so page-granularity mappings are created for the entire memory range.
> - * Hence a slightly slower boot is observed.
> - *
> - * Note: Page-granularity mappings are necessary for crash kernel memory
> - * range for shrinking its size via /sys/kernel/kexec_crash_size interface.
> + * Reserving crash kernel memory early allows mapping it with base pages in
> + * the linear map so that it can be protected, without preventing usage of
> + * block mappings for creation of the linear map.
> */
> #if IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32)
> phys_addr_t __ro_after_init arm64_dma_phys_limit;
> @@ -90,6 +84,8 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
> phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
> #endif
>
> +bool __ro_after_init crashkres_protection_possible;
> +
> /* Current arm64 boot protocol requires 2MB alignment */
> #define CRASH_ALIGN SZ_2M
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index c5065abec55a..7b40f38dd3ee 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -502,21 +502,6 @@ void __init mark_linear_text_alias_ro(void)
> PAGE_KERNEL_RO);
> }
>
> -static bool crash_mem_map __initdata;
> -
> -static int __init enable_crash_mem_map(char *arg)
> -{
> - /*
> - * Proper parameter parsing is done by reserve_crashkernel(). We only
> - * need to know if the linear map has to avoid block mappings so that
> - * the crashkernel reservations can be unmapped later.
> - */
> - crash_mem_map = true;
> -
> - return 0;
> -}
> -early_param("crashkernel", enable_crash_mem_map);
> -
> static void __init map_mem(pgd_t *pgdp)
> {
> static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
> @@ -547,13 +532,9 @@ static void __init map_mem(pgd_t *pgdp)
> memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
>
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map) {
> - if (defer_reserve_crashkernel())
> - flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> - else if (crashk_res.end)
> - memblock_mark_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end)
> + memblock_mark_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> #endif
>
> /* map all the memory banks */
> @@ -584,20 +565,17 @@ static void __init map_mem(pgd_t *pgdp)
> memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
>
> /*
> - * Use page-level mappings here so that we can shrink the region
> - * in page granularity and put back unused memory to buddy system
> - * through /sys/kernel/kexec_crash_size interface.
> + * Use page-level mappings here so that we can protect crash kernel
> + * memory to allow post-mortem analysis despite memory errors in
> + * the main kernel.
> */
> #ifdef CONFIG_KEXEC_CORE
> - if (crash_mem_map && !defer_reserve_crashkernel()) {
> - if (crashk_res.end) {
> - __map_memblock(pgdp, crashk_res.start,
> - crashk_res.end + 1,
> - PAGE_KERNEL,
> - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> - memblock_clear_nomap(crashk_res.start,
> - resource_size(&crashk_res));
> - }
> + if (crashk_res.end) {
> + __map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
> + PAGE_KERNEL, NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
> + memblock_clear_nomap(crashk_res.start,
> + resource_size(&crashk_res));
> + crashkres_protection_possible = true;
> }
> #endif
> }
> --
> 2.35.3
>
>
> --
> Sincerely yours,
> Mike.
>
On 09/30/22 at 03:04pm, Baoquan He wrote:
> Hi Mike,
>
> On 09/21/22 at 10:45am, Mike Rapoport wrote:
> > On Tue, Sep 06, 2022 at 03:05:57PM +0200, Ard Biesheuvel wrote:
> > >
> > > While I appreciate the effort that has gone into solving this problem,
> > > I don't think there is any consensus that an elaborate fix is required
> > > to ensure that the crash kernel can be unmapped from the linear map at
> > > all cost. In fact, I personally think we shouldn't bother, and IIRC,
> > > Will made a remark along the same lines back when the Huawei engineers
> > > were still driving this effort.
> > >
> > > So perhaps we could align on that before doing yet another version of this?
> >
> > I suggest to start with disabling crash kernel protection when its memory
> > reservation is deferred and then Baoquan and kdump folks can take it from
> > here.
>
> Thanks for the attempt, really appreciated. We all tried and all see
> everybody's effort on this issue.
>
> If disabling protection is chosen, I would suggest disable it at all.
> The system w/o having CONFIG_ZONE_DMA|32 is rarely seen, I don't think
> we need do the protection for it specifically to make code inconsistent
> and could confuse people. We can revert below commit and its later
> change do to that.
>
> commit 98d2e1539b84 ("arm64: kdump: protect crash dump kernel memory")
>
> For RPi4, I tried to find one to test and figure out if it can do crash
> dumping with buffer above 1G. However, nobody care about kdump when I
> asked people around who have one at hand for testing, hobby, or
> developping. Since they are not familiar with kdump setting and not so
> eager to get to know, and I don't want to take up too much time, finally
> I just give up.
Finally, I got help from Chao who is team lead of our kernel QE. Chao
has a RPi4 and he tested different cases of kdump, now we can confirm
that on RPi4 the TF card and usb disk are all 30bit DMA addressing
limited. Just FYI. And thanks again to Chao for his help and patience.