Re: [PATCH] drm/amd/display: fix compilation error
On Wed, Jun 12, 2019 at 10:35:26PM -0400, Alex Deucher wrote: > On Wed, Jun 12, 2019 at 10:34 PM Hariprasad Kelam > wrote: > > > > this patch fixes below compilation error > > > > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c: In > > function ‘dcn10_apply_ctx_for_surface’: > > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c:2378:3: > > error: implicit declaration of function ‘udelay’ > > [-Werror=implicit-function-declaration] > >udelay(underflow_check_delay_us); > > > > Signed-off-by: Hariprasad Kelam > > What branch is this patch based on? > > Alex Hi Alex, It is on linux-next Thanks, Hariprasad k > > --- > > drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > > b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > > index d2352949..1ac9a4f 100644 > > --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > > +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > > @@ -23,6 +23,7 @@ > > * > > */ > > > > +#include > > #include "dm_services.h" > > #include "core_types.h" > > #include "resource.h" > > -- > > 2.7.4 > > > > ___ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: fix compilation error
On Wed, Jun 12, 2019 at 10:34 PM Hariprasad Kelam wrote: > > this patch fixes below compilation error > > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c: In > function ‘dcn10_apply_ctx_for_surface’: > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c:2378:3: > error: implicit declaration of function ‘udelay’ > [-Werror=implicit-function-declaration] >udelay(underflow_check_delay_us); > > Signed-off-by: Hariprasad Kelam What branch is this patch based on? Alex > --- > drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > index d2352949..1ac9a4f 100644 > --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c > @@ -23,6 +23,7 @@ > * > */ > > +#include > #include "dm_services.h" > #include "core_types.h" > #include "resource.h" > -- > 2.7.4 > > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: fix compilation error
this patch fixes below compilation error drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c: In function ‘dcn10_apply_ctx_for_surface’: drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c:2378:3: error: implicit declaration of function ‘udelay’ [-Werror=implicit-function-declaration] udelay(underflow_check_delay_us); Signed-off-by: Hariprasad Kelam --- drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c index d2352949..1ac9a4f 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c @@ -23,6 +23,7 @@ * */ +#include #include "dm_services.h" #include "core_types.h" #include "resource.h" -- 2.7.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[pull] amdgpu drm-fixes-5.2
Hi Dave, Daniel,
Fixes for 5.2:
- Extend previous vce fix for resume to uvd and vcn
- Fix bounds checking in ras debugfs interface
- Fix a regression on SI using amdgpu
The following changes since commit 671e2ee5ee2127179ca884b439ab6001a623edd6:
Merge branch 'linux-5.2' of git://github.com/skeggsb/linux into drm-fixes
(2019-06-07 17:16:00 +1000)
are available in the Git repository at:
git://people.freedesktop.org/~agd5f/linux drm-fixes-5.2
for you to fetch changes up to f3a5231c8f14acd42845e9e60f506b4e948f0e68:
drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware (2019-06-12
20:39:49 -0500)
Alex Deucher (1):
drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware
Dan Carpenter (1):
drm/amdgpu: Fix bounds checking in amdgpu_ras_is_supported()
Shirish S (1):
drm/amdgpu/{uvd,vcn}: fetch ring's read_ptr after alloc
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 +++-
drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 -
drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c | 5 -
5 files changed, 15 insertions(+), 5 deletions(-)
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/powerplay/smu7_hwmgr: replace blocking delay with non-blocking
CC trimmed Hello Alex and Harry, any comments on this? I'm running my Xeon X3470 (4c/8t) with this under amd-staging-drm-next (as always ;-) ) and see no issues so far. Thanks! Dieter Am 04.06.2019 22:21, schrieb Yrjan Skrimstad: On Thu, May 30, 2019 at 02:08:21AM +0200, Yrjan Skrimstad wrote: This driver currently contains a repeated 500ms blocking delay call which causes frequent major buffer underruns in PulseAudio. This patch fixes this issue by replacing the blocking delay with a non-blocking sleep call. I see that I have not explained this bug well enough, and I hope that is the reason for the lack of replies on this patch. I will here attempt to explain the situation better. To start with some hardware description I am here using an AMD R9 380 GPU, an AMD Ryzen 7 1700 Eight-Core Processor and an AMD X370 chipset. If any more hardware or software specifications are necessary, please ask. The bug is as follows: When playing audio I will regularly have major audio issues, similar to that of a skipping CD. This is reported by PulseAudio as scheduling delays and buffer underruns when running PulseAudio verbosely and these scheduling delays are always just under 500ms, typically around 490ms. This makes listening to any music quite the horrible experience as PulseAudio constantly will attempt to rewind and catch up. It is not a great situation, and seems to me to quite clearly be a case where regular user space behaviour has been broken. I want to note that this audio problem was not something I experienced until recently, it is therefore a new bug. I have bisected the kernel to find out where the problem originated and found the following commit: # first bad commit: [f5742ec36422a39b57f0256e4847f61b3c432f8c] drm/amd/powerplay: correct power reading on fiji This commit introduces a blocking delay (mdelay) of 500ms, whereas the old behaviour was a smaller blocking delay of only 1ms. This seems to me to be very curious as the scheduling delays of PulseAudio are always almost 500ms. I have therefore with the previous patch replaced the scheduling delay with a non-blocking sleep (msleep). The results of the patch seems promising as I have yet to encounter any of the old <500ms scheduling delays when using it and I have also not encountered any kernel log messages regarding sleeping in an atomic context. ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 00/11] Various revisions from a locking/code review
Rebase to https://github.com/jgunthorpe/linux.git hmm branch, need some
changes because of interface hmm_range_register change. Then run a quick
amdgpu_test. Test is finished, result is ok. But there is below kernel
BUG message, seems hmm_free_rcu calls down_write.
[ 1171.919921] BUG: sleeping function called from invalid context at
/home/yangp/git/compute_staging/kernel/kernel/locking/rwsem.c:65
[ 1171.919933] in_atomic(): 1, irqs_disabled(): 0, pid: 53, name:
kworker/1:1
[ 1171.919938] 2 locks held by kworker/1:1/53:
[ 1171.919940] #0: 1c7c19d4 ((wq_completion)rcu_gp){+.+.}, at:
process_one_work+0x20e/0x630
[ 1171.919951] #1: 923f2cfa
((work_completion)(>work)){+.+.}, at: process_one_work+0x20e/0x630
[ 1171.919959] CPU: 1 PID: 53 Comm: kworker/1:1 Tainted: GW
5.2.0-rc1-kfd-yangp #196
[ 1171.919961] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac)/USB 3.1,
BIOS 9001 03/07/2016
[ 1171.919965] Workqueue: rcu_gp srcu_invoke_callbacks
[ 1171.919968] Call Trace:
[ 1171.919974] dump_stack+0x67/0x9b
[ 1171.919980] ___might_sleep+0x149/0x230
[ 1171.919985] down_write+0x1c/0x70
[ 1171.919989] hmm_free_rcu+0x24/0x80
[ 1171.919993] srcu_invoke_callbacks+0xc9/0x150
[ 1171.92] process_one_work+0x28e/0x630
[ 1171.920008] worker_thread+0x39/0x3f0
[ 1171.920014] ? process_one_work+0x630/0x630
[ 1171.920017] kthread+0x11c/0x140
[ 1171.920021] ? kthread_park+0x90/0x90
[ 1171.920026] ret_from_fork+0x24/0x30
Philip
On 2019-06-12 1:54 p.m., Kuehling, Felix wrote:
> [+Philip]
>
> Hi Jason,
>
> I'm out of the office this week.
>
> Hi Philip, can you give this a go? Not sure how much you've been
> following this patch series review. Message or call me on Skype to
> discuss any questions.
>
> Thanks,
> Felix
>
> On 2019-06-11 12:48, Jason Gunthorpe wrote:
>> On Thu, Jun 06, 2019 at 03:44:27PM -0300, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe
>>>
>>> For hmm.git:
>>>
>>> This patch series arised out of discussions with Jerome when looking at the
>>> ODP changes, particularly informed by use after free races we have already
>>> found and fixed in the ODP code (thanks to syzkaller) working with mmu
>>> notifiers, and the discussion with Ralph on how to resolve the lifetime
>>> model.
>>>
>>> Overall this brings in a simplified locking scheme and easy to explain
>>> lifetime model:
>>>
>>>If a hmm_range is valid, then the hmm is valid, if a hmm is valid then
>>> the mm
>>>is allocated memory.
>>>
>>>If the mm needs to still be alive (ie to lock the mmap_sem, find a vma,
>>> etc)
>>>then the mmget must be obtained via mmget_not_zero().
>>>
>>> Locking of mm->hmm is shifted to use the mmap_sem consistently for all
>>> read/write and unlocked accesses are removed.
>>>
>>> The use unlocked reads on 'hmm->dead' are also eliminated in favour of using
>>> standard mmget() locking to prevent the mm from being released. Many of the
>>> debugging checks of !range->hmm and !hmm->mm are dropped in favour of
>>> poison -
>>> which is much clearer as to the lifetime intent.
>>>
>>> The trailing patches are just some random cleanups I noticed when reviewing
>>> this code.
>>>
>>> This v2 incorporates alot of the good off list changes & feedback Jerome
>>> had,
>>> and all the on-list comments too. However, now that we have the shared git I
>>> have kept the one line change to nouveau_svm.c rather than the compat
>>> funtions.
>>>
>>> I believe we can resolve this merge in the DRM tree now and keep the core
>>> mm/hmm.c clean. DRM maintainers, please correct me if I'm wrong.
>>>
>>> It is on top of hmm.git, and I have a git tree of this series to ease
>>> testing
>>> here:
>>>
>>> https://github.com/jgunthorpe/linux/tree/hmm
>>>
>>> There are still some open locking issues, as I think this remains
>>> unaddressed:
>>>
>>> https://lore.kernel.org/linux-mm/20190527195829.gb18...@mellanox.com/
>>>
>>> I'm looking for some more acks, reviews and tests so this can move ahead to
>>> hmm.git.
>> AMD Folks, this is looking pretty good now, can you please give at
>> least a Tested-by for the new driver code using this that I see in
>> linux-next?
>>
>> Thanks,
>> Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
On 2019-06-12 3:28 p.m., Christian König wrote: > Am 12.06.19 um 17:13 schrieb Yang, Philip: >> TTM create two zones, kernel zone and dma32 zone for system memory. If >> system memory address allocated is below 4GB, this account to dma32 zone >> and will exhaust dma32 zone and trigger unnesssary TTM eviction. >> >> Patch "drm/ttm: Account for kernel allocations in kernel zone only" only >> handle the allocation for acc_size, the system memory page allocation is >> through ttm_mem_global_alloc_page which still account to dma32 zone if >> page is below 4GB. > > NAK, as the name says the mem_glob is global for all devices in the system. > > So this will break if you mix DMA32 and non DMA32 in the same system > which is exactly the configuration my laptop here has :( > I didn't find path use dma32 zone, but I may missed something. There is an issue found by KFDTest.BigBufStressTest, it allocates buffers up to 3/8 of total 256GB system memory, each buffer size is 128MB, then use queue to write to the buffers. If ttm_mem_global_alloc_page get page pfn is below 4GB, it account to dma32 zone and will exhaust 2GB limit, then ttm_check_swapping will schedule ttm_shrink_work to start eviction. It takes minutes to finish restore (retry many times if busy), the test failed because queue timeout. This eviction is unnecessary because we still have enough free system memory. It's random case, happens about 1/5. I can change test to increase the timeout value to workaround this, but this seems TTM bug. This will slow application performance a lot if this random issue happens. Thanks, Philip > Christian. > >> >> Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 >> Signed-off-by: Philip Yang >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 2778ff63d97d..79bb9dfe617b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) >> } >> adev->mman.initialized = true; >> + /* Only kernel zone (no dma32 zone) if device does not require >> dma32 */ >> + if (!adev->need_dma32) >> + adev->mman.bdev.glob->mem_glob->num_zones = 1; >> + >> /* We opt to avoid OOM on system pages allocations */ >> adev->mman.bdev.no_retry = true; > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
Well that we evict here is perfectly intentional. The laptop I'm typing on actually don't work without this, so please don't touch any of that. Christian. Am 12.06.19 um 20:06 schrieb Yang, Philip: That's kind of hack because dma32 zone is not needed, it has bad effect to trigger unnecessary eviction for KFDTest.BigBufStressTest. But ttm_bo_global_init->ttm_mem_global_init always create dma32 zone without accepting any parameter. To avoid ttm_mem_global_alloc_page account to dma32 zone, another option is to add a new flag to ttm_operation_ctx->flags, that looks not good either. Thanks, Philip On 2019-06-12 1:23 p.m., Kuehling, Felix wrote: TTM itself has some logic for need_dma32 and TTM_PAGE_FLAG_DMA32. I believe that should already handle this. need_dma32 is passed from amdgpu to ttm_bo_device_init to bdev->need_dma32. ttm_tt_create translates that to page_flags |= TTM_PAGE_FLAG_DMA32 and passes that to bdev->driver->ttm_tt_create. The two page allocators in ttm_page_alloc.c and ttm_page_alloc_dma.c check ttm->page_flags. Is that chain broken somewhere? Overriding glob->mem_glob->num_zones from amdgpu seems to be a bit of a hack. Regards, Felix On 2019-06-12 8:13, Yang, Philip wrote: TTM create two zones, kernel zone and dma32 zone for system memory. If system memory address allocated is below 4GB, this account to dma32 zone and will exhaust dma32 zone and trigger unnesssary TTM eviction. Patch "drm/ttm: Account for kernel allocations in kernel zone only" only handle the allocation for acc_size, the system memory page allocation is through ttm_mem_global_alloc_page which still account to dma32 zone if page is below 4GB. Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 Signed-off-by: Philip Yang --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 2778ff63d97d..79bb9dfe617b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) } adev->mman.initialized = true; + /* Only kernel zone (no dma32 zone) if device does not require dma32 */ + if (!adev->need_dma32) + adev->mman.bdev.glob->mem_glob->num_zones = 1; + /* We opt to avoid OOM on system pages allocations */ adev->mman.bdev.no_retry = true; ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
Am 12.06.19 um 17:13 schrieb Yang, Philip: TTM create two zones, kernel zone and dma32 zone for system memory. If system memory address allocated is below 4GB, this account to dma32 zone and will exhaust dma32 zone and trigger unnesssary TTM eviction. Patch "drm/ttm: Account for kernel allocations in kernel zone only" only handle the allocation for acc_size, the system memory page allocation is through ttm_mem_global_alloc_page which still account to dma32 zone if page is below 4GB. NAK, as the name says the mem_glob is global for all devices in the system. So this will break if you mix DMA32 and non DMA32 in the same system which is exactly the configuration my laptop here has :( Christian. Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 Signed-off-by: Philip Yang --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 2778ff63d97d..79bb9dfe617b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) } adev->mman.initialized = true; + /* Only kernel zone (no dma32 zone) if device does not require dma32 */ + if (!adev->need_dma32) + adev->mman.bdev.glob->mem_glob->num_zones = 1; + /* We opt to avoid OOM on system pages allocations */ adev->mman.bdev.no_retry = true; ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Reserve space for shared fence
On 2019-06-11 9:20, Zeng, Oak wrote: > Call reservation_object_reserve_shared to reserve > space for shared fence. Otherwise it will trigger > BUG_ON condition in reservation_object_add_shared_fence. > > Change-Id: Ib0fae16247e33ee68f95bffa723451c0cd619344 > Signed-off-by: Oak Zeng Reviewed-by: Felix Kuehling > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > index 81e0e75..74e8695 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > @@ -2152,12 +2152,16 @@ int amdgpu_amdkfd_add_gws_to_process(void *info, void > *gws, struct kgd_mem **mem >* Add process eviction fence to bo so they can >* evict each other. >*/ > + ret = reservation_object_reserve_shared(gws_bo->tbo.resv, 1); > + if (ret) > + goto reserve_shared_fail; > amdgpu_bo_fence(gws_bo, _info->eviction_fence->base, true); > amdgpu_bo_unreserve(gws_bo); > mutex_unlock(&(*mem)->process_info->lock); > > return ret; > > +reserve_shared_fail: > bo_validation_failure: > amdgpu_bo_unreserve(gws_bo); > bo_reservation_failure: ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
That's kind of hack because dma32 zone is not needed, it has bad effect to trigger unnecessary eviction for KFDTest.BigBufStressTest. But ttm_bo_global_init->ttm_mem_global_init always create dma32 zone without accepting any parameter. To avoid ttm_mem_global_alloc_page account to dma32 zone, another option is to add a new flag to ttm_operation_ctx->flags, that looks not good either. Thanks, Philip On 2019-06-12 1:23 p.m., Kuehling, Felix wrote: > TTM itself has some logic for need_dma32 and TTM_PAGE_FLAG_DMA32. I > believe that should already handle this. need_dma32 is passed from > amdgpu to ttm_bo_device_init to bdev->need_dma32. ttm_tt_create > translates that to page_flags |= TTM_PAGE_FLAG_DMA32 and passes that to > bdev->driver->ttm_tt_create. The two page allocators in ttm_page_alloc.c > and ttm_page_alloc_dma.c check ttm->page_flags. Is that chain broken > somewhere? Overriding glob->mem_glob->num_zones from amdgpu seems to be > a bit of a hack. > > Regards, > Felix > > On 2019-06-12 8:13, Yang, Philip wrote: >> TTM create two zones, kernel zone and dma32 zone for system memory. If >> system memory address allocated is below 4GB, this account to dma32 zone >> and will exhaust dma32 zone and trigger unnesssary TTM eviction. >> >> Patch "drm/ttm: Account for kernel allocations in kernel zone only" only >> handle the allocation for acc_size, the system memory page allocation is >> through ttm_mem_global_alloc_page which still account to dma32 zone if >> page is below 4GB. >> >> Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 >> Signed-off-by: Philip Yang >> --- >>drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 >>1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> index 2778ff63d97d..79bb9dfe617b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c >> @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) >> } >> adev->mman.initialized = true; >> >> +/* Only kernel zone (no dma32 zone) if device does not require dma32 */ >> +if (!adev->need_dma32) >> +adev->mman.bdev.glob->mem_glob->num_zones = 1; >> + >> /* We opt to avoid OOM on system pages allocations */ >> adev->mman.bdev.no_retry = true; >> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v3)
On 2019-06-12 4:08, StDenis, Tom wrote:
> (v2): Return 0 and set mem->mm_node to NULL.
> (v3): Use atomic64_add_return instead.
>
> Signed-off-by: Tom St Denis
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> index 8aea2f21b202..c963ad86072e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> @@ -276,7 +276,7 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> struct drm_mm_node *nodes;
> enum drm_mm_insert_mode mode;
> unsigned long lpfn, num_nodes, pages_per_node, pages_left;
> - uint64_t usage = 0, vis_usage = 0;
> + uint64_t vis_usage = 0;
> unsigned i;
> int r;
>
> @@ -284,6 +284,13 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> if (!lpfn)
> lpfn = man->size;
>
> + /* bail out quickly if there's likely not enough VRAM for this BO */
> + if (atomic64_add_return(mem->num_pages << PAGE_SHIFT, >usage) >
> adev->gmc.mc_vram_size) {
> + atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
On 32-bit systems mem->num_pages is only 32-bit (unsigned long) and the
<< PAGE_SHIFT may overflow. You probably want to cast this to u64
explicitly before the shift.
Regards,
Felix
> + mem->mm_node = NULL;
> + return 0;
> + }
> +
> if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
> pages_per_node = ~0ul;
> num_nodes = 1;
> @@ -300,8 +307,10 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
>
> nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
> GFP_KERNEL | __GFP_ZERO);
> - if (!nodes)
> + if (!nodes) {
> + atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
> return -ENOMEM;
> + }
>
> mode = DRM_MM_INSERT_BEST;
> if (place->flags & TTM_PL_FLAG_TOPDOWN)
> @@ -321,7 +330,6 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> if (unlikely(r))
> break;
>
> - usage += nodes[i].size << PAGE_SHIFT;
> vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
> amdgpu_vram_mgr_virt_start(mem, [i]);
> pages_left -= pages;
> @@ -341,14 +349,12 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> if (unlikely(r))
> goto error;
>
> - usage += nodes[i].size << PAGE_SHIFT;
> vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
> amdgpu_vram_mgr_virt_start(mem, [i]);
> pages_left -= pages;
> }
> spin_unlock(>lock);
>
> - atomic64_add(usage, >usage);
> atomic64_add(vis_usage, >vis_usage);
>
> mem->mm_node = nodes;
> @@ -359,6 +365,7 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> while (i--)
> drm_mm_remove_node([i]);
> spin_unlock(>lock);
> + atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
>
> kvfree(nodes);
> return r == -ENOSPC ? 0 : r;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 00/11] Various revisions from a locking/code review
[+Philip] Hi Jason, I'm out of the office this week. Hi Philip, can you give this a go? Not sure how much you've been following this patch series review. Message or call me on Skype to discuss any questions. Thanks, Felix On 2019-06-11 12:48, Jason Gunthorpe wrote: > On Thu, Jun 06, 2019 at 03:44:27PM -0300, Jason Gunthorpe wrote: >> From: Jason Gunthorpe >> >> For hmm.git: >> >> This patch series arised out of discussions with Jerome when looking at the >> ODP changes, particularly informed by use after free races we have already >> found and fixed in the ODP code (thanks to syzkaller) working with mmu >> notifiers, and the discussion with Ralph on how to resolve the lifetime >> model. >> >> Overall this brings in a simplified locking scheme and easy to explain >> lifetime model: >> >> If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the >> mm >> is allocated memory. >> >> If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, >> etc) >> then the mmget must be obtained via mmget_not_zero(). >> >> Locking of mm->hmm is shifted to use the mmap_sem consistently for all >> read/write and unlocked accesses are removed. >> >> The use unlocked reads on 'hmm->dead' are also eliminated in favour of using >> standard mmget() locking to prevent the mm from being released. Many of the >> debugging checks of !range->hmm and !hmm->mm are dropped in favour of poison >> - >> which is much clearer as to the lifetime intent. >> >> The trailing patches are just some random cleanups I noticed when reviewing >> this code. >> >> This v2 incorporates alot of the good off list changes & feedback Jerome had, >> and all the on-list comments too. However, now that we have the shared git I >> have kept the one line change to nouveau_svm.c rather than the compat >> funtions. >> >> I believe we can resolve this merge in the DRM tree now and keep the core >> mm/hmm.c clean. DRM maintainers, please correct me if I'm wrong. >> >> It is on top of hmm.git, and I have a git tree of this series to ease testing >> here: >> >> https://github.com/jgunthorpe/linux/tree/hmm >> >> There are still some open locking issues, as I think this remains >> unaddressed: >> >> https://lore.kernel.org/linux-mm/20190527195829.gb18...@mellanox.com/ >> >> I'm looking for some more acks, reviews and tests so this can move ahead to >> hmm.git. > AMD Folks, this is looking pretty good now, can you please give at > least a Tested-by for the new driver code using this that I see in > linux-next? > > Thanks, > Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
TTM itself has some logic for need_dma32 and TTM_PAGE_FLAG_DMA32. I believe that should already handle this. need_dma32 is passed from amdgpu to ttm_bo_device_init to bdev->need_dma32. ttm_tt_create translates that to page_flags |= TTM_PAGE_FLAG_DMA32 and passes that to bdev->driver->ttm_tt_create. The two page allocators in ttm_page_alloc.c and ttm_page_alloc_dma.c check ttm->page_flags. Is that chain broken somewhere? Overriding glob->mem_glob->num_zones from amdgpu seems to be a bit of a hack. Regards, Felix On 2019-06-12 8:13, Yang, Philip wrote: > TTM create two zones, kernel zone and dma32 zone for system memory. If > system memory address allocated is below 4GB, this account to dma32 zone > and will exhaust dma32 zone and trigger unnesssary TTM eviction. > > Patch "drm/ttm: Account for kernel allocations in kernel zone only" only > handle the allocation for acc_size, the system memory page allocation is > through ttm_mem_global_alloc_page which still account to dma32 zone if > page is below 4GB. > > Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 > Signed-off-by: Philip Yang > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > index 2778ff63d97d..79bb9dfe617b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c > @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) > } > adev->mman.initialized = true; > > + /* Only kernel zone (no dma32 zone) if device does not require dma32 */ > + if (!adev->need_dma32) > + adev->mman.bdev.glob->mem_glob->num_zones = 1; > + > /* We opt to avoid OOM on system pages allocations */ > adev->mman.bdev.no_retry = true; > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v17 14/15] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
Hi Andrey,
On 6/12/19 1:43 PM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> vaddr_get_pfn() uses provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Reviewed-by: Catalin Marinas
> Reviewed-by: Kees Cook
> Signed-off-by: Andrey Konovalov
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> drivers/vfio/vfio_iommu_type1.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 3ddc375e7063..528e39a1c2dd 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -384,6 +384,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> long vaddr,
>
> down_read(>mmap_sem);
>
> + vaddr = untagged_addr(vaddr);
> +
> vma = find_vma_intersection(mm, vaddr, vaddr + 1);
>
> if (vma && vma->vm_flags & VM_PFNMAP) {
>
[PATCH] drm/amdgpu: only use kernel zone if need_dma32 is not required
TTM create two zones, kernel zone and dma32 zone for system memory. If system memory address allocated is below 4GB, this account to dma32 zone and will exhaust dma32 zone and trigger unnesssary TTM eviction. Patch "drm/ttm: Account for kernel allocations in kernel zone only" only handle the allocation for acc_size, the system memory page allocation is through ttm_mem_global_alloc_page which still account to dma32 zone if page is below 4GB. Change-Id: I289b85d891b8f64a1422c42b1eab398098ab7ef7 Signed-off-by: Philip Yang --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 2778ff63d97d..79bb9dfe617b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -1686,6 +1686,10 @@ int amdgpu_ttm_init(struct amdgpu_device *adev) } adev->mman.initialized = true; + /* Only kernel zone (no dma32 zone) if device does not require dma32 */ + if (!adev->need_dma32) + adev->mman.bdev.glob->mem_glob->num_zones = 1; + /* We opt to avoid OOM on system pages allocations */ adev->mman.bdev.no_retry = true; -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v17 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
On Wed, Jun 12, 2019 at 01:30:36PM +0100, Szabolcs Nagy wrote: > On 12/06/2019 12:43, Andrey Konovalov wrote: > > --- /dev/null > > +++ b/tools/testing/selftests/arm64/tags_lib.c > > @@ -0,0 +1,62 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include > > +#include > > + > > +#define TAG_SHIFT (56) > > +#define TAG_MASK (0xffUL << TAG_SHIFT) > > + > > +#define PR_SET_TAGGED_ADDR_CTRL55 > > +#define PR_GET_TAGGED_ADDR_CTRL56 > > +#define PR_TAGGED_ADDR_ENABLE (1UL << 0) > > + > > +void *__libc_malloc(size_t size); > > +void __libc_free(void *ptr); > > +void *__libc_realloc(void *ptr, size_t size); > > +void *__libc_calloc(size_t nmemb, size_t size); > > this does not work on at least musl. > > the most robust solution would be to implement > the malloc apis with mmap/munmap/mremap, if that's > too cumbersome then use dlsym RTLD_NEXT (although > that has the slight wart that in glibc it may call > calloc so wrapping calloc that way is tricky). > > in simple linux tests i'd just use static or > stack allocations or mmap. > > if a generic preloadable lib solution is needed > then do it properly with pthread_once to avoid > races etc. Thanks for the feedback Szabolcs. I guess we can go back to the initial simple test that Andrey had and drop the whole LD_PRELOAD hack (I'll just use it for my internal testing). BTW, when you get some time, please review Vincenzo's ABI documentation patches from a user/libc perspective. Once agreed, they should become part of this series. -- Catalin ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > From: Catalin Marinas > > It is not desirable to relax the ABI to allow tagged user addresses into > the kernel indiscriminately. This patch introduces a prctl() interface > for enabling or disabling the tagged ABI with a global sysctl control > for preventing applications from enabling the relaxed ABI (meant for > testing user-space prctl() return error checking without reconfiguring > the kernel). The ABI properties are inherited by threads of the same > application and fork()'ed children but cleared on execve(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas You need your signed-off-by here since you are contributing it. And thanks for adding the comment to the TIF definition. -- Catalin
Re: [PATCH v17 14/15] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
On 12/06/2019 12:43, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> vaddr_get_pfn() uses provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Reviewed-by: Catalin Marinas
> Reviewed-by: Kees Cook
> Signed-off-by: Andrey Konovalov
Reviewed-by: Vincenzo Frascino
> ---
> drivers/vfio/vfio_iommu_type1.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 3ddc375e7063..528e39a1c2dd 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -384,6 +384,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> long vaddr,
>
> down_read(>mmap_sem);
>
> + vaddr = untagged_addr(vaddr);
> +
> vma = find_vma_intersection(mm, vaddr, vaddr + 1);
>
> if (vma && vma->vm_flags & VM_PFNMAP) {
>
--
Regards,
Vincenzo
Re: [PATCH v17 08/15] userfaultfd, arm64: untag user pointers
On 12/06/2019 12:43, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> userfaultfd code use provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in validate_range().
>
> Reviewed-by: Catalin Marinas
> Reviewed-by: Kees Cook
> Signed-off-by: Andrey Konovalov
Reviewed-by: Vincenzo Frascino
> ---
> fs/userfaultfd.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 3b30301c90ec..24d68c3b5ee2 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1263,21 +1263,23 @@ static __always_inline void wake_userfault(struct
> userfaultfd_ctx *ctx,
> }
>
> static __always_inline int validate_range(struct mm_struct *mm,
> - __u64 start, __u64 len)
> + __u64 *start, __u64 len)
> {
> __u64 task_size = mm->task_size;
>
> - if (start & ~PAGE_MASK)
> + *start = untagged_addr(*start);
> +
> + if (*start & ~PAGE_MASK)
> return -EINVAL;
> if (len & ~PAGE_MASK)
> return -EINVAL;
> if (!len)
> return -EINVAL;
> - if (start < mmap_min_addr)
> + if (*start < mmap_min_addr)
> return -EINVAL;
> - if (start >= task_size)
> + if (*start >= task_size)
> return -EINVAL;
> - if (len > task_size - start)
> + if (len > task_size - *start)
> return -EINVAL;
> return 0;
> }
> @@ -1327,7 +1329,7 @@ static int userfaultfd_register(struct userfaultfd_ctx
> *ctx,
> goto out;
> }
>
> - ret = validate_range(mm, uffdio_register.range.start,
> + ret = validate_range(mm, _register.range.start,
>uffdio_register.range.len);
> if (ret)
> goto out;
> @@ -1516,7 +1518,7 @@ static int userfaultfd_unregister(struct
> userfaultfd_ctx *ctx,
> if (copy_from_user(_unregister, buf, sizeof(uffdio_unregister)))
> goto out;
>
> - ret = validate_range(mm, uffdio_unregister.start,
> + ret = validate_range(mm, _unregister.start,
>uffdio_unregister.len);
> if (ret)
> goto out;
> @@ -1667,7 +1669,7 @@ static int userfaultfd_wake(struct userfaultfd_ctx *ctx,
> if (copy_from_user(_wake, buf, sizeof(uffdio_wake)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len);
> + ret = validate_range(ctx->mm, _wake.start, uffdio_wake.len);
> if (ret)
> goto out;
>
> @@ -1707,7 +1709,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
> sizeof(uffdio_copy)-sizeof(__s64)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
> + ret = validate_range(ctx->mm, _copy.dst, uffdio_copy.len);
> if (ret)
> goto out;
> /*
> @@ -1763,7 +1765,7 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx
> *ctx,
> sizeof(uffdio_zeropage)-sizeof(__s64)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_zeropage.range.start,
> + ret = validate_range(ctx->mm, _zeropage.range.start,
>uffdio_zeropage.range.len);
> if (ret)
> goto out;
>
--
Regards,
Vincenzo
Re: [PATCH v17 07/15] fs, arm64: untag user pointers in copy_mount_options
On 12/06/2019 12:43, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > In copy_mount_options a user address is being subtracted from TASK_SIZE. > If the address is lower than TASK_SIZE, the size is calculated to not > allow the exact_copy_from_user() call to cross TASK_SIZE boundary. > However if the address is tagged, then the size will be calculated > incorrectly. > > Untag the address before subtracting. > > Reviewed-by: Kees Cook > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov Reviewed-by: Vincenzo Frascino > --- > fs/namespace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/namespace.c b/fs/namespace.c > index b26778bdc236..2e85712a19ed 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2993,7 +2993,7 @@ void *copy_mount_options(const void __user * data) >* the remainder of the page. >*/ > /* copy_from_user cannot cross TASK_SIZE ! */ > - size = TASK_SIZE - (unsigned long)data; > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > if (size > PAGE_SIZE) > size = PAGE_SIZE; > > -- Regards, Vincenzo
Re: [PATCH v17 06/15] mm, arm64: untag user pointers in get_vaddr_frames
On 12/06/2019 12:43, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > get_vaddr_frames uses provided user pointers for vma lookups, which can > only by done with untagged pointers. Instead of locating and changing > all callers of this function, perform untagging in it. > > Acked-by: Catalin Marinas > Reviewed-by: Kees Cook > Signed-off-by: Andrey Konovalov Reviewed-by: Vincenzo Frascino > --- > mm/frame_vector.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/frame_vector.c b/mm/frame_vector.c > index c64dca6e27c2..c431ca81dad5 100644 > --- a/mm/frame_vector.c > +++ b/mm/frame_vector.c > @@ -46,6 +46,8 @@ int get_vaddr_frames(unsigned long start, unsigned int > nr_frames, > if (WARN_ON_ONCE(nr_frames > vec->nr_allocated)) > nr_frames = vec->nr_allocated; > > + start = untagged_addr(start); > + > down_read(>mmap_sem); > locked = 1; > vma = find_vma_intersection(mm, start, start + 1); > -- Regards, Vincenzo
Re: [PATCH v17 05/15] mm, arm64: untag user pointers in mm/gup.c
On 12/06/2019 12:43, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > mm/gup.c provides a kernel interface that accepts user addresses and > manipulates user pages directly (for example get_user_pages, that is used > by the futex syscall). Since a user can provided tagged addresses, we need > to handle this case. > > Add untagging to gup.c functions that use user addresses for vma lookups. > > Reviewed-by: Kees Cook > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov Reviewed-by: Vincenzo Frascino > --- > mm/gup.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/mm/gup.c b/mm/gup.c > index ddde097cf9e4..c37df3d455a2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -802,6 +802,8 @@ static long __get_user_pages(struct task_struct *tsk, > struct mm_struct *mm, > if (!nr_pages) > return 0; > > + start = untagged_addr(start); > + > VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); > > /* > @@ -964,6 +966,8 @@ int fixup_user_fault(struct task_struct *tsk, struct > mm_struct *mm, > struct vm_area_struct *vma; > vm_fault_t ret, major = 0; > > + address = untagged_addr(address); > + > if (unlocked) > fault_flags |= FAULT_FLAG_ALLOW_RETRY; > > -- Regards, Vincenzo
Re: [PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls
On 12/06/2019 12:43, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch allows tagged pointers to be passed to the following memory
> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
> mremap, msync, munlock, move_pages.
>
> The mmap and mremap syscalls do not currently accept tagged addresses.
> Architectures may interpret the tag as a background colour for the
> corresponding vma.
>
> Reviewed-by: Catalin Marinas
> Reviewed-by: Kees Cook
> Signed-off-by: Andrey Konovalov
Reviewed-by: Vincenzo Frascino
> ---
> mm/madvise.c | 2 ++
> mm/mempolicy.c | 3 +++
> mm/migrate.c | 2 +-
> mm/mincore.c | 2 ++
> mm/mlock.c | 4
> mm/mprotect.c | 2 ++
> mm/mremap.c| 7 +++
> mm/msync.c | 2 ++
> 8 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 628022e674a7..39b82f8a698f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t,
> len_in, int, behavior)
> size_t len;
> struct blk_plug plug;
>
> + start = untagged_addr(start);
> +
> if (!madvise_behavior_valid(behavior))
> return error;
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d80ae01..78e0a88b2680 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned
> long len,
> int err;
> unsigned short mode_flags;
>
> + start = untagged_addr(start);
> mode_flags = mode & MPOL_MODE_FLAGS;
> mode &= ~MPOL_MODE_FLAGS;
> if (mode >= MPOL_MAX)
> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
> int uninitialized_var(pval);
> nodemask_t nodes;
>
> + addr = untagged_addr(addr);
> +
> if (nmask != NULL && maxnode < nr_node_ids)
> return -EINVAL;
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f2ecc2855a12..d22c45cf36b2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm,
> nodemask_t task_nodes,
> goto out_flush;
> if (get_user(node, nodes + i))
> goto out_flush;
> - addr = (unsigned long)p;
> + addr = (unsigned long)untagged_addr(p);
>
> err = -ENODEV;
> if (node < 0 || node >= MAX_NUMNODES)
> diff --git a/mm/mincore.c b/mm/mincore.c
> index c3f058bd0faf..64c322ed845c 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t,
> len,
> unsigned long pages;
> unsigned char *tmp;
>
> + start = untagged_addr(start);
> +
> /* Check the start address: needs to be page-aligned.. */
> if (start & ~PAGE_MASK)
> return -EINVAL;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 080f3b36415b..e82609eaa428 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start,
> size_t len, vm_flags_t fla
> unsigned long lock_limit;
> int error = -ENOMEM;
>
> + start = untagged_addr(start);
> +
> if (!can_do_mlock())
> return -EPERM;
>
> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t,
> len)
> {
> int ret;
>
> + start = untagged_addr(start);
> +
> len = PAGE_ALIGN(len + (offset_in_page(start)));
> start &= PAGE_MASK;
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index bf38dfbbb4b4..19f981b733bc 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t
> len,
> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
> (prot & PROT_READ);
>
> + start = untagged_addr(start);
> +
> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
> return -EINVAL;
> diff --git a/mm/mremap.c b/mm/mremap.c
> index fc241d23cd97..64c9a3b8be0a 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned
> long, old_len,
> LIST_HEAD(uf_unmap_early);
> LIST_HEAD(uf_unmap);
>
> + /*
> + * Architectures may interpret the tag passed to mmap as a background
> + * colour for the corresponding vma. For mremap we don't allow tagged
> + * new_addr to preserve similar behaviour to mmap.
> + */
> + addr = untagged_addr(addr);
> +
> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> return ret;
>
> diff --git a/mm/msync.c b/mm/msync.c
> index ef30a429623a..c3bd3e75f687 100644
> ---
Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
On 12/06/2019 12:43, Andrey Konovalov wrote:
> From: Catalin Marinas
>
> It is not desirable to relax the ABI to allow tagged user addresses into
> the kernel indiscriminately. This patch introduces a prctl() interface
> for enabling or disabling the tagged ABI with a global sysctl control
> for preventing applications from enabling the relaxed ABI (meant for
> testing user-space prctl() return error checking without reconfiguring
> the kernel). The ABI properties are inherited by threads of the same
> application and fork()'ed children but cleared on execve().
>
> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> MTE-specific settings like imprecise vs precise exceptions.
>
> Signed-off-by: Catalin Marinas
Reviewed-by: Vincenzo Frascino
> ---
> arch/arm64/include/asm/processor.h | 6 +++
> arch/arm64/include/asm/thread_info.h | 1 +
> arch/arm64/include/asm/uaccess.h | 3 +-
> arch/arm64/kernel/process.c | 67
> include/uapi/linux/prctl.h | 5 +++
> kernel/sys.c | 16 +++
> 6 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/processor.h
> b/arch/arm64/include/asm/processor.h
> index fcd0e691b1ea..fee457456aa8 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void);
> /* PR_PAC_RESET_KEYS prctl */
> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>
> +/* PR_TAGGED_ADDR prctl */
> +long set_tagged_addr_ctrl(unsigned long arg);
> +long get_tagged_addr_ctrl(void);
> +#define SET_TAGGED_ADDR_CTRL(arg)set_tagged_addr_ctrl(arg)
> +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
> +
> /*
> * For CONFIG_GCC_PLUGIN_STACKLEAK
> *
> diff --git a/arch/arm64/include/asm/thread_info.h
> b/arch/arm64/include/asm/thread_info.h
> index f1d032be628a..354a31d2b737 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -99,6 +99,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SVE 23 /* Scalable Vector Extension in
> use */
> #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
> #define TIF_SSBD 25 /* Wants SSB mitigation */
> +#define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses
> */
>
> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> #define _TIF_NEED_RESCHED(1 << TIF_NEED_RESCHED)
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index df729afca0ba..995b9ea11a89 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -73,7 +73,8 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> - addr = untagged_addr(addr);
> + if (test_thread_flag(TIF_TAGGED_ADDR))
> + addr = untagged_addr(addr);
>
> __chk_user_ptr(addr);
> asm volatile(
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb21a5b8..69d0be1fc708 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -30,6 +30,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -323,6 +324,7 @@ void flush_thread(void)
> fpsimd_flush_thread();
> tls_thread_flush();
> flush_ptrace_hw_breakpoint(current);
> + clear_thread_flag(TIF_TAGGED_ADDR);
> }
>
> void release_thread(struct task_struct *dead_task)
> @@ -552,3 +554,68 @@ void arch_setup_new_exec(void)
>
> ptrauth_thread_init_user(current);
> }
> +
> +/*
> + * Control the relaxed ABI allowing tagged user addresses into the kernel.
> + */
> +static unsigned int tagged_addr_prctl_allowed = 1;
> +
> +long set_tagged_addr_ctrl(unsigned long arg)
> +{
> + if (!tagged_addr_prctl_allowed)
> + return -EINVAL;
> + if (is_compat_task())
> + return -EINVAL;
> + if (arg & ~PR_TAGGED_ADDR_ENABLE)
> + return -EINVAL;
> +
> + if (arg & PR_TAGGED_ADDR_ENABLE)
> + set_thread_flag(TIF_TAGGED_ADDR);
> + else
> + clear_thread_flag(TIF_TAGGED_ADDR);
> +
> + return 0;
> +}
> +
> +long get_tagged_addr_ctrl(void)
> +{
> + if (!tagged_addr_prctl_allowed)
> + return -EINVAL;
> + if (is_compat_task())
> + return -EINVAL;
> +
> + if (test_thread_flag(TIF_TAGGED_ADDR))
> + return PR_TAGGED_ADDR_ENABLE;
> +
> + return 0;
> +}
> +
> +/*
> + * Global sysctl to disable the tagged user addresses support. This control
> + * only prevents the tagged address ABI enabling via prctl() and does not
> + * disable it for tasks that already opted in to the relaxed ABI.
> + */
> +static
Re: [PATCH v17 02/15] lib, arm64: untag user pointers in strn*_user
On 12/06/2019 12:43, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> strncpy_from_user and strnlen_user accept user addresses as arguments, and
> do not go through the same path as copy_from_user and others, so here we
> need to handle the case of tagged user addresses separately.
>
> Untag user pointers passed to these functions.
>
> Note, that this patch only temporarily untags the pointers to perform
> validity checks, but then uses them as is to perform user memory accesses.
>
> Reviewed-by: Khalid Aziz
> Acked-by: Kees Cook
> Reviewed-by: Catalin Marinas
> Signed-off-by: Andrey Konovalov
Reviewed-by: Vincenzo Frascino
> ---
> lib/strncpy_from_user.c | 3 ++-
> lib/strnlen_user.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 023ba9f3b99f..dccb95af6003 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -108,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src,
> long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)src;
> + src_addr = (unsigned long)untagged_addr(src);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..28ff554a1be8 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,6 +2,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -109,7 +110,7 @@ long strnlen_user(const char __user *str, long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)str;
> + src_addr = (unsigned long)untagged_addr(str);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
>
--
Regards,
Vincenzo
Re: [PATCH v17 01/15] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On 12/06/2019 12:43, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> copy_from_user (and a few other similar functions) are used to copy data
> from user memory into the kernel memory or vice versa. Since a user can
> provided a tagged pointer to one of the syscalls that use copy_from_user,
> we need to correctly handle such pointers.
>
> Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr,
> before performing access validity checks.
>
> Note, that this patch only temporarily untags the pointers to perform the
> checks, but then passes them as is into the kernel internals.
>
> Reviewed-by: Kees Cook
> Reviewed-by: Catalin Marinas
> Signed-off-by: Andrey Konovalov
Reviewed-by: Vincenzo Frascino
> ---
> arch/arm64/include/asm/uaccess.h | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index e5d5f31c6d36..df729afca0ba 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -73,6 +73,8 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> + addr = untagged_addr(addr);
> +
> __chk_user_ptr(addr);
> asm volatile(
> // A + B <= C + 1 for all A,B,C, in four easy steps:
> @@ -226,7 +228,8 @@ static inline void uaccess_enable_not_uao(void)
>
> /*
> * Sanitise a uaccess pointer such that it becomes NULL if above the
> - * current addr_limit.
> + * current addr_limit. In case the pointer is tagged (has the top byte set),
> + * untag the pointer before checking.
> */
> #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
> static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> @@ -234,10 +237,11 @@ static inline void __user *__uaccess_mask_ptr(const
> void __user *ptr)
> void __user *safe_ptr;
>
> asm volatile(
> - " bicsxzr, %1, %2\n"
> + " bicsxzr, %3, %2\n"
> " csel%0, %1, xzr, eq\n"
> : "=" (safe_ptr)
> - : "r" (ptr), "r" (current_thread_info()->addr_limit)
> + : "r" (ptr), "r" (current_thread_info()->addr_limit),
> + "r" (untagged_addr(ptr))
> : "cc");
>
> csdb();
>
--
Regards,
Vincenzo
Re: [PATCH v3] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Am 12.06.19 um 15:11 schrieb Zhu, James:
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
Reviewed-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..215a4a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = >gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,21 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; i < adev->usec_timeout; i++) {
+ if (ring->wptr == gfx_v9_0_ring_get_rptr_compute(ring))
+ break;
+ udelay(1);
+ }
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On Wed, Jun 12, 2019 at 12:12:34AM -0700, Christoph Hellwig wrote: > On Tue, Jun 11, 2019 at 04:44:31PM -0300, Jason Gunthorpe wrote: > > On Sat, Jun 08, 2019 at 01:54:25AM -0700, Christoph Hellwig wrote: > > > FYI, I very much disagree with the direction this is moving. > > > > > > struct hmm_mirror literally is a trivial duplication of the > > > mmu_notifiers. All these drivers should just use the mmu_notifiers > > > directly for the mirroring part instead of building a thing wrapper > > > that adds nothing but helping to manage the lifetime of struct hmm, > > > which shouldn't exist to start with. > > > > Christoph: What do you think about this sketch below? > > > > It would replace the hmm_range/mirror/etc with a different way to > > build the same locking scheme using some optional helpers linked to > > the mmu notifier? > > > > (just a sketch, still needs a lot more thinking) > > I like the idea. A few nitpicks: Can we avoid having to store the > mm in struct mmu_notifier? I think we could just easily pass it as a > parameter to the helpers. Yes, but I think any driver that needs to use this API will have to hold the 'struct mm_struct' and the 'struct mmu_notifier' together (ie ODP does this in ib_ucontext_per_mm), so if we put it in the notifier then it is trivially available everwhere it is needed, and the mmu_notifier code takes care of the lifetime for the driver. > The write lock case of mm_invlock_start_write_and_lock is probably > worth factoring into separate helper? I can see cases where drivers > want to just use it directly if they need to force getting the lock > without the chance of a long wait. The entire purpose of the invlock is to avoid getting the write lock on mmap_sem as a fast path - if the driver wishes to use mmap_sem locking only then it should just do so directly and forget about the invlock. Note that this patch is just an API sketch, I haven't fully checked that the range_start/end are actually always called under mmap_sem, and I already found that release is not. So there will need to be some preperatory adjustments before we can use down_write(mmap_sem) as a locking strategy here. Thanks, Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On Wed, Jun 12, 2019 at 08:41:25AM -0300, Jason Gunthorpe wrote: > > I like the idea. A few nitpicks: Can we avoid having to store the > > mm in struct mmu_notifier? I think we could just easily pass it as a > > parameter to the helpers. > > Yes, but I think any driver that needs to use this API will have to > hold the 'struct mm_struct' and the 'struct mmu_notifier' together (ie > ODP does this in ib_ucontext_per_mm), so if we put it in the notifier > then it is trivially available everwhere it is needed, and the > mmu_notifier code takes care of the lifetime for the driver. True. Well, maybe keep it for now at least. > The entire purpose of the invlock is to avoid getting the write lock > on mmap_sem as a fast path - if the driver wishes to use mmap_sem > locking only then it should just do so directly and forget about the > invlock. May worry here is that there migh be cases where the driver needs to expedite the action, in which case jumping straight to the write lock. But again we can probably skip this for now and see if it really ends up being needed. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
On Tue, Jun 11, 2019 at 04:44:31PM -0300, Jason Gunthorpe wrote: > On Sat, Jun 08, 2019 at 01:54:25AM -0700, Christoph Hellwig wrote: > > FYI, I very much disagree with the direction this is moving. > > > > struct hmm_mirror literally is a trivial duplication of the > > mmu_notifiers. All these drivers should just use the mmu_notifiers > > directly for the mirroring part instead of building a thing wrapper > > that adds nothing but helping to manage the lifetime of struct hmm, > > which shouldn't exist to start with. > > Christoph: What do you think about this sketch below? > > It would replace the hmm_range/mirror/etc with a different way to > build the same locking scheme using some optional helpers linked to > the mmu notifier? > > (just a sketch, still needs a lot more thinking) I like the idea. A few nitpicks: Can we avoid having to store the mm in struct mmu_notifier? I think we could just easily pass it as a parameter to the helpers. The write lock case of mm_invlock_start_write_and_lock is probably worth factoring into separate helper? I can see cases where drivers want to just use it directly if they need to force getting the lock without the chance of a long wait. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v3] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..215a4a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = >gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,21 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; i < adev->usec_timeout; i++) {
+ if (ring->wptr == gfx_v9_0_ring_get_rptr_compute(ring))
+ break;
+ udelay(1);
+ }
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Please use udelay directly rather than DRM_UDELAY() those old macros are
deprecated and going away.
Alex
From: amd-gfx on behalf of Zhu, James
Sent: Wednesday, June 12, 2019 8:59 AM
To: amd-gfx@lists.freedesktop.org
Cc: Shamis, Leonid; ckoenig.leichtzumer...@gmail.com; Zhu, James
Subject: [PATCH v2] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..2e058bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = >gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,21 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; i < adev->usec_timeout; i++) {
+ if (ring->wptr == gfx_v9_0_ring_get_rptr_compute(ring))
+ break;
+ DRM_UDELAY(1);
+ }
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 35 +--
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..2e058bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = >gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,21 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; i < adev->usec_timeout; i++) {
+ if (ring->wptr == gfx_v9_0_ring_get_rptr_compute(ring))
+ break;
+ DRM_UDELAY(1);
+ }
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v17 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
On 12/06/2019 12:43, Andrey Konovalov wrote:
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_lib.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +
> +#define TAG_SHIFT(56)
> +#define TAG_MASK (0xffUL << TAG_SHIFT)
> +
> +#define PR_SET_TAGGED_ADDR_CTRL 55
> +#define PR_GET_TAGGED_ADDR_CTRL 56
> +#define PR_TAGGED_ADDR_ENABLE(1UL << 0)
> +
> +void *__libc_malloc(size_t size);
> +void __libc_free(void *ptr);
> +void *__libc_realloc(void *ptr, size_t size);
> +void *__libc_calloc(size_t nmemb, size_t size);
this does not work on at least musl.
the most robust solution would be to implement
the malloc apis with mmap/munmap/mremap, if that's
too cumbersome then use dlsym RTLD_NEXT (although
that has the slight wart that in glibc it may call
calloc so wrapping calloc that way is tricky).
in simple linux tests i'd just use static or
stack allocations or mmap.
if a generic preloadable lib solution is needed
then do it properly with pthread_once to avoid
races etc.
> +
> +static void *tag_ptr(void *ptr)
> +{
> + static int tagged_addr_err = 1;
> + unsigned long tag = 0;
> +
> + /*
> + * Note that this code is racy. We only use it as a part of a single
> + * threaded test application. Beware of using in multithreaded ones.
> + */
> + if (tagged_addr_err == 1)
> + tagged_addr_err = prctl(PR_SET_TAGGED_ADDR_CTRL,
> + PR_TAGGED_ADDR_ENABLE, 0, 0, 0);
> +
> + if (!ptr)
> + return ptr;
> + if (!tagged_addr_err)
> + tag = rand() & 0xff;
> +
> + return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +}
> +
> +static void *untag_ptr(void *ptr)
> +{
> + return (void *)((unsigned long)ptr & ~TAG_MASK);
> +}
> +
> +void *malloc(size_t size)
> +{
> + return tag_ptr(__libc_malloc(size));
> +}
...
Re: [PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v3)
Am 12.06.19 um 13:08 schrieb StDenis, Tom:
(v2): Return 0 and set mem->mm_node to NULL.
(v3): Use atomic64_add_return instead.
Signed-off-by: Tom St Denis
Reviewed-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 8aea2f21b202..c963ad86072e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -276,7 +276,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
struct drm_mm_node *nodes;
enum drm_mm_insert_mode mode;
unsigned long lpfn, num_nodes, pages_per_node, pages_left;
- uint64_t usage = 0, vis_usage = 0;
+ uint64_t vis_usage = 0;
unsigned i;
int r;
@@ -284,6 +284,13 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man,
if (!lpfn)
lpfn = man->size;
+ /* bail out quickly if there's likely not enough VRAM for this BO */
+ if (atomic64_add_return(mem->num_pages << PAGE_SHIFT, >usage) >
adev->gmc.mc_vram_size) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
+ mem->mm_node = NULL;
+ return 0;
+ }
+
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
pages_per_node = ~0ul;
num_nodes = 1;
@@ -300,8 +307,10 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
GFP_KERNEL | __GFP_ZERO);
- if (!nodes)
+ if (!nodes) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
return -ENOMEM;
+ }
mode = DRM_MM_INSERT_BEST;
if (place->flags & TTM_PL_FLAG_TOPDOWN)
@@ -321,7 +330,6 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (unlikely(r))
break;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
@@ -341,14 +349,12 @@ static int amdgpu_vram_mgr_new(struct
ttm_mem_type_manager *man,
if (unlikely(r))
goto error;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
}
spin_unlock(>lock);
- atomic64_add(usage, >usage);
atomic64_add(vis_usage, >vis_usage);
mem->mm_node = nodes;
@@ -359,6 +365,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
while (i--)
drm_mm_remove_node([i]);
spin_unlock(>lock);
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
kvfree(nodes);
return r == -ENOSPC ? 0 : r;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
Hi Catalin,
On 12/06/2019 10:32, Catalin Marinas wrote:
> Hi Vincenzo,
>
> On Tue, Jun 11, 2019 at 06:09:10PM +0100, Vincenzo Frascino wrote:
>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>>> index 3767fb21a5b8..69d0be1fc708 100644
>>> --- a/arch/arm64/kernel/process.c
>>> +++ b/arch/arm64/kernel/process.c
>>> @@ -30,6 +30,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -323,6 +324,7 @@ void flush_thread(void)
>>> fpsimd_flush_thread();
>>> tls_thread_flush();
>>> flush_ptrace_hw_breakpoint(current);
>>> + clear_thread_flag(TIF_TAGGED_ADDR);
>>
>> Nit: in line we the other functions in thread_flush we could have something
>> like
>> "tagged_addr_thread_flush", maybe inlined.
>
> The other functions do a lot more than clearing a TIF flag, so they
> deserved their own place. We could do this when adding MTE support. I
> think we also need to check what other TIF flags we may inadvertently
> pass on execve(), maybe have a mask clearing.
>
Agreed. All the comments I provided are meant to simplify the addition of MTE
support.
>>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>>> index 094bb03b9cc2..2e927b3e9d6c 100644
>>> --- a/include/uapi/linux/prctl.h
>>> +++ b/include/uapi/linux/prctl.h
>>> @@ -229,4 +229,9 @@ struct prctl_mm_map {
>>> # define PR_PAC_APDBKEY(1UL << 3)
>>> # define PR_PAC_APGAKEY(1UL << 4)
>>>
>>> +/* Tagged user address controls for arm64 */
>>> +#define PR_SET_TAGGED_ADDR_CTRL55
>>> +#define PR_GET_TAGGED_ADDR_CTRL56
>>> +# define PR_TAGGED_ADDR_ENABLE (1UL << 0)
>>> +
>>> #endif /* _LINUX_PRCTL_H */
>>> diff --git a/kernel/sys.c b/kernel/sys.c
>>> index 2969304c29fe..ec48396b4943 100644
>>> --- a/kernel/sys.c
>>> +++ b/kernel/sys.c
>>> @@ -124,6 +124,12 @@
>>> #ifndef PAC_RESET_KEYS
>>> # define PAC_RESET_KEYS(a, b) (-EINVAL)
>>> #endif
>>> +#ifndef SET_TAGGED_ADDR_CTRL
>>> +# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL)
>>> +#endif
>>> +#ifndef GET_TAGGED_ADDR_CTRL
>>> +# define GET_TAGGED_ADDR_CTRL()(-EINVAL)
>>> +#endif
>>>
>>> /*
>>> * this is where the system-wide overflow UID and GID are defined, for
>>> @@ -2492,6 +2498,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
>>> arg2, unsigned long, arg3,
>>> return -EINVAL;
>>> error = PAC_RESET_KEYS(me, arg2);
>>> break;
>>> + case PR_SET_TAGGED_ADDR_CTRL:
>>> + if (arg3 || arg4 || arg5)
>>> + return -EINVAL;
>>> + error = SET_TAGGED_ADDR_CTRL(arg2);
>>> + break;
>>> + case PR_GET_TAGGED_ADDR_CTRL:
>>> + if (arg2 || arg3 || arg4 || arg5)
>>> + return -EINVAL;
>>> + error = GET_TAGGED_ADDR_CTRL();
>>> + break;
>>
>> Why do we need two prctl here? We could have only one and use arg2 as set/get
>> and arg3 as a parameter. What do you think?
>
> This follows the other PR_* options, e.g. PR_SET_VL/GET_VL,
> PR_*_FP_MODE. We will use other bits in arg2, for example to set the
> precise vs imprecise MTE trapping.
>
Indeed. I was not questioning the pre-existing interface definition, but trying
more to reduce the changes to the ABI to the minimum since:
- prctl does not mandate how to use the arg[2-5]
- prctl interface is flexible enough for the problem to be solved with only one
PR_ command.
I agree on reusing the interface for MTE for the purposes you specified.
--
Regards,
Vincenzo
[PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
This patch allows tagged pointers to be passed to the following memory
syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
mremap, msync, munlock, move_pages.
The mmap and mremap syscalls do not currently accept tagged addresses.
Architectures may interpret the tag as a background colour for the
corresponding vma.
Reviewed-by: Catalin Marinas
Reviewed-by: Kees Cook
Signed-off-by: Andrey Konovalov
---
mm/madvise.c | 2 ++
mm/mempolicy.c | 3 +++
mm/migrate.c | 2 +-
mm/mincore.c | 2 ++
mm/mlock.c | 4
mm/mprotect.c | 2 ++
mm/mremap.c| 7 +++
mm/msync.c | 2 ++
8 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index 628022e674a7..39b82f8a698f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t,
len_in, int, behavior)
size_t len;
struct blk_plug plug;
+ start = untagged_addr(start);
+
if (!madvise_behavior_valid(behavior))
return error;
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 01600d80ae01..78e0a88b2680 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned
long len,
int err;
unsigned short mode_flags;
+ start = untagged_addr(start);
mode_flags = mode & MPOL_MODE_FLAGS;
mode &= ~MPOL_MODE_FLAGS;
if (mode >= MPOL_MAX)
@@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
int uninitialized_var(pval);
nodemask_t nodes;
+ addr = untagged_addr(addr);
+
if (nmask != NULL && maxnode < nr_node_ids)
return -EINVAL;
diff --git a/mm/migrate.c b/mm/migrate.c
index f2ecc2855a12..d22c45cf36b2 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t
task_nodes,
goto out_flush;
if (get_user(node, nodes + i))
goto out_flush;
- addr = (unsigned long)p;
+ addr = (unsigned long)untagged_addr(p);
err = -ENODEV;
if (node < 0 || node >= MAX_NUMNODES)
diff --git a/mm/mincore.c b/mm/mincore.c
index c3f058bd0faf..64c322ed845c 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
unsigned long pages;
unsigned char *tmp;
+ start = untagged_addr(start);
+
/* Check the start address: needs to be page-aligned.. */
if (start & ~PAGE_MASK)
return -EINVAL;
diff --git a/mm/mlock.c b/mm/mlock.c
index 080f3b36415b..e82609eaa428 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start,
size_t len, vm_flags_t fla
unsigned long lock_limit;
int error = -ENOMEM;
+ start = untagged_addr(start);
+
if (!can_do_mlock())
return -EPERM;
@@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
{
int ret;
+ start = untagged_addr(start);
+
len = PAGE_ALIGN(len + (offset_in_page(start)));
start &= PAGE_MASK;
diff --git a/mm/mprotect.c b/mm/mprotect.c
index bf38dfbbb4b4..19f981b733bc 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
(prot & PROT_READ);
+ start = untagged_addr(start);
+
prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
return -EINVAL;
diff --git a/mm/mremap.c b/mm/mremap.c
index fc241d23cd97..64c9a3b8be0a 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned
long, old_len,
LIST_HEAD(uf_unmap_early);
LIST_HEAD(uf_unmap);
+ /*
+* Architectures may interpret the tag passed to mmap as a background
+* colour for the corresponding vma. For mremap we don't allow tagged
+* new_addr to preserve similar behaviour to mmap.
+*/
+ addr = untagged_addr(addr);
+
if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
return ret;
diff --git a/mm/msync.c b/mm/msync.c
index ef30a429623a..c3bd3e75f687 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len,
int, flags)
int unmapped_error = 0;
int error = -EINVAL;
+ start = untagged_addr(start);
+
if (flags & ~(MS_ASYNC | MS_INVALIDATE |
[PATCH v17 07/15] fs, arm64: untag user pointers in copy_mount_options
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. In copy_mount_options a user address is being subtracted from TASK_SIZE. If the address is lower than TASK_SIZE, the size is calculated to not allow the exact_copy_from_user() call to cross TASK_SIZE boundary. However if the address is tagged, then the size will be calculated incorrectly. Untag the address before subtracting. Reviewed-by: Kees Cook Reviewed-by: Catalin Marinas Signed-off-by: Andrey Konovalov --- fs/namespace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/namespace.c b/fs/namespace.c index b26778bdc236..2e85712a19ed 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2993,7 +2993,7 @@ void *copy_mount_options(const void __user * data) * the remainder of the page. */ /* copy_from_user cannot cross TASK_SIZE ! */ - size = TASK_SIZE - (unsigned long)data; + size = TASK_SIZE - (unsigned long)untagged_addr(data); if (size > PAGE_SIZE) size = PAGE_SIZE; -- 2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 09/15] drm/amdgpu, arm64: untag user pointers
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
In amdgpu_gem_userptr_ioctl() and amdgpu_amdkfd_gpuvm.c/init_user_pages()
an MMU notifier is set up with a (tagged) userspace pointer. The untagged
address should be used so that MMU notifiers for the untagged address get
correctly matched up with the right BO. This patch untag user pointers in
amdgpu_gem_userptr_ioctl() for the GEM case and in amdgpu_amdkfd_gpuvm_
alloc_memory_of_gpu() for the KFD case. This also makes sure that an
untagged pointer is passed to amdgpu_ttm_tt_get_user_pages(), which uses
it for vma lookups.
Suggested-by: Felix Kuehling
Acked-by: Felix Kuehling
Signed-off-by: Andrey Konovalov
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index a6e5184d436c..5d476e9bbc43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1108,7 +1108,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
alloc_flags = 0;
if (!offset || !*offset)
return -EINVAL;
- user_addr = *offset;
+ user_addr = untagged_addr(*offset);
} else if (flags & ALLOC_MEM_FLAGS_DOORBELL) {
domain = AMDGPU_GEM_DOMAIN_GTT;
alloc_domain = AMDGPU_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index d4fcf5475464..e91df1407618 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -287,6 +287,8 @@ int amdgpu_gem_userptr_ioctl(struct drm_device *dev, void
*data,
uint32_t handle;
int r;
+ args->addr = untagged_addr(args->addr);
+
if (offset_in_page(args->addr | args->size))
return -EINVAL;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
This patch adds a simple test, that calls the uname syscall with a
tagged user pointer as an argument. Without the kernel accepting tagged
user pointers the test fails with EFAULT.
Co-developed-by: Catalin Marinas
Signed-off-by: Andrey Konovalov
---
tools/testing/selftests/arm64/.gitignore | 2 +
tools/testing/selftests/arm64/Makefile| 22 +++
.../testing/selftests/arm64/run_tags_test.sh | 12
tools/testing/selftests/arm64/tags_lib.c | 62 +++
tools/testing/selftests/arm64/tags_test.c | 18 ++
5 files changed, 116 insertions(+)
create mode 100644 tools/testing/selftests/arm64/.gitignore
create mode 100644 tools/testing/selftests/arm64/Makefile
create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
create mode 100644 tools/testing/selftests/arm64/tags_lib.c
create mode 100644 tools/testing/selftests/arm64/tags_test.c
diff --git a/tools/testing/selftests/arm64/.gitignore
b/tools/testing/selftests/arm64/.gitignore
new file mode 100644
index ..9b6a568de17f
--- /dev/null
+++ b/tools/testing/selftests/arm64/.gitignore
@@ -0,0 +1,2 @@
+tags_test
+tags_lib.so
diff --git a/tools/testing/selftests/arm64/Makefile
b/tools/testing/selftests/arm64/Makefile
new file mode 100644
index ..9dee18727923
--- /dev/null
+++ b/tools/testing/selftests/arm64/Makefile
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0
+
+include ../lib.mk
+
+# ARCH can be overridden by the user for cross compiling
+ARCH ?= $(shell uname -m 2>/dev/null || echo not)
+
+ifneq (,$(filter $(ARCH),aarch64 arm64))
+
+TEST_CUSTOM_PROGS := $(OUTPUT)/tags_test
+
+$(OUTPUT)/tags_test: tags_test.c $(OUTPUT)/tags_lib.so
+ $(CC) -o $@ $(CFLAGS) $(LDFLAGS) $<
+
+$(OUTPUT)/tags_lib.so: tags_lib.c
+ $(CC) -o $@ -shared $(CFLAGS) $(LDFLAGS) $^
+
+TEST_PROGS := run_tags_test.sh
+
+all: $(TEST_CUSTOM_PROGS)
+
+endif
diff --git a/tools/testing/selftests/arm64/run_tags_test.sh
b/tools/testing/selftests/arm64/run_tags_test.sh
new file mode 100755
index ..2bbe0cd4220b
--- /dev/null
+++ b/tools/testing/selftests/arm64/run_tags_test.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+echo ""
+echo "running tags test"
+echo ""
+LD_PRELOAD=./tags_lib.so ./tags_test
+if [ $? -ne 0 ]; then
+ echo "[FAIL]"
+else
+ echo "[PASS]"
+fi
diff --git a/tools/testing/selftests/arm64/tags_lib.c
b/tools/testing/selftests/arm64/tags_lib.c
new file mode 100644
index ..55f64fc1aae6
--- /dev/null
+++ b/tools/testing/selftests/arm64/tags_lib.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+#define TAG_SHIFT (56)
+#define TAG_MASK (0xffUL << TAG_SHIFT)
+
+#define PR_SET_TAGGED_ADDR_CTRL55
+#define PR_GET_TAGGED_ADDR_CTRL56
+#define PR_TAGGED_ADDR_ENABLE (1UL << 0)
+
+void *__libc_malloc(size_t size);
+void __libc_free(void *ptr);
+void *__libc_realloc(void *ptr, size_t size);
+void *__libc_calloc(size_t nmemb, size_t size);
+
+static void *tag_ptr(void *ptr)
+{
+ static int tagged_addr_err = 1;
+ unsigned long tag = 0;
+
+ /*
+* Note that this code is racy. We only use it as a part of a single
+* threaded test application. Beware of using in multithreaded ones.
+*/
+ if (tagged_addr_err == 1)
+ tagged_addr_err = prctl(PR_SET_TAGGED_ADDR_CTRL,
+ PR_TAGGED_ADDR_ENABLE, 0, 0, 0);
+
+ if (!ptr)
+ return ptr;
+ if (!tagged_addr_err)
+ tag = rand() & 0xff;
+
+ return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
+}
+
+static void *untag_ptr(void *ptr)
+{
+ return (void *)((unsigned long)ptr & ~TAG_MASK);
+}
+
+void *malloc(size_t size)
+{
+ return tag_ptr(__libc_malloc(size));
+}
+
+void free(void *ptr)
+{
+ __libc_free(untag_ptr(ptr));
+}
+
+void *realloc(void *ptr, size_t size)
+{
+ return tag_ptr(__libc_realloc(untag_ptr(ptr), size));
+}
+
+void *calloc(size_t nmemb, size_t size)
+{
+ return tag_ptr(__libc_calloc(nmemb, size));
+}
diff --git a/tools/testing/selftests/arm64/tags_test.c
b/tools/testing/selftests/arm64/tags_test.c
new file mode 100644
index ..263b302874ed
--- /dev/null
+++ b/tools/testing/selftests/arm64/tags_test.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include
+#include
+
+int main(void)
+{
+ struct utsname *ptr;
+ int err;
+
+ ptr = (struct utsname *)malloc(sizeof(*ptr));
+ err = uname(ptr);
+ free(ptr);
+ return err;
+}
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 13/15] tee/shm, arm64: untag user pointers in tee_shm_register
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. tee_shm_register()->optee_shm_unregister()->check_mem_type() uses provided user pointers for vma lookups (via __check_mem_type()), which can only by done with untagged pointers. Untag user pointers in this function. Reviewed-by: Kees Cook Acked-by: Jens Wiklander Signed-off-by: Andrey Konovalov --- drivers/tee/tee_shm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 2da026fd12c9..09ddcd06c715 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -254,6 +254,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, shm->teedev = teedev; shm->ctx = ctx; shm->id = -1; + addr = untagged_addr(addr); start = rounddown(addr, PAGE_SIZE); shm->offset = addr - start; shm->size = length; -- 2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 14/15] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
vaddr_get_pfn() uses provided user pointers for vma lookups, which can
only by done with untagged pointers.
Untag user pointers in this function.
Reviewed-by: Catalin Marinas
Reviewed-by: Kees Cook
Signed-off-by: Andrey Konovalov
---
drivers/vfio/vfio_iommu_type1.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 3ddc375e7063..528e39a1c2dd 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -384,6 +384,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
long vaddr,
down_read(>mmap_sem);
+ vaddr = untagged_addr(vaddr);
+
vma = find_vma_intersection(mm, vaddr, vaddr + 1);
if (vma && vma->vm_flags & VM_PFNMAP) {
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 08/15] userfaultfd, arm64: untag user pointers
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
userfaultfd code use provided user pointers for vma lookups, which can
only by done with untagged pointers.
Untag user pointers in validate_range().
Reviewed-by: Catalin Marinas
Reviewed-by: Kees Cook
Signed-off-by: Andrey Konovalov
---
fs/userfaultfd.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 3b30301c90ec..24d68c3b5ee2 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1263,21 +1263,23 @@ static __always_inline void wake_userfault(struct
userfaultfd_ctx *ctx,
}
static __always_inline int validate_range(struct mm_struct *mm,
- __u64 start, __u64 len)
+ __u64 *start, __u64 len)
{
__u64 task_size = mm->task_size;
- if (start & ~PAGE_MASK)
+ *start = untagged_addr(*start);
+
+ if (*start & ~PAGE_MASK)
return -EINVAL;
if (len & ~PAGE_MASK)
return -EINVAL;
if (!len)
return -EINVAL;
- if (start < mmap_min_addr)
+ if (*start < mmap_min_addr)
return -EINVAL;
- if (start >= task_size)
+ if (*start >= task_size)
return -EINVAL;
- if (len > task_size - start)
+ if (len > task_size - *start)
return -EINVAL;
return 0;
}
@@ -1327,7 +1329,7 @@ static int userfaultfd_register(struct userfaultfd_ctx
*ctx,
goto out;
}
- ret = validate_range(mm, uffdio_register.range.start,
+ ret = validate_range(mm, _register.range.start,
uffdio_register.range.len);
if (ret)
goto out;
@@ -1516,7 +1518,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx
*ctx,
if (copy_from_user(_unregister, buf, sizeof(uffdio_unregister)))
goto out;
- ret = validate_range(mm, uffdio_unregister.start,
+ ret = validate_range(mm, _unregister.start,
uffdio_unregister.len);
if (ret)
goto out;
@@ -1667,7 +1669,7 @@ static int userfaultfd_wake(struct userfaultfd_ctx *ctx,
if (copy_from_user(_wake, buf, sizeof(uffdio_wake)))
goto out;
- ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len);
+ ret = validate_range(ctx->mm, _wake.start, uffdio_wake.len);
if (ret)
goto out;
@@ -1707,7 +1709,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
sizeof(uffdio_copy)-sizeof(__s64)))
goto out;
- ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
+ ret = validate_range(ctx->mm, _copy.dst, uffdio_copy.len);
if (ret)
goto out;
/*
@@ -1763,7 +1765,7 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx
*ctx,
sizeof(uffdio_zeropage)-sizeof(__s64)))
goto out;
- ret = validate_range(ctx->mm, uffdio_zeropage.range.start,
+ ret = validate_range(ctx->mm, _zeropage.range.start,
uffdio_zeropage.range.len);
if (ret)
goto out;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 12/15] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
videobuf_dma_contig_user_get() uses provided user pointers for vma
lookups, which can only by done with untagged pointers.
Untag the pointers in this function.
Reviewed-by: Kees Cook
Acked-by: Mauro Carvalho Chehab
Signed-off-by: Andrey Konovalov
---
drivers/media/v4l2-core/videobuf-dma-contig.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c
b/drivers/media/v4l2-core/videobuf-dma-contig.c
index e1bf50df4c70..8a1ddd146b17 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -160,6 +160,7 @@ static void videobuf_dma_contig_user_put(struct
videobuf_dma_contig_memory *mem)
static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
struct videobuf_buffer *vb)
{
+ unsigned long untagged_baddr = untagged_addr(vb->baddr);
struct mm_struct *mm = current->mm;
struct vm_area_struct *vma;
unsigned long prev_pfn, this_pfn;
@@ -167,22 +168,22 @@ static int videobuf_dma_contig_user_get(struct
videobuf_dma_contig_memory *mem,
unsigned int offset;
int ret;
- offset = vb->baddr & ~PAGE_MASK;
+ offset = untagged_baddr & ~PAGE_MASK;
mem->size = PAGE_ALIGN(vb->size + offset);
ret = -EINVAL;
down_read(>mmap_sem);
- vma = find_vma(mm, vb->baddr);
+ vma = find_vma(mm, untagged_baddr);
if (!vma)
goto out_up;
- if ((vb->baddr + mem->size) > vma->vm_end)
+ if ((untagged_baddr + mem->size) > vma->vm_end)
goto out_up;
pages_done = 0;
prev_pfn = 0; /* kill warning */
- user_address = vb->baddr;
+ user_address = untagged_baddr;
while (pages_done < (mem->size >> PAGE_SHIFT)) {
ret = follow_pfn(vma, user_address, _pfn);
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 10/15] drm/radeon, arm64: untag user pointers in radeon_gem_userptr_ioctl
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. In radeon_gem_userptr_ioctl() an MMU notifier is set up with a (tagged) userspace pointer. The untagged address should be used so that MMU notifiers for the untagged address get correctly matched up with the right BO. This funcation also calls radeon_ttm_tt_pin_userptr(), which uses provided user pointers for vma lookups, which can only by done with untagged pointers. This patch untags user pointers in radeon_gem_userptr_ioctl(). Suggested-by: Felix Kuehling Acked-by: Felix Kuehling Signed-off-by: Andrey Konovalov --- drivers/gpu/drm/radeon/radeon_gem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 44617dec8183..90eb78fb5eb2 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -291,6 +291,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void *data, uint32_t handle; int r; + args->addr = untagged_addr(args->addr); + if (offset_in_page(args->addr | args->size)) return -EINVAL; -- 2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 11/15] IB/mlx4, arm64: untag user pointers in mlx4_get_umem_mr
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
mlx4_get_umem_mr() uses provided user pointers for vma lookups, which can
only by done with untagged pointers.
Untag user pointers in this function.
Signed-off-by: Andrey Konovalov
---
drivers/infiniband/hw/mlx4/mr.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/mlx4/mr.c b/drivers/infiniband/hw/mlx4/mr.c
index 355205a28544..13d9f917f249 100644
--- a/drivers/infiniband/hw/mlx4/mr.c
+++ b/drivers/infiniband/hw/mlx4/mr.c
@@ -378,6 +378,7 @@ static struct ib_umem *mlx4_get_umem_mr(struct ib_udata
*udata, u64 start,
* again
*/
if (!ib_access_writable(access_flags)) {
+ unsigned long untagged_start = untagged_addr(start);
struct vm_area_struct *vma;
down_read(>mm->mmap_sem);
@@ -386,9 +387,9 @@ static struct ib_umem *mlx4_get_umem_mr(struct ib_udata
*udata, u64 start,
* cover the memory, but for now it requires a single vma to
* entirely cover the MR to support RO mappings.
*/
- vma = find_vma(current->mm, start);
- if (vma && vma->vm_end >= start + length &&
- vma->vm_start <= start) {
+ vma = find_vma(current->mm, untagged_start);
+ if (vma && vma->vm_end >= untagged_start + length &&
+ vma->vm_start <= untagged_start) {
if (vma->vm_flags & VM_WRITE)
access_flags |= IB_ACCESS_LOCAL_WRITE;
} else {
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 00/15] arm64: untag user pointers passed to the kernel
=== Overview
arm64 has a feature called Top Byte Ignore, which allows to embed pointer
tags into the top byte of each pointer. Userspace programs (such as
HWASan, a memory debugging tool [1]) might use this feature and pass
tagged user pointers to the kernel through syscalls or other interfaces.
Right now the kernel is already able to handle user faults with tagged
pointers, due to these patches:
1. 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a
tagged pointer")
2. 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged
pointers")
3. 276e9327 ("arm64: entry: improve data abort handling of tagged
pointers")
This patchset extends tagged pointer support to syscall arguments.
As per the proposed ABI change [3], tagged pointers are only allowed to be
passed to syscalls when they point to memory ranges obtained by anonymous
mmap() or sbrk() (see the patchset [3] for more details).
For non-memory syscalls this is done by untaging user pointers when the
kernel performs pointer checking to find out whether the pointer comes
from userspace (most notably in access_ok). The untagging is done only
when the pointer is being checked, the tag is preserved as the pointer
makes its way through the kernel and stays tagged when the kernel
dereferences the pointer when perfoming user memory accesses.
The mmap and mremap (only new_addr) syscalls do not currently accept
tagged addresses. Architectures may interpret the tag as a background
colour for the corresponding vma.
Other memory syscalls (mprotect, etc.) don't do user memory accesses but
rather deal with memory ranges, and untagged pointers are better suited to
describe memory ranges internally. Thus for memory syscalls we untag
pointers completely when they enter the kernel.
=== Other approaches
One of the alternative approaches to untagging that was considered is to
completely strip the pointer tag as the pointer enters the kernel with
some kind of a syscall wrapper, but that won't work with the countless
number of different ioctl calls. With this approach we would need a custom
wrapper for each ioctl variation, which doesn't seem practical.
An alternative approach to untagging pointers in memory syscalls prologues
is to inspead allow tagged pointers to be passed to find_vma() (and other
vma related functions) and untag them there. Unfortunately, a lot of
find_vma() callers then compare or subtract the returned vma start and end
fields against the pointer that was being searched. Thus this approach
would still require changing all find_vma() callers.
=== Testing
The following testing approaches has been taken to find potential issues
with user pointer untagging:
1. Static testing (with sparse [2] and separately with a custom static
analyzer based on Clang) to track casts of __user pointers to integer
types to find places where untagging needs to be done.
2. Static testing with grep to find parts of the kernel that call
find_vma() (and other similar functions) or directly compare against
vm_start/vm_end fields of vma.
3. Static testing with grep to find parts of the kernel that compare
user pointers with TASK_SIZE or other similar consts and macros.
4. Dynamic testing: adding BUG_ON(has_tag(addr)) to find_vma() and running
a modified syzkaller version that passes tagged pointers to the kernel.
Based on the results of the testing the requried patches have been added
to the patchset.
=== Notes
This patchset is meant to be merged together with "arm64 relaxed ABI" [3].
This patchset is a prerequisite for ARM's memory tagging hardware feature
support [4].
This patchset has been merged into the Pixel 2 & 3 kernel trees and is
now being used to enable testing of Pixel phones with HWASan.
Thanks!
[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
[2]
https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292
[3] https://lkml.org/lkml/2019/3/18/819
[4]
https://community.arm.com/processors/b/blog/posts/arm-a-profile-architecture-2018-developments-armv85a
=== History
Changes in v17:
- The "uaccess: add noop untagged_addr definition" patch is dropped, as it
was merged into upstream named as "uaccess: add noop untagged_addr
definition".
- Merged "mm, arm64: untag user pointers in do_pages_move" into
"mm, arm64: untag user pointers passed to memory syscalls".
- Added "arm64: Introduce prctl() options to control the tagged user
addresses ABI" patch from Catalin.
- Add tags_lib.so to tools/testing/selftests/arm64/.gitignore.
- Added a comment clarifying untagged in mremap.
- Moved untagging back into mlx4_get_umem_mr() for the IB patch.
Changes in v16:
- Moved untagging for memory syscalls from arm64 wrappers back to generic
code.
- Dropped untagging for the following memory syscalls: brk, mmap, munmap;
mremap (only dropped for new_address); mmap_pgoff (not used on arm64);
remap_file_pages
[PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Catalin Marinas
It is not desirable to relax the ABI to allow tagged user addresses into
the kernel indiscriminately. This patch introduces a prctl() interface
for enabling or disabling the tagged ABI with a global sysctl control
for preventing applications from enabling the relaxed ABI (meant for
testing user-space prctl() return error checking without reconfiguring
the kernel). The ABI properties are inherited by threads of the same
application and fork()'ed children but cleared on execve().
The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
MTE-specific settings like imprecise vs precise exceptions.
Signed-off-by: Catalin Marinas
---
arch/arm64/include/asm/processor.h | 6 +++
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/include/asm/uaccess.h | 3 +-
arch/arm64/kernel/process.c | 67
include/uapi/linux/prctl.h | 5 +++
kernel/sys.c | 16 +++
6 files changed, 97 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/processor.h
b/arch/arm64/include/asm/processor.h
index fcd0e691b1ea..fee457456aa8 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void);
/* PR_PAC_RESET_KEYS prctl */
#define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
+/* PR_TAGGED_ADDR prctl */
+long set_tagged_addr_ctrl(unsigned long arg);
+long get_tagged_addr_ctrl(void);
+#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg)
+#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
+
/*
* For CONFIG_GCC_PLUGIN_STACKLEAK
*
diff --git a/arch/arm64/include/asm/thread_info.h
b/arch/arm64/include/asm/thread_info.h
index f1d032be628a..354a31d2b737 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -99,6 +99,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#define TIF_SVE23 /* Scalable Vector Extension in
use */
#define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
#define TIF_SSBD 25 /* Wants SSB mitigation */
+#define TIF_TAGGED_ADDR26 /* Allow tagged user addresses
*/
#define _TIF_SIGPENDING(1 << TIF_SIGPENDING)
#define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index df729afca0ba..995b9ea11a89 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -73,7 +73,8 @@ static inline unsigned long __range_ok(const void __user
*addr, unsigned long si
{
unsigned long ret, limit = current_thread_info()->addr_limit;
- addr = untagged_addr(addr);
+ if (test_thread_flag(TIF_TAGGED_ADDR))
+ addr = untagged_addr(addr);
__chk_user_ptr(addr);
asm volatile(
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3767fb21a5b8..69d0be1fc708 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -323,6 +324,7 @@ void flush_thread(void)
fpsimd_flush_thread();
tls_thread_flush();
flush_ptrace_hw_breakpoint(current);
+ clear_thread_flag(TIF_TAGGED_ADDR);
}
void release_thread(struct task_struct *dead_task)
@@ -552,3 +554,68 @@ void arch_setup_new_exec(void)
ptrauth_thread_init_user(current);
}
+
+/*
+ * Control the relaxed ABI allowing tagged user addresses into the kernel.
+ */
+static unsigned int tagged_addr_prctl_allowed = 1;
+
+long set_tagged_addr_ctrl(unsigned long arg)
+{
+ if (!tagged_addr_prctl_allowed)
+ return -EINVAL;
+ if (is_compat_task())
+ return -EINVAL;
+ if (arg & ~PR_TAGGED_ADDR_ENABLE)
+ return -EINVAL;
+
+ if (arg & PR_TAGGED_ADDR_ENABLE)
+ set_thread_flag(TIF_TAGGED_ADDR);
+ else
+ clear_thread_flag(TIF_TAGGED_ADDR);
+
+ return 0;
+}
+
+long get_tagged_addr_ctrl(void)
+{
+ if (!tagged_addr_prctl_allowed)
+ return -EINVAL;
+ if (is_compat_task())
+ return -EINVAL;
+
+ if (test_thread_flag(TIF_TAGGED_ADDR))
+ return PR_TAGGED_ADDR_ENABLE;
+
+ return 0;
+}
+
+/*
+ * Global sysctl to disable the tagged user addresses support. This control
+ * only prevents the tagged address ABI enabling via prctl() and does not
+ * disable it for tasks that already opted in to the relaxed ABI.
+ */
+static int zero;
+static int one = 1;
+
+static struct ctl_table tagged_addr_sysctl_table[] = {
+ {
+ .procname = "tagged_addr",
+ .mode = 0644,
+ .data = _addr_prctl_allowed,
+ .maxlen =
[PATCH v17 02/15] lib, arm64: untag user pointers in strn*_user
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to handle the case of tagged user addresses separately.
Untag user pointers passed to these functions.
Note, that this patch only temporarily untags the pointers to perform
validity checks, but then uses them as is to perform user memory accesses.
Reviewed-by: Khalid Aziz
Acked-by: Kees Cook
Reviewed-by: Catalin Marinas
Signed-off-by: Andrey Konovalov
---
lib/strncpy_from_user.c | 3 ++-
lib/strnlen_user.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 023ba9f3b99f..dccb95af6003 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
@@ -108,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src,
long count)
return 0;
max_addr = user_addr_max();
- src_addr = (unsigned long)src;
+ src_addr = (unsigned long)untagged_addr(src);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 7f2db3fe311f..28ff554a1be8 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -2,6 +2,7 @@
#include
#include
#include
+#include
#include
@@ -109,7 +110,7 @@ long strnlen_user(const char __user *str, long count)
return 0;
max_addr = user_addr_max();
- src_addr = (unsigned long)str;
+ src_addr = (unsigned long)untagged_addr(str);
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 06/15] mm, arm64: untag user pointers in get_vaddr_frames
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. get_vaddr_frames uses provided user pointers for vma lookups, which can only by done with untagged pointers. Instead of locating and changing all callers of this function, perform untagging in it. Acked-by: Catalin Marinas Reviewed-by: Kees Cook Signed-off-by: Andrey Konovalov --- mm/frame_vector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm/frame_vector.c b/mm/frame_vector.c index c64dca6e27c2..c431ca81dad5 100644 --- a/mm/frame_vector.c +++ b/mm/frame_vector.c @@ -46,6 +46,8 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, if (WARN_ON_ONCE(nr_frames > vec->nr_allocated)) nr_frames = vec->nr_allocated; + start = untagged_addr(start); + down_read(>mmap_sem); locked = 1; vma = find_vma_intersection(mm, start, start + 1); -- 2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 05/15] mm, arm64: untag user pointers in mm/gup.c
This patch is a part of a series that extends arm64 kernel ABI to allow to pass tagged user pointers (with the top byte set to something else other than 0x00) as syscall arguments. mm/gup.c provides a kernel interface that accepts user addresses and manipulates user pages directly (for example get_user_pages, that is used by the futex syscall). Since a user can provided tagged addresses, we need to handle this case. Add untagging to gup.c functions that use user addresses for vma lookups. Reviewed-by: Kees Cook Reviewed-by: Catalin Marinas Signed-off-by: Andrey Konovalov --- mm/gup.c | 4 1 file changed, 4 insertions(+) diff --git a/mm/gup.c b/mm/gup.c index ddde097cf9e4..c37df3d455a2 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -802,6 +802,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, if (!nr_pages) return 0; + start = untagged_addr(start); + VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); /* @@ -964,6 +966,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, struct vm_area_struct *vma; vm_fault_t ret, major = 0; + address = untagged_addr(address); + if (unlocked) fault_flags |= FAULT_FLAG_ALLOW_RETRY; -- 2.22.0.rc2.383.gf4fbbf30c2-goog
[PATCH v17 01/15] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
This patch is a part of a series that extends arm64 kernel ABI to allow to
pass tagged user pointers (with the top byte set to something else other
than 0x00) as syscall arguments.
copy_from_user (and a few other similar functions) are used to copy data
from user memory into the kernel memory or vice versa. Since a user can
provided a tagged pointer to one of the syscalls that use copy_from_user,
we need to correctly handle such pointers.
Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr,
before performing access validity checks.
Note, that this patch only temporarily untags the pointers to perform the
checks, but then passes them as is into the kernel internals.
Reviewed-by: Kees Cook
Reviewed-by: Catalin Marinas
Signed-off-by: Andrey Konovalov
---
arch/arm64/include/asm/uaccess.h | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index e5d5f31c6d36..df729afca0ba 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -73,6 +73,8 @@ static inline unsigned long __range_ok(const void __user
*addr, unsigned long si
{
unsigned long ret, limit = current_thread_info()->addr_limit;
+ addr = untagged_addr(addr);
+
__chk_user_ptr(addr);
asm volatile(
// A + B <= C + 1 for all A,B,C, in four easy steps:
@@ -226,7 +228,8 @@ static inline void uaccess_enable_not_uao(void)
/*
* Sanitise a uaccess pointer such that it becomes NULL if above the
- * current addr_limit.
+ * current addr_limit. In case the pointer is tagged (has the top byte set),
+ * untag the pointer before checking.
*/
#define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
@@ -234,10 +237,11 @@ static inline void __user *__uaccess_mask_ptr(const void
__user *ptr)
void __user *safe_ptr;
asm volatile(
- " bicsxzr, %1, %2\n"
+ " bicsxzr, %3, %2\n"
" csel%0, %1, xzr, eq\n"
: "=" (safe_ptr)
- : "r" (ptr), "r" (current_thread_info()->addr_limit)
+ : "r" (ptr), "r" (current_thread_info()->addr_limit),
+ "r" (untagged_addr(ptr))
: "cc");
csdb();
--
2.22.0.rc2.383.gf4fbbf30c2-goog
Re: [PATCH v16 08/16] fs, arm64: untag user pointers in copy_mount_options
On Tue, Jun 11, 2019 at 4:38 PM Andrey Konovalov wrote: > > On Sat, Jun 8, 2019 at 6:02 AM Kees Cook wrote: > > > > On Mon, Jun 03, 2019 at 06:55:10PM +0200, Andrey Konovalov wrote: > > > This patch is a part of a series that extends arm64 kernel ABI to allow to > > > pass tagged user pointers (with the top byte set to something else other > > > than 0x00) as syscall arguments. > > > > > > In copy_mount_options a user address is being subtracted from TASK_SIZE. > > > If the address is lower than TASK_SIZE, the size is calculated to not > > > allow the exact_copy_from_user() call to cross TASK_SIZE boundary. > > > However if the address is tagged, then the size will be calculated > > > incorrectly. > > > > > > Untag the address before subtracting. > > > > > > Reviewed-by: Catalin Marinas > > > Signed-off-by: Andrey Konovalov > > > > One thing I just noticed in the commit titles... "arm64" is in the > > prefix, but these are arch-indep areas. Should the ", arm64" be left > > out? > > > > I would expect, instead: > > > > fs/namespace: untag user pointers in copy_mount_options > > Hm, I've added the arm64 tag in all of the patches because they are > related to changes in arm64 kernel ABI. I can remove it from all the > patches that only touch common code if you think that it makes sense. I'll keep the arm64 tags in commit titles for v17. Please reply explicitly if you think I should remove them. Thanks! :) > > Thanks! > > > > > Reviewed-by: Kees Cook > > > > -Kees > > > > > --- > > > fs/namespace.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/namespace.c b/fs/namespace.c > > > index b26778bdc236..2e85712a19ed 100644 > > > --- a/fs/namespace.c > > > +++ b/fs/namespace.c > > > @@ -2993,7 +2993,7 @@ void *copy_mount_options(const void __user * data) > > >* the remainder of the page. > > >*/ > > > /* copy_from_user cannot cross TASK_SIZE ! */ > > > - size = TASK_SIZE - (unsigned long)data; > > > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > > > if (size > PAGE_SIZE) > > > size = PAGE_SIZE; > > > > > > -- > > > 2.22.0.rc1.311.g5d7573a151-goog > > > > > > > -- > > Kees Cook
Re: [PATCH v16 16/16] selftests, arm64: add a selftest for passing tagged pointers to kernel
On Tue, Jun 11, 2019 at 7:50 PM Catalin Marinas wrote:
>
> On Tue, Jun 11, 2019 at 07:18:04PM +0200, Andrey Konovalov wrote:
> > On Tue, Jun 11, 2019 at 5:01 PM Catalin Marinas
> > wrote:
> > > static void *tag_ptr(void *ptr)
> > > {
> > > static int tagged_addr_err = 1;
> > > unsigned long tag = 0;
> > >
> > > if (tagged_addr_err == 1)
> > > tagged_addr_err = prctl(PR_SET_TAGGED_ADDR_CTRL,
> > > PR_TAGGED_ADDR_ENABLE, 0, 0, 0);
> >
> > I think this requires atomics. malloc() can be called from multiple threads.
>
> It's slightly racy but I assume in a real libc it can be initialised
> earlier than the hook calls while still in single-threaded mode (I had
> a quick attempt with __attribute__((constructor)) but didn't get far).
>
> Even with the race, under normal circumstances calling the prctl() twice
> is not a problem. I think the risk here is that someone disables the ABI
> via sysctl and the ABI is enabled for some of the threads only.
OK, I'll keep the code racy, but add a comment pointing it out. Thanks!
>
> --
> Catalin
Re: [PATCH v16 05/16] arm64: untag user pointers passed to memory syscalls
On Tue, Jun 11, 2019 at 7:45 PM Catalin Marinas wrote: > > On Tue, Jun 11, 2019 at 05:35:31PM +0200, Andrey Konovalov wrote: > > On Mon, Jun 10, 2019 at 4:28 PM Catalin Marinas > > wrote: > > > On Mon, Jun 03, 2019 at 06:55:07PM +0200, Andrey Konovalov wrote: > > > > This patch is a part of a series that extends arm64 kernel ABI to allow > > > > to > > > > pass tagged user pointers (with the top byte set to something else other > > > > than 0x00) as syscall arguments. > > > > > > > > This patch allows tagged pointers to be passed to the following memory > > > > syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, > > > > mprotect, > > > > mremap, msync, munlock. > > > > > > > > Signed-off-by: Andrey Konovalov > > > > > > I would add in the commit log (and possibly in the code with a comment) > > > that mremap() and mmap() do not currently accept tagged hint addresses. > > > Architectures may interpret the hint tag as a background colour for the > > > corresponding vma. With this: > > > > I'll change the commit log. Where do you you think I should put this > > comment? Before mmap and mremap definitions in mm/? > > On arm64 we use our own sys_mmap(). I'd say just add a comment on the > generic mremap() just before the untagged_addr() along the lines that > new_address is not untagged for preserving similar behaviour to mmap(). Will do in v17, thanks! > > -- > Catalin
Re: [PATCH v16 04/16] mm: untag user pointers in do_pages_move
On Tue, Jun 11, 2019 at 10:18 PM Khalid Aziz wrote: > > On 6/3/19 10:55 AM, Andrey Konovalov wrote: > > This patch is a part of a series that extends arm64 kernel ABI to allow to > > pass tagged user pointers (with the top byte set to something else other > > than 0x00) as syscall arguments. > > > > do_pages_move() is used in the implementation of the move_pages syscall. > > > > Untag user pointers in this function. > > > > Reviewed-by: Catalin Marinas > > Signed-off-by: Andrey Konovalov > > --- > > mm/migrate.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/mm/migrate.c b/mm/migrate.c > > index f2ecc2855a12..3930bb6fa656 100644 > > --- a/mm/migrate.c > > +++ b/mm/migrate.c > > @@ -1617,6 +1617,7 @@ static int do_pages_move(struct mm_struct *mm, > > nodemask_t task_nodes, > > if (get_user(node, nodes + i)) > > goto out_flush; > > addr = (unsigned long)p; > > + addr = untagged_addr(addr); > > Why not just "addr = (unsigned long)untagged_addr(p);" Will do in the next version. I think I'll also merge this commit into the "untag user pointers passed to memory syscalls" one. > > -- > Khalid >
[PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v3)
(v2): Return 0 and set mem->mm_node to NULL.
(v3): Use atomic64_add_return instead.
Signed-off-by: Tom St Denis
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 8aea2f21b202..c963ad86072e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -276,7 +276,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
struct drm_mm_node *nodes;
enum drm_mm_insert_mode mode;
unsigned long lpfn, num_nodes, pages_per_node, pages_left;
- uint64_t usage = 0, vis_usage = 0;
+ uint64_t vis_usage = 0;
unsigned i;
int r;
@@ -284,6 +284,13 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (!lpfn)
lpfn = man->size;
+ /* bail out quickly if there's likely not enough VRAM for this BO */
+ if (atomic64_add_return(mem->num_pages << PAGE_SHIFT, >usage) >
adev->gmc.mc_vram_size) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
+ mem->mm_node = NULL;
+ return 0;
+ }
+
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
pages_per_node = ~0ul;
num_nodes = 1;
@@ -300,8 +307,10 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
GFP_KERNEL | __GFP_ZERO);
- if (!nodes)
+ if (!nodes) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
return -ENOMEM;
+ }
mode = DRM_MM_INSERT_BEST;
if (place->flags & TTM_PL_FLAG_TOPDOWN)
@@ -321,7 +330,6 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (unlikely(r))
break;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
@@ -341,14 +349,12 @@ static int amdgpu_vram_mgr_new(struct
ttm_mem_type_manager *man,
if (unlikely(r))
goto error;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
}
spin_unlock(>lock);
- atomic64_add(usage, >usage);
atomic64_add(vis_usage, >vis_usage);
mem->mm_node = nodes;
@@ -359,6 +365,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
while (i--)
drm_mm_remove_node([i]);
spin_unlock(>lock);
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
kvfree(nodes);
return r == -ENOSPC ? 0 : r;
--
2.21.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Wed, Jun 12, 2019 at 01:03:10PM +0200, Andrey Konovalov wrote: > On Tue, Jun 11, 2019 at 7:39 PM Catalin Marinas > wrote: > > On Tue, Jun 11, 2019 at 07:09:46PM +0200, Andrey Konovalov wrote: > > > Should I drop access_ok() change from my patch, since yours just reverts > > > it? > > > > Not necessary, your patch just relaxes the ABI for all apps, mine > > tightens it. You could instead move the untagging to __range_ok() and > > rebase my patch accordingly. > > OK, will do. I'll also add a comment next to TIF_TAGGED_ADDR as Vincenzo > asked. Thanks. -- Catalin
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Tue, Jun 11, 2019 at 7:39 PM Catalin Marinas wrote: > > On Tue, Jun 11, 2019 at 07:09:46PM +0200, Andrey Konovalov wrote: > > On Tue, Jun 11, 2019 at 4:57 PM Catalin Marinas > > wrote: > > > > > > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > > > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > > > > diff --git a/arch/arm64/include/asm/uaccess.h > > > > > b/arch/arm64/include/asm/uaccess.h > > > > > index e5d5f31c6d36..9164ecb5feca 100644 > > > > > --- a/arch/arm64/include/asm/uaccess.h > > > > > +++ b/arch/arm64/include/asm/uaccess.h > > > > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void > > > > > __user *addr, unsigned long si > > > > > return ret; > > > > > } > > > > > > > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > > > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), > > > > > size) > > > > > > > > I'm going to propose an opt-in method here (RFC for now). We can't have > > > > a check in untagged_addr() since this is already used throughout the > > > > kernel for both user and kernel addresses (khwasan) but we can add one > > > > in __range_ok(). The same prctl() option will be used for controlling > > > > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > > > > assuming that this will be called early on and any cloned thread will > > > > inherit this. > > > > > > Updated patch, inlining it below. Once we agreed on the approach, I > > > think Andrey can insert in in this series, probably after patch 2. The > > > differences from the one I posted yesterday: > > > > > > - renamed PR_* macros together with get/set variants and the possibility > > > to disable the relaxed ABI > > > > > > - sysctl option - /proc/sys/abi/tagged_addr to disable the ABI globally > > > (just the prctl() opt-in, tasks already using it won't be affected) > > > > > > And, of course, it needs more testing. > > > > Sure, I'll add it to the series. > > > > Should I drop access_ok() change from my patch, since yours just reverts it? > > Not necessary, your patch just relaxes the ABI for all apps, mine > tightens it. You could instead move the untagging to __range_ok() and > rebase my patch accordingly. OK, will do. I'll also add a comment next to TIF_TAGGED_ADDR as Vincenzo asked. > > -- > Catalin
Re: [PATCH v16 12/16] IB, arm64: untag user pointers in ib_uverbs_(re)reg_mr()
On Tue, Jun 04, 2019 at 03:09:26PM +0200, Andrey Konovalov wrote: > On Tue, Jun 4, 2019 at 3:02 PM Jason Gunthorpe wrote: > > On Tue, Jun 04, 2019 at 02:45:32PM +0200, Andrey Konovalov wrote: > > > On Tue, Jun 4, 2019 at 2:27 PM Jason Gunthorpe wrote: > > > > On Tue, Jun 04, 2019 at 02:18:19PM +0200, Andrey Konovalov wrote: > > > > > On Mon, Jun 3, 2019 at 7:46 PM Jason Gunthorpe wrote: > > > > > > On Mon, Jun 03, 2019 at 06:55:14PM +0200, Andrey Konovalov wrote: > > > > > > > This patch is a part of a series that extends arm64 kernel ABI to > > > > > > > allow to > > > > > > > pass tagged user pointers (with the top byte set to something > > > > > > > else other > > > > > > > than 0x00) as syscall arguments. > > > > > > > > > > > > > > ib_uverbs_(re)reg_mr() use provided user pointers for vma lookups > > > > > > > (through > > > > > > > e.g. mlx4_get_umem_mr()), which can only by done with untagged > > > > > > > pointers. > > > > > > > > > > > > > > Untag user pointers in these functions. > > > > > > > > > > > > > > Signed-off-by: Andrey Konovalov > > > > > > > drivers/infiniband/core/uverbs_cmd.c | 4 > > > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c > > > > > > > b/drivers/infiniband/core/uverbs_cmd.c > > > > > > > index 5a3a1780ceea..f88ee733e617 100644 > > > > > > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > > > > > > @@ -709,6 +709,8 @@ static int ib_uverbs_reg_mr(struct > > > > > > > uverbs_attr_bundle *attrs) > > > > > > > if (ret) > > > > > > > return ret; > > > > > > > > > > > > > > + cmd.start = untagged_addr(cmd.start); > > > > > > > + > > > > > > > if ((cmd.start & ~PAGE_MASK) != (cmd.hca_va & ~PAGE_MASK)) > > > > > > > return -EINVAL; > > > > > > > > > > > > I feel like we shouldn't thave to do this here, surely the cmd.start > > > > > > should flow unmodified to get_user_pages, and gup should untag it? > > > > > > > > > > > > ie, this sort of direction for the IB code (this would be a giant > > > > > > patch, so I didn't have time to write it all, but I think it is much > > > > > > saner): > > > > > > > > > > ib_uverbs_reg_mr() passes cmd.start to mlx4_get_umem_mr(), which calls > > > > > find_vma(), which only accepts untagged addresses. Could you explain > > > > > how your patch helps? > > > > > > > > That mlx4 is just a 'weird duck', it is not the normal flow, and I > > > > don't think the core code should be making special consideration for > > > > it. > > > > > > How do you think we should do untagging (or something else) to deal > > > with this 'weird duck' case? > > > > mlx4 should handle it around the call to find_vma like other patches > > do, ideally as part of the cast from a void __user * to the unsigned > > long that find_vma needs > > So essentially what we had a few versions ago > (https://lkml.org/lkml/2019/4/30/785) plus changing unsigned longs to > __user * across all IB code? I think the second part is something > that's not related to this series and needs to be done separately. I > can move untagging back to mlx4_get_umem_mr() though. > > Catalin, you've initially asked to to move untagging out of > mlx4_get_umem_mr(), do you have any comments on this? It's fine by me either way. My original reasoning was to untag this at the higher level as tags may not be relevant to the mlx4 code. If that's what Jason prefers, go for it. -- Catalin
Re: [PATCH v16 09/16] fs, arm64: untag user pointers in fs/userfaultfd.c
On Mon, Jun 03, 2019 at 06:55:11PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > userfaultfd code use provided user pointers for vma lookups, which can > only by done with untagged pointers. > > Untag user pointers in validate_range(). > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v16 15/16] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
On Mon, Jun 03, 2019 at 06:55:17PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > vaddr_get_pfn() uses provided user pointers for vma lookups, which can > only by done with untagged pointers. > > Untag user pointers in this function. > > Signed-off-by: Andrey Konovalov Reviewed-by: Catalin Marinas
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
Hi Vincenzo,
On Tue, Jun 11, 2019 at 06:09:10PM +0100, Vincenzo Frascino wrote:
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 3767fb21a5b8..69d0be1fc708 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -30,6 +30,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -323,6 +324,7 @@ void flush_thread(void)
> > fpsimd_flush_thread();
> > tls_thread_flush();
> > flush_ptrace_hw_breakpoint(current);
> > + clear_thread_flag(TIF_TAGGED_ADDR);
>
> Nit: in line we the other functions in thread_flush we could have something
> like
> "tagged_addr_thread_flush", maybe inlined.
The other functions do a lot more than clearing a TIF flag, so they
deserved their own place. We could do this when adding MTE support. I
think we also need to check what other TIF flags we may inadvertently
pass on execve(), maybe have a mask clearing.
> > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> > index 094bb03b9cc2..2e927b3e9d6c 100644
> > --- a/include/uapi/linux/prctl.h
> > +++ b/include/uapi/linux/prctl.h
> > @@ -229,4 +229,9 @@ struct prctl_mm_map {
> > # define PR_PAC_APDBKEY(1UL << 3)
> > # define PR_PAC_APGAKEY(1UL << 4)
> >
> > +/* Tagged user address controls for arm64 */
> > +#define PR_SET_TAGGED_ADDR_CTRL55
> > +#define PR_GET_TAGGED_ADDR_CTRL56
> > +# define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> > +
> > #endif /* _LINUX_PRCTL_H */
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 2969304c29fe..ec48396b4943 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -124,6 +124,12 @@
> > #ifndef PAC_RESET_KEYS
> > # define PAC_RESET_KEYS(a, b) (-EINVAL)
> > #endif
> > +#ifndef SET_TAGGED_ADDR_CTRL
> > +# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL)
> > +#endif
> > +#ifndef GET_TAGGED_ADDR_CTRL
> > +# define GET_TAGGED_ADDR_CTRL()(-EINVAL)
> > +#endif
> >
> > /*
> > * this is where the system-wide overflow UID and GID are defined, for
> > @@ -2492,6 +2498,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> > arg2, unsigned long, arg3,
> > return -EINVAL;
> > error = PAC_RESET_KEYS(me, arg2);
> > break;
> > + case PR_SET_TAGGED_ADDR_CTRL:
> > + if (arg3 || arg4 || arg5)
> > + return -EINVAL;
> > + error = SET_TAGGED_ADDR_CTRL(arg2);
> > + break;
> > + case PR_GET_TAGGED_ADDR_CTRL:
> > + if (arg2 || arg3 || arg4 || arg5)
> > + return -EINVAL;
> > + error = GET_TAGGED_ADDR_CTRL();
> > + break;
>
> Why do we need two prctl here? We could have only one and use arg2 as set/get
> and arg3 as a parameter. What do you think?
This follows the other PR_* options, e.g. PR_SET_VL/GET_VL,
PR_*_FP_MODE. We will use other bits in arg2, for example to set the
precise vs imprecise MTE trapping.
--
Catalin
Re: [PATCH] drm/amdgpu: explicitly set mmGDS_VMID0_BASE to 0
Am 10.06.19 um 19:26 schrieb Zhu, James:
Explicitly set mmGDS_VMID0_BASE to 0. Also update
GDS_VMID0_BASE/_SIZE with direct register writes.
Signed-off-by: James Zhu
---
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 33 +++--
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ba36a28..78c79e9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -305,6 +305,7 @@ static int gfx_v9_0_get_cu_info(struct amdgpu_device *adev,
static uint64_t gfx_v9_0_get_gpu_clock_counter(struct amdgpu_device *adev);
static void gfx_v9_0_select_se_sh(struct amdgpu_device *adev, u32 se_num, u32
sh_num, u32 instance);
static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring);
+static u64 gfx_v9_0_ring_get_rptr_compute(struct amdgpu_ring *ring);
static void gfx_v9_0_init_golden_registers(struct amdgpu_device *adev)
{
@@ -3630,25 +3631,20 @@ static const struct soc15_reg_entry
sec_ded_counter_registers[] = {
{ SOC15_REG_ENTRY(GC, 0, mmSQC_EDC_CNT3), 0, 4, 6},
};
-
static int gfx_v9_0_do_edc_gds_workarounds(struct amdgpu_device *adev)
{
struct amdgpu_ring *ring = >gfx.compute_ring[0];
- int r;
+ int i, r;
- r = amdgpu_ring_alloc(ring, 17);
+ r = amdgpu_ring_alloc(ring, 7);
if (r) {
DRM_ERROR("amdgpu: GDS workarounds failed to lock ring %s
(%d).\n",
ring->name, r);
return r;
}
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, adev->gds.gds_size);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_BASE, 0x);
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, adev->gds.gds_size);
amdgpu_ring_write(ring, PACKET3(PACKET3_DMA_DATA, 5));
amdgpu_ring_write(ring, (PACKET3_DMA_DATA_CP_SYNC |
@@ -3662,18 +3658,19 @@ static int gfx_v9_0_do_edc_gds_workarounds(struct
amdgpu_device *adev)
amdgpu_ring_write(ring, PACKET3_DMA_DATA_CMD_RAW_WAIT |
adev->gds.gds_size);
- amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
- amdgpu_ring_write(ring, WRITE_DATA_ENGINE_SEL(0) |
- WRITE_DATA_DST_SEL(0));
- amdgpu_ring_write(ring, SOC15_REG_OFFSET(GC, 0, mmGDS_VMID0_SIZE));
- amdgpu_ring_write(ring, 0);
- amdgpu_ring_write(ring, 0x0);
-
amdgpu_ring_commit(ring);
- return 0;
-}
+ for (i = 0; (i < adev->usec_timeout) &&
+ (ring->wptr != gfx_v9_0_ring_get_rptr_compute(ring));
i++)
The indentation here looks wrong on first glance and you don't need the
extra ().
Might be better to write this as for + if..break.
+ DRM_UDELAY(1);
+
+ if (i >= adev->usec_timeout)
+ r = -ETIMEDOUT;
+
+ WREG32_SOC15(GC, 0, mmGDS_VMID0_SIZE, 0x);
+ return r;
+}
static int gfx_v9_0_do_edc_gpr_workarounds(struct amdgpu_device *adev)
{
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v2)
Am 12.06.19 um 09:26 schrieb Michel Dänzer:
On 2019-06-11 6:54 p.m., StDenis, Tom wrote:
(v2): Return 0 and set mem->mm_node to NULL.
Signed-off-by: Tom St Denis
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
[...]
@@ -284,6 +284,14 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man,
if (!lpfn)
lpfn = man->size;
+ /* bail out quickly if there's likely not enough VRAM for this BO */
+ atomic64_add(mem->num_pages << PAGE_SHIFT, >usage);
+ if (atomic64_read(>usage) > adev->gmc.mc_vram_size) {
Should probably use atomic64_add_return instead of atomic64_add +
atomic64_read.
Also, AFAICT this doesn't allow any VRAM overcommit, which seems a bit
conservative?
This is the low level VRAM manager. Here we can't overcomit anyway and
need to return early for TTM to evict things.
Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v2)
Am 11.06.19 um 18:54 schrieb StDenis, Tom:
(v2): Return 0 and set mem->mm_node to NULL.
Signed-off-by: Tom St Denis
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 8aea2f21b202..c2730e5c1081 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -276,7 +276,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
struct drm_mm_node *nodes;
enum drm_mm_insert_mode mode;
unsigned long lpfn, num_nodes, pages_per_node, pages_left;
- uint64_t usage = 0, vis_usage = 0;
+ uint64_t vis_usage = 0;
unsigned i;
int r;
@@ -284,6 +284,14 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man,
if (!lpfn)
lpfn = man->size;
+ /* bail out quickly if there's likely not enough VRAM for this BO */
+ atomic64_add(mem->num_pages << PAGE_SHIFT, >usage);
+ if (atomic64_read(>usage) > adev->gmc.mc_vram_size) {
You should use atomic64_add_return here instead of two operations.
Otherwise using an atomic doesn't make to much sense.
Christian.
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
+ mem->mm_node = NULL;
+ return 0;
+ }
+
if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
pages_per_node = ~0ul;
num_nodes = 1;
@@ -300,8 +308,10 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
GFP_KERNEL | __GFP_ZERO);
- if (!nodes)
+ if (!nodes) {
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
return -ENOMEM;
+ }
mode = DRM_MM_INSERT_BEST;
if (place->flags & TTM_PL_FLAG_TOPDOWN)
@@ -321,7 +331,6 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
if (unlikely(r))
break;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
@@ -341,14 +350,12 @@ static int amdgpu_vram_mgr_new(struct
ttm_mem_type_manager *man,
if (unlikely(r))
goto error;
- usage += nodes[i].size << PAGE_SHIFT;
vis_usage += amdgpu_vram_mgr_vis_size(adev, [i]);
amdgpu_vram_mgr_virt_start(mem, [i]);
pages_left -= pages;
}
spin_unlock(>lock);
- atomic64_add(usage, >usage);
atomic64_add(vis_usage, >vis_usage);
mem->mm_node = nodes;
@@ -359,6 +366,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager
*man,
while (i--)
drm_mm_remove_node([i]);
spin_unlock(>lock);
+ atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
kvfree(nodes);
return r == -ENOSPC ? 0 : r;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Reserve space for shared fence
Am 11.06.19 um 18:20 schrieb Zeng, Oak: Call reservation_object_reserve_shared to reserve space for shared fence. Otherwise it will trigger BUG_ON condition in reservation_object_add_shared_fence. Change-Id: Ib0fae16247e33ee68f95bffa723451c0cd619344 Signed-off-by: Oak Zeng Acked-by: Christian König --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 81e0e75..74e8695 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -2152,12 +2152,16 @@ int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem **mem * Add process eviction fence to bo so they can * evict each other. */ + ret = reservation_object_reserve_shared(gws_bo->tbo.resv, 1); + if (ret) + goto reserve_shared_fail; amdgpu_bo_fence(gws_bo, _info->eviction_fence->base, true); amdgpu_bo_unreserve(gws_bo); mutex_unlock(&(*mem)->process_info->lock); return ret; +reserve_shared_fail: bo_validation_failure: amdgpu_bo_unreserve(gws_bo); bo_reservation_failure: ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/amdgpu: Bail out of BO node creation if not enough VRAM (v2)
On 2019-06-11 6:54 p.m., StDenis, Tom wrote:
> (v2): Return 0 and set mem->mm_node to NULL.
>
> Signed-off-by: Tom St Denis
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> [...]
>
> @@ -284,6 +284,14 @@ static int amdgpu_vram_mgr_new(struct
> ttm_mem_type_manager *man,
> if (!lpfn)
> lpfn = man->size;
>
> + /* bail out quickly if there's likely not enough VRAM for this BO */
> + atomic64_add(mem->num_pages << PAGE_SHIFT, >usage);
> + if (atomic64_read(>usage) > adev->gmc.mc_vram_size) {
Should probably use atomic64_add_return instead of atomic64_add +
atomic64_read.
Also, AFAICT this doesn't allow any VRAM overcommit, which seems a bit
conservative?
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
