Re: [PATCH v2 14/19] iommu/arm-smmu-v3: Remove arm_smmu_master->domain

[Eric Auger <eauger@redhat.com>]

Hi Jason,

On 11/30/23 13:03, Jason Gunthorpe wrote:
> On Mon, Nov 27, 2023 at 06:14:30PM +0100, Eric Auger wrote:
>> Hi Jason,
>>
>> On 11/13/23 18:53, Jason Gunthorpe wrote:
>>> Introducing global statics which are of type struct iommu_domain, not
>>> struct arm_smmu_domain makes it difficult to retain
>>> arm_smmu_master->domain, as it can no longer point to an IDENTITY or
>>> BLOCKED domain.
>>>
>>> The only place that uses the value is arm_smmu_detach_dev(). Change things
>>> to work like other drivers and call iommu_get_domain_for_dev() to obtain
>>> the current domain.
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>
>> This patch introduces a crash on my machine. See below.
> 
> Ah, your system must have multi-device groups
> 
> The master->domain was subtly protecting the domain_head to ensure
> we don't touch it unless it is already in a domain list. This issue is
> solved in part 2 (iommu/arm-smmu-v3: Make smmu_domain->devices into an
> allocated list) which removes the domain_head.
> 
> This hunk should fix this patch. I updated the github

I confirm that the hunk hereafter fixes the crash.

Thanks

Eric
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 23dda64722ea17..102e13b65bcdec 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2491,7 +2491,7 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  	arm_smmu_disable_ats(master, smmu_domain);
>  
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> +	list_del_init(&master->domain_head);
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
>  	master->ats_enabled = false;
> @@ -2606,7 +2606,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  
>  out_list_del:
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> +	list_del_init(&master->domain_head);
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
>  out_unlock:
> @@ -2810,6 +2810,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  	master->dev = dev;
>  	master->smmu = smmu;
>  	INIT_LIST_HEAD(&master->bonds);
> +	INIT_LIST_HEAD(&master->domain_head);
>  	dev_iommu_priv_set(dev, master);
>  
>  	ret = arm_smmu_insert_master(smmu, master);
> 
> 
> 
> Thank!!
> Jason
>