Message-ID: <BL1PR11MB52711FA152B59686DE3165648C96A@BL1PR11MB5271.namprd11.prod.outlook.com>
Date: Wed, 21 Jan 2026 08:12:23 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
 Gunthorpe" <jgg@...dia.com>
CC: Dmytro Maluka <dmaluka@...omium.org>, Samiullah Khawaja
	<skhawaja@...gle.com>, "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/3] iommu/vt-d: Clear Present bit before tearing down
 context entry

> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, January 21, 2026 4:04 PM
> 
> On 1/21/26 15:50, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@...ux.intel.com>
> >> Sent: Wednesday, January 21, 2026 3:29 PM
> >>
> >> On 1/21/26 14:23, Tian, Kevin wrote:
> >>>> From: Lu Baolu <baolu.lu@...ux.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@...omium.org>
> >>>> Closes: https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/
> >>>> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> >>>
> >>> Reviewed-by: Kevin Tian <kevin.tian@...el.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

Powered by blists - more mailing lists