Re: [PATCH 2/6] iommu/amd: Clear DTE with update_dte256 in iommu_ignore_device()

[Pranjal Shrivastava <praan@google.com>]

On Mon, Jun 01, 2026 at 11:19:56AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 01, 2026 at 01:42:00PM +0000, Pranjal Shrivastava wrote:
> > The iommu_ignore_device() function currently uses memset() to clear
> > the Device Table Entry (DTE), which risks torn writes because the
> > hardware reads DTEs as atomic 256-bit qwords. Fix this by using
> > update_dte256() to perform a hardware-safe atomic clear when a live
> > dev_data entry is available.
> > 
> > Fixes: 99fc4ac3d297 ("iommu/amd: Introduce per PCI segment alias_table")
> > Reported-by: sashiko-bot@kernel.org
> > Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  drivers/iommu/amd/iommu.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index a94de66a885e..9b5861e241d7 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -750,7 +750,16 @@ static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
> >  	setup_aliases(iommu, dev);
> >  
> >  	pci_seg->rlookup_table[devid] = NULL;
> > -	memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
> > +
> > +	/* Clear DTE if we have a live entry */
> > +	if (dev_data) {
> > +		struct dev_table_entry new = {};
> > +
> > +		amd_iommu_make_clear_dte(dev_data, &new);
> > +		update_dte256(iommu, dev_data, &new);
> > +	} else {
> > +		memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
> > +	}
> 
> This seems a little weird, an ignored device shouldn't have a dev_data
> really, or it will soon be freed.
> 
> I think you are better to replace the memset with a dedicated function
> 
> /* Cannot not be used on a probe'd device with a live dev_data */
> disable_dte(..)
> {
> 	struct dev_table_entry new = {};
> 
> 	write_dte_lower128(ptr, new);
> 	write_dte_upper128(ptr, new);
> }
> 
> And then this new ordering breaks the clone_aliases flow, it was
> supposed to copy the 0 from the current DTE to the aliases..

Ack.. I'll fix it.

Thanks,
Praan