Jason Wang
2023-Sep-11 03:42 UTC
[PATCH RFC v2 1/4] vdpa: introduce .reset_map operation callback
Hi Si-Wei: On Sat, Sep 9, 2023 at 9:34?PM Si-Wei Liu <si-wei.liu at oracle.com> wrote:> > On-chip IOMMU parent driver could use it to restore memory mapping > to the initial state.As discussed before. On-chip IOMMU is the hardware details that need to be hidden by the vDPA bus. Exposing this will complicate the implementation of bus drivers. Thanks> > Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com> > --- > include/linux/vdpa.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > index 17a4efa..daecf55 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -324,6 +324,12 @@ struct vdpa_map_file { > * @iova: iova to be unmapped > * @size: size of the area > * Returns integer: success (0) or error (< 0) > + * @reset_map: Reset device memory mapping (optional) > + * Needed for device that using device > + * specific DMA translation (on-chip IOMMU) > + * @vdev: vdpa device > + * @asid: address space identifier > + * Returns integer: success (0) or error (< 0) > * @get_vq_dma_dev: Get the dma device for a specific > * virtqueue (optional) > * @vdev: vdpa device > @@ -401,6 +407,7 @@ struct vdpa_config_ops { > u64 iova, u64 size, u64 pa, u32 perm, void *opaque); > int (*dma_unmap)(struct vdpa_device *vdev, unsigned int asid, > u64 iova, u64 size); > + int (*reset_map)(struct vdpa_device *vdev, unsigned int asid); > int (*set_group_asid)(struct vdpa_device *vdev, unsigned int group, > unsigned int asid); > struct device *(*get_vq_dma_dev)(struct vdpa_device *vdev, u16 idx); > -- > 1.8.3.1 >
Si-Wei Liu
2023-Sep-11 23:31 UTC
[PATCH RFC v2 1/4] vdpa: introduce .reset_map operation callback
Hi Jason, On 9/10/2023 8:42 PM, Jason Wang wrote:> Hi Si-Wei: > > On Sat, Sep 9, 2023 at 9:34?PM Si-Wei Liu <si-wei.liu at oracle.com> wrote: >> On-chip IOMMU parent driver could use it to restore memory mapping >> to the initial state. > As discussed before. On-chip IOMMU is the hardware details that need > to be hidden by the vDPA bus.I guess today this is exposed to the bus driver layer already, for e.g. vhost_vdpa_map() can call into the? .dma_map, or .set_map, or iommu_map() flavors depending on the specific hardware IOMMU implementation underneath? Specifically, "struct iommu_domain *domain" is now part of "struct vhost_vdpa" at an individual bus driver (vhost-vdpa), rather than being wrapped around under the vdpa core "struct vdpa_device" as vdpa device level object. Do we know for what reason the hardware details could be exposed to bus callers like vhost_vdpa_map and vhost_vdpa_general_unmap, while it's prohibited for other similar cases on the other hand? Or is there a boundary in between I was not aware of? I think a more fundamental question I don't quite understand, is adding an extra API to on-chip IOMMU itself an issue, or just that you don't like the way how the IOMMU model gets exposed via this specific API of .reset_map? For the platform IOMMU case, internally there exists distinction between the 1:1 identify (passthrough) mode and DMA page mapping mode, and this distinction is somehow getting exposed and propagated through the IOMMU API - for e.g. iommu_domain_alloc() and iommu_attach_device() are being called explicitly from vhost_vdpa_alloc_domain() by vhost-vdpa (and the opposite from within vhost_vdpa_free_domain), while for virtio-vdpa it doesn't call any IOMMU API at all on the other hand - which is to inherit what default IOMMU domain has. Ideally for on-chip IOMMU we can and should do pretty much the same, but I don't think there's a clean way without introducing any driver API to make vhost-vdpa case distinguish from the virtio-vdpa case. I'm afraid to say that it was just a hack to hide the necessary distinction needed by vdpa bus users for e.g. in the deep of vdpa_reset(), if not introducing any new driver API is the goal here...> Exposing this will complicate the implementation of bus drivers.As said above, this distinction is needed by bus drivers, and it's already done by platform IOMMU via IOMMU API. I can drop the .reset_map API while add another set of similar driver API to mimic iommu_domain_alloc/iommu_domain_free, but doing this will complicate the parent driver's implementation on the other hand. While .reset_map is what I can think of to be the simplest for parent, I can do the other way if you're fine with it. Let me know how it sounds. Thanks, -Siwei> > Thanks > >> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com> >> --- >> include/linux/vdpa.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h >> index 17a4efa..daecf55 100644 >> --- a/include/linux/vdpa.h >> +++ b/include/linux/vdpa.h >> @@ -324,6 +324,12 @@ struct vdpa_map_file { >> * @iova: iova to be unmapped >> * @size: size of the area >> * Returns integer: success (0) or error (< 0) >> + * @reset_map: Reset device memory mapping (optional) >> + * Needed for device that using device >> + * specific DMA translation (on-chip IOMMU) >> + * @vdev: vdpa device >> + * @asid: address space identifier >> + * Returns integer: success (0) or error (< 0) >> * @get_vq_dma_dev: Get the dma device for a specific >> * virtqueue (optional) >> * @vdev: vdpa device >> @@ -401,6 +407,7 @@ struct vdpa_config_ops { >> u64 iova, u64 size, u64 pa, u32 perm, void *opaque); >> int (*dma_unmap)(struct vdpa_device *vdev, unsigned int asid, >> u64 iova, u64 size); >> + int (*reset_map)(struct vdpa_device *vdev, unsigned int asid); >> int (*set_group_asid)(struct vdpa_device *vdev, unsigned int group, >> unsigned int asid); >> struct device *(*get_vq_dma_dev)(struct vdpa_device *vdev, u16 idx); >> -- >> 1.8.3.1 >>