All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iommu: Add a check to avoid invalid iotlb sync
@ 2021-03-27  6:23 chenxiang
  2021-03-29 14:45 ` Will Deacon
  0 siblings, 1 reply; 6+ messages in thread
From: chenxiang @ 2021-03-27  6:23 UTC (permalink / raw)
  To: robin.murphy, will; +Cc: iommu, linuxarm

From: Xiang Chen <chenxiang66@hisilicon.com>

Currently it will send a iotlb sync at end of iommu unmap even if
iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
necessary, so add a check to avoid invalid iotlb sync.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
 include/linux/iommu.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ca6e6b..6afa61b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
 static inline void iommu_iotlb_sync(struct iommu_domain *domain,
 				  struct iommu_iotlb_gather *iotlb_gather)
 {
+	if (!iotlb_gather->pgsize)
+		return;
+
 	if (domain->ops->iotlb_sync)
 		domain->ops->iotlb_sync(domain, iotlb_gather);
 
-- 
2.8.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
  2021-03-27  6:23 [PATCH] iommu: Add a check to avoid invalid iotlb sync chenxiang
@ 2021-03-29 14:45 ` Will Deacon
  2021-03-30  1:22   ` chenxiang (M)
  0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2021-03-29 14:45 UTC (permalink / raw)
  To: chenxiang; +Cc: iommu, robin.murphy, linuxarm

On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> Currently it will send a iotlb sync at end of iommu unmap even if
> iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
> necessary, so add a check to avoid invalid iotlb sync.
> 
> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> ---
>  include/linux/iommu.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9ca6e6b..6afa61b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
>  static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>  				  struct iommu_iotlb_gather *iotlb_gather)
>  {
> +	if (!iotlb_gather->pgsize)
> +		return;

In which circumstances does this occur?

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
  2021-03-29 14:45 ` Will Deacon
@ 2021-03-30  1:22   ` chenxiang (M)
  2021-03-30  9:04     ` Robin Murphy
  0 siblings, 1 reply; 6+ messages in thread
From: chenxiang (M) @ 2021-03-30  1:22 UTC (permalink / raw)
  To: Will Deacon; +Cc: iommu, robin.murphy, linuxarm

Hi Will,


在 2021/3/29 22:45, Will Deacon 写道:
> On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> Currently it will send a iotlb sync at end of iommu unmap even if
>> iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
>> necessary, so add a check to avoid invalid iotlb sync.
>>
>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>> ---
>>   include/linux/iommu.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 9ca6e6b..6afa61b 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct iommu_domain *domain)
>>   static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>>   				  struct iommu_iotlb_gather *iotlb_gather)
>>   {
>> +	if (!iotlb_gather->pgsize)
>> +		return;
> In which circumstances does this occur?
>
> Will

When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize 
= 0).
If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will 
fill the iotlb_gather (set iotlb_gather->pgsize = granule);
but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb 
flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize 
= 0),
it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is 
actually invalid iotlb sync at last.


>
> .
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
  2021-03-30  1:22   ` chenxiang (M)
@ 2021-03-30  9:04     ` Robin Murphy
  2021-03-30  9:25       ` Will Deacon
  0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2021-03-30  9:04 UTC (permalink / raw)
  To: chenxiang (M), Will Deacon; +Cc: iommu, linuxarm

On 2021-03-30 02:22, chenxiang (M) wrote:
> Hi Will,
> 
> 
> 在 2021/3/29 22:45, Will Deacon 写道:
>> On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>
>>> Currently it will send a iotlb sync at end of iommu unmap even if
>>> iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
>>> necessary, so add a check to avoid invalid iotlb sync.
>>>
>>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>>> ---
>>>   include/linux/iommu.h | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 9ca6e6b..6afa61b 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct 
>>> iommu_domain *domain)
>>>   static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>>>                     struct iommu_iotlb_gather *iotlb_gather)
>>>   {
>>> +    if (!iotlb_gather->pgsize)
>>> +        return;
>> In which circumstances does this occur?
>>
>> Will
> 
> When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize 
> = 0).
> If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will 
> fill the iotlb_gather (set iotlb_gather->pgsize = granule);
> but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb 
> flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize 
> = 0),
> it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is 
> actually invalid iotlb sync at last.

I guess this can happen in DMA API usage if we've previously mapped a 
block's worth of scatterlist pages into a block-=sized IOVA region, but 
this is not the place to do anything about it. The main thing this patch 
will do is break all the other drivers that implement .iotlb_sync but do 
not use iotlb_gather.

By all means optimise SMMUv3's sync behaviour, but the only valid place 
to do that is in SMMUv3's own sync callback. These particular details 
are beyond what the IOMMU core knows about.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
  2021-03-30  9:04     ` Robin Murphy
@ 2021-03-30  9:25       ` Will Deacon
  2021-03-30 10:46         ` chenxiang (M)
  0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2021-03-30  9:25 UTC (permalink / raw)
  To: Robin Murphy; +Cc: iommu, linuxarm

