Re: [PATCH v5 15/18] iommufd: Persist iommu hardware pagetables for live update

[Samiullah Khawaja <skhawaja@google.com>]

On Wed, Oct 07, 2026 at 02:31:47PM -0700, Nicolin Chen wrote:
>On Mon, Sep 21, 2026 at 12:48:31AM +0000, Samiullah Khawaja wrote:
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index a33b32708afa..a4ddc29ec5ae 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -98,6 +98,9 @@ struct io_pagetable {
>>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>>  	struct rb_root_cached reserved_itree;
>>  	u8 disable_large_pages;
>> +#ifdef CONFIG_IOMMU_LIVEUPDATE
>> +	u32 nr_preserved_domains;
>> +#endif
>
>In iommufd, "num_" is used more often than "nr_".

Will update.
>
>>  	unsigned long iova_alignment;
>>  };
>>
>> @@ -398,6 +401,7 @@ struct iommufd_hwpt_paging {
>>  	bool enforce_cache_coherency : 1;
>>  	bool nest_parent : 1;
>>  #ifdef CONFIG_IOMMU_LIVEUPDATE
>> +	bool liveupdate_preserved;
>>  	u64 liveupdate_token;
>>  #endif
>>  	/* Head at iommufd_ioas::hwpt_list */
>
>> +static inline bool iopt_liveupdate_immutable(const struct io_pagetable *iopt)
>> +{
>> +	return iopt->nr_preserved_domains > 0;
>> +}
>
>It's used in io_pagetable file only. I'd move it out of the header:
>
>static inline bool iopt_liveupdate_immutable(const struct io_pagetable *iopt)
>{
>	bool immutable = false;
>
>	lockdep_assert_held(&iopt->domains_rwsem);
>
>#ifdef CONFIG_IOMMU_LIVEUPDATE
>	/* iopt becomes immutable once liveupdate preservation starts */
>	immutable = iopt->num_preserved_domains;
>#endif
>	return immutable;
>}

Will update.
>
>> diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c
>> +static bool ioas_set_immutable(struct iommufd_ioas *ioas, bool set)
>> +{
>> +	bool was_immutable;
>> +
>> +	down_write(&ioas->iopt.domains_rwsem);
>> +	was_immutable = ioas->iopt.nr_preserved_domains > 0;
>> +	if (set)
>> +		ioas->iopt.nr_preserved_domains++;
>> +	else if (!WARN_ON(!was_immutable))
>> +		ioas->iopt.nr_preserved_domains--;
>> +
>> +	up_write(&ioas->iopt.domains_rwsem);
>> +
>> +	return was_immutable;
>> +}
>
>Since it has to handle refcount, an unset doesn't really unset the
>flag, which is confusing.
>
>Maybe just:
>  iopt_inc_num_preserved_domains
>  iopt_dec_num_preserved_domains

Sounds good. Will update.
>
>> +static int check_iopt_pages_preserved(struct liveupdate_session *s,
>> +				      struct iommufd_hwpt_paging *hwpt)
>
>static int iopt_validate_preserved_memory(struct io_pagetable *iopt,
>					  struct liveupdate_session *s)

Will update.
>
>> +{
>> +	u32 req_seals = F_SEAL_SEAL | F_SEAL_GROW | F_SEAL_SHRINK;
>> +	struct iopt_area *area;
>> +	int ret = 0;
>> +
>> +	down_read(&hwpt->ioas->iopt.iova_rwsem);
>> +	for (area = iopt_area_iter_first(&hwpt->ioas->iopt, 0, ULONG_MAX); area;
>> +	     area = iopt_area_iter_next(area, 0, ULONG_MAX)) {
>> +		struct iopt_pages *pages = area->pages;
>
>Move req_seals inside the for loop.

Will update.
>
>> +static int iommufd_liveupdate_preserve(struct liveupdate_file_op_args *args)
>[...]
>> +	iommufd_ser = mem;
>> +	iommufd_ser->nr_hwpts = nr_hwpts;
>> +
>> +	/* Preserve HWPTs */
>> +	i = 0;
>> +	xa_lock(&ictx->objects);
>> +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
>[...]
>> +	}
>> +	xa_unlock(&ictx->objects);
>> +
>> +	/* Store the actual number of HWPTs that are preserved */
>> +	iommufd_ser->nr_hwpts = i;
>
>iommufd_ser->nr_hwpts is set twice. First one seems redundant.

Agreed.
>
>> +++ b/include/linux/kho/abi/iommufd.h
>> +/**
>> + * struct iommu_hwpt_ser - IOMMUFD HWPT serialized state
>> + * @domain_data: Physical address of the serialized state of associated domain
>
>Then, it could be domain_ser_phys?

Agreed.
>
>> + * @token: User provided token
>> + * @reclaimed: Whether the HWPT is reclaimed
>
>Once the hwpt is reclaimed, could we clear domain_data to indicate
>"it is reclaimed" instead of a separate field?

Agreed.
>
>Nicolin

Thanks,
Sami