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

[Si-Wei Liu <si-wei.liu@oracle.com>]

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@oracle.com> 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@oracle.com>
>> ---
>>   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
>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization