Re: [PATCH 2/3] riscv: Implement Zicbom-based cache management operations

[Christoph Hellwig <hch@lst.de>]

On Fri, Jun 10, 2022 at 02:43:07AM +0200, Heiko Stuebner wrote:
> +config RISCV_ISA_ZICBOM
> +	bool "Zicbom extension support for non-coherent dma operation"
> +	select ARCH_HAS_DMA_PREP_COHERENT
> +	select ARCH_HAS_SYNC_DMA_FOR_DEVICE
> +	select ARCH_HAS_SYNC_DMA_FOR_CPU
> +	select ARCH_HAS_SETUP_DMA_OPS
> +	select DMA_DIRECT_REMAP
> +	select RISCV_ALTERNATIVE
> +	default y
> +	help
> +	   Adds support to dynamically detect the presence of the ZICBOM extension

Overly long line here.

> +	   (Cache Block Management Operations) and enable its usage.
> +
> +	   If you don't know what to do here, say Y.

But more importantly I think the whole text here is not very helpful.
What users care about is non-coherent DMA support.  What extension is
used for that is rather secondary.  Also please capitalize DMA.

> +void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> +{
> +	switch (dir) {
> +	case DMA_TO_DEVICE:
> +		ALT_CMO_OP(CLEAN, (unsigned long)phys_to_virt(paddr), size, riscv_cbom_block_size);
> +		break;
> +	case DMA_FROM_DEVICE:
> +		ALT_CMO_OP(INVAL, (unsigned long)phys_to_virt(paddr), size, riscv_cbom_block_size);
> +		break;
> +	case DMA_BIDIRECTIONAL:
> +		ALT_CMO_OP(FLUSH, (unsigned long)phys_to_virt(paddr), size, riscv_cbom_block_size);
> +		break;
> +	default:
> +		break;
> +	}

Pleae avoid all these crazy long lines.  and use a logical variable
for the virtual address.  And why do you pass that virtual address
as an unsigned long to ALT_CMO_OP?  You're going to make your life
much easier if you simply always pass a pointer.

Last but not last, does in RISC-V clean mean writeback and flush mean
writeback plus invalidate?  If so the code is correct, but the choice
of names in the RISC-V spec is extremely unfortunate.

> +void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, enum dma_data_direction dir)
> +{
> +	switch (dir) {
> +	case DMA_TO_DEVICE:
> +		break;
> +	case DMA_FROM_DEVICE:
> +	case DMA_BIDIRECTIONAL:
> +		ALT_CMO_OP(INVAL, (unsigned long)phys_to_virt(paddr), size, riscv_cbom_block_size);
> +		break;
> +	default:
> +		break;
> +	}
> +}

Same comment here and in few other places.

> +
> +void arch_dma_prep_coherent(struct page *page, size_t size)
> +{
> +	void *flush_addr = page_address(page);
> +
> +	memset(flush_addr, 0, size);
> +	ALT_CMO_OP(FLUSH, (unsigned long)flush_addr, size, riscv_cbom_block_size);
> +}

arch_dma_prep_coherent should never zero the memory, that is left
for the upper layers.`

> +void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +		const struct iommu_ops *iommu, bool coherent)
> +{
> +	/* If a specific device is dma-coherent, set it here */

This comment isn't all that useful.

> +	dev->dma_coherent = coherent;
> +}

But more importantly, this assums that once this code is built all
devices are non-coherent by default.  I.e. with this patch applied
and the config option enabled we'll now suddenly start doing cache
management operations or setups that didn't do it before.