Re: [PATCH 2/4] vhost-vdpa: reset vendor specific mapping to initial state in .release

[Si-Wei Liu]

On 10/11/2023 4:21 AM, Eugenio Perez Martin wrote:
> On Tue, Oct 10, 2023 at 11:05 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
> > Devices with on-chip IOMMU or vendor specific IOTLB implementation
> > may need to restore iotlb mapping to the initial or default state
> > using the .reset_map op, as it's desirable for some parent devices
> > to solely manipulate mappings by its own, independent of virtio device
> > state. For instance, device reset does not cause mapping go away on
> > such IOTLB model in need of persistent mapping. Before vhost-vdpa
> > is going away, give them a chance to reset iotlb back to the initial
> > state in vhost_vdpa_cleanup().
> >
> > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > ---
> >   drivers/vhost/vdpa.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 851535f..a3f8160 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -131,6 +131,15 @@ static struct vhost_vdpa_as *vhost_vdpa_find_alloc_as(struct vhost_vdpa *v,
> >          return vhost_vdpa_alloc_as(v, asid);
> >   }
> >
> > +static void vhost_vdpa_reset_map(struct vhost_vdpa *v, u32 asid)
> > +{
> > +       struct vdpa_device *vdpa = v->vdpa;
> > +       const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > +       if (ops->reset_map)
> > +               ops->reset_map(vdpa, asid);
> > +}
> > +
> >   static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> >   {
> >          struct vhost_vdpa_as *as = asid_to_as(v, asid);
> > @@ -140,6 +149,13 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> >
> >          hlist_del(&as->hash_link);
> >          vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1, asid);
> Now I'm wondering, does this call to vhost_vdpa_iotlb_unmap sets a
> different map (via .set_map) per element of the vhost_iotlb_itree?
Yes and no, effectively this vhost_vdpa_iotlb_unmap call will pass an 
empty iotlb with zero map entry down to the driver via .set_map, so for 
.set_map interface it's always a different map no matter what. As for 
this special case, the internal implementation of mlx5_vdpa .set_map may 
choose to either destroy MR and recreate a new one, or remove all 
mappings on the existing MR (currently it uses destroy+recreate for 
simplicity without have to special case). But .reset_map is different - 
the 1:1 DMA MR has to be recreated explicitly after destroying the 
regular MR, so you see this is driver/device implementation specifics.

>   Not
> a big deal since we're in the cleanup path, but it could be a nice
> optimization on top as we're going to reset the map of the asid
> anyway.
You mean wrap up what's done in vhost_vdpa_iotlb_unmap and 
vhost_vdpa_reset_map to a new call, say vhost_vdpa_iotlb_reset? Yes this 
is possible, but be noted that the vhost_vdpa_iotlb_unmap also takes 
charge of pinning accounting other than mapping, and it has to also 
maintain it's own vhost_iotlb copy in sync. There's no such much code 
that can be consolidated or generalized at this point, as 
vhost_vdpa_reset_map() is very specific to some device implementation, 
and I don't see common need to optimize this further up in the map/unmap 
hot path rather than this cleanup slow path, just as you alluded to.

Regards,
-Siwei
> > +       /*
> > +        * Devices with vendor specific IOMMU may need to restore
> > +        * iotlb to the initial or default state which is not done
> > +        * through device reset, as the IOTLB mapping manipulation
> > +        * could be decoupled from the virtio device life cycle.
> > +        */
> > +       vhost_vdpa_reset_map(v, asid);
> >          kfree(as);
> >
> >          return 0;
> > --
> > 1.8.3.1