Re: [PATCH v5 05/14] iommu/amd: Introduce helper function amd_iommu_update_dte()

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

On 11/14/2025 2:18 AM, Nicolin Chen wrote:
> On Wed, Nov 12, 2025 at 06:24:57PM +0000, Suravee Suthikulpanit wrote:
>> +void amd_iommu_update_dte(struct amd_iommu *iommu,
>> +			     struct iommu_dev_data *dev_data,
>> +			     struct dev_table_entry *new)
>> +{
>> +	update_dte256(iommu, dev_data, new);
>> +	clone_aliases(iommu, dev_data->dev);
>> +	device_flush_dte(dev_data);
>> +	iommu_completion_wait(iommu);
>> +}
>> +
>>   static void get_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
>>   		      struct dev_table_entry *dte)
>>   {
>> @@ -2088,7 +2104,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
>>   
>>   	set_dte_gcr3_table(iommu, dev_data, &new);
>>   
>> -	update_dte256(iommu, dev_data, &new);
>> +	amd_iommu_update_dte(iommu, dev_data, &new);
>>   
>>   	/*
>>   	 * A kdump kernel might be replacing a domain ID that was copied from
>> @@ -2108,7 +2124,7 @@ static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_
>>   	struct dev_table_entry new = {};
>>   
>>   	amd_iommu_make_clear_dte(dev_data, &new);
>> -	update_dte256(iommu, dev_data, &new);
>> +	amd_iommu_update_dte(iommu, dev_data, &new);
>>   }
>>   
>>   /* Update and flush DTE for the given device */
>> @@ -2120,10 +2136,6 @@ static void dev_update_dte(struct iommu_dev_data *dev_data, bool set)
>>   		set_dte_entry(iommu, dev_data, 0, 0);
>>   	else
>>   		clear_dte_entry(iommu, dev_data);
> 
> I found these two are somewhat unnecessary.
> 
> set_dte_entry()
> {
> 	u32 old_domid;
> 
> 	make_clear_dte(dev_data, dte, &new);
> 	....
> 	amd_iommu_update_dte(iommu, dev_data, &new);
> 	if (old_domid)
> 		amd_iommu_flush_tlb_domid(iommu, old_domid);
> }
> 
> clear_dte_entry()
> {
> 	make_clear_dte(dev_data, dte, &new);
> 	amd_iommu_update_dte(iommu, dev_data, &new);
> }
> 
> And given that dev_update_dte() now are just calling these them
> without any other thing to do. Why not just unwrap them:
> 
> dev_update_dte(struct iommu_dev_data *dev_data, bool set)
> {
> 	u32 old_domid = 0;
> 
> 	make_clear_dte(dev_data, dte, &new);
> 	if (!set)
> 		goto update_dte;
> 	....
> update_dte:
> 	amd_iommu_update_dte(iommu, dev_data, &new);
> 	if (old_domid)
> 		amd_iommu_flush_tlb_domid(iommu, old_domid);
> }
> 
> ?

Currently, set_dte_entry() is called from multiple call-path. Therefore, 
I would like to keep this way to simplify the code reuse.  The 
clear_dte_entry() is called only from one place, but I feel that having 
a separate function is simpler to read.

Thanks,
Suravee