linux-kernel - Re: [RFC PATCH v3 24/37] kvx: Add memory management

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: <be04b692-d4ef-4e59-896c-085d92e3d193@kalrayinc.com>
Date: Fri, 23 Aug 2024 18:02:15 +0200
From: Yann Sionneau <ysionneau@...rayinc.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>, "Aneesh
 Kumar K.V" <aneesh.kumar@...nel.org>, Andrew Morton
 <akpm@...ux-foundation.org>, Nick Piggin <npiggin@...il.com>, Peter Zijlstra
 <peterz@...radead.org>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer
 Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Jonathan
 Borne <jborne@...rayinc.com>, Julian Vetter <jvetter@...rayinc.com>, Clement
 Leger <clement@...ment-leger.fr>, Guillaume Thouvenin <thouveng@...il.com>,
 Jean-Christophe Pince <jcpince@...il.com>, Jules Maselbas
 <jmaselbas@...v.net>, Julien Hascoet <jhascoet@...rayinc.com>, Louis Morhet
 <lmorhet@...rayinc.com>, Marc Poulhiès <dkm@...aplop.net>,
 Marius Gligor <mgligor@...rayinc.com>, Vincent Chardon
 <vincent.chardon@...ys-design.com>, linux-arch@...r.kernel.org,
 linux-mm@...ck.org, linux-riscv@...ts.infradead.org, Robin Murphy
 <robin.murphy@....com>
Subject: Re: [RFC PATCH v3 24/37] kvx: Add memory management

Hello Christoph,

On 22/07/2024 16:58, Christoph Hellwig wrote:
>> +#include "../../../drivers/iommu/dma-iommu.h"
> This is not a public header as you can guess from the file path.

Yes indeed this is not "correct" however I didn't know how to do it properly so I am sending this as "RFC" to get some help to clean this up properly :)

If I understood correctly the remaining part of your review: including this header is not needed anymore since we don't need to call `iommu_setup_dma_ops()` anymore.

So let's remove both the incorrect non-public header include and the call to iommu_setup_dma_ops().

>
>> +	switch (dir) {
>> +	case DMA_TO_DEVICE:
>> +		break;
>> +	case DMA_FROM_DEVICE:
>> +		break;
>> +
>> +	case DMA_BIDIRECTIONAL:
>> +		inval_dcache_range(paddr, size);
> Doing this just for bidirectional is weird unless your architecture
> never does any speculative prefetching.  Other architectures
> include DMA_FROM_DEVICE here.
Indeed our arch never does any speculative prefetching on data.
>
>> +#ifdef CONFIG_IOMMU_DMA
>> +void arch_teardown_dma_ops(struct device *dev)
>> +{
>> +	dev->dma_ops = NULL;
>> +}
>> +#endif /* CONFIG_IOMMU_DMA*/
> This should not be needed right now.  And will be completley
> useless once we do the direct calls to dma-iommu which we plan
> to do for Linux 6.12.
Alright, let's remove this.
>
>> +void arch_setup_dma_ops(struct device *dev, bool coherent)
>> +{
>> +	dev->dma_coherent = coherent;
>> +	if (device_iommu_mapped(dev))
>> +		iommu_setup_dma_ops(dev);
>> +}
> And this seems odd, as iommu_setup_dma_ops is called from the iommu
> code and you shouldn't need it here.
Let's remove the `if device_iommu_mapped(dev)) iommu_setup_dma_ops(dev);` then.
>
> I also wonder if we can come up with a way to do the ->dma_coherent
> setup in common code and remove a few of these arch hooks entirely.

Thanks!

Regards,

-- 

Yann