In preparation for 256-bit DTE update, each DTE access/update needs
to be protected using synchronication mechanism to prevent conflict.

Introduce a new rw-semaphore struct dev_data.dte_sem, which is per
device. Also update certain helper functions to use the new dte_sem.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  1 +
 drivers/iommu/amd/iommu.c           | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index c9f9a598eb82..65f3a073999d 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -833,6 +833,7 @@ struct devid_map {
 struct iommu_dev_data {
 	/*Protect against attach/detach races */
 	spinlock_t lock;
+	struct rw_semaphore dte_sem;
 
 	struct list_head list;		  /* For domain->dev_list */
 	struct llist_node dev_data_list;  /* For global dev_data_list */
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 87c5385ce3f2..994ed02842b9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -205,6 +205,7 @@ static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 		return NULL;
 
 	spin_lock_init(&dev_data->lock);
+	init_rwsem(&dev_data->dte_sem);
 	dev_data->devid = devid;
 	ratelimit_default_init(&dev_data->rs);
 
@@ -1946,10 +1947,13 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	}
 }
 
-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;
 
@@ -1959,6 +1963,8 @@ static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
 	dev_table[devid].data[1] &= DTE_FLAG_MASK;
 
 	amd_iommu_apply_erratum_63(iommu, devid);
+
+	up_write(&dev_data->dte_sem);
 }
 
 /* Update and flush DTE for the given device */
@@ -1969,7 +1975,7 @@ void amd_iommu_dev_update_dte(struct iommu_dev_data *dev_data, bool set)
 	if (set)
 		set_dte_entry(iommu, dev_data);
 	else
-		clear_dte_entry(iommu, dev_data->devid);
+		clear_dte_entry(iommu, dev_data);
 
 	clone_aliases(iommu, dev_data->dev);
 	device_flush_dte(dev_data);
-- 
2.34.1

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

Hi,

On 8/30/2024 2:34 AM, Jason Gunthorpe wrote:
> 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.

Thanks for pointing this out. I missed the detail in this function while 
going through the driver to update the code, which updates live DTE. 
I'll change this.

> 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?

Yes, it's called from blocked_domain->attach_dev() and when doing 
amd_iommu_detach_device().

> 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.

Actually, we Need to preserve DTE[96:106] because certain fields are 
programmed using value in IVRS table from early init phase. So, to avoid 
try_cmpxchg128 failure, I need to spin_lock on dte_lock across the 
get_dte256() and update_dte256()

Thanks,
Suravee

On Thu, Sep 05, 2024 at 01:20:42PM +0700, Suthikulpanit, Suravee wrote:

> > 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?
> 
> Yes, it's called from blocked_domain->attach_dev() and when doing
> amd_iommu_detach_device().

Could give it a better name then too
 
> > 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.
> 
> Actually, we Need to preserve DTE[96:106] because certain fields are
> programmed using value in IVRS table from early init phase. So, to avoid
> try_cmpxchg128 failure, I need to spin_lock on dte_lock across the
> get_dte256() and update_dte256()

But who is concurrently writing those bits? Isn't it just set at boot
time once and sort of stored in the DTE, never changing?

There are two locking regimes here, all the normal DTE touches from
iomm ops are locked by the group mutex and they are non-concurrent
already.

Jason