Re: [PATCH v4 09/18] iommu: Add APIs to get iommu and device preserved state

[Samiullah Khawaja <skhawaja@google.com>]

On Wed, Aug 12, 2026 at 06:29:18AM +0000, Ankit Soni wrote:
>On Sat, Aug 08, 2026 at 02:27:14AM +0000, Samiullah Khawaja wrote:
>> The preserved state of the device and IOMMU needs to be fetched during
>> shutdown and boot in the next kernel. Add APIs that can be used to fetch
>> the preserved state of a device and IOMMU. The APIs will only be used
>> during shutdown and after liveupdate so no locking needed.
>>
>> Reviewed-by: Pranjal Shrivastava <praan@google.com>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>> ---
>>  drivers/iommu/liveupdate.c       | 122 +++++++++++++++++++++++++++++++
>>  include/linux/iommu-liveupdate.h |  45 ++++++++++++
>>  2 files changed, 167 insertions(+)
>>
>> diff --git a/drivers/iommu/liveupdate.c b/drivers/iommu/liveupdate.c
>> index 7f349ae5a124..20acf123b47a 100644
>> --- a/drivers/iommu/liveupdate.c
>> +++ b/drivers/iommu/liveupdate.c
>
>../..
>
>> @@ -262,6 +273,117 @@ void iommu_liveupdate_unregister_flb(struct liveupdate_file_handler *handler)
>>  }
>>  EXPORT_SYMBOL(iommu_liveupdate_unregister_flb);
>>
>> +/*
>> + * iommu_liveupdate_flb_get_incoming() - Helper function to get FLB state
>> + * @flb_objp: Pointer to get the restored FLB object
>> + *
>> + * Return: 0 if FLB state found and restored, error if no data found
>> + */
>> +static int iommu_liveupdate_flb_get_incoming(struct iommu_flb_obj **flb_objp)
>> +{
>> +	struct iommu_flb_obj *flb_obj;
>> +	int ret;
>> +
>> +	ret = liveupdate_flb_get_incoming(&iommu_flb, (void **)flb_objp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	flb_obj = *flb_objp;
>> 	mutex_lock(&flb_obj->lock);
>> +
>> +	/*
>> +	 * FLB version mismatch is considered fatal for security reasons for
>> +	 * now.
>> +	 */
>> +	if (flb_obj->ser->version != IOMMU_LUO_FLB_VERSION)
>> +		goto err_fatal;
>> +
>> +	/*
>> +	 * Array Phys of each type should be valid if the FLB was created for
>> +	 * preservation. This is true even if no devices, iommus or domains were
>> +	 * preserved.
>> +	 */
>> +	if (!flb_obj->ser->iommu_array_phys ||
>> +	    !flb_obj->ser->device_array_phys ||
>> +	    !flb_obj->ser->iommu_domain_array_phys)
>> +		goto err_fatal;
>> +
>> +	mutex_unlock(&flb_obj->lock);
>> +	return 0;
>> +
>> +err_fatal:
>> +	panic("Failed to restore IOMMU Live Update FLB\n");
>> +}
>
>../..
>
>> +/**
>> + * iommu_get_preserved_data() - Get preserved data for an IOMMU HW
>> + * @token: Token used to preserve this IOMMU HW
>> + * @type: IOMMU type in preserved state
>> + *
>> + * Gets the preserved state of an IOMMU HW using token and the IOMMU type.
>> + *
>> + * Return: struct iommu_hw_ser on success, NULL if no preserved state found.
>> + */
>> +struct iommu_hw_ser *iommu_get_preserved_data(u64 token, enum iommu_type_ser type)
>> +{
>> +	struct iommu_hw_ser *iommu_ser = NULL;
>> +	struct iommu_hw_array_ser *array;
>> +	struct iommu_flb_obj *flb_obj;
>> +	int ret, idx;
>> +
>> +	ret = iommu_liveupdate_flb_get_incoming(&flb_obj);
>> +	if (ret)
>> +		return NULL;
>
>Hi,

Hi,

Thanks for looking at this.
>
>Returning NULL for every failure loses a distinction the caller needs.
>"Nothing was handed over" and "something was handed over and the lookup
>failed" are different states, and this helper is the only place that can
>tell them apart.
>

My intention is to handle these details in the helper function
iommu_liveupdate_flb_get_incoming() to keep the driver code clean.
Basically to allow the caller to decide whether the state is available
or not. The various error handling cases can be covered internally, and
I based it on the errors that the liveupdate_flb_get_incoming() can
return.

>Of the errors reachable here, three mean nothing was handed over:
>
>        -EOPNOTSUPP     live update disabled or not built

This means that the is state not available so returning NULL
>        -ENODATA        no incoming handover data
>        -ENOENT         the handover carried no IOMMU FLB

Both mean that no data is available from IOMMU point of view as version
is moved into FLB state, so returning NULL.

Now down the road we might have cases where the available data has a
different version as compared to what this kernel expects. We can handle
all those things in this function and if possible hand over data to the
caller in the right format it expects. Otherwise we panic() considering
mismatch fatal. I will add these details in the documentation.

>
>An -ENOMEM out of a .retrieve() callback means the opposite, and returns
>the same NULL.

Ah yes... This needs to be handled internally also. Probably in the
.retrieve() callback as -ENOMEM when getting the FLB should be fatal.
>
>The distinction matters because callers read NULL as a cold boot and act
>on it. Both VT-d call sites in 10/18 do. init_dmars():
>
>        if (!iommu_ser)
>                init_translation_status(iommu);
>
>        if (translation_pre_enabled(iommu) && !is_kdump_kernel()) {
>                iommu_disable_translation(iommu);
>                clear_translation_pre_enabled(iommu);
>                pr_warn("Translation was enabled for %s but we are not in kdump mode\n",
>                        iommu->name);
>        }
>
>and intel_iommu_add():
>
>        if (!iommu_ser && iommu->gcmd & DMA_GCMD_TE)
>                iommu_disable_translation(iommu);
>
>On an -ENOMEM both disable an IOMMU that the previous kernel left
>translating, underneath devices still doing DMA through it. The warning
>attributes it to a stale enable outside kdump, so the log points away from
>the actual cause.
>
>The helper already treats this class of failure as fatal a few lines
>lower. A version mismatch and a missing array pointer both reach err_fatal
>and panic(), because data was handed over and cannot be trusted. A failed
>fetch is the same class -- something was handed over and could not be read
>back -- but it returns an error that becomes NULL, and callers read that
>as a cold boot.
>
>A single failure also spreads. luo_flb_retrieve_one() caches a failed
>.retrieve() in incoming.retrieve_status and returns it directly on every
>later call, so one -ENOMEM makes the lookup return NULL for every IOMMU in
>the system, not just the one that hit it.
>
>Is returning NULL for all of them intentional? If callers are meant to
>treat every failure as "nothing was handed over", that is worth saying in
>a comment here.
>
>If not, the smallest fix is to return NULL for the absent cases only and
>an ERR_PTR for everything else:

This is what I was doing in previous version, but it gets too messy.
Moving it into the helper handles fatal errors, and versioning down the
road, cleanly.

Thanks,
Sami