Re: [PATCH v2 2/5] iommu/amd: Introduce rw_semaphore for Device Table Entry (DTE)

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Aug 29, 2024 at 06:07:23PM +0000, Suravee Suthikulpanit wrote:

> -static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> +static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
>  {
> +	u16 devid = dev_data->devid;
>  	struct dev_table_entry *dev_table = get_dev_table(iommu);
>  
> +	down_write(&dev_data->dte_sem);
> +
>  	/* remove entry from the device table seen by the hardware */
>  	dev_table[devid].data[0]  = DTE_FLAG_V;

The rest of this function doesn't seem very good either:

	/* remove entry from the device table seen by the hardware */
	dev_table[devid].data[0]  = DTE_FLAG_V;

	if (!amd_iommu_snp_en)
		dev_table[devid].data[0] |= DTE_FLAG_TV;

	dev_table[devid].data[1] &= DTE_FLAG_MASK;

This should also cmpxchg the whole 128 bit quantity, you don't want
the HW to see a tear on HostPageTableRootPtr.

Which is back to my other point. Don't edit the DTE in place.

There is no such thing as 'clear' in the iommu domain API. The DTE is
either PAGING, BLOCKED or IDENTITY, and any write to it should be
writing an entire DTE for that target.

I guess clear is actually trying to set the DTE to BLOCKING?

Also no need to get the lock here because you don't touch 128 bit
quanta [1] which holds the IRQ stuff that is racey. This is already
locked by the iommu core group lock.

Jason