Re: [PATCH v5 08/18] iommu/vt-d: Clear unpreserved context entries during shutdown

[Baolu Lu <baolu.lu@linux.intel.com>]

On 10/10/26 08:32, Samiullah Khawaja wrote:
> On Fri, Oct 09, 2026 at 08:43:26AM +0800, Baolu Lu wrote:
>> On 9/21/26 08:48, Samiullah Khawaja wrote:
>>> During normal shutdown the iommu translation is disabled. Since the root
>>> table is preserved during live update, it needs to be cleaned up and the
>>> context entries of the unpreserved devices and root entries for the
>>> unpreserved context tables need to be cleared.
>>>
>>> This is required because during kexec reboot and shutdown the devices do
>>> not go through the release flow, so there might be stale entries in the
>>> root table and context tables. Also note that new unpreserved context
>>> tables for unpreserved devices might have been added after preservation,
>>> so the root table entries for unpreserved context tables also need to
>>> removed.
>>>
>>> Signed-off-by: Samiullah Khawaja<skhawaja@google.com>
>>> ---
>>>  drivers/iommu/intel/iommu.c      |  15 +++-
>>>  drivers/iommu/intel/iommu.h      |   5 ++
>>>  drivers/iommu/intel/liveupdate.c | 139 +++++++++++++++++++++++++++++++
>>>  3 files changed, 157 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index 4bfc2f173010..474c926172c5 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -1852,6 +1852,14 @@ static int iommu_suspend(void *data)
>>>      iommu_flush_all();
>>> +    /*
>>> +     * Note that IOMMU suspend doesn't affect live update. The state
>>> +     * preserved during live update is not released and remains 
>>> valid during
>>> +     * suspend and reused during IOMMU resume.
>>> +     *
>>> +     * Also note deployment of suspend/resume and live updated use case
>>> +     * should be mostly mutually exclusive.
>>> +     */
>>>      for_each_active_iommu(iommu, drhd) {
>>>          iommu_disable_translation(iommu);
>>> @@ -2397,8 +2405,11 @@ void intel_iommu_shutdown(void)
>>>          /* Disable PMRs explicitly here. */
>>>          iommu_disable_protect_mem_regions(iommu);
>>> -        /* Make sure the IOMMUs are switched off */
>>> -        iommu_disable_translation(iommu);
>>> +        /* Make sure the IOMMUs are switched off if not preserved. */
>>> +        if (iommu_preserved_state(&iommu->iommu))
>>> +            clear_unpreserved_context_entries(iommu);
>>> +        else
>>> +            iommu_disable_translation(iommu);
>>>      }
>>>  }
>>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>>> index 785057236e7c..4feb5bd76b18 100644
>>> --- a/drivers/iommu/intel/iommu.h
>>> +++ b/drivers/iommu/intel/iommu.h
>>> @@ -1307,6 +1307,11 @@ int intel_iommu_preserve(struct iommu_device 
>>> *iommu,
>>>               struct iommu_hw_ser *iommu_ser);
>>>  void intel_iommu_unpreserve(struct iommu_device *iommu,
>>>                  struct iommu_hw_ser *iommu_ser);
>>> +void clear_unpreserved_context_entries(struct intel_iommu *iommu);
>>> +#else
>>> +static inline void clear_unpreserved_context_entries(struct 
>>> intel_iommu *iommu)
>>> +{
>>> +}
>>>  #endif
>>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>>> diff --git a/drivers/iommu/intel/liveupdate.c b/drivers/iommu/intel/ 
>>> liveupdate.c
>>> index 0837bb889fed..501dc0e9cc0a 100644
>>> --- a/drivers/iommu/intel/liveupdate.c
>>> +++ b/drivers/iommu/intel/liveupdate.c
>>> @@ -77,6 +77,145 @@ static int preserve_context_table(struct 
>>> intel_iommu *iommu,
>>>      return 0;
>>>  }
>>> +static void clear_unpreserved_context_root_entries(struct 
>>> intel_iommu *iommu,
>>> +                           struct iommu_hw_ser *ser)
>>> +{
>>> +    struct root_entry *root;
>>> +    int i;
>>> +
>>> +    /*
>>> +     * Individual invalidations for each context table removal are not
>>> +     * needed as we issue global invalidations later.
>>> +     */
>>> +    for (i = 0; i < ROOT_ENTRY_NR; i++) {
>>> +        root = &iommu->root_entry[i];
>>> +
>>> +        if (!is_context_table_preserved(iommu, ser, i, 0) && (root- 
>>> >lo & 1)) {
>>> +            root->lo = 0;
>>> +            __iommu_flush_cache(iommu,
>>> +                        &root->lo,
>>> +                        sizeof(root->lo));
>>> +        }
>>> +
>>> +        if (!sm_supported(iommu))
>>> +            continue;
>>> +
>>> +        if (!is_context_table_preserved(iommu, ser, i, 0x80) && 
>>> (root->hi & 1)) {
>>> +            root->hi = 0;
>>> +            __iommu_flush_cache(iommu,
>>> +                        &root->hi,
>>> +                        sizeof(root->hi));
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void clear_unpreserved_context(struct device_domain_info 
>>> *info, u8 bus, u8 devfn)
>>> +{
>>> +    struct context_entry *context;
>>> +
>>> +    /*
>>> +     * This cleanup is done during shutdown, so it should be fine to 
>>> only
>>> +     * clear the entries here and issue one global invalidation 
>>> later to
>>> +     * invalidate all cleared entries.
>>> +     *
>>> +     * Note that the device IOTLB invalidation for unpreserved 
>>> devices is
>>> +     * skipped this way, but that should not be needed as the 
>>> devices are
>>> +     * quiesced at this point. This should improve the performance 
>>> of the
>>> +     * cleanup process and avoids any invalidation timeouts because 
>>> drivers
>>> +     * might have moved devices to D3 state.
>>
>> I don't quite follow why device-TLB flushes could be skipped when the
>> device is quiesced. Nothing guarantees that an unpreserved device that
>> has ATS enabled doesn't carry stale cache entries. It may keep
>> translations to memory that the next kernel reuses.
> 
> I agree with your assessment, the device-TLB might still have stale
> entries. But this assumption is based on the existing kexec reboot
> behaviour. That is, during normal shutdown the iommu translation is
> disabled but the device-TLB invalidations are not issued either. The
> unpreserved devices go through the same flow in the next kernel as they
> do after a normal kexec reboot. I will update the comment to add this
> detail.

I guess device-TLB invalidation is omitted during a normal kexec reboot
because the device will be reset or re-initialized during shutdown or
probe, which usually clears the ATC as a side effect. But I don't think
that's guaranteed. Anyway, this might be a pre-existing issue and not
specific to this series.

>>
>> I would suggest at least a global device-TLB flush for ATS-enabled
>> unpreserved devices, or is there anything I've overlooked?
> 
> This might be tricky as this late during shutdown some devices might
> already be in a low power state, and sending them device-TLB
> invalidations would cause invalidation timeouts. Even if that doesn't
> happen, it will still add to the shutdown time.

Okay, fair enough.

>>
>>> +     *
>>> +     * The iommu lock is not needed as the context tables are never 
>>> removed
>>> +     * and the new ones are not added during shutdown.
>>> +     */
>>> +    context = iommu_context_addr(info->iommu, bus, devfn, 0);
>>> +    if (context) {
>>> +        /*
>>> +         * Individual invalidations are not needed as we issue global
>>> +         * invalidations later once all the cleanups are done.
>>> +         */
>>> +        context_clear_present(context);
>>> +        __iommu_flush_cache(info->iommu, context, sizeof(*context));
>>> +        context_clear_entry(context);
>>> +        __iommu_flush_cache(info->iommu, context, sizeof(*context));
>>> +    }
>>> +}
>>
>> [-snipped-]
>>
>> Thanks,
>> baolu
>>
> 
> Thanks,
> Sami

Thanks,
baolu