Re: [PATCH v3 2/5] iommu/amd: Introduce helper functions to access and update 256-bit DTE

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

On 9/7/2024 12:00 AM, Jason Gunthorpe wrote:
> On Fri, Sep 06, 2024 at 12:13:05PM +0000, Suravee Suthikulpanit wrote:
> 
>> +static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
>> +			  struct dev_table_entry *new)
>> +{
>> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
>> +	struct dev_table_entry *ptr = &dev_table[dev_data->devid];
>> +	struct dev_table_entry old;
>> +	u128 tmp;
>> +
>> +	lockdep_assert_held(&dev_data->dte_lock);
>> +
>> +	old.data128[0] = ptr->data128[0];
>> +	old.data128[1] = ptr->data128[1];
>> +
>> +	tmp = cmpxchg128(&ptr->data128[1], old.data128[1], new->data128[1]);
>> +	if (tmp == old.data128[1]) {
>> +		if (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0])) {
>> +			/* Restore hi 128-bit */
>> +			cmpxchg128(&ptr->data128[1], new->data128[1], tmp);
> 
> Like I said before, you can't fix this. Just go fowards. Keeping an
> old DTE will UAF things, that is much worse than just forcing a new
> DTE and loosing some interrupt settings.
> 
>> @@ -243,13 +285,28 @@ static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
>>   	if (!iommu)
>>   		return 0;
>>   
>> -	amd_iommu_set_rlookup_table(iommu, alias);
>> -	dev_table = get_dev_table(iommu);
>> -	memcpy(dev_table[alias].data,
>> -	       dev_table[devid].data,
>> -	       sizeof(dev_table[alias].data));
>> +	/* Get DTE for pdev */
>> +	dev_data = dev_iommu_priv_get(&pdev->dev);
>> +	if (!dev_data)
>> +		return -EINVAL;
>>   
>> -	return 0;
>> +	spin_lock(&dev_data->dte_lock);
>> +	get_dte256(iommu, dev_data, &dte);
>> +	spin_unlock(&dev_data->dte_lock);
> 
> You can't unlock after reading the DTE and the relock it to program
> it. The interrupt data can have changed while unlocked.
> 
> Put the lock inside update_dte256() and force the interrupt bits
> under the lock.
> 
> Something like this is what I'm expecting:
> 
> static void write_upper(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> 	struct dev_table_entry old;
> 
> 	lockdep_assert_held(&dev_data->dte_lock);
> 
> 	do {
> 		old->data128[1] = ptr->data128[1];
> 		new->data64[2] &= ~INTR_MASK;
> 		new->data64[2] |= old->data64[2] & INTR_MASK;
> 	} while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
> 				 new->data128[1]));
> }
> 
> static void write_lower(struct dev_table_entry *ptr, struct dev_table_entry *new)
> {
> 	struct dev_table_entry old;
> 
> 	do {
> 		old->data128[0] = ptr->data128[0];
> 	} while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
> 				 new->data128[0]));
> }
> 
> static void update_dte256(struct amd_iommu *iommu,
> 			  struct iommu_dev_data *dev_data,
> 			  struct dev_table_entry *new)
> {
> 
> 	spin_lock(&dev_data->dte_lock);
> 	if (!(ptr->data64[0] & V)) {
> 		write_upper(ptr, new);
> 		/* NO FLUSH assume V=0 never cached */
> 		write_lower(ptr, new);
> 		/* FLUSH */
> 	} else if (!(new->data64[0] & V) {
> 		write_lower(ptr, new);
> 		/* FLUSH */
> 		write_upper(ptr, new);
> 		/* NO FLUSH assume V=0 never cached */
> 	} else {
> 		/* both are valid */
> 		if (FIELD_GET(ptr->data[2], GUEST_PAGING_MODE) ==
> 		    FIELD_GET(new->data[2], GUEST_PAGING_MODE)) {
> 			/* Upper doesn't change */
> 			write_upper(ptr, new);
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		else if (old has no guest page table) {
> 			write_upper(ptr, new);
> 			/* FLUSH */
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		else if (new has no guest page table) {
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 			write_upper(ptr, new);
> 			/* FLUSH */
> 		} else {
> 			struct dev_table_entry clear = {};
> 
> 			write_lower(ptr, &clear);
> 			/* FLUSH to set V=0 */
> 			write_upper(ptr, new);
> 			/* NO FLUSH assume V=0 never cached */
> 			write_lower(ptr, new);
> 			/* FLUSH */
> 		}
> 	}
> 
> 	spin_unlock(&dev_data->dte_lock);
> }

Got your point. I will update based on your suggestion here.

> And it probably needs more logic to accomodate the VIOMMU valid bits
> in the 2nd 128.

I'll take care of this when enable the HW-vIOMMU stuff.

Thanks,
Suravee