On Tue, Mar 30, 2021 at 10:04:53AM +0100, Robin Murphy wrote:
> On 2021-03-30 02:22, chenxiang (M) wrote:
> > Hi Will,
> > 
> > 
> > 在 2021/3/29 22:45, Will Deacon 写道:
> > > On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
> > > > From: Xiang Chen <chenxiang66@hisilicon.com>
> > > > 
> > > > Currently it will send a iotlb sync at end of iommu unmap even if
> > > > iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
> > > > necessary, so add a check to avoid invalid iotlb sync.
> > > > 
> > > > Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> > > > ---
> > > >   include/linux/iommu.h | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > > index 9ca6e6b..6afa61b 100644
> > > > --- a/include/linux/iommu.h
> > > > +++ b/include/linux/iommu.h
> > > > @@ -529,6 +529,9 @@ static inline void
> > > > iommu_flush_iotlb_all(struct iommu_domain *domain)
> > > >   static inline void iommu_iotlb_sync(struct iommu_domain *domain,
> > > >                     struct iommu_iotlb_gather *iotlb_gather)
> > > >   {
> > > > +    if (!iotlb_gather->pgsize)
> > > > +        return;
> > > In which circumstances does this occur?
> > > 
> > > Will
> > 
> > When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize
> > = 0).
> > If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will
> > fill the iotlb_gather (set iotlb_gather->pgsize = granule);
> > but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb
> > flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize
> > = 0),
> > it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is
> > actually invalid iotlb sync at last.
> 
> I guess this can happen in DMA API usage if we've previously mapped a
> block's worth of scatterlist pages into a block-=sized IOVA region, but this
> is not the place to do anything about it. The main thing this patch will do
> is break all the other drivers that implement .iotlb_sync but do not use
> iotlb_gather.
> 
> By all means optimise SMMUv3's sync behaviour, but the only valid place to
> do that is in SMMUv3's own sync callback. These particular details are
> beyond what the IOMMU core knows about.

Yes, that's where I was heading. Chenxiang, please could you send a v2
with the check inside arm_smmu_iotlb_sync() instead?

Thanks,

Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
  2021-03-30  9:25       ` Will Deacon
@ 2021-03-30 10:46         ` chenxiang (M)
  0 siblings, 0 replies; 6+ messages in thread
From: chenxiang (M) @ 2021-03-30 10:46 UTC (permalink / raw)
  To: Will Deacon, Robin Murphy; +Cc: iommu, linuxarm



在 2021/3/30 17:25, Will Deacon 写道:
> On Tue, Mar 30, 2021 at 10:04:53AM +0100, Robin Murphy wrote:
>> On 2021-03-30 02:22, chenxiang (M) wrote:
>>> Hi Will,
>>>
>>>
>>> 在 2021/3/29 22:45, Will Deacon 写道:
>>>> On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
>>>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>>>
>>>>> Currently it will send a iotlb sync at end of iommu unmap even if
>>>>> iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
>>>>> necessary, so add a check to avoid invalid iotlb sync.
>>>>>
>>>>> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
>>>>> ---
>>>>>    include/linux/iommu.h | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>>> index 9ca6e6b..6afa61b 100644
>>>>> --- a/include/linux/iommu.h
>>>>> +++ b/include/linux/iommu.h
>>>>> @@ -529,6 +529,9 @@ static inline void
>>>>> iommu_flush_iotlb_all(struct iommu_domain *domain)
>>>>>    static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>>>>>                      struct iommu_iotlb_gather *iotlb_gather)
>>>>>    {
>>>>> +    if (!iotlb_gather->pgsize)
>>>>> +        return;
>>>> In which circumstances does this occur?
>>>>
>>>> Will
>>> When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize
>>> = 0).
>>> If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will
>>> fill the iotlb_gather (set iotlb_gather->pgsize = granule);
>>> but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb
>>> flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize
>>> = 0),
>>> it will still send iommu_iotlb_sync(domain, &iotlb_gather) which is
>>> actually invalid iotlb sync at last.
>> I guess this can happen in DMA API usage if we've previously mapped a
>> block's worth of scatterlist pages into a block-=sized IOVA region, but this
>> is not the place to do anything about it. The main thing this patch will do
>> is break all the other drivers that implement .iotlb_sync but do not use
>> iotlb_gather.
>>
>> By all means optimise SMMUv3's sync behaviour, but the only valid place to
>> do that is in SMMUv3's own sync callback. These particular details are
>> beyond what the IOMMU core knows about.
> Yes, that's where I was heading. Chenxiang, please could you send a v2
> with the check inside arm_smmu_iotlb_sync() instead?

Ok, thanks, i will send a v2 later.

>
> Thanks,
>
> Will
>
> .
>


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-30 10:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-27  6:23 [PATCH] iommu: Add a check to avoid invalid iotlb sync chenxiang
2021-03-29 14:45 ` Will Deacon
2021-03-30  1:22   ` chenxiang (M)
2021-03-30  9:04     ` Robin Murphy
2021-03-30  9:25       ` Will Deacon
2021-03-30 10:46         ` chenxiang (M)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.