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

[Robert VanVossen <robert.vanvossen@xxxxxxxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel