Re: [PATCH V4 7/9] vdpa: rename dma_dev to map_token

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 23, 2025 at 01:54:37PM +0800, Jason Wang wrote:
> On Wed, Jul 23, 2025 at 1:26 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jul 21, 2025 at 04:05:17PM +0800, Jason Wang wrote:
> > > On Fri, Jul 18, 2025 at 6:09 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 18, 2025 at 5:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jul 18, 2025 at 05:16:14PM +0800, Jason Wang wrote:
> > > > > > Virtio core switches from DMA device to mapping token, let's do that
> > > > > > as well for vDPA.
> > > > >
> > > > > Pls switch to imperative mood.
> > > > >
> > > > > And add explanation about what is going on and why please.
> > > >
> > > > I think this has been explained in patch [PATCH V4 6/9] virtio:
> > > > introduce map ops in virtio core?
> > > >
> > > > """
> > > > For the device or transport that doesn't do DMA,
> > > > they can implement their own mapping logic without the need to trick
> > > > DMA core. In this case the map_token is opaque to the virtio core that
> > > > will be passed back to the transport or device specific map
> > > > operations. For other devices, DMA API will still be used, so map
> > > > token will still be the dma device to minimize the changeset and
> > > > performance impact.
> > > > """
> > > >
> > > > >
> > > > > At least, document what are the actual types stored here.
> > > >
> > > > It is opaque to the virtio core and will be used as a token to be
> > > > passed back when the virtio core wants to map/unmap a buffer. So it
> > > > doesn't have a type.
> > > >
> > > > > I checked and it looks like vduse actually returns struct device * here, too?
> > > >
> > > > It's just a token, vduse can return anything that can be used to
> > > > identify an iova domain. For example, it can return a pointer to
> > > > iova_domain which will also work. If you prefer to use iova domain, I
> > > > can change to use that in the next version.
> > > >
> > > > > So why do we need this, why lose all type safety?
> > > >
> > > > If you worry about the case that assumes map_token as dma device in
> > > > virtio core. I can keep both map_token and dma_device and make the
> > > > mutually exclusive, then:
> > > >
> > > > - when transport reports dma device, use DMA API
> > > > - when transport reports map token, use map ops
> > > >
> > > > Does this work for you?
> > >
> > > This seems to result in a lot of unnecessary code. I wonder if the
> > > following would work:
> > >
> > > Just document the requirement in vring_create_virtqueue_map, say
> > > something like when a virtio device doesn't have a map operation,
> > > map_token must be a pointer to struct device.
> > >
> > > Please give me some advice so I can work on a new version..
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> >
> >
> > How about something like this?
> >
> > typedef union {
> >         struct device *dma_dev;
> >         struct vduse_iova_domain *iova_domain;
> >         void *ptr;
> > } virtio_dma_dev_t;
> >
> >
> >
> > Now, just pass this around.
> > You can also add an enum with the pointer:
> >
> > typedef struct {
> >         union {
> >                 struct device *dma_dev;
> >                 struct vduse_iova_domain *iova_domain;
> >                 void *ptr;
> >         };
> >         enum {
> >                 VIRTIO_DMA,
> >                 VIRTIO_IOVA
> >         } type;
> > } virtio_dma_dev_t;
> >
> > (possibly only on debug builds, if you find it's costly?)
> 
> I've considered something like this, but it looks more like trying to
> hard-code the runtime type information which I'm not sure we really
> need.
> Assuming the token or DMA device won't be changed after the probe. Let
> the transport to report one of the dma_dev or mapping_token should be
> sufficient.

you mean the enum? Not strictly necessary - more of a defence in depth
thing.

> 
> Let me try and see.
> 
> Thanks
> 
> >
> > This allows encapsulation and asserts that the type is right.
> >
> >
> > --
> > MST
> >