On 07/30/2018 02:54 PM, Christoph Hellwig wrote:
>> +/*
>> + * Virtio direct mapping DMA API operations structure
>> + *
>> + * This defines DMA API structure for all virtio devices which would not
>> + * either bring in their own DMA OPS from architecture or they would not
>> + * like to use architecture specific IOMMU based DMA OPS because QEMU
>> + * expects GPA instead of an IOVA in absence of VIRTIO_F_IOMMU_PLATFORM.
>> + */
>> +dma_addr_t virtio_direct_map_page(struct device *dev, struct page *page,
>> + unsigned long offset, size_t size,
>> + enum dma_data_direction dir,
>> + unsigned long attrs)
>
> All these functions should probably be marked static.
Sure.
>
>> +void virtio_direct_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
>> + size_t size, enum dma_data_direction dir,
>> + unsigned long attrs)
>> +{
>> +}
>
> No need to implement no-op callbacks in struct dma_map_ops.
Okay.
>
>> +
>> +int virtio_direct_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
>> +{
>> + return 0;
>> +}
>
> Including this one.
>
>> +void *virtio_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
>> + gfp_t gfp, unsigned long attrs)
>> +{
>> + void *queue = alloc_pages_exact(PAGE_ALIGN(size), gfp);
>> +
>> + if (queue) {
>> + phys_addr_t phys_addr = virt_to_phys(queue);
>> + *dma_handle = (dma_addr_t)phys_addr;
>> +
>> + if (WARN_ON_ONCE(*dma_handle != phys_addr)) {
>> + free_pages_exact(queue, PAGE_ALIGN(size));
>> + return NULL;
>> + }
>> + }
>> + return queue;
>
> queue is a very odd name in a generic memory allocator.
Will change it to addr.
>
>> +void virtio_direct_free(struct device *dev, size_t size, void *vaddr,
>> + dma_addr_t dma_addr, unsigned long attrs)
>> +{
>> + free_pages_exact(vaddr, PAGE_ALIGN(size));
>> +}
>> +
>> +const struct dma_map_ops virtio_direct_dma_ops = {
>> + .alloc = virtio_direct_alloc,
>> + .free = virtio_direct_free,
>> + .map_page = virtio_direct_map_page,
>> + .unmap_page = virtio_direct_unmap_page,
>> + .mapping_error = virtio_direct_mapping_error,
>> +};
>
> This is missing a dma_map_sg implementation. In general this is
> mandatory for dma_ops. So either you implement it or explain in
> a common why you think you can skip it.
Hmm. IIUC virtio core never used dma_map_sg(). Am I missing something
here ? The only reference to dma_map_sg() is inside a comment.
$git grep dma_map_sg drivers/virtio/
drivers/virtio/virtio_ring.c: * We can't use dma_map_sg, because we don't use scatterlists in
>
>> +EXPORT_SYMBOL(virtio_direct_dma_ops);
>
> EXPORT_SYMBOL_GPL like all virtio symbols, please.
I am planning to drop EXPORT_SYMBOL from virtio_direct_dma_ops structure.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
