[PATCH v2 0/2] iova: Some misc changes

[PATCH v2 0/2] iova: Some misc changes

[John Garry]

This series removes some checks in the code which are not required.

Differences to v1:
- Drop re-org
- Add RB/AB tags

Based on v6.0-rc4

John Garry (2):
  iova: Remove some magazine pointer NULL checks
  iova: Remove magazine BUG_ON() checks

 drivers/iommu/iova.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

-- 
2.35.3

[PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

Since commit 32e92d9f6f87 ("iommu/iova: Separate out rcache init") it
has not been possible to have NULL CPU rcache "loaded" or "prev" magazine
pointers. As such, the checks in iova_magazine_full(),
iova_magazine_empty(), and iova_magazine_free_pfns() may be dropped.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iova.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 47d1983dfa2a..580fdf669922 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
 	unsigned long flags;
 	int i;
 
-	if (!mag)
-		return;
-
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
 
 	for (i = 0 ; i < mag->size; ++i) {
@@ -683,12 +680,12 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
 
 static bool iova_magazine_full(struct iova_magazine *mag)
 {
-	return (mag && mag->size == IOVA_MAG_SIZE);
+	return mag->size == IOVA_MAG_SIZE;
 }
 
 static bool iova_magazine_empty(struct iova_magazine *mag)
 {
-	return (!mag || mag->size == 0);
+	return mag->size == 0;
 }
 
 static unsigned long iova_magazine_pop(struct iova_magazine *mag,
-- 
2.35.3

[PATCH v2 2/2] iova: Remove magazine BUG_ON() checks

[John Garry]

Two of the magazine helpers have BUG_ON() checks, as follows:
- iova_magazine_pop() - here we ensure that the mag is not empty. However we
  already ensure that in the only caller, __iova_rcache_get().
- iova_magazine_push() - here we ensure that the mag is not full. However
  we already ensure that in the only caller, __iova_rcache_insert().

As described, the two bug checks are pointless so drop them.

Signed-off-by: John Garry <john.garry@huawei.com>
Acked-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iova.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 580fdf669922..8aece052ce72 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -694,8 +694,6 @@ static unsigned long iova_magazine_pop(struct iova_magazine *mag,
 	int i;
 	unsigned long pfn;
 
-	BUG_ON(iova_magazine_empty(mag));
-
 	/* Only fall back to the rbtree if we have no suitable pfns at all */
 	for (i = mag->size - 1; mag->pfns[i] > limit_pfn; i--)
 		if (i == 0)
@@ -710,8 +708,6 @@ static unsigned long iova_magazine_pop(struct iova_magazine *mag,
 
 static void iova_magazine_push(struct iova_magazine *mag, unsigned long pfn)
 {
-	BUG_ON(iova_magazine_full(mag));
-
 	mag->pfns[mag->size++] = pfn;
 }
 
-- 
2.35.3

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Jerry Snitselaar]

On Mon, Sep 05, 2022 at 05:11:22PM +0800, John Garry wrote:
> Since commit 32e92d9f6f87 ("iommu/iova: Separate out rcache init") it
> has not been possible to have NULL CPU rcache "loaded" or "prev" magazine
> pointers. As such, the checks in iova_magazine_full(),
> iova_magazine_empty(), and iova_magazine_free_pfns() may be dropped.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com

> ---
>  drivers/iommu/iova.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 47d1983dfa2a..580fdf669922 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
>  	unsigned long flags;
>  	int i;
>  
> -	if (!mag)
> -		return;
> -
>  	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>  
>  	for (i = 0 ; i < mag->size; ++i) {
> @@ -683,12 +680,12 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
>  
>  static bool iova_magazine_full(struct iova_magazine *mag)
>  {
> -	return (mag && mag->size == IOVA_MAG_SIZE);
> +	return mag->size == IOVA_MAG_SIZE;
>  }
>  
>  static bool iova_magazine_empty(struct iova_magazine *mag)
>  {
> -	return (!mag || mag->size == 0);
> +	return mag->size == 0;
>  }
>  
>  static unsigned long iova_magazine_pop(struct iova_magazine *mag,
> -- 
> 2.35.3
> 

Re: [PATCH v2 2/2] iova: Remove magazine BUG_ON() checks

[Jerry Snitselaar]

On Mon, Sep 05, 2022 at 05:11:23PM +0800, John Garry wrote:
> Two of the magazine helpers have BUG_ON() checks, as follows:
> - iova_magazine_pop() - here we ensure that the mag is not empty. However we
>   already ensure that in the only caller, __iova_rcache_get().
> - iova_magazine_push() - here we ensure that the mag is not full. However
>   we already ensure that in the only caller, __iova_rcache_insert().
> 
> As described, the two bug checks are pointless so drop them.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Acked-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com

> ---
>  drivers/iommu/iova.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 580fdf669922..8aece052ce72 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -694,8 +694,6 @@ static unsigned long iova_magazine_pop(struct iova_magazine *mag,
>  	int i;
>  	unsigned long pfn;
>  
> -	BUG_ON(iova_magazine_empty(mag));
> -
>  	/* Only fall back to the rbtree if we have no suitable pfns at all */
>  	for (i = mag->size - 1; mag->pfns[i] > limit_pfn; i--)
>  		if (i == 0)
> @@ -710,8 +708,6 @@ static unsigned long iova_magazine_pop(struct iova_magazine *mag,
>  
>  static void iova_magazine_push(struct iova_magazine *mag, unsigned long pfn)
>  {
> -	BUG_ON(iova_magazine_full(mag));
> -
>  	mag->pfns[mag->size++] = pfn;
>  }
>  
> -- 
> 2.35.3
> 

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Ethan Zhao]

John,

在 2022/9/5 17:11, John Garry 写道:
> Since commit 32e92d9f6f87 ("iommu/iova: Separate out rcache init") it
> has not been possible to have NULL CPU rcache "loaded" or "prev" magazine
> pointers. As such, the checks in iova_magazine_full(),
> iova_magazine_empty(), and iova_magazine_free_pfns() may be dropped.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/iova.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 47d1983dfa2a..580fdf669922 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
>   	unsigned long flags;
>   	int i;
>   
> -	if (!mag)
> -		return;
> -

iommu_probe_device
   ops->probe_finalize(dev);
     intel_iommu_probe_finalize
        iommu_setup_dma_ops
          iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
            iova_domain_init_rcaches
              {
              ...
              cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
              cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
   	     if (!cpu_rcache->loaded || !cpu_rcache->prev) {
			    ret = -ENOMEM;
   		            goto out_err;

Do you mean iova_magazine_alloc() is impossible to fail ?

Thanks,

Ethan

>   	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
>   
>   	for (i = 0 ; i < mag->size; ++i) {
> @@ -683,12 +680,12 @@ iova_magazine_free_pfns(struct iova_magazine *mag, struct iova_domain *iovad)
>   
>   static bool iova_magazine_full(struct iova_magazine *mag)
>   {
> -	return (mag && mag->size == IOVA_MAG_SIZE);
> +	return mag->size == IOVA_MAG_SIZE;
>   }
>   
>   static bool iova_magazine_empty(struct iova_magazine *mag)
>   {
> -	return (!mag || mag->size == 0);
> +	return mag->size == 0;
>   }
>   
>   static unsigned long iova_magazine_pop(struct iova_magazine *mag,

-- 
"firm, enduring, strong, and long-lived"

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

On 06/09/2022 10:28, Ethan Zhao wrote:

Hi Ethan,

>> Signed-off-by: John Garry <john.garry@huawei.com>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/iova.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 47d1983dfa2a..580fdf669922 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine *mag, 
>> struct iova_domain *iovad)
>>       unsigned long flags;
>>       int i;
>> -    if (!mag)
>> -        return;
>> -
> 
> iommu_probe_device
>    ops->probe_finalize(dev);
>      intel_iommu_probe_finalize
>         iommu_setup_dma_ops
>           iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
>             iova_domain_init_rcaches
>               {
>               ...
>               cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
>               cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
>             if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>                  ret = -ENOMEM;
>                        goto out_err;
> 
> Do you mean iova_magazine_alloc() is impossible to fail ?

No, iova_magazine_alloc() may fail and return NULL. But if it does then 
we set iovad rcache pointer = NULL in the error path and don't use the 
rcache.

However we have a !iovad->rcache check on the "fast" alloc but not 
"insert". I need to check why that is again.

Thanks,
john

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Robin Murphy]

On 2022-09-06 11:50, John Garry wrote:
> On 06/09/2022 10:28, Ethan Zhao wrote:
> 
> Hi Ethan,
> 
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   drivers/iommu/iova.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index 47d1983dfa2a..580fdf669922 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine 
>>> *mag, struct iova_domain *iovad)
>>>       unsigned long flags;
>>>       int i;
>>> -    if (!mag)
>>> -        return;
>>> -
>>
>> iommu_probe_device
>>    ops->probe_finalize(dev);
>>      intel_iommu_probe_finalize
>>         iommu_setup_dma_ops
>>           iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
>>             iova_domain_init_rcaches
>>               {
>>               ...
>>               cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
>>               cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
>>             if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>>                  ret = -ENOMEM;
>>                        goto out_err;
>>
>> Do you mean iova_magazine_alloc() is impossible to fail ?
> 
> No, iova_magazine_alloc() may fail and return NULL. But if it does then 
> we set iovad rcache pointer = NULL in the error path and don't use the 
> rcache.
> 
> However we have a !iovad->rcache check on the "fast" alloc but not 
> "insert". I need to check why that is again.

Right, if you find a good reason to respin the patch then perhaps also 
tweaking the commit message to clarify that it's impossible to have a 
NULL rcache *at any point where those checks are made* might avoid all 
possible doubt, however I'd hope that it's clear enough that the 
transient case while iova_domain_init_rcaches() is in the process of 
failing really doesn't need consideration in its own right.

I guess the check in iova_rcache_get() was maybe with the intent of 
allowing alloc_iova_fast() to seamlessly fall back to standard 
allocation, so an API user can treat iova_domain_init_rcaches() failure 
as non-fatal? That makes a fair amount of sense, but does mean that 
we're missing the equivalent in iova_rcache_insert() for it to actually 
work. Or we just remove it and tighten up the documentation to say 
that's not valid - I would like a way to make rcaches optional in 
iommu-dma for systems where they're a pointless waste of memory, but we 
can always revisit this when we get there.

Cheers,
Robin.

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

>>>
>>> iommu_probe_device
>>>    ops->probe_finalize(dev);
>>>      intel_iommu_probe_finalize
>>>         iommu_setup_dma_ops
>>>           iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
>>>             iova_domain_init_rcaches
>>>               {
>>>               ...
>>>               cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
>>>               cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
>>>             if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>>>                  ret = -ENOMEM;
>>>                        goto out_err;
>>>
>>> Do you mean iova_magazine_alloc() is impossible to fail ?
>>
>> No, iova_magazine_alloc() may fail and return NULL. But if it does 
>> then we set iovad rcache pointer = NULL in the error path and don't 
>> use the rcache.
>>
>> However we have a !iovad->rcache check on the "fast" alloc but not 
>> "insert". I need to check why that is again.
> 
> Right, if you find a good reason to respin the patch then perhaps also 
> tweaking the commit message to clarify that it's impossible to have a 
> NULL rcache *at any point where those checks are made* might avoid all 
> possible doubt, however I'd hope that it's clear enough that the 
> transient case while iova_domain_init_rcaches() is in the process of 
> failing really doesn't need consideration in its own right.

Yeah, I would think so. But I still don't mind tweaking it to be extra 
clear.

> 
> I guess the check in iova_rcache_get() was maybe with the intent of 
> allowing alloc_iova_fast() to seamlessly fall back to standard 
> allocation, so an API user can treat iova_domain_init_rcaches() failure 
> as non-fatal?

The 2x users treat iova_domain_init_rcaches() as fatal:
- dma-iommu falls back to platform ops in iommu_setup_dma_ops()

Caveat: on the chance that the IOVA domain init fails due to the rcache 
init failing, then, if there were another device in the group which 
probes later, its probe would be ok as the start_pfn is set. Not Good.

- vdpa just fails to create the domain in vduse_domain_create()

> That makes a fair amount of sense, but does mean that 
> we're missing the equivalent in iova_rcache_insert() for it to actually 
> work. Or we just remove it and tighten up the documentation to say 
> that's not valid 

I'd be more inclined to remove it. I would rather remove fathpath checks 
as much as possible and have robust error handling in the domain init.

Afterall I do have the "remove check" craze going.

> - I would like a way to make rcaches optional in 
> iommu-dma for systems where they're a pointless waste of memory, but we 
> can always revisit this when we get there.
> 

thanks,
John

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Robin Murphy]

On 2022-09-06 18:36, John Garry wrote:
>>>>
>>>> iommu_probe_device
>>>>    ops->probe_finalize(dev);
>>>>      intel_iommu_probe_finalize
>>>>         iommu_setup_dma_ops
>>>>           iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
>>>>             iova_domain_init_rcaches
>>>>               {
>>>>               ...
>>>>               cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
>>>>               cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
>>>>             if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>>>>                  ret = -ENOMEM;
>>>>                        goto out_err;
>>>>
>>>> Do you mean iova_magazine_alloc() is impossible to fail ?
>>>
>>> No, iova_magazine_alloc() may fail and return NULL. But if it does 
>>> then we set iovad rcache pointer = NULL in the error path and don't 
>>> use the rcache.
>>>
>>> However we have a !iovad->rcache check on the "fast" alloc but not 
>>> "insert". I need to check why that is again.
>>
>> Right, if you find a good reason to respin the patch then perhaps also 
>> tweaking the commit message to clarify that it's impossible to have a 
>> NULL rcache *at any point where those checks are made* might avoid all 
>> possible doubt, however I'd hope that it's clear enough that the 
>> transient case while iova_domain_init_rcaches() is in the process of 
>> failing really doesn't need consideration in its own right.
> 
> Yeah, I would think so. But I still don't mind tweaking it to be extra 
> clear.
> 
>>
>> I guess the check in iova_rcache_get() was maybe with the intent of 
>> allowing alloc_iova_fast() to seamlessly fall back to standard 
>> allocation, so an API user can treat iova_domain_init_rcaches() 
>> failure as non-fatal?
> 
> The 2x users treat iova_domain_init_rcaches() as fatal:
> - dma-iommu falls back to platform ops in iommu_setup_dma_ops()
> 
> Caveat: on the chance that the IOVA domain init fails due to the rcache 
> init failing, then, if there were another device in the group which 
> probes later, its probe would be ok as the start_pfn is set. Not Good.

Yeah, there's a lot not to like about iommu_dma_init_domain() - I've 
been banking on it all getting cleaned up when I get to refactoring that 
area of probing (remember the issue you reported years ago with PCI 
groups being built in the wrong order? All related...), but in fact 
since the cookie management got pulled into core code, we can probably 
tie the IOVA domain setup to that right now without much other 
involvement. That could be a cheap win, so I'll give it a go soon.

> - vdpa just fails to create the domain in vduse_domain_create()
> 
>> That makes a fair amount of sense, but does mean that we're missing 
>> the equivalent in iova_rcache_insert() for it to actually work. Or we 
>> just remove it and tighten up the documentation to say that's not valid 
> 
> I'd be more inclined to remove it. I would rather remove fathpath checks 
> as much as possible and have robust error handling in the domain init.
> 
> Afterall I do have the "remove check" craze going.

Sure, like I say I'm happy to be consistent either way. If I do end up 
reinstating such a check I think I'd prefer to have it explicit in 
{alloc,free}_iova_fast() anyway, rather than buried in internal 
implementation details.

Cheers,
Robin.

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

On 06/09/2022 19:25, Robin Murphy wrote:
>>
>> Caveat: on the chance that the IOVA domain init fails due to the 
>> rcache init failing, then, if there were another device in the group 
>> which probes later, its probe would be ok as the start_pfn is set. Not 
>> Good.
> 
> Yeah, there's a lot not to like about iommu_dma_init_domain() - I've 
> been banking on it all getting cleaned up when I get to refactoring that 
> area of probing (remember the issue you reported years ago with PCI 
> groups being built in the wrong order? All related...), but in fact 
> since the cookie management got pulled into core code, we can probably 
> tie the IOVA domain setup to that right now without much other 
> involvement. That could be a cheap win, so I'll give it a go soon.

ok, great.

On a related topic, another thing to consider is that errors in IOVA 
domain init are not handled gracefully in terms of how we deal with the 
device probe and setting dma mapping ops, ref iommu_setup_dma_ops(). I 
assume you know all this.

> 
>> - vdpa just fails to create the domain in vduse_domain_create()
>>
>>> That makes a fair amount of sense, but does mean that we're missing 
>>> the equivalent in iova_rcache_insert() for it to actually work. Or we 
>>> just remove it and tighten up the documentation to say that's not valid 
>>
>> I'd be more inclined to remove it. I would rather remove fathpath 
>> checks as much as possible and have robust error handling in the 
>> domain init.
>>
>> Afterall I do have the "remove check" craze going.
> 
> Sure, like I say I'm happy to be consistent either way. If I do end up 
> reinstating such a check I think I'd prefer to have it explicit in 
> {alloc,free}_iova_fast() anyway, rather than buried in internal 
> implementation details.

I'm not sure what you would like to see now, if anything.

I could just remove the iovad->rcache check in iova_rcache_get().  It's 
pretty useless (on its own) since we don't have the same check on the 
"insert" path.

Or also add this:

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 0d6d8edf782d..e8f0b8f47f45 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -578,6 +578,12 @@ static int iommu_dma_init_domain(struct 
iommu_domain *domain, dma_addr_t base,
  			goto done_unlock;
  		}

+		if (!iovad->rcaches) {
+			pr_warn("IOVA domain rcache not properly initialised\n");
+			ret = -EFAULT;
+			goto done_unlock;
+		}
+
  		ret = 0;
  		goto done_unlock;


But I figure that you don't want more crud there now, considering the 
work you mention above.

Thanks,
John

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Robin Murphy]

On 2022-09-07 09:46, John Garry wrote:
> On 06/09/2022 19:25, Robin Murphy wrote:
>>>
>>> Caveat: on the chance that the IOVA domain init fails due to the 
>>> rcache init failing, then, if there were another device in the group 
>>> which probes later, its probe would be ok as the start_pfn is set. 
>>> Not Good.
>>
>> Yeah, there's a lot not to like about iommu_dma_init_domain() - I've 
>> been banking on it all getting cleaned up when I get to refactoring 
>> that area of probing (remember the issue you reported years ago with 
>> PCI groups being built in the wrong order? All related...), but in 
>> fact since the cookie management got pulled into core code, we can 
>> probably tie the IOVA domain setup to that right now without much 
>> other involvement. That could be a cheap win, so I'll give it a go soon.
> 
> ok, great.
> 
> On a related topic, another thing to consider is that errors in IOVA 
> domain init are not handled gracefully in terms of how we deal with the 
> device probe and setting dma mapping ops, ref iommu_setup_dma_ops(). I 
> assume you know all this.
> 
>>
>>> - vdpa just fails to create the domain in vduse_domain_create()
>>>
>>>> That makes a fair amount of sense, but does mean that we're missing 
>>>> the equivalent in iova_rcache_insert() for it to actually work. Or 
>>>> we just remove it and tighten up the documentation to say that's not 
>>>> valid 
>>>
>>> I'd be more inclined to remove it. I would rather remove fathpath 
>>> checks as much as possible and have robust error handling in the 
>>> domain init.
>>>
>>> Afterall I do have the "remove check" craze going.
>>
>> Sure, like I say I'm happy to be consistent either way. If I do end up 
>> reinstating such a check I think I'd prefer to have it explicit in 
>> {alloc,free}_iova_fast() anyway, rather than buried in internal 
>> implementation details.
> 
> I'm not sure what you would like to see now, if anything.
> 
> I could just remove the iovad->rcache check in iova_rcache_get().  It's 
> pretty useless (on its own) since we don't have the same check on the 
> "insert" path.

Yup, just remove it. Sorting iommu-dma is yet another issue, but let's 
skip straight to fixing that properly by allocating the IOVA domain 
up-front with the cookie (is this the last remnant of my 7-year-old 
misunderstanding of dma_32bit_pfn? Let's hope so...)

Thanks,
Robin.

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Ethan Zhao]

John,

在 2022/9/6 18:50, John Garry 写道:
> On 06/09/2022 10:28, Ethan Zhao wrote:
>
> Hi Ethan,
>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>   drivers/iommu/iova.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index 47d1983dfa2a..580fdf669922 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -661,9 +661,6 @@ iova_magazine_free_pfns(struct iova_magazine 
>>> *mag, struct iova_domain *iovad)
>>>       unsigned long flags;
>>>       int i;
>>> -    if (!mag)
>>> -        return;
>>> -
>>
>> iommu_probe_device
>>    ops->probe_finalize(dev);
>>      intel_iommu_probe_finalize
>>         iommu_setup_dma_ops
>>           iommu_dma_init_domain(domain, dma_base, dma_limit, dev)
>>             iova_domain_init_rcaches
>>               {
>>               ...
>>               cpu_rcache->loaded = iova_magazine_alloc(GFP_KERNEL);
>>               cpu_rcache->prev = iova_magazine_alloc(GFP_KERNEL);
>>             if (!cpu_rcache->loaded || !cpu_rcache->prev) {
>>                  ret = -ENOMEM;
>>                        goto out_err;
>>
>> Do you mean iova_magazine_alloc() is impossible to fail ?
>
> No, iova_magazine_alloc() may fail and return NULL. But if it does 
> then we set iovad rcache pointer = NULL in the error path and don't 
> use the rcache.

Yup,  if iova_magazine_alloc() failed ,

iovad->rcaches = NULL;

was set by free_iova_rcaches()

in error path of iova_domain_init_rcache().

and checked in

alloc_iova_fast()->iova_rcache_get().

More comment in code would wipe off my curiosity.


Thanks,

Ethan

>
> However we have a !iovad->rcache check on the "fast" alloc but not 
> "insert". I need to check why that is again.
>
> Thanks,
> john

-- 
"firm, enduring, strong, and long-lived"

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

On 07/09/2022 10:33, Ethan Zhao wrote:

Hi Ethan,

>>> Do you mean iova_magazine_alloc() is impossible to fail ?
>>
>> No, iova_magazine_alloc() may fail and return NULL. But if it does 
>> then we set iovad rcache pointer = NULL in the error path and don't 
>> use the rcache.
> 
> Yup,  if iova_magazine_alloc() failed ,
> 
> iovad->rcaches = NULL;
> 
> was set by free_iova_rcaches()
> 
> in error path of iova_domain_init_rcache().
> 
> and checked in
> 
> alloc_iova_fast()->iova_rcache_get().
> 
> More comment in code would wipe off my curiosity.

As discussed with Robin, we will actually remove that check in 
iova_rcache_get() for now and in future make the IOVA domain init more 
robust.

As for the "loaded" and "prev" NULL checks removal in this specific 
patch, I will add more words in the commit message to make it clearer 
that failure in init was the only way in which they NULL previously.

thanks,
John

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[Ethan Zhao]

John,

在 2022/9/7 16:46, John Garry 写道:
> On 06/09/2022 19:25, Robin Murphy wrote:
>>>
>>> Caveat: on the chance that the IOVA domain init fails due to the 
>>> rcache init failing, then, if there were another device in the group 
>>> which probes later, its probe would be ok as the start_pfn is set. 
>>> Not Good.
>>
>> Yeah, there's a lot not to like about iommu_dma_init_domain() - I've 
>> been banking on it all getting cleaned up when I get to refactoring 
>> that area of probing (remember the issue you reported years ago with 
>> PCI groups being built in the wrong order? All related...), but in 
>> fact since the cookie management got pulled into core code, we can 
>> probably tie the IOVA domain setup to that right now without much 
>> other involvement. That could be a cheap win, so I'll give it a go soon.
>
> ok, great.
>
> On a related topic, another thing to consider is that errors in IOVA 
> domain init are not handled gracefully in terms of how we deal with 
> the device probe and setting dma mapping ops, ref 
> iommu_setup_dma_ops(). I assume you know all this.
>
>>
>>> - vdpa just fails to create the domain in vduse_domain_create()
>>>
>>>> That makes a fair amount of sense, but does mean that we're missing 
>>>> the equivalent in iova_rcache_insert() for it to actually work. Or 
>>>> we just remove it and tighten up the documentation to say that's 
>>>> not valid 
>>>
>>> I'd be more inclined to remove it. I would rather remove fathpath 
>>> checks as much as possible and have robust error handling in the 
>>> domain init.
>>>
>>> Afterall I do have the "remove check" craze going.
>>
>> Sure, like I say I'm happy to be consistent either way. If I do end 
>> up reinstating such a check I think I'd prefer to have it explicit in 
>> {alloc,free}_iova_fast() anyway, rather than buried in internal 
>> implementation details.
>
> I'm not sure what you would like to see now, if anything.
>
> I could just remove the iovad->rcache check in iova_rcache_get().  
> It's pretty useless (on its own) since we don't have the same check on 
> the "insert" path.
>
> Or also add this:
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 0d6d8edf782d..e8f0b8f47f45 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -578,6 +578,12 @@ static int iommu_dma_init_domain(struct 
> iommu_domain *domain, dma_addr_t base,
>              goto done_unlock;
>          }
>
> +        if (!iovad->rcaches) {
> +            pr_warn("IOVA domain rcache not properly initialised\n");
> +            ret = -EFAULT;
> +            goto done_unlock;
> +        }
> +
>          ret = 0;
>          goto done_unlock;
>
If the iovad->rcaches allocation failed, will skip iommu domain dma ops, 
so no need *any* iovad,->rcaches check, right ?

and there is already warning about the fallback.

Thanks,

Ethan

>
> But I figure that you don't want more crud there now, considering the 
> work you mention above.
>
> Thanks,
> John
>
>
>
-- 
"firm, enduring, strong, and long-lived"

Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

[John Garry]

On 07/09/2022 10:58, Ethan Zhao wrote:

Hi Ethan,

>> Or also add this:
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 0d6d8edf782d..e8f0b8f47f45 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -578,6 +578,12 @@ static int iommu_dma_init_domain(struct 
>> iommu_domain *domain, dma_addr_t base,
>>              goto done_unlock;
>>          }
>>
>> +        if (!iovad->rcaches) {
>> +            pr_warn("IOVA domain rcache not properly initialised\n");
>> +            ret = -EFAULT;
>> +            goto done_unlock;
>> +        }
>> +
>>          ret = 0;
>>          goto done_unlock;
>>
> If the iovad->rcaches allocation failed, will skip iommu domain dma ops, 
> so no need *any* iovad,->rcaches check, right ?
> 
> and there is already warning about the fallback.

It's not as simple as that. We use the iovad->start_pfn member as a flag 
for the IOVA domain being initialized. However that does not mean always 
properly initialized. As above, the rcache init may fail, but in this 
case we still set start_pfn.

This comes into play when we have multiple devices in the same IOMMU 
group, for example, as I mentioned yesterday.

Thanks,
John