Re: [PATCH v6 RESED 1/2] dma: replace zone_dma_bits by zone_dma_limit

[Petr Tesařík]

On Fri, 16 Aug 2024 12:52:47 +0100
Will Deacon <will@xxxxxxxxxx> wrote:

> On Sun, Aug 11, 2024 at 10:09:35AM +0300, Baruch Siach wrote:
> > From: Catalin Marinas <catalin.marinas@xxxxxxx>
> > 
> > Hardware DMA limit might not be power of 2. When RAM range starts above
> > 0, say 4GB, DMA limit of 30 bits should end at 5GB. A single high bit
> > can not encode this limit.
> > 
> > Use plain address for DMA zone limit.
> > 
> > Since DMA zone can now potentially span beyond 4GB physical limit of
> > DMA32, make sure to use DMA zone for GFP_DMA32 allocations in that case.
> > 
> > Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Co-developed-by: Baruch Siach <baruch@xxxxxxxxxx>
> > Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>
> > ---
> >  arch/arm64/mm/init.c       | 30 +++++++++++++++---------------
> >  arch/powerpc/mm/mem.c      |  5 ++++-
> >  arch/s390/mm/init.c        |  2 +-
> >  include/linux/dma-direct.h |  2 +-
> >  kernel/dma/direct.c        |  6 +++---
> >  kernel/dma/pool.c          |  4 ++--
> >  kernel/dma/swiotlb.c       |  6 +++---
> >  7 files changed, 29 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index 9b5ab6818f7f..c45e2152ca9e 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -115,35 +115,35 @@ static void __init arch_reserve_crashkernel(void)
> >  }
> >  
> >  /*
> > - * Return the maximum physical address for a zone accessible by the given bits
> > - * limit. If DRAM starts above 32-bit, expand the zone to the maximum
> > + * Return the maximum physical address for a zone given its limit.
> > + * If DRAM starts above 32-bit, expand the zone to the maximum
> >   * available memory, otherwise cap it at 32-bit.
> >   */
> > -static phys_addr_t __init max_zone_phys(unsigned int zone_bits)
> > +static phys_addr_t __init max_zone_phys(phys_addr_t zone_limit)
> >  {
> > -	phys_addr_t zone_mask = DMA_BIT_MASK(zone_bits);
> >  	phys_addr_t phys_start = memblock_start_of_DRAM();
> >  
> >  	if (phys_start > U32_MAX)
> > -		zone_mask = PHYS_ADDR_MAX;
> > -	else if (phys_start > zone_mask)
> > -		zone_mask = U32_MAX;
> > +		zone_limit = PHYS_ADDR_MAX;
> > +	else if (phys_start > zone_limit)
> > +		zone_limit = U32_MAX;
> >  
> > -	return min(zone_mask, memblock_end_of_DRAM() - 1) + 1;
> > +	return min(zone_limit, memblock_end_of_DRAM() - 1) + 1;  
> 
> Why do we need to adjust +-1 now that we're no longer using a mask?

Subtracting 1 is needed to get the highest valid DRAM address so it can
be compared to the highest address in the zone (zone_limit). Adding 1
is necessary to get the lowest address beyond the zone.

AFAICT this is the right thing here:

	arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
	max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);

max_zone_pfns[] max values are exclusive, i.e. the lowest PFN which is
_not_ within the zone.

It is also the right thing when arm64_dma_phys_limit is passed to
dma_contiguous_reserve().

It would be subtly broken if phys_addr_t could be a 32-bit integer, but
that's not possible on arm64.

In short, LGTM.

Petr T