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

[Samiullah Khawaja]

On Thu, Aug 27, 2026 at 02:09:26PM +0800, Baolu Lu wrote:
> On 8/27/26 04:30, Samiullah Khawaja wrote:
> > > > +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.
> > > > +     */
> > > > +    context = iommu_context_addr(info->iommu, bus, devfn, 0);
> > > > +    if (context) {
> > > > +        context_clear_entry(context);
> > > > +        __iommu_flush_cache(info->iommu, context, sizeof(*context));
> > >
> > > For tearing down a present context entry, please follow the VT-d
> > > recommended sequence:
> > >
> > > - clear only the Present bit,
> > > - flush the updated entry to memory if necessary,
> > > - issue the required cache invalidations,
> >
> > Do we still need the individual cache invalidations if we issue global
> > invalidations (context, pasid-cache and iotlb), after clearing all
> > entries, as they would be done if a new root table was being setup?
> >
> > Looking at the VT-d specs (Invalidation of Translation Caches), each
> > invalidation type (cache, pasid and iotlb) defines granularity in both
> > register and queue based interface. And the granularity indicates that
> > Global invalidations clear the cached entries for that specific type.
> > For example following text is used for each cache type (in queue
> > interface):
> >
> >   Context-cache:
> >   Global Invalidation (01b): All context-cache entries cached at the
> >   remapping hardware are invalidated.
> >
> >   Pasid-cache:
> >   Global Invalidation (11b): All PASID-cache entries are invalidated.
> >
> >   Iotlb:
> >   Global Invalidation (01b):
> >   - All IOTLB entries are invalidated.
> >   - All paging-structure-cache entries are invalidated.
> >
> > A similar note about using Global invalidation is suggested in the specs
> > when setting up root table (Set Root Table Pointer Operation).
> >
> >   ... software must perform a global invalidate of the contextcache,
> >   PASID-cache (if applicable), and IOTLB, in that order. This is
> >   required to ensure hardware references only the remapping structures
> >   referenced by the new root table pointer and not stale cached entries.
> >
> > Also please note that this is happening during dmar unit teardown and
> > system shutdown, and while the context table entries in root table are
> > being cleared, the memory is not freed until the global invalidation is
> > issued.
> >
> > Since this is during shutdown, issuing global invalidations instead of
> > multiple individual invalidations for devices and aliases is simpler and
> > would likely also have shutdown time improvements and reduce the
> > blackout time during liveupdate.
> >
> > Please let me know if my global invalidations and granularity
> > understanding is not correct.
>
> The global-invalidation approach (similar to the “set new root table”
> flow) looks correct to me.
>
> On the device side: if ATS is enabled, some translations may still be
> cached in the device. My understanding is that unpreserved devices are
> already DMA-quiesced and then go through reset + reprobe (as in a normal
> reboot), which should flush those device-side caches. Are we aligned on
> that assumption?

Yes, we are aligned on it. I already added a note about unpreserved
devices being quiesced at this point, in a comment above in this
function.
> > I added a comment at the top of this function to explain this, let me
> > know if you want me to expand it with more details.
> >
> > > - then clear the remaining fields of the entry.
>
> One important point remains: even if cache invalidation is deferred and
> done globally, we should still clear the Present bit before clearing the
> rest of a context entry. While Present is set, hardware may fetch the
> 256-bit entry in multiple chunks. Rewriting the full entry is not atomic
> (it becomes multiple CPU writes), so hardware could observe a torn value
> (a mix of old and new fields), which may lead to undefined behavior or
> spurious faults.

Ah yes, I wanted to write that in my previous reply but it seems I
missed that.
> So the recommended sequence is:
> - clear only the Present bit,
> - flush the updated entry to memory if necessary,
> - [add a comment about the delayed global invalidation approach,]
> - then clear the remaining fields of the entry.

This is exactly my plan for the next revision. We are aligned on it.
> Thanks,
> baolu

Thanks,
Sami