Re: [PATCH] xen/passthrough: Support a single iommu_domain(context bank) per xen domain per SMMU

[Robert VanVossen <robert.vanvossen@dornerworks.com>]

On 3/20/2015 8:39 AM, Julien Grall wrote:
> Hello Robert,
> 
> Thank you for the patch.
> 

No problem. :)

>>   xen/drivers/passthrough/arm/smmu.c |  113 ++++++++++++++++++++++++++++--------
>>   1 file changed, 88 insertions(+), 25 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index a7a7da9..9b46054 100644
> 
> [..]
> 
>> @@ -1596,7 +1617,7 @@ static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
>>   	if (!cfg)
>>   		return;
>>
>> -	dev_iommu_domain(dev) = NULL;
>> +	iommu_domain_remove_device(domain, dev);
>>   	arm_smmu_domain_remove_master(smmu_domain, cfg);
>>   }
> 
> I'd like to avoid modifying arm_smmu_attach_dev and arm_smmu_detach_dev 
> as it's part of the Linux code.
> 
> I think you can increment the reference counter in arm_smmu_assign_dev 
> and arm_smmu_deassign_dev. That would avoid some possible race condition 
> (see below).
> 

I can definitely increment the reference counter in arm_smmu_assign_dev, but
arm_smmu_detach_dev doesn't return any results, so there isn't a guarantee that
the iommu_domain has actually been dereferenced for the device.

>> +			if(domain->priv->smmu == smmu)
>> +			{
>> +				/* We have found a context already associated with the same xen domain and SMMU */
>> +				ret = arm_smmu_attach_dev(domain, dev);
>> +				if (ret) {
>> +					/*
>> +					 * TODO: If arm_smmu_attach_dev fails, should we perform arm_smmu_domain_destroy,
>> +					 * eventhough another smmu_master is configured correctly? If Not, what error
>> +					 * code should we use
>> +					 */
> 
> The failure may be because we don't have any stream register available. 
> So I don't think we should detach all the other devices within the same 
> context.
> 

Agreed.

>> +					dev_err(dev, "cannot attach device to already existing iommu_domain\n");
>> +					return -ENXIO;
>> +				}

Is this an appropriate return error?

>> +
>> +				existing_ctxt_fnd = 1;
>> +				break;
>> +			}
>> +			
>> +		}
>> +		spin_unlock(&xen_domain->lock);
>> +	}
>> +	
>> +	if(!existing_ctxt_fnd){
> 
> if (!existing_ctx_fnd) {
> 
> [..]
> 
>> @@ -2633,12 +2692,16 @@ static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
>>
>>   	arm_smmu_detach_dev(domain, dev);
>>
>> -	spin_lock(&xen_domain->lock);
>> -	list_del(&domain->list);
>> -	spin_unlock(&xen_domain->lock);
>> +	if (domain->ref.counter == 0)
>> +	{
> 
> There is a possible race with the previous function. arm_smmu_assign_dev 
> still have time to increment domain->ref and we will free a domain with 
> 1 device assigned.
> 
> Overall, I think the 2 functions should be completely protected by the 
> xen_domain->lock.

Agreed. I will move the locks around.

Thanks,
Robbie VanVossen