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

[Yann Sionneau <ysionneau@kalrayinc.com>]

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