Re: [PATCH 11/14] iommufd-lu: Persist iommu hardware pagetables for live update

[Samiullah Khawaja]

On Tue, Mar 03, 2026 at 05:56:15AM +0000, Ankit Soni wrote:
> Hi,
>
> On Tue, Feb 03, 2026 at 10:09:45PM +0000, Samiullah Khawaja wrote:
> > From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> >
> > The caller is expected to mark each HWPT to be preserved with an ioctl
> > call, with a token that will be used in restore. At preserve time, each
> > HWPT's domain is then called with iommu_domain_preserve to preserve the
> > iommu domain.
> >
> > The HWPTs containing dma mappings backed by unpreserved memory should
> > not be preserved. During preservation check if the mappings contained in
> > the HWPT being preserved are only file based and all the files are
> > preserved.
> >
> > The memfd file preservation check is not enough when preserving iommufd.
> > The memfd might have shrunk between the mapping and memfd preservation.
> > This means that if it shrunk some pages that are right now pinned due to
> > iommu mappings are not preserved with the memfd. Only allow iommufd
> > preservation when all the iopt_pages are file backed and the memory file
> > was seal sealed during mapping. This guarantees that all the pages that
> > were backing memfd when it was mapped are preserved.
> >
> > Once HWPT is preserved the iopt associated with the HWPT is made
> > immutable. Since the map and unmap ioctls operates directly on iopt,
> > which contains an array of domains, while each hwpt contains only one
> > domain. The logic then becomes that mapping and unmapping is prohibited
> > if any of the domains in an iopt belongs to a preserved hwpt. However,
> > tracing to the hwpt through the domain is a lot more tedious than
> > tracing through the ioas, so if an hwpt is preserved, hwpt->ioas->iopt
> > is made immutable.
> >
> > When undoing this (making the iopts mutable again), there's never
> > a need to make some iopts mutable and some kept immutable, since
> > the undo only happen on unpreserve and error path of preserve.
> > Simply iterate all the ioas and clear the immutability flag on all
> > their iopts.
> >
> > Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> > Signed-off-by: Samiullah Khawaja <skhawaja@xxxxxxxxxx>
> > ---
> >  drivers/iommu/iommufd/io_pagetable.c    |  17 ++
> >  drivers/iommu/iommufd/io_pagetable.h    |   1 +
> >  drivers/iommu/iommufd/iommufd_private.h |  25 ++
> >  drivers/iommu/iommufd/liveupdate.c      | 300 ++++++++++++++++++++++++
> >  drivers/iommu/iommufd/main.c            |  14 +-
> >  drivers/iommu/iommufd/pages.c           |   8 +
> >  include/linux/kho/abi/iommufd.h         |  39 +++
> >  7 files changed, 403 insertions(+), 1 deletion(-)
> >  create mode 100644 include/linux/kho/abi/iommufd.h
> >
> > +
> > +static int iommufd_save_hwpts(struct iommufd_ctx *ictx,
> > +			      struct iommufd_lu *iommufd_lu,
> > +			      struct liveupdate_session *session)
> > +{
> > +	struct iommufd_hwpt_paging *hwpt, **hwpts = NULL;
> > +	struct iommu_domain_ser *domain_ser;
> > +	struct iommufd_hwpt_lu *hwpt_lu;
> > +	struct iommufd_object *obj;
> > +	unsigned int nr_hwpts = 0;
> > +	unsigned long index;
> > +	unsigned int i;
> > +	int rc = 0;
> > +
> > +	if (iommufd_lu) {
> > +		hwpts = kcalloc(iommufd_lu->nr_hwpts, sizeof(*hwpts),
> > +				GFP_KERNEL);
> > +		if (!hwpts)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	xa_lock(&ictx->objects);
> > +	xa_for_each(&ictx->objects, index, obj) {
> > +		if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
> > +			continue;
> > +
> > +		hwpt = container_of(obj, struct iommufd_hwpt_paging, common.obj);
> > +		if (!hwpt->lu_preserve)
> > +			continue;
> > +
> > +		if (hwpt->ioas) {
> > +			/*
> > +			 * Obtain exclusive access to the IOAS and IOPT while we
> > +			 * set immutability
> > +			 */
> > +			mutex_lock(&hwpt->ioas->mutex);
> > +			down_write(&hwpt->ioas->iopt.domains_rwsem);
> > +			down_write(&hwpt->ioas->iopt.iova_rwsem);
>
> Taking mutex/rwsem under spin-lock is not a good idea.

Agreed. I will move this out by taking a reference on the object and
then setting it separately without xa_lock.
> > +
> > +			hwpt->ioas->iopt.lu_map_immutable = true;
> > +
> > +			up_write(&hwpt->ioas->iopt.iova_rwsem);
> > +			up_write(&hwpt->ioas->iopt.domains_rwsem);
> > +			mutex_unlock(&hwpt->ioas->mutex);
> > +		}
> > +
> > +		if (!hwpt->common.domain) {
> > +			rc = -EINVAL;
> > +			xa_unlock(&ictx->objects);
> > +			goto out;
> > +		}
> > +
> > +		if (!iommufd_lu) {
> > +			rc = check_iopt_pages_preserved(session, hwpt);
> > +			if (rc) {
> > +				xa_unlock(&ictx->objects);
> > +				goto out;
> > +			}
> > +		} else if (iommufd_lu) {
>
> Redundant else_if().

Will remove
> -Ankit
>
> > +			hwpts[nr_hwpts] = hwpt;
> > +			hwpt_lu = &iommufd_lu->hwpts[nr_hwpts];
> > +
> > +			hwpt_lu->token = hwpt->lu_token;
> > +			hwpt_lu->reclaimed = false;
> > +		}
> > +
> > +		nr_hwpts++;
> > +	}
> > +	xa_unlock(&ictx->objects);
> > +

Thanks for looking into this.

Sami