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);
}
