iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

[PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

Posted by Wei Wang 8 months, 2 weeks ago

When dirty_tracking is enabled, first_stage is set to false to use the
second stage translation table. dmar_domain->use_first_level, which is
assigned from first_page, is guaranteed to be false when the execution
reaches the location of the code to be removed by this patch. So the
handling for dmar_domain->use_first_level being true there will never
be executed.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 drivers/iommu/intel/iommu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cb0b993bebb4..1145567c60f9 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3418,13 +3418,8 @@ intel_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 		spin_lock_init(&dmar_domain->s1_lock);
 	}
 
-	if (dirty_tracking) {
-		if (dmar_domain->use_first_level) {
-			iommu_domain_free(domain);
-			return ERR_PTR(-EOPNOTSUPP);
-		}
+	if (dirty_tracking)
 		domain->dirty_ops = &intel_dirty_ops;
-	}
 
 	return domain;
 }

base-commit: e0797d3b91de75b6c95b4a0e0649ebd4aac1d9d1
-- 
2.43.0

Re: [PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

Posted by Jason Gunthorpe 8 months, 1 week ago

On Fri, May 30, 2025 at 05:13:25PM +0800, Wei Wang wrote:
> When dirty_tracking is enabled, first_stage is set to false to use the
> second stage translation table. dmar_domain->use_first_level, which is
> assigned from first_page, is guaranteed to be false when the execution
> reaches the location of the code to be removed by this patch. So the
> handling for dmar_domain->use_first_level being true there will never
> be executed.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

I noticed this too

Jason

Re: [PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

Posted by Baolu Lu 8 months, 1 week ago

On 5/30/25 17:13, Wei Wang wrote:
> When dirty_tracking is enabled, first_stage is set to false to use the
> second stage translation table. dmar_domain->use_first_level, which is
> assigned from first_page, is guaranteed to be false when the execution
> reaches the location of the code to be removed by this patch. So the
> handling for dmar_domain->use_first_level being true there will never
> be executed.
> 
> Signed-off-by: Wei Wang<wei.w.wang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cb0b993bebb4..1145567c60f9 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3418,13 +3418,8 @@ intel_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>   		spin_lock_init(&dmar_domain->s1_lock);
>   	}
>   
> -	if (dirty_tracking) {
> -		if (dmar_domain->use_first_level) {

This *explicit* check enforces that dirty tracking cannot be supported
for a domain that relies on first-stage translation due to the lack of
enabling/disabling dirty tracking support.

While this might appear redundant, this prevents potential issues
if related code is modified without awareness of this dependency.

> -			iommu_domain_free(domain);
> -			return ERR_PTR(-EOPNOTSUPP);
> -		}

Thanks,
baolu

Re: [PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

Posted by Ethan Zhao 8 months, 1 week ago


On 6/3/2025 10:39 AM, Baolu Lu wrote:
> On 5/30/25 17:13, Wei Wang wrote:
>> When dirty_tracking is enabled, first_stage is set to false to use the
>> second stage translation table. dmar_domain->use_first_level, which is
>> assigned from first_page, is guaranteed to be false when the execution
>> reaches the location of the code to be removed by this patch. So the
>> handling for dmar_domain->use_first_level being true there will never
>> be executed.
>>
>> Signed-off-by: Wei Wang<wei.w.wang@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index cb0b993bebb4..1145567c60f9 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3418,13 +3418,8 @@ intel_iommu_domain_alloc_paging_flags(struct 
>> device *dev, u32 flags,
>>           spin_lock_init(&dmar_domain->s1_lock);
>>       }
>> -    if (dirty_tracking) {
>> -        if (dmar_domain->use_first_level) {
> 
> This *explicit* check enforces that dirty tracking cannot be supported
> for a domain that relies on first-stage translation due to the lack of
> enabling/disabling dirty tracking support.
> 
> While this might appear redundant, this prevents potential issues
> if related code is modified without awareness of this dependency. 
There would always something sooner or later like this to stop the 
improper configuration/coding , under an assumption that caller path 
always coded perfect, indeed, we could remove a lot of error stopper code.

But that world seems never works like that.

Thanks,
Ethan
> 
>> -            iommu_domain_free(domain);
>> -            return ERR_PTR(-EOPNOTSUPP);
>> -        }
> 
> Thanks,
> baolu
>

RE: [PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging_flags()

Posted by Wang, Wei W 8 months, 1 week ago

On Tuesday, June 3, 2025 10:40 AM, Baolu Lu wrote:
> On 5/30/25 17:13, Wei Wang wrote:
> > When dirty_tracking is enabled, first_stage is set to false to use the
> > second stage translation table. dmar_domain->use_first_level, which is
> > assigned from first_page, is guaranteed to be false when the execution
> > reaches the location of the code to be removed by this patch. So the
> > handling for dmar_domain->use_first_level being true there will never
> > be executed.
> >
> > Signed-off-by: Wei Wang<wei.w.wang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 7 +------
> >   1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index cb0b993bebb4..1145567c60f9 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -3418,13 +3418,8 @@ intel_iommu_domain_alloc_paging_flags(struct
> device *dev, u32 flags,
> >   		spin_lock_init(&dmar_domain->s1_lock);
> >   	}
> >
> > -	if (dirty_tracking) {
> > -		if (dmar_domain->use_first_level) {
> 
> This *explicit* check enforces that dirty tracking cannot be supported for a
> domain that relies on first-stage translation due to the lack of
> enabling/disabling dirty tracking support.
> 
> While this might appear redundant, this prevents potential issues if related
> code is modified without awareness of this dependency.

Would a warning (i.e., WARN_ON(dmar_domain->use_first_level)) be better here?
(this condition seems unlikely to occur in practice, especially in official kernel releases,
and the current handling logic appears somewhat confusing. If developers add some
new logic that could change dmar_domain->use_first_level to ture (maybe by mistake)
in the future, they could bring the handling back, which might look clearer?)

> 
> > -			iommu_domain_free(domain);
> > -			return ERR_PTR(-EOPNOTSUPP);
> > -		}
> 
> Thanks,
> baolu