linux-kernel - Re: [PATCH RFC 3/4] dma-direct: add offset to zone_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZZw3FDy8800NScEk@arm.com>
Date: Mon, 8 Jan 2024 17:55:32 +0000
From: Catalin Marinas <catalin.marinas@....com>
To: Baruch Siach <baruch@...s.co.il>
Cc: Christoph Hellwig <hch@....de>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
	iommu@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Petr Tesařík <petr@...arici.cz>,
	Ramon Fried <ramon@...reality.ai>
Subject: Re: [PATCH RFC 3/4] dma-direct: add offset to zone_dma_bits

On Wed, Dec 27, 2023 at 05:04:27PM +0200, Baruch Siach wrote:
> Current code using zone_dma_bits assume that all addresses range in the
> bits mask are suitable for DMA. For some existing platforms this
> assumption is not correct. DMA range might have non zero lower limit.
> 
> Add 'zone_dma_off' for platform code to set base address for DMA zone.
> 
> Rename the dma_direct_supported() local 'min_mask' variable to better
> describe its use as limit.
> 
> Suggested-by: Catalin Marinas <catalin.marinas@....com>

When I suggested taking the DMA offsets into account, that's not exactly
what I meant. Based on patch 4, it looks like zone_dma_off is equivalent
to the lower CPU address. Let's say a system has DRAM starting at 2GB
and all 32-bit DMA-capable devices has a DMA offset of 0. We want
ZONE_DMA32 to end at 4GB rather than 6GB.

> @@ -59,7 +60,7 @@ static gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 *phys_limit)
>  	 * zones.
>  	 */
>  	*phys_limit = dma_to_phys(dev, dma_limit);
> -	if (*phys_limit <= DMA_BIT_MASK(zone_dma_bits))
> +	if (*phys_limit <= zone_dma_off + DMA_BIT_MASK(zone_dma_bits))
>  		return GFP_DMA;
>  	if (*phys_limit <= DMA_BIT_MASK(32))
>  		return GFP_DMA32;

Ah, you ignore the zone_dma_off for 32-bit calculations. But the
argument still stands, the start of DRAM does not necessarily mean that
all non-64-bit devices have such DMA offset.

The current dma_direct_optimal_gfp_mask() confuses me a bit, I think it
gives the wrong flag if we have a zone_dma_bits of 30 and a device with
a coherent_dma_mask of 31, it incorrectly ends up with GFP_DMA32 (I'm
ignoring dma offsets in this example). Luckily I don't think we have any
set up where this would fail. Basically if *phys_limit is strictly
smaller than DMA_BIT_MASK(32), we want GFP_DMA rather than GFP_DMA32
even if it is larger than DMA_BIT_MASK(zone_dma_bits).

Anyway, current mainline assumes that DMA_BIT_MASK(zone_dma_bits) and
DMA_BIT_MASK(32) are CPU addresses. The problem is that we may have the
start of RAM well above 4GB and neither ZONE_DMA nor ZONE_DMA32 upper
limits would be a power-of-two. We could change the DMA_BIT_MASK(...) to
be DMA address limits and we end up with something like:

static gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 *phys_limit)
{
	u64 dma_limit = min_not_zero(
		dev->coherent_dma_mask,
		dev->bus_dma_limit);
	u64 dma32_limit = dma_to_phys(dev, DMA_BIT_MASK(32));

	*phys_limit = dma_to_phys(dev, dma_limit);
	if (*phys_limit > dma_limit)
		return 0;
	if (*phys_limit = dma32_limit)
		return GFP_DMA32;
	return GFP_DMA;
}

The alternative is to get rid of the *_bits variants and go for
zone_dma_limit and zone_dma32_limit in the generic code. For most
architectures they would match the current DMA_BIT_MASK(32) etc. but
arm64 would be able to set some higher values.

My preference would be to go for zone_dma{,32}_limit, it's easier to
change all the places where DMA_BIT_MASK({zone_dma_bits,32}) is used.

-- 
Catalin