Re: [PATCH 4.19 17/32] iommu/vt-d: Dont queue_iova() if there is no flush queue

[Dmitry Safonov <dima@arista.com>]

On 8/6/19 11:47 PM, Dmitry Safonov wrote:
> Hi Pavel,
> 
> On 8/3/19 10:34 PM, Pavel Machek wrote:
>> Hi!
>>
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -3721,7 +3721,7 @@ static void intel_unmap(struct device *d
>>>  
>>>  	freelist = domain_unmap(domain, start_pfn, last_pfn);
>>>  
>>> -	if (intel_iommu_strict) {
>>> +	if (intel_iommu_strict || !has_iova_flush_queue(&domain->iovad)) {
>>>  		iommu_flush_iotlb_psi(iommu, domain, start_pfn,
>>>  				      nrpages, !freelist, 0);
>>>  		/* free iova */
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -65,9 +65,14 @@ init_iova_domain(struct iova_domain *iov
>>>  }
>>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>>  
>>> +bool has_iova_flush_queue(struct iova_domain *iovad)
>>> +{
>>> +	return !!iovad->fq;
>>
>> Should this be READ_ONCE()?
> 
> Why? Compiler can't anyhow assume that it's always true/false and there
> is a clear data dependency between this and:
> :	queue_iova(&domain->iovad, iova_pfn, nrpages,
> :			   (unsigned long)freelist);
> 
>>
>>> @@ -100,13 +106,17 @@ int init_iova_flush_queue(struct iova_do
>>>  	for_each_possible_cpu(cpu) {
>>>  		struct iova_fq *fq;
>>>  
>>> -		fq = per_cpu_ptr(iovad->fq, cpu);
>>> +		fq = per_cpu_ptr(queue, cpu);
>>>  		fq->head = 0;
>>>  		fq->tail = 0;
>>>  
>>>  		spin_lock_init(&fq->lock);
>>>  	}
>>>  
>>> +	smp_wmb();
>>> +
>>> +	iovad->fq = queue;
>>> +
>>
>> Could we have a comment why the barrier is needed,
> 
> I'm up for the comment if you feel like it - in my POV it's quite
> obvious that we want finish initializing the queue's internals before
> assigning the queue. I didn't put the comment exactly because I felt
> like it would state something evident [in my POV].
> 
>> and perhaps there
>> should be oposing smp_rmb() somewhere? Does this need to be
>> WRITE_ONCE() as it is racing against reader?
> 
> I feel confused. I might have forgotten everything about barriers, but
> again if I'm not mistaken, one doesn't need a barrier in:
> : if (A->a != NULL)
> :     use(A); /* dereferences A->a */
> : else
> :     /* don't use `a' */

And in this simplified example I mean that use() will either see A->a
initialized (IOW, CPU can't load A->a->field1 before checking the
condition) or use() will not be called.

Thanks,
          Dmitry