From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14B43E77184 for ; Tue, 10 Dec 2024 11:17:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 962AE6B017E; Tue, 10 Dec 2024 06:17:29 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 911986B0180; Tue, 10 Dec 2024 06:17:29 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 78BA16B0181; Tue, 10 Dec 2024 06:17:29 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 5664F6B017E for ; Tue, 10 Dec 2024 06:17:29 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id F2CFAC075B for ; Tue, 10 Dec 2024 11:17:28 +0000 (UTC) X-FDA: 82878797736.25.83CABBC Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by imf25.hostedemail.com (Postfix) with ESMTP id CEE24A0010 for ; Tue, 10 Dec 2024 11:17:10 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=ffwll.ch header.s=google header.b=J81bbX1+; dmarc=none; spf=none (imf25.hostedemail.com: domain of simona.vetter@ffwll.ch has no SPF policy when checking 209.85.128.53) smtp.mailfrom=simona.vetter@ffwll.ch ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733829426; a=rsa-sha256; cv=none; b=wAADnwuULvdnmU84uD/djb436bewY1uCJMrXiBqjaX1/nr/6XVcwlcRy/4T4tMlYtXDQux aqJG9mg1r8NHNd6/DXzJQWFLMdgU8K/pLZ0RV96KzIGUg/bdHW+bvOEUS8a5ABhiTbgyQb KfICqM5kHBjAUQUWUHimlcOd5HzMoNs= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=ffwll.ch header.s=google header.b=J81bbX1+; dmarc=none; spf=none (imf25.hostedemail.com: domain of simona.vetter@ffwll.ch has no SPF policy when checking 209.85.128.53) smtp.mailfrom=simona.vetter@ffwll.ch ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733829426; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=IDojbZ2VONgpjznrfPM339c4Ui/XuZ8qmsW07fn2uQY=; b=h/D4MBCYjPH72gptKtmDef+MrsV7NDGZV2GQeqS89jZUYvXg9f7bIuATvscjkxaPTNl7Ep Ej9onMyRpLYlngEq0IZuvLk/CqKkmALHJLIlTp5CVCP0GjTIrgnhnoigVB7SDjqcAZhFCw XnKfKjB1TVdOfNBdJMDVxvyKcX2kHn0= Received: by mail-wm1-f53.google.com with SMTP id 5b1f17b1804b1-434e3953b65so22064145e9.1 for ; Tue, 10 Dec 2024 03:17:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1733829445; x=1734434245; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:from:to:cc:subject:date:message-id:reply-to; bh=IDojbZ2VONgpjznrfPM339c4Ui/XuZ8qmsW07fn2uQY=; b=J81bbX1+EuPOe8vJrqWZTeyo8DU4qliA9N8dr+gSB1QlvqJpbRlOrwLYOMiLWBIUfO 6rRCEbcOhq4YXeJ+huF/R4k1a5wrmJd3tcq/FV+eWHr2thzwXon88aZ8fHf1XE60KoU2 tCvWpBX5wwEeen7gWeMw1k9mqSnrHSjDOcOE0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733829445; x=1734434245; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=IDojbZ2VONgpjznrfPM339c4Ui/XuZ8qmsW07fn2uQY=; b=ZaQ+bpIOddX86+vbDUutlMYdjIlKYDxw1dq5ko2Y8kwwgvl9X7NpyxN4UpyiTDxABr L0b03RgtywSH+k4M87mBol/XYcnQZ1OFeFhohXwaumZJJNDR3y9pTFTQEXGM6kID9LNj HD3I6Ex85cc5+Uj2lbxlNt7+vO7VHrJPZKLrYlsO7QwJhwnFcOiuoiyLaaI2o2vc5H6w bMvZRHMY1Bs5Tl/ComK+XAV8JskbG944hCw3vCM8n4gmOkyUuukxG3/IKMulHjHFkvcT CIRIMhT/DmVCoKSo2mGIDCJdDhQxLxGyeSAH0ilHVPwitda31t6frffv84OE2avajviC BIsA== X-Forwarded-Encrypted: i=1; AJvYcCW2HxJWiD0xbHJ0xtZ2SH2j9qiCgkQNbSCCPI+8XXqJQzxQ1ajlDdhVy8LWMq4Wex9Vq7Hm5edhtw==@kvack.org X-Gm-Message-State: AOJu0YxDG0JGDzex/FlOC5KkP0yfIzEBc9dUzmPp1FUzMB1uYb8yT31K CpBuf1GR3Z9nJf2J+JQUUHFFRUTLZiF1ZqoZDwgGW5FxUs/2Bu0h9tP9o8Wmazw= X-Gm-Gg: ASbGncs5ZRtE5Zs/x1A0+1jSLMB8nv72TjrMOR8MlqRj189QAKomj0V2rJIigM+VKNb MUrHDpuqfOo0md3RtWb7/3rX++8qfW3Ys+jjfX/D78VuRsUriO/m8Bi1YpxXyvrpc9q3/SIuStZ oubgXyxpht2qfU8pJwRMbpN0r8j4epCQDBY2kKzT5Ljk/Ap+2T1JJ0NZo+6a95G2wir8qeWMoAR Z/Gz+rSu6nLcJhPSM2NtOs+BMCCu5NxZ/rPiBvlZkfwCVj06E3GzIsbQhZIbZInBQ== X-Google-Smtp-Source: AGHT+IG0c0/kJCByoAXTvAWZSCghtb2kRyPv2cAHMRWAM/x2Lb1XI1VpZSJ/qQlmCYfTOguR1LwFBA== X-Received: by 2002:a05:600c:458b:b0:434:f739:7ce3 with SMTP id 5b1f17b1804b1-434fff41b59mr33461855e9.8.1733829443037; Tue, 10 Dec 2024 03:17:23 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:5485:d4b2:c087:b497]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434d52cbd72sm227349055e9.44.2024.12.10.03.17.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Dec 2024 03:17:22 -0800 (PST) Date: Tue, 10 Dec 2024 12:17:20 +0100 From: Simona Vetter To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: Mika Kuoppala , intel-xe@lists.freedesktop.org, lkml , Linux MM , dri-devel@lists.freedesktop.org, Andrzej Hajda , Maciej Patelczyk , Jonathan Cavitt Subject: Re: [PATCH 14/26] drm/xe/eudebug: implement userptr_vma access Message-ID: Mail-Followup-To: Christian =?iso-8859-1?Q?K=F6nig?= , Mika Kuoppala , intel-xe@lists.freedesktop.org, lkml , Linux MM , dri-devel@lists.freedesktop.org, Andrzej Hajda , Maciej Patelczyk , Jonathan Cavitt References: <20241209133318.1806472-1-mika.kuoppala@linux.intel.com> <20241209133318.1806472-15-mika.kuoppala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Operating-System: Linux phenom 6.11.6-amd64 X-Stat-Signature: uwk4u9gnzb8f9zazf7szeuso8pxygyib X-Rspam-User: X-Rspamd-Queue-Id: CEE24A0010 X-Rspamd-Server: rspam08 X-HE-Tag: 1733829430-360571 X-HE-Meta: U2FsdGVkX1+d0+sFT6pdXfFxDqW2edcsa4CKSZGzqTMitvSk/xq2sctD/WJ8i+AZvo9Rm/W6lzj3yJY+p5gY8pBJnIqsTt6kFgbhsKwUspXyBhVvuc70BgqvoGrSlRDKwON0xkqyx5jf8tM4TC1PmkvSc0P8l87JpvRfhkDvVeG6FRQ9d2WIFN1gBF3nr7EWaou3eWy/HvCt0kcaOnSGImTlpOLs+3Cn+FkuMMIR8W3eHn3yG39qwd7A+ykvdii1vf6377VmmmLfFcQmpuN46G365QoHSvzrfnI72lkRbMKDPYn83E8jhhVKZGganJqxeuDP0amDP+Knz197SUpEi+y8Qum3wWSHAKHTkE2kjdkZCBaVT/1t/U+/bB00D9tNmIwxWGmg+lI2IYYiz5XEarPOitJiwlrIM0/Fw/81ADGvzwA62Ewl0vTX8rT7OUHx3qf4B40+rh/TgnXweETN53CuV7Rkz6/q1C2kZgaqp/e79HGVQK67jfMMwsqgX7oJvQlH59k7zsUV1CH1PPUKO4pVTgsZXteH/q7YWBiYu8GQb2+m9WrktAmsWcoLxI7aMfLWIm+jNK1i1eDUm5GV70n82kmSOH7PmBJAYRVuAlwZdIawQagKW/uiCeiQ/JqMJEC8K0KkK3od5ru3aAeJsgnnF9YXabx4OQfokX3xUDv/PUk6Nr8GTwzG6WepGCYu1vqwtyZrjBS8A2aOw70qg142e5yOKefP9y8qp2rJ8SzfVegxCukI90NGthFye2O8YlemQCtLrwlIVjUlfHGLUVAgrEWq1MkNPJwmgoXyuRvFy8uHhYFK9KaHwoiMk94HPXQb/L3C3FLK7lvUhFUllaxCMM3A7SCIEoQLPKQnX/4utF7vCf/1qSJqHWxRhy2OP37vbL/m9JJ+wgivuphVS0tdABW5x9BisKdOKKUw+sNkRciX/Uy7tKFvFGFv7AZ/QV3TCOkEGBzl4qnjLXw cwTPuiDA BKr8P/D2IawhcJRrJmcKtzXYZsIaD3uS36Fhplfeq8+/OMtVW1g2UbRT/ZZvc/C7QPkalqG5yvImeTUOcdWzlZ+Xhd6z3trUGWvMQtZg8mbMoTKNRL7OXHrUsi5D9w2eb4wwM8ji6IMN1f2nS9iJ9/lVTwtKocedxJIiNmv2mYGARWvQFS57lO+AiQi7PM9pKyh6m4ZkRmY2oCyejkNWufRvVcHcjQJ5drBSG+kHfPkLXNWBoLWQI5ZqOMc9jRZR5Os4BBU62IdkSaHHFW3sEQIEf9l1usFHfzWFuZerjTtRaP/qU5qdMMXwSa7zoHjdoXArikNjfc0wZcMU2LfZLvpB2M2/TcXQXKg0W/TqqhhYQ4Gj5CKOtF4sEdO6ZIuNRN9DZ5JBZNotGeuY= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Mon, Dec 09, 2024 at 04:42:32PM +0100, Christian König wrote: > Am 09.12.24 um 16:31 schrieb Simona Vetter: > > On Mon, Dec 09, 2024 at 03:03:04PM +0100, Christian König wrote: > > > Am 09.12.24 um 14:33 schrieb Mika Kuoppala: > > > > From: Andrzej Hajda > > > > > > > > Debugger needs to read/write program's vmas including userptr_vma. > > > > Since hmm_range_fault is used to pin userptr vmas, it is possible > > > > to map those vmas from debugger context. > > > Oh, this implementation is extremely questionable as well. Adding the LKML > > > and the MM list as well. > > > > > > First of all hmm_range_fault() does *not* pin anything! > > > > > > In other words you don't have a page reference when the function returns, > > > but rather just a sequence number you can check for modifications. > > I think it's all there, holds the invalidation lock during the critical > > access/section, drops it when reacquiring pages, retries until it works. > > > > I think the issue is more that everyone hand-rolls userptr. > > Well that is part of the issue. Yeah I ignored the other part, because that didn't seem super interesting really. Like for compute you can make the architectural assumption that gpu addresses match cpu addresses, and this all becomes easy. Or at least easier, there's still the issue of having to call into the driver for gpu side flushes. But for 3d userptr that's not the case, and you get two options: - Have some tracking structure that umd and debugger agree on, so stable abi fun and all that, so you can find the mapping. And I think in some cases this would need to be added first. - Just ask the kernel, which already knows. Like for cpu mmaps we also don't inject tracking code into mmap/munmap, we just ask the kernel through /proc/pid/maps. This sounds like the same question, probably should have a similar answer. I guess you can do a bit a bikeshed about whether the kernel should only do the address translation and then you do a ptrace poke/peek for the actual access. But again you need some flushes, so this might be a bit silly. But fundamentally this makes sense to me to ask the entity that actually knows how userptr areas map to memory. -Sima > > The general problem here is that the eudebug interface tries to simulate the > memory accesses as they would have happened by the hardware. > > What the debugger should probably do is to cleanly attach to the > application, get the information which CPU address is mapped to which GPU > address and then use the standard ptrace interfaces. > > The whole interface re-invents a lot of functionality which is already there > just because you don't like the idea to attach to the debugged application > in userspace. > > As far as I can see this whole idea is extremely questionable. This looks > like re-inventing the wheel in a different color. > > Regards, > Christian. > > > Probably time > > we standardize that and put it into gpuvm as an optional part, with > > consistent locking, naming (like not calling it _pin_pages when it's > > unpinnged userptr), kerneldoc and all the nice things so that we > > stop consistently getting confused by other driver's userptr code. > > > > I think that was on the plan originally as an eventual step, I guess time > > to pump that up. Matt/Thomas, thoughts? > > -Sima > > > > > > v2: pin pages vs notifier, move to vm.c (Matthew) > > > > v3: - iterate over system pages instead of DMA, fixes iommu enabled > > > > - s/xe_uvma_access/xe_vm_uvma_access/ (Matt) > > > > > > > > Signed-off-by: Andrzej Hajda > > > > Signed-off-by: Maciej Patelczyk > > > > Signed-off-by: Mika Kuoppala > > > > Reviewed-by: Jonathan Cavitt #v1 > > > > --- > > > > drivers/gpu/drm/xe/xe_eudebug.c | 3 ++- > > > > drivers/gpu/drm/xe/xe_vm.c | 47 +++++++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/xe/xe_vm.h | 3 +++ > > > > 3 files changed, 52 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c > > > > index 9d87df75348b..e5949e4dcad8 100644 > > > > --- a/drivers/gpu/drm/xe/xe_eudebug.c > > > > +++ b/drivers/gpu/drm/xe/xe_eudebug.c > > > > @@ -3076,7 +3076,8 @@ static int xe_eudebug_vma_access(struct xe_vma *vma, u64 offset_in_vma, > > > > return ret; > > > > } > > > > - return -EINVAL; > > > > + return xe_vm_userptr_access(to_userptr_vma(vma), offset_in_vma, > > > > + buf, bytes, write); > > > > } > > > > static int xe_eudebug_vm_access(struct xe_vm *vm, u64 offset, > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > > > > index 0f17bc8b627b..224ff9e16941 100644 > > > > --- a/drivers/gpu/drm/xe/xe_vm.c > > > > +++ b/drivers/gpu/drm/xe/xe_vm.c > > > > @@ -3414,3 +3414,50 @@ void xe_vm_snapshot_free(struct xe_vm_snapshot *snap) > > > > } > > > > kvfree(snap); > > > > } > > > > + > > > > +int xe_vm_userptr_access(struct xe_userptr_vma *uvma, u64 offset, > > > > + void *buf, u64 len, bool write) > > > > +{ > > > > + struct xe_vm *vm = xe_vma_vm(&uvma->vma); > > > > + struct xe_userptr *up = &uvma->userptr; > > > > + struct xe_res_cursor cur = {}; > > > > + int cur_len, ret = 0; > > > > + > > > > + while (true) { > > > > + down_read(&vm->userptr.notifier_lock); > > > > + if (!xe_vma_userptr_check_repin(uvma)) > > > > + break; > > > > + > > > > + spin_lock(&vm->userptr.invalidated_lock); > > > > + list_del_init(&uvma->userptr.invalidate_link); > > > > + spin_unlock(&vm->userptr.invalidated_lock); > > > > + > > > > + up_read(&vm->userptr.notifier_lock); > > > > + ret = xe_vma_userptr_pin_pages(uvma); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + if (!up->sg) { > > > > + ret = -EINVAL; > > > > + goto out_unlock_notifier; > > > > + } > > > > + > > > > + for (xe_res_first_sg_system(up->sg, offset, len, &cur); cur.remaining; > > > > + xe_res_next(&cur, cur_len)) { > > > > + void *ptr = kmap_local_page(sg_page(cur.sgl)) + cur.start; > > > The interface basically creates a side channel to access userptrs in the way > > > an userspace application would do without actually going through userspace. > > > > > > That is generally not something a device driver should ever do as far as I > > > can see. > > > > > > > + > > > > + cur_len = min(cur.size, cur.remaining); > > > > + if (write) > > > > + memcpy(ptr, buf, cur_len); > > > > + else > > > > + memcpy(buf, ptr, cur_len); > > > > + kunmap_local(ptr); > > > > + buf += cur_len; > > > > + } > > > > + ret = len; > > > > + > > > > +out_unlock_notifier: > > > > + up_read(&vm->userptr.notifier_lock); > > > I just strongly hope that this will prevent the mapping from changing. > > > > > > Regards, > > > Christian. > > > > > > > + return ret; > > > > +} > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h > > > > index 23adb7442881..372ad40ad67f 100644 > > > > --- a/drivers/gpu/drm/xe/xe_vm.h > > > > +++ b/drivers/gpu/drm/xe/xe_vm.h > > > > @@ -280,3 +280,6 @@ struct xe_vm_snapshot *xe_vm_snapshot_capture(struct xe_vm *vm); > > > > void xe_vm_snapshot_capture_delayed(struct xe_vm_snapshot *snap); > > > > void xe_vm_snapshot_print(struct xe_vm_snapshot *snap, struct drm_printer *p); > > > > void xe_vm_snapshot_free(struct xe_vm_snapshot *snap); > > > > + > > > > +int xe_vm_userptr_access(struct xe_userptr_vma *uvma, u64 offset, > > > > + void *buf, u64 len, bool write); > -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch