Re: [RFC v3 15/29] vhost: Add enable_custom_iommu to VhostOps

[Eugenio Perez Martin]

On Mon, May 31, 2021 at 11:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/5/20 上午12:28, Eugenio Pérez 写道:
> > This operation enable the backend-specific IOTLB entries.
> >
> > If a backend support this, it start managing its own entries, and vhost
> > can disable it through this operation and recover control.
> >
> > Every enable/disable operation must also clear all IOTLB device entries.
> >
> > At the moment, the only backend that does so is vhost-vdpa. To fully
> > support these, vdpa needs also to expose a way for vhost subsystem to
> > map and unmap entries. This will be done in future commits.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> I think there's probably no need to introduce this helper.
>
> Instead, we can introduce ops like shadow_vq_start()/stop(). Then the
> details like this could be hided there.
>

I'm also fine with your approach, but then the ownership of the shadow
virtqueue would be splitted between vhost code and the vhost backend
code.

With the current code, vhost is in charge for mapping DMA entries, and
delegates in the backend when the latter has its own means of map [1].
If we just expose shadow_vq_start/stop, the logic of when to map gets
somehow duplicated in vhost and in the backend, and it is not obvious
that future code changes in one side need to be duplicated in the
backend.

I understand that this way needs to expose more vhost operations, but
I think each of these are smaller and fit better in "vhost backend
implementation of an operation" than just telling the backend that
shadow vq is started.

> (And hide the backend deatils (avoid calling vhost_vdpa_dma_map())
> directly from the vhost.c)
>

Sure, the direct call of vhost_vdpa_dma_map is not intended to be that
way in the final series, it's just an intermediate step. I could have
been more explicit about that, sorry.

[1] At the moment it just calls vhost_vdpa_dma_map directly, but this
should be changed by a vhost_ops, and that op is optional: If not
present, vIOMMU is used.

> Thanks
>
>
> > ---
> >   include/hw/virtio/vhost-backend.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/include/hw/virtio/vhost-backend.h 
> > b/include/hw/virtio/vhost-backend.h
> > index bcb112c166..f8eed2ace5 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -128,6 +128,9 @@ typedef bool (*vhost_force_iommu_op)(struct vhost_dev 
> > *dev);
> >
> >   typedef int (*vhost_vring_pause_op)(struct vhost_dev *dev);
> >
> > +typedef int (*vhost_enable_custom_iommu_op)(struct vhost_dev *dev,
> > +                                            bool enable);
> > +
> >   typedef int (*vhost_get_iova_range)(struct vhost_dev *dev,
> >                                       hwaddr *first, hwaddr *last);
> >
> > @@ -177,6 +180,7 @@ typedef struct VhostOps {
> >       vhost_get_device_id_op vhost_get_device_id;
> >       vhost_vring_pause_op vhost_vring_pause;
> >       vhost_force_iommu_op vhost_force_iommu;
> > +    vhost_enable_custom_iommu_op vhost_enable_custom_iommu;
> >       vhost_get_iova_range vhost_get_iova_range;
> >   } VhostOps;
> >
>