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

[Dmytro Maluka <dmaluka@chromium.org>]

On Tue, Jan 20, 2026 at 02:18:13PM +0800, Lu Baolu wrote:
> 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>
> ---
>  drivers/iommu/intel/iommu.h | 21 ++++++++++++++++++++-
>  drivers/iommu/intel/iommu.c |  4 +++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 25c5e22096d4..599913fb65d5 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -900,7 +900,26 @@ static inline int pfn_level_offset(u64 pfn, int level)
>  
>  static inline void context_set_present(struct context_entry *context)
>  {
> -	context->lo |= 1;
> +	u64 val;
> +
> +	dma_wmb();
> +	val = READ_ONCE(context->lo) | 1;

As IIRC Jason noted, READ_ONCE is not really necessary?

> +	WRITE_ONCE(context->lo, val);
> +}
> +
> +/*
> + * Clear the Present (P) bit (bit 0) of a context table entry. This initiates
> + * the transition of the entry's ownership from hardware to software. The
> + * caller is responsible for fulfilling the invalidation handshake recommended
> + * by the VT-d spec, Section 6.5.3.3 (Guidance to Software for Invalidations).
> + */
> +static inline void context_clear_present(struct context_entry *context)
> +{
> +	u64 val;
> +
> +	val = READ_ONCE(context->lo) & GENMASK_ULL(63, 1);

Maybe "& ~1ULL" would be a bit more readable? (and READ_ONCE not
necessary here either?)

Anyway,

Reviewed-by: Dmytro Maluka <dmaluka@chromium.org>

> +	WRITE_ONCE(context->lo, val);
> +	dma_wmb();
>  }
>  
>  static inline void context_set_fault_enable(struct context_entry *context)
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 134302fbcd92..c66cc51f9e51 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1240,10 +1240,12 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
>  	}
>  
>  	did = context_domain_id(context);
> -	context_clear_entry(context);
> +	context_clear_present(context);
>  	__iommu_flush_cache(iommu, context, sizeof(*context));
>  	spin_unlock(&iommu->lock);
>  	intel_context_flush_no_pasid(info, context, did);
> +	context_clear_entry(context);
> +	__iommu_flush_cache(iommu, context, sizeof(*context));
>  }
>  
>  int __domain_setup_first_level(struct intel_iommu *iommu, struct device *dev,
> -- 
> 2.43.0
>