Re: [PATCH 1/4] vdpa: introduce .reset_map operation callback

[Jason Wang]

On Fri, Oct 13, 2023 at 3:36 PM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
>
>
>
> On 10/12/2023 7:49 PM, Jason Wang wrote:
> > On Tue, Oct 10, 2023 at 5:05 PM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
> >> Device specific IOMMU parent driver who wishes to see mapping to be
> >> decoupled from virtio or vdpa device life cycle (device reset) can use
> >> it to restore memory mapping in the device IOMMU to the initial or
> >> default state. The reset of mapping is done per address space basis.
> >>
> >> The reason why a separate .reset_map op is introduced is because this
> >> allows a simple on-chip IOMMU model without exposing too much device
> >> implementation details to the upper vdpa layer. The .dma_map/unmap or
> >> .set_map driver API is meant to be used to manipulate the IOTLB mappings,
> >> and has been abstracted in a way similar to how a real IOMMU device maps
> >> or unmaps pages for certain memory ranges. However, apart from this there
> >> also exists other mapping needs, in which case 1:1 passthrough mapping
> >> has to be used by other users (read virtio-vdpa). To ease parent/vendor
> >> driver implementation and to avoid abusing DMA ops in an unexpacted way,
> >> these on-chip IOMMU devices can start with 1:1 passthrough mapping mode
> >> initially at the he time of creation. Then the .reset_map op can be used
> >> to switch iotlb back to this initial state without having to expose a
> >> complex two-dimensional IOMMU device model.
> >>
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> >> ---
> >>   include/linux/vdpa.h | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >> index d376309..26ae6ae 100644
> >> --- a/include/linux/vdpa.h
> >> +++ b/include/linux/vdpa.h
> >> @@ -327,6 +327,15 @@ 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 to the default
> >> + *                             state (optional)
> > I think we need to mention that this is a must for parents that use set_map()?
> It's not a must IMO, some .set_map() user for e.g. VDUSE or vdpa-sim-blk
> can deliberately choose to implement .reset_map() depending on its own
> need. Those user_va software devices mostly don't care about mapping
> cost during reset, as they don't have to pin kernel memory in general.
> It's just whether or not they care about mapping being decoupled from
> device reset at all.

Ok, let's document this in the changelog at least.

Thanks

>
> And the exact implementation requirement of this interface has been
> documented right below.
>
> -Siwei
> >
> > Other than this:
> >
> > Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
> >
> > Thanks
> >
> >> + *                             Needed for devices that are using device
> >> + *                             specific DMA translation and prefer mapping
> >> + *                             to be decoupled from the virtio life cycle,
> >> + *                             i.e. device .reset op does not reset mapping
> >> + *                             @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
> >> @@ -405,6 +414,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
> >>
>