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

[Suthikulpanit, Suravee]

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