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

[Julien Grall <julien.grall@linaro.org>]

On 20/03/2015 13:56, Robert VanVossen wrote:

>>>    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.

AFAICT arm_smmu_detach_dev will only fail it's not able to find an 
iommu_group (i.e the list of Stream IDs) for a device.

On Xen case, the check in arm_smmu_deassign_dev guaranty us that the 
device is used by the SMMU and therefore an iommu_group exits for it.

So I think we can safely move the call in arm_smmu_detach_dev.

[..]

>>> +					dev_err(dev, "cannot attach device to already existing iommu_domain\n");
>>> +					return -ENXIO;
>>> +				}
>
> Is this an appropriate return error?

I would return the result of arm_smmu_attach_dev (i.e ret).

[..]

>>> @@ -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.

Regards,

-- 
Julien Grall