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

[Robin Murphy <robin.murphy@arm.com>]

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

More than that, per 8b80549f1bc6, it's now actually a latent bug.

>  And will be completley
> useless once we do the direct calls to dma-iommu which we plan
> to do for Linux 6.12.
> 
>> +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.

Yeah, this smells like it was based on the old arm64 code, but then 
rebased without reference to the equivalent arm64 changes, hence the 
#include hack. Enabling iommu-dma for an architecture should now be a 
one-liner in drivers/iommu/Kconfig, however I don't see an IOMMU driver 
being added in this series so it's not clear it's actually necessary (yet).

Thanks,
Robin.