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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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