Virtualization - Aug 2023 - [PATCH RFC 1/4] vdpa: introduce .reset_map operation callback

On 8/14/2023 7:21 PM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 9:46?AM Si-Wei Liu <si-wei.liu at oracle.com>
wrote:
>> 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 db1b0ea..3a3878d 100644
>> --- a/include/linux/vdpa.h
>> +++ b/include/linux/vdpa.h
>> @@ -314,6 +314,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)
> This exposes the device internal to the upper layer which is not optimal.
Not sure what does it mean by "device internal", but this op callback 
just follows existing convention to describe what vdpa parent this API 
targets.

 ?* @set_map:??????????????????? Set device memory mapping (optional)
 ?*????????????????????????????? Needed for device that using device
 ?*????????????????????????????? specific DMA translation (on-chip IOMMU)
:
:
 ?* @dma_map:??????????????????? Map an area of PA to IOVA (optional)
 ?*????????????????????????????? Needed for device that using device
 ?*????????????????????????????? specific DMA translation (on-chip IOMMU)
 ?*????????????????????????????? and preferring incremental map.
:
:
 ?* @dma_unmap:????????????????? Unmap an area of IOVA (optional but
 ?*????????????????????????????? must be implemented with dma_map)
 ?*????????????????????????????? Needed for device that using device
 ?*????????????????????????????? specific DMA translation (on-chip IOMMU)
 ?*????????????????????????????? and preferring incremental unmap.

> Btw, what's the difference between this and a simple
>
> set_map(NULL)?
I don't think parent drivers support this today - they can accept 
non-NULL iotlb containing empty map entry, but not a NULL iotlb. The 
behavior is undefined or it even causes panic when a NULL iotlb is 
passed in. Further this doesn't work with .dma_map parent drivers.

The reason why a new op is needed or better is because it allows 
userspace to tell apart different reset behavior from the older kernel 
(via the F_IOTLB_PERSIST feature bit in patch 4), while this behavior 
could vary between parent drivers.

Regards,
-Siwei
>
> Thanks
>
>> + *                             @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
>> @@ -390,6 +396,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
>>

On Wed, Aug 16, 2023 at 3:49?AM Si-Wei Liu <si-wei.liu at oracle.com>
wrote:
>
>
>
> On 8/14/2023 7:21 PM, Jason Wang wrote:
> > On Tue, Aug 15, 2023 at 9:46?AM Si-Wei Liu <si-wei.liu at
oracle.com> wrote:
> >> 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 db1b0ea..3a3878d 100644
> >> --- a/include/linux/vdpa.h
> >> +++ b/include/linux/vdpa.h
> >> @@ -314,6 +314,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)
> > This exposes the device internal to the upper layer which is not
optimal.
> Not sure what does it mean by "device internal", but this op
callback
> just follows existing convention to describe what vdpa parent this API
> targets.
I meant the bus tries to hide the differences among vendors. So it
needs to hide on-chip IOMMU stuff to the upper layer.

We can expose two dimensional IO mappings models but it looks like
over engineering for this issue. More below.
>
>   * @set_map:                    Set device memory mapping (optional)
>   *                              Needed for device that using device
>   *                              specific DMA translation (on-chip IOMMU)
> :
> :
>   * @dma_map:                    Map an area of PA to IOVA (optional)
>   *                              Needed for device that using device
>   *                              specific DMA translation (on-chip IOMMU)
>   *                              and preferring incremental map.
> :
> :
>   * @dma_unmap:                  Unmap an area of IOVA (optional but
>   *                              must be implemented with dma_map)
>   *                              Needed for device that using device
>   *                              specific DMA translation (on-chip IOMMU)
>   *                              and preferring incremental unmap.
>
>
> > Btw, what's the difference between this and a simple
> >
> > set_map(NULL)?
> I don't think parent drivers support this today - they can accept
> non-NULL iotlb containing empty map entry, but not a NULL iotlb. The
> behavior is undefined or it even causes panic when a NULL iotlb is
> passed in.
We can do this simple change if it can work.
>  Further this doesn't work with .dma_map parent drivers.
Probably, but I'd remove dma_map as it doesn't have any real users
except for the simulator.
>
> The reason why a new op is needed or better is because it allows
> userspace to tell apart different reset behavior from the older kernel
> (via the F_IOTLB_PERSIST feature bit in patch 4), while this behavior
> could vary between parent drivers.
I'm ok with a new feature flag, but we need to first seek a way to
reuse the existing API.

Thanks
>
> Regards,
> -Siwei
>
> >
> > Thanks
> >
> >> + *                             @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
> >> @@ -390,6 +396,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
> >>
>