RE: [PATCH v2 2/3] iommu/vt-d: Clear Present bit before tearing down context entry

Tian, Kevin
2026-01-21T08:12:26Z

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, January 21, 2026 4:04 PM
>
> On 1/21/26 15:50, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Wednesday, January 21, 2026 3:29 PM
> >>
> >> On 1/21/26 14:23, Tian, Kevin wrote:
> >>>> From: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> Sent: Tuesday, January 20, 2026 2:18 PM
> >>>>
> >>>> When tearing down a context entry, the current implementation zeros
> >> the
> >>>> entire 128-bit entry using multiple 64-bit writes. This creates a window
> >>>> where the hardware can fetch a "torn" entry — where some fields are
> >>>> already zeroed while the 'Present' bit is still set — leading to
> >>>> unpredictable behavior or spurious faults.
> >>>>
> >>>> While x86 provides strong write ordering, the compiler may reorder
> >> writes
> >>>> to the two 64-bit halves of the context entry. Even without compiler
> >>>> reordering, the hardware fetch is not guaranteed to be atomic with
> >>>> respect to multiple CPU writes.
> >>>>
> >>>> Align with the "Guidance to Software for Invalidations" in the VT-d spec
> >>>> (Section 6.5.3.3) by implementing the recommended ownership
> >> handshake:
> >>>>
> >>>> 1. Clear only the 'Present' (P) bit of the context entry first to
> >>>> signal the transition of ownership from hardware to software.
> >>>> 2. Use dma_wmb() to ensure the cleared bit is visible to the IOMMU.
> >>>> 3. Perform the required cache and context-cache invalidation to ensure
> >>>> hardware no longer has cached references to the entry.
> >>>> 4. Fully zero out the entry only after the invalidation is complete.
> >>>>
> >>>> Also, add a dma_wmb() to context_set_present() to ensure the entry
> >>>> is fully initialized before the 'Present' bit becomes visible.
> >>>>
> >>>> Fixes: ba39592764ed2 ("Intel IOMMU: Intel IOMMU driver")
> >>>> Reported-by: Dmytro Maluka <dmaluka@chromium.org>
> >>>> Closes: https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/
> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> >>>
> >>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >>>
> >>> btw there is a context_clear_entry() for copied context entry in
> >>> device_pasid_table_setup(), but this patch doesn't touch that
> >>> path. It seems to assume that no in-flight DMA will exist at that
> >>> point:
> >>>
> >>> if (context_copied(iommu, bus, devfn)) {
> >>> context_clear_entry(context);
> >>> ...
> >>> /*
> >>> * At this point, the device is supposed to finish reset at
> >>> * its driver probe stage, so no in-flight DMA will exist,
> >>> * and we don't need to worry anymore hereafter.
> >>> */
> >>> clear_context_copied(iommu, bus, devfn);
> >>>
> >>> Is that guaranteed by all devices? from kdump feature p.o.v. if
> >>> that assumption is broken it just means potential DMA errors
> >>> in this transition window. But regarding to the issue which this
> >>> patch tries to fix, in-fly DMAs may lead to undesired behaviors
> >>> including memory corruption etc.
> >>>
> >>> So, should it be fixed too?
> >>
> >> This path is triggered when the device driver has probed the device
> >> (ensuring it has been reset) and then calls the kernel DMA API for the
> >> first time. At this stage, there should be no in-flight DMAs. We can
> >> apply the same logic here to improve code readability, but this is not a
> >> bug that requires a fix. Or not?
> >>
> >
> > device could be in whatever state when kdump is triggered. I'm not
> > sure whether all device drivers will reset the device at probe time.
>
> Okay, agreed.
>
> > Just thought that applying the same due diligence here could prevent
> > any undesired damage just in case. Not exactly for backporting, but
> > it's always good to have consistent logic to avoid special case based
> > on subtle assumptions...
>
> So I will add the following additional change:
>
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 34f4af4e9b5c..b63a71904cfb 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -840,7 +840,7 @@ static int device_pasid_table_setup(struct device
> *dev, u8 bus, u8 devfn)
> }
>
> if (context_copied(iommu, bus, devfn)) {
> - context_clear_entry(context);
> + context_clear_present(context);
> __iommu_flush_cache(iommu, context, sizeof(*context));
>
> /*
> @@ -860,6 +860,9 @@ static int device_pasid_table_setup(struct device
> *dev, u8 bus, u8 devfn)
> iommu->flush.flush_iotlb(iommu, 0, 0, 0,
> DMA_TLB_GLOBAL_FLUSH);
> devtlb_invalidation_with_pasid(iommu, dev, IOMMU_NO_PASID);
>
> + context_clear_entry(context);
> + __iommu_flush_cache(iommu, context, sizeof(*context));
> +
> /*
> * At this point, the device is supposed to finish reset at
> * its driver probe stage, so no in-flight DMA will exist,
>
> Appears good to you?
>

yes