| Message ID | 20200929084425.24052-4-sherry.sun@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Change vring space from nomal memory to dma coherent memory | expand |
On Tue, Sep 29, 2020 at 04:44:24PM +0800, Sherry Sun wrote: > The device page and vring should use consistent memory which are > allocated by dma_alloc_coherent api, when user space wants to get its > physical address, virt_to_phys cannot be used, should simply return the > saved device page dma address by get_dp_dma callback and the vring dma > address saved in mic_vqconfig. More importantly you can't just all virt_to_phys on a return value from dma_alloc_coherent, so this needs to be folded into patch 1. > if (!offset) { > - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); > + if (vpdev->hw_ops->get_dp_dma) > + *pa = vpdev->hw_ops->get_dp_dma(vpdev); > + else { > + dev_err(vop_dev(vdev), "can't get device page physical address\n"); > + return -EINVAL; > + } I don't think we need the NULL check here. Wouldn't it also make sense to return the virtual and DMA address from ->get_dp instead of adding another method?
Hi Christoph, > On Tue, Sep 29, 2020 at 04:44:24PM +0800, Sherry Sun wrote: > > The device page and vring should use consistent memory which are > > allocated by dma_alloc_coherent api, when user space wants to get its > > physical address, virt_to_phys cannot be used, should simply return > > the saved device page dma address by get_dp_dma callback and the vring > > dma address saved in mic_vqconfig. > > More importantly you can't just all virt_to_phys on a return value from > dma_alloc_coherent, so this needs to be folded into patch 1. Okay, will move this change into patch 1. > > > if (!offset) { > > - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); > > + if (vpdev->hw_ops->get_dp_dma) > > + *pa = vpdev->hw_ops->get_dp_dma(vpdev); > > + else { > > + dev_err(vop_dev(vdev), "can't get device page > physical address\n"); > > + return -EINVAL; > > + } > > I don't think we need the NULL check here. Wouldn't it also make sense to > return the virtual and DMA address from ->get_dp instead of adding another > method? Do you mean that we should only change the original ->get_dp callback to return virtual and DMA address at the same time, instead of adding the ->get_dp_dma callback? Regards Sherry
On Tue, Sep 29, 2020 at 01:10:12PM +0000, Sherry Sun wrote: > > > if (!offset) { > > > - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); > > > + if (vpdev->hw_ops->get_dp_dma) > > > + *pa = vpdev->hw_ops->get_dp_dma(vpdev); > > > + else { > > > + dev_err(vop_dev(vdev), "can't get device page > > physical address\n"); > > > + return -EINVAL; > > > + } > > > > I don't think we need the NULL check here. Wouldn't it also make sense to > > return the virtual and DMA address from ->get_dp instead of adding another > > method? > > Do you mean that we should only change the original ->get_dp callback to return virtual > and DMA address at the same time, instead of adding the ->get_dp_dma callback? That was my intention when writing it, yes. But it seems like most other callers don't care. Maybe move the invocation of dma_mmap_coherent into a new ->mmap helper, that way it seems like the calling code doesn't need to know about the dma_addr_t at all. That being said the layering in the code keeps puzzling me. As far as I can tell only a single instance of struct vop_driver even exists, so we might be able to kill all the indirections entirely.
Hi Christoph, > On Tue, Sep 29, 2020 at 01:10:12PM +0000, Sherry Sun wrote: > > > > if (!offset) { > > > > - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); > > > > + if (vpdev->hw_ops->get_dp_dma) > > > > + *pa = vpdev->hw_ops->get_dp_dma(vpdev); > > > > + else { > > > > + dev_err(vop_dev(vdev), "can't get device page > > > physical address\n"); > > > > + return -EINVAL; > > > > + } > > > > > > I don't think we need the NULL check here. Wouldn't it also make > > > sense to return the virtual and DMA address from ->get_dp instead of > > > adding another method? > > > > Do you mean that we should only change the original ->get_dp callback > > to return virtual and DMA address at the same time, instead of adding the - > >get_dp_dma callback? > > That was my intention when writing it, yes. But it seems like most other > callers don't care. Maybe move the invocation of dma_mmap_coherent into > a new ->mmap helper, that way it seems like the calling code doesn't need to > know about the dma_addr_t at all. > > That being said the layering in the code keeps puzzling me. As far as I can tell > only a single instance of struct vop_driver even exists, so we might be able to > kill all the indirections entirely. There may be some misunderstandings here. For ->get_dp_dma callback, it is used to get the device page dma address, which is allocated by MIC layer instead of vop layer. For Intel mic, it still use kzalloc and dma_map_single apis, although we recommended and we did use dma_alloc_coherent to get consistent device page memory on our i.MX platform, but we didn't change the original implementation method of Intel mic till now, as our main goal is to change the vop code to make it more generic. Which is means that the device page may use different allocate methods for different platform, and now it is transparent to the vop layer. So I think here use ->get_dp_dma callback to get device page dma address is the most simple and convenient way. We change to use dma_alloc_coherent in patch 1 to allocate vrings memory, as it is the main job that the vop layer is responsible for. So I still suggest to use ->get_dp or ->get_dp_dma callback for device page here, what do you think? Regards Sherry
On Wed, Sep 30, 2020 at 07:30:21AM +0000, Sherry Sun wrote: > There may be some misunderstandings here. > For ->get_dp_dma callback, it is used to get the device page dma address, > which is allocated by MIC layer instead of vop layer. > For Intel mic, it still use kzalloc and dma_map_single apis, although we > recommended and we did use dma_alloc_coherent to get consistent device > page memory on our i.MX platform, but we didn't change the original implementation > method of Intel mic till now, as our main goal is to change the vop code to make it > more generic. Given how the memory is used everyone should use dma_alloc_coherent. Note that for x86 the ony difference is that dma_alloc_coherent also dips into the CMA pools where available, which eases allocator pressure, and that it properly deals with the AMD SEV memory encryption, which does't matter for Intel platforms. > Which is means that the device page may use different allocate methods for > different platform, and now it is transparent to the vop layer. > So I think here use ->get_dp_dma callback to get device page dma address > is the most simple and convenient way. > > We change to use dma_alloc_coherent in patch 1 to allocate vrings memory, as it is > the main job that the vop layer is responsible for. > So I still suggest to use ->get_dp or ->get_dp_dma callback for device page here, what do you think? As mentioned you need to move the code to mmap the buffers to the same layer as the one doing the allocation. If that is taken care of we're fine, and I think a ->mmap callback might be the best way to archive that, but this is not code I know intimately.
diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h index 4fa02808c1e2..d891eacae358 100644 --- a/drivers/misc/mic/bus/vop_bus.h +++ b/drivers/misc/mic/bus/vop_bus.h @@ -75,6 +75,7 @@ struct vop_driver { * node to add/remove/configure virtio devices. * @get_dp: Get access to the virtio device page used by the self * node to add/remove/configure virtio devices. + * @get_dp_dma: Get dma address of the virtio device page. * @send_intr: Send an interrupt to the peer node on a specified doorbell. * @remap: Map a buffer with the specified DMA address and length. * @unmap: Unmap a buffer previously mapped. @@ -92,6 +93,7 @@ struct vop_hw_ops { void (*ack_interrupt)(struct vop_device *vpdev, int num); void __iomem * (*get_remote_dp)(struct vop_device *vpdev); void * (*get_dp)(struct vop_device *vpdev); + dma_addr_t (*get_dp_dma)(struct vop_device *vpdev); void (*send_intr)(struct vop_device *vpdev, int db); void __iomem * (*remap)(struct vop_device *vpdev, dma_addr_t pa, size_t len); diff --git a/drivers/misc/mic/host/mic_boot.c b/drivers/misc/mic/host/mic_boot.c index fb5b3989753d..ced03662cd8f 100644 --- a/drivers/misc/mic/host/mic_boot.c +++ b/drivers/misc/mic/host/mic_boot.c @@ -88,6 +88,13 @@ static void *__mic_get_dp(struct vop_device *vpdev) return mdev->dp; } +static dma_addr_t __mic_get_dp_dma(struct vop_device *vpdev) +{ + struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev); + + return mdev->dp_dma_addr; +} + static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev) { return NULL; @@ -119,6 +126,7 @@ static struct vop_hw_ops vop_hw_ops = { .ack_interrupt = __mic_ack_interrupt, .next_db = __mic_next_db, .get_dp = __mic_get_dp, + .get_dp_dma = __mic_get_dp_dma, .get_remote_dp = __mic_get_remote_dp, .send_intr = __mic_send_intr, .remap = __mic_ioremap, diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 422a28c1bb7a..4d5feb39aeb7 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -995,6 +995,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, unsigned long *size, unsigned long *pa) { struct vop_device *vpdev = vdev->vpdev; + struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); unsigned long start = MIC_DP_SIZE; int i; @@ -1007,7 +1008,13 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, * .... */ if (!offset) { - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); + if (vpdev->hw_ops->get_dp_dma) + *pa = vpdev->hw_ops->get_dp_dma(vpdev); + else { + dev_err(vop_dev(vdev), "can't get device page physical address\n"); + return -EINVAL; + } + *size = MIC_DP_SIZE; return 0; } @@ -1016,7 +1023,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, struct vop_vringh *vvr = &vdev->vvr[i]; if (offset == start) { - *pa = virt_to_phys(vvr->vring.va); + *pa = vqconfig[i].address; *size = vvr->vring.len; return 0; }