lkml.org 
[lkml]   [2018]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC 1/4] virtio: Define virtio_direct_dma_ops structure
    From
    Date
    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.

    \
     
     \ /
      Last update: 2018-07-31 06:02    [W:4.466 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site