Date:   Tue, 6 Sep 2022 19:25:51 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     John Garry <john.garry@...wei.com>,
        Ethan Zhao <haifeng.zhao@...ux.intel.com>, joro@...tes.org,
        will@...nel.org
Cc:     iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
        linuxarm@...wei.com
Subject: Re: [PATCH v2 1/2] iova: Remove some magazine pointer NULL checks

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.

Powered by blists - more mailing lists