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 <[email protected]>
Reviewed-by: Robin Murphy <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: Robin Murphy <[email protected]>
Reviewed-by: Jerry Snitselaar <[email protected]
> ---
> 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
>
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 <[email protected]>
> Reviewed-by: Robin Murphy <[email protected]>
> ---
> 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"
On 06/09/2022 10:28, Ethan Zhao wrote:
Hi Ethan,
>> Signed-off-by: John Garry <[email protected]>
>> Reviewed-by: Robin Murphy <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> Reviewed-by: Robin Murphy <[email protected]>
>>> ---
>>> 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.
>>>
>>> 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
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.
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.
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
John,
在 2022/9/6 18:50, John Garry 写道:
> On 06/09/2022 10:28, Ethan Zhao wrote:
>
> Hi Ethan,
>
>>> Signed-off-by: John Garry <[email protected]>
>>> Reviewed-by: Robin Murphy <[email protected]>
>>> ---
>>> 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"
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"
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
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