Si-Wei Liu
2023-Oct-12 06:18 UTC
[PATCH 2/4] vhost-vdpa: reset vendor specific mapping to initial state in .release
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 at 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 at 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 >>