| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7a1d4010-5804-c806-31cb-a6bac50f6e1e@redhat.com>
Date: Mon, 6 Mar 2023 14:39:32 +0100
From: Danilo Krummrich <dakr@...hat.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, airlied@...il.com,
daniel@...ll.ch, tzimmermann@...e.de, mripard@...nel.org,
corbet@....net, christian.koenig@....com, bskeggs@...hat.com,
matthew.brost@...el.com, boris.brezillon@...labora.com,
alexdeucher@...il.com, ogabbay@...nel.org, bagasdotme@...il.com,
willy@...radead.org, jason@...kstrand.net,
dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>
Subject: Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA
mappings
On 2/28/23 17:24, Liam R. Howlett wrote:
> * Danilo Krummrich <dakr@...hat.com> [230227 21:17]:
>> On Tue, Feb 21, 2023 at 01:20:50PM -0500, Liam R. Howlett wrote:
>>> * Danilo Krummrich <dakr@...hat.com> [230217 08:45]:
>>>> Add infrastructure to keep track of GPU virtual address (VA) mappings
>>>> with a decicated VA space manager implementation.
>>>>
>>>> New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers
>>>> start implementing, allow userspace applications to request multiple and
>>>> arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is
>>>> intended to serve the following purposes in this context.
>>>>
>>>> 1) Provide infrastructure to track GPU VA allocations and mappings,
>>>> making use of the maple_tree.
>>>>
>>>> 2) Generically connect GPU VA mappings to their backing buffers, in
>>>> particular DRM GEM objects.
>>>>
>>>> 3) Provide a common implementation to perform more complex mapping
>>>> operations on the GPU VA space. In particular splitting and merging
>>>> of GPU VA mappings, e.g. for intersecting mapping requests or partial
>>>> unmap requests.
>>>>
>>>> Suggested-by: Dave Airlie <airlied@...hat.com>
>>>> Signed-off-by: Danilo Krummrich <dakr@...hat.com>
>>>> ---
>>>> Documentation/gpu/drm-mm.rst | 31 +
>>>> drivers/gpu/drm/Makefile | 1 +
>>>> drivers/gpu/drm/drm_gem.c | 3 +
>>>> drivers/gpu/drm/drm_gpuva_mgr.c | 1704 +++++++++++++++++++++++++++++++
>>>> include/drm/drm_drv.h | 6 +
>>>> include/drm/drm_gem.h | 75 ++
>>>> include/drm/drm_gpuva_mgr.h | 714 +++++++++++++
>>>> 7 files changed, 2534 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c
>>>> create mode 100644 include/drm/drm_gpuva_mgr.h
>>>>
>>>> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
>>>> index a52e6f4117d6..c9f120cfe730 100644
>>>> --- a/Documentation/gpu/drm-mm.rst
>>>> +++ b/Documentation/gpu/drm-mm.rst
>>>> @@ -466,6 +466,37 @@ DRM MM Range Allocator Function References
>>>> .. kernel-doc:: drivers/gpu/drm/drm_mm.c
>>>> :export:
>>>>
>>> ...
>>>
>>>> +
>>>> +/**
>>>> + * drm_gpuva_remove_iter - removes the iterators current element
>>>> + * @it: the &drm_gpuva_iterator
>>>> + *
>>>> + * This removes the element the iterator currently points to.
>>>> + */
>>>> +void
>>>> +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
>>>> +{
>>>> + mas_erase(&it->mas);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_gpuva_iter_remove);
>>>> +
>>>> +/**
>>>> + * drm_gpuva_insert - insert a &drm_gpuva
>>>> + * @mgr: the &drm_gpuva_manager to insert the &drm_gpuva in
>>>> + * @va: the &drm_gpuva to insert
>>>> + * @addr: the start address of the GPU VA
>>>> + * @range: the range of the GPU VA
>>>> + *
>>>> + * Insert a &drm_gpuva with a given address and range into a
>>>> + * &drm_gpuva_manager.
>>>> + *
>>>> + * Returns: 0 on success, negative error code on failure.
>>>> + */
>>>> +int
>>>> +drm_gpuva_insert(struct drm_gpuva_manager *mgr,
>>>> + struct drm_gpuva *va)
>>>> +{
>>>> + u64 addr = va->va.addr;
>>>> + u64 range = va->va.range;
>>>> + MA_STATE(mas, &mgr->va_mt, addr, addr + range - 1);
>>>> + struct drm_gpuva_region *reg = NULL;
>>>> + int ret;
>>>> +
>>>> + if (unlikely(!drm_gpuva_in_mm_range(mgr, addr, range)))
>>>> + return -EINVAL;
>>>> +
>>>> + if (unlikely(drm_gpuva_in_kernel_region(mgr, addr, range)))
>>>> + return -EINVAL;
>>>> +
>>>> + if (mgr->flags & DRM_GPUVA_MANAGER_REGIONS) {
>>>> + reg = drm_gpuva_in_region(mgr, addr, range);
>>>> + if (unlikely(!reg))
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>
>>> -----
>>>
>>>> + if (unlikely(drm_gpuva_find_first(mgr, addr, range)))
>>>> + return -EEXIST;
>>>> +
>>>> + ret = mas_store_gfp(&mas, va, GFP_KERNEL);
>>>
>>> mas_walk() will set the internal maple state to the limits to what it
>>> finds. So, instead of an iterator, you can use the walk function and
>>> ensure there is a large enough area in the existing NULL:
>>>
>>> /*
>>> * Nothing at addr, mas now points to the location where the store would
>>> * happen
>>> */
>>> if (mas_walk(&mas))
>>> return -EEXIST;
>>>
>>
>> For some reason mas_walk() finds an entry and hence this function returns
>> -EEXIST for the following sequence of insertions.
>>
>> A = [0xc0000 - 0xfffff]
>> B = [0x0 - 0xbffff]
>>
>> Interestingly, inserting B before A works fine.
>>
>> I attached a test module that reproduces the issue. I hope its just a stupid
>> mistake I just can't spot though.
>
> This is probably my fault in how I explained things, I seem to have had
> a bug in my code.
>
> Let me try again.
>
> mas_walk(&mas) will go to the range of mas.index
> It will set mas.index = range_start
> It will set mas.last = range_end
> It will return entry in that range.
>
> Your code is walking to addr (0xc0000, say)
> You get NULL
> and the range is now: mas.index = 0, mas.last = ULONG_MAX
>
> You set mas.last = 0xc0000 + 0x40000 -1
> You store your va in the range of 0 - 0xfffff - This isn't what you want
> to do and this is why you are seeing it exists when done in this order.
>
> In the reverse order, your lower limit is fine so it works out.
>
> Try adding a check to ensure the lower range is still accurate as well:
> if (mas.index < addr)
> mas.index = addr;
>
Thanks for the clarification, this indeed fixes the issue.
> If you compile with CONFIG_DEBUG_MAPLE_TREE, you can use mt_dump() to
> dump the tree for debugging.
That's pretty handy.
>
> I also have some quality of life patches I'm developing to configure the
> format of the dump (hex/dec) and a mas_dump() for more information as
> well.
Great, I tried it out and really missed a hex option. Are they in any
git repository already, such that I could fetch them?
>
>>
>>> /* The NULL entry ends at mas.last, make sure there is room */
>>> if (mas.last < (addr + range - 1))
>>> return -EEXIST;
>>>
>>> /* Limit the store size to the correct end address, and store */
>>> mas.last = addr + range - 1;
>>> ret = mas_store_gfp(&mas, va, GFP_KERNEL);
>>>
>
>
Powered by blists - more mailing lists