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 7D0BDE77182 for ; Thu, 12 Dec 2024 10:12:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F2E426B0083; Thu, 12 Dec 2024 05:12:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EDE8F6B0085; Thu, 12 Dec 2024 05:12:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DA66E6B0088; Thu, 12 Dec 2024 05:12:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id BDA016B0083 for ; Thu, 12 Dec 2024 05:12:46 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 6FC13121A6D for ; Thu, 12 Dec 2024 10:12:46 +0000 (UTC) X-FDA: 82885892628.11.62A0C90 Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by imf13.hostedemail.com (Postfix) with ESMTP id 1CBEE20009 for ; Thu, 12 Dec 2024 10:12:19 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=ffwll.ch header.s=google header.b=PDWdBHDn; spf=none (imf13.hostedemail.com: domain of simona.vetter@ffwll.ch has no SPF policy when checking 209.85.128.50) smtp.mailfrom=simona.vetter@ffwll.ch; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733998346; 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=/6L/m3gKnrPsaMxDrU89bXCWhOH8qgzGCoqHbE1Yj18=; b=nY9t4rdSYxDUzm6d/VYAD8/wCdgAC5DddoGcVbND4T8Ds99m/WChisxpCQn3ynUpZI9MNR B+3cWso7QCqTzZ3UncrumVci6MM7FbxiWso5eFDypgtSR29K8mZCljED1pdn76xNNTGLV9 fT4Ln9qY4R3JEHjLgHA04Dyf5+9n8Vg= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=ffwll.ch header.s=google header.b=PDWdBHDn; spf=none (imf13.hostedemail.com: domain of simona.vetter@ffwll.ch has no SPF policy when checking 209.85.128.50) smtp.mailfrom=simona.vetter@ffwll.ch; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733998346; a=rsa-sha256; cv=none; b=uF9Fb4fKwVkntfKFE9QqTtXF9xo39jPpqTBMyBeMTyAQ6OnI1OtWIJLMPVevcx2pobAZ9l J3lrKJ8ALSjP00m8DIbTQfy8gA7nfpbH0jKttIyW9Oh9n83AvI8GPAAFUK13vx6JLWJX1d iFHckWpEV5FZDFfsukeu7L4coJiEGTU= Received: by mail-wm1-f50.google.com with SMTP id 5b1f17b1804b1-434a1fe2b43so3780055e9.2 for ; Thu, 12 Dec 2024 02:12:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1733998362; x=1734603162; 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=/6L/m3gKnrPsaMxDrU89bXCWhOH8qgzGCoqHbE1Yj18=; b=PDWdBHDnyjyTs9WoAdqrwUHw9vuuvLSp6an9A8kMoqTaPJ35wQQFwCu6gFbGaV+0y/ 2LQhLJA/ppjH1B3iaD0h+PmVxRLukd77gVUmVRFXellzhKd5bnM+QL1vX2B3gYvM7MKH 6cMHMl8eiW+sPeX3MCguJ6mOM9nkffJROZFSA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733998362; x=1734603162; 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=/6L/m3gKnrPsaMxDrU89bXCWhOH8qgzGCoqHbE1Yj18=; b=nq/EaP9FqK0tBa6nGUxIWGtEH4nV/nMxdFOU6QQz1KsBoI9vyUd4aiVxGdIpGfigfF TVC7lmgAKcMsNSaFlnyiRtqvp04SG5R4JorL8hwmVUhkfhdRUH2UF/zXqgUu3JzgsFTi gDhHdDUjjZNNGemlqZ57XePAg5GYl5cuvRJWRwaH4DckPUDPE4XN84XE7k3tzH97LaW/ tqTbCgwU4UsKBSmxI8jgOwNsydf1030Ywy3GdQ0A+mwtaGZXZjj/sx5FyZSk7jBPg/+u E3TF9AYqh0ZPqKEVcrTK9LFCsj9lUm2nhMiSENqfweKHCUwdcAtyIPFDisWVcp59hBFU i1aw== X-Forwarded-Encrypted: i=1; AJvYcCWZufr44vAVUMxIztX7x4wWuE/NQW2CgnApvnSAtW2uNri2b3SaCVnh3KgW0FFJ7vmGCIWLDLT+RA==@kvack.org X-Gm-Message-State: AOJu0Yzy2IA83Rfow7s/CKqcD+wwrI5pa478ZYQrZ1z66FplJ6PwOY8B T2GB9lPxPYQ2eCEBeFYVfBzmepVsDSszUmTvTLZrlJUgB+lIOEWHvqrhdkAfXOI= X-Gm-Gg: ASbGncuSif2bzujCkKmXlpdoZfHUVRvnHB9AbNmHMLxbw7a4ZmZ1BYc65OeH5tlIER/ K2aCwKJCXUX7kMrzRq0MPXad7fNMoWm2RzOd5RUrhfQXt0bjX58KnYmI+7XGUCaYepcxlZ3Fiti fnTuG7mjtiqNMw4B14g9qzxtsR2zoMb323rJIm5jguQXm7kaci7SZLmXjTT9LIHcyGXN58aqhum fJpg4LwcwmbjGHhasTAvScciJUBI8t1NT0g3qMooqHtGOZohGosJRksclVAKal6MlRc X-Google-Smtp-Source: AGHT+IEvyP9FitM66cSZMMwydxM3tJNlBph+iYVkomM+fbp/4XM+0mwafLdeLXGjgJiZl+wAHekCvA== X-Received: by 2002:a05:600c:3b0e:b0:435:32e:8270 with SMTP id 5b1f17b1804b1-4362282e38dmr25784385e9.14.1733998362260; Thu, 12 Dec 2024 02:12:42 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:5485:d4b2:c087:b497]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4362559f9b5sm12200215e9.26.2024.12.12.02.12.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Dec 2024 02:12:41 -0800 (PST) Date: Thu, 12 Dec 2024 11:12:39 +0100 From: Simona Vetter To: Thomas =?iso-8859-1?Q?Hellstr=F6m?= Cc: 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 Subject: Re: [PATCH 14/26] drm/xe/eudebug: implement userptr_vma access Message-ID: Mail-Followup-To: Thomas =?iso-8859-1?Q?Hellstr=F6m?= , 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> <3c1cc9403eb50bc8c532d180f766eb7a429e8913.camel@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: <3c1cc9403eb50bc8c532d180f766eb7a429e8913.camel@linux.intel.com> X-Operating-System: Linux phenom 6.11.6-amd64 X-Rspamd-Queue-Id: 1CBEE20009 X-Rspamd-Server: rspam12 X-Stat-Signature: gy1ftamrtywn4juitin1m3c4ei64j9kg X-Rspam-User: X-HE-Tag: 1733998339-59872 X-HE-Meta: U2FsdGVkX1/ilJE7iC/+JHYGeY2WLa2vGRrzLf4OPM9/DsJFsZHC7n9/EcpR+SL/SePSKnfpNxKASTU0ji53sz3ZNLVKDQFmBjkek/dp5Fn0qsKoqoEwKz0uNXgqbSw49R/QJVo6UwyeoKxwuTJL+aOyP3jO3o6HRrvBNuiQcT4VtChBqYvE+fKNsait+gHQu9AUeTd/iMn1fC4A3xyD/XuGzGihfyHhnuZmg+RsWWGcUvcy3x+lL7BvJjLUuHGwN0yqfLe0bky5cYFBK6ZSV8AlB0G5EqcHTSGYsmHtoCnmmhSmHj+k5B3pPie7ZWbseQUEKrHOuqSpYPYNron/Oi3nkDrTHwnjUJljSlPKqUkWhODgAQVK7LHNiKPDYex5R8Uj00qyMtppPRbzBwlziOnKJeILOAVchjDOnzhqtFr+fQrJDc0OI4VlXIxp9sUxVOuiwg8n2NlAUe99zkFHtCxFBVbSy8HdvyBBc90BLqT3xAVn10Wdo8DZgQFSNgeTWUswmFXDgoOhNP1tZJ9l3xlA0jQVG+HtxxQpnypKD+glhrDE4T07PMGkGSydERhm4G3sXb2TyoM3WMCQONeX20K/ptQjZVCsbhT+dDOG423S1hqV7UIxh2nU/3LPiiML71cZJH3+jIcqmFL5m3dnd+nyMewRKzNrktRs2sHLqJQm+X/VzCiQDAjXUOdcgxHYnq1nkaFke/DQ8UKmhPIubQgKnj2z0LFSv8GthH1E3zkeWBoA3JNOfKXQL/1dgHQnbHqvvU8HGz+20L2wxD2yIgLCFaEfFbBkkZvDvwdXcMLDINa8vZVR3m+sHoc6dxjh1tnlHQdcu1urlVcEJoDmcjf7oYXUDioI43UDYM1Qmd/fBY+DodNFc49EJXmeq7Z2VtP4VPSLfVnze7WUUUdKC3/mmrtQslsAIpyR9l1OgJp5sBHtSeOuaGZPrx2oXn54U/F9lup/HWKg3IKL/1v Sc5MnAkG kA9Mm5zFC7uLLpRlA58owcclhFRhOU8wJyiW6eZTtfyFUAjg3LuNoYbNA0h36YBPzy+W26/TwEktTpTZ2/K0W/3IploXk9xy27mG64NHHXCSetZKpk96Moeh9xIyR3lAq7BIrtqi22e9UN4V2pkd0vITB7pnixhsLx6Un1IZOth+XpYQ/IHLSqt+F4fndBDp1GlQxxBuaXIboEbOE1c14pgZ8DBMIFBQTRucdB9/ExbIuQJEzBvZlBNLjmryKMeHcOjfSYOoH/BpL0QN3krjamboJDeYxwDgjvvjbw8qX78D6bipoXY8eaReLujW3EreEcRdTmxjnV4Q6oTmzarHf6HEkPBOIZOiFkXBcU9CxGMP18RxlE7Cdn1SoT34E0xNQGX3JXoJrumw54ns= 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 Thu, Dec 12, 2024 at 09:49:24AM +0100, Thomas Hellström wrote: > On Mon, 2024-12-09 at 16:31 +0100, Simona Vetter wrote: > > 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. 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? > > It looks like we have this planned and ongoing but there are some > complications and thoughts. > > 1) A drm_gpuvm implementation would be based on vma userptrs, and would > be pretty straightforward based on xe's current implementation and, as > you say, renaming. > > 2) Current Intel work to land this on the drm level is based on > drm_gpusvm (minus migration to VRAM). I'm not fully sure yet how this > will integrate with drm_gpuvm. > > 3) Christian mentioned a plan to have a common userptr implementation > based off drm_exec. I figure that would be bo-based like the amdgpu > implemeentation still is. Possibly i915 would be interested in this but > I think any VM_BIND based driver would want to use drm_gpuvm / > drm_gpusvm implementation, which is also typically O(1), since userptrs > are considered vm-local. > > Ideas / suggestions welcome So just discussed this a bit with Joonas, and if we use access_remote_vm for the userptr access instead of hand-rolling then we really only need bare-bones data structure changes in gpuvm, and nothing more. So - add the mm pointer to struct drm_gpuvm - add a flag indicating that it's a userptr + userspace address to struct drm_gpuva - since we already have userptr in drivers I guess there should be any need to adjust the actual drm_gpuvm code to cope with these Then with this you can write the access helper using access_remote_vm since that does the entire remote mm walking internally, and so there's no need to also have all the mmu notifier and locking lifted to gpuvm. But it does already give us some great places to put relevant kerneldocs (not just for debugging architecture, but userptr stuff in general), which is already a solid step forward. Plus I think it'd would also be a solid first step that we need no matter what for figuring out the questions/options you have above. Thoughts? -Sima > > > -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