Re: [PATCH v2 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 16 Jan 2017, Andrii Anisov wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> This function creates userspace mapping for the DMA-coherent memory.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  arch/arm/xen/mm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index bd62d94..ff812a2 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -163,6 +163,19 @@ bool xen_arch_need_swiotlb(struct device *dev,
>  		!is_device_dma_coherent(dev));
>  }
>  
> +/*
> + * Create userspace mapping for the DMA-coherent memory.
> + */
> +static int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> +			 void *cpu_addr, dma_addr_t dma_addr, size_t size,
> +			 unsigned long attrs)
> +{
> +	if (__generic_dma_ops(dev)->mmap)
> +		return __generic_dma_ops(dev)->mmap(dev, vma, cpu_addr, dma_addr, size, attrs);
> +
> +	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> +}
> +
>  int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
>  				 unsigned int address_bits,
>  				 dma_addr_t *dma_handle)
> @@ -198,6 +211,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
>  	.unmap_page = xen_swiotlb_unmap_page,
>  	.dma_supported = xen_swiotlb_dma_supported,
>  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> +	.mmap = xen_swiotlb_dma_mmap,
>  };
>  
>  int __init xen_mm_init(void)

The patch should work fine and looks OK. It is better written like this,
compared to the previous versions that reimplemented dma_common_mmap. I
like the fact that we are reusing the arm specific generic mmap
functions via __generic_dma_ops.

For consistency, I would prefer to have xen_swiotlb_dma_mmap in
drivers/xen/swiotlb-xen.c, even if it needs to be #ifdef'ed CONFIG_ARM
(at least the __generic_dma_ops calls need to be #ifdef'ed).

Konrad, what do you think?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel