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 0DF44E7717F for ; Thu, 12 Dec 2024 08:49:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8FD496B008A; Thu, 12 Dec 2024 03:49:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8AD316B008C; Thu, 12 Dec 2024 03:49:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 74DBD6B0092; Thu, 12 Dec 2024 03:49:34 -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 5797A6B008A for ; Thu, 12 Dec 2024 03:49:34 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 0DB3E4457D for ; Thu, 12 Dec 2024 08:49:34 +0000 (UTC) X-FDA: 82885682712.26.1DC6905 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.13]) by imf19.hostedemail.com (Postfix) with ESMTP id C8AF21A0012 for ; Thu, 12 Dec 2024 08:49:05 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b="MjKPtmI/"; spf=none (imf19.hostedemail.com: domain of thomas.hellstrom@linux.intel.com has no SPF policy when checking 198.175.65.13) smtp.mailfrom=thomas.hellstrom@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733993346; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=4WbxO5NSqJGUXp2Z0t/+dlVjlgo52vC1flpC10YkqtM=; b=UlA6lC53fAqr10mdac/C9hFxB84HqIi7VeMXImoqMDeuKEkEe4Nea/Uzu/bTQUhsPs7rba 8o5YIK8c/RKondSUfuQM0CSIHZ1LBZzBX+O0x/suhlZwKiuuaCxGjuphWBISjVg1pQ5FUw Ozb//RekV+qWXdSMo+mJoPYV7IgkVvo= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b="MjKPtmI/"; spf=none (imf19.hostedemail.com: domain of thomas.hellstrom@linux.intel.com has no SPF policy when checking 198.175.65.13) smtp.mailfrom=thomas.hellstrom@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733993346; a=rsa-sha256; cv=none; b=KSIvwq/mCaVLSB9U2S5+1hLSI3/rzJMF6ooGVf+Li1hPWJcVquNBZGF4T8wGcLOl2bukLX 6mPIiNRQBMr3APp2CnH53m2KBlxa+sVJqzu1rRC4d0jEwUvRDbIfZgznoIL3LLdPAhS8aG ssIN7WFswHlKAC12sddzAqBh0Y4V4h4= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1733993372; x=1765529372; h=message-id:subject:from:to:date:in-reply-to:references: content-transfer-encoding:mime-version; bh=nc0EXFS466VP4d7lMSVjOKJedH2T3ksIlkvVX9BTgFY=; b=MjKPtmI/yZ8KImcoK6gB34b8va07Iug7JGYYz2GyLe+LoXvylDauejMT USaj1FcZv+/4Tq11dS0Vj5d1ZNoChmIYSCAja66q9DB6Or3tuCllw9T9X LbCdNHf0qFukP2rQzjNl4nKRstUCyKQHLleposcmNaqgLEgS7mp37mV2J A91XHPQjf7Bp8u/xKnl8rGMfmqAAelkJOVNuR0V1uQ6n49bTQdQwvomHe 2pY0DaARzWOc3RF5PIs2mEXq7iuJoJtSVlpWHFSzikIhnGswuAM/y1hjw tBI8XiZ20pdURcrrYlCO30XtAeJKySkX+Pv6dpYAPpI6gyjOC6UUhNjWn w==; X-CSE-ConnectionGUID: lYnfmrQHQHexbn6BigiAMg== X-CSE-MsgGUID: RG/wxsGAT0qbXTP5Zcwplw== X-IronPort-AV: E=McAfee;i="6700,10204,11282"; a="45409992" X-IronPort-AV: E=Sophos;i="6.12,224,1728975600"; d="scan'208";a="45409992" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by orvoesa105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Dec 2024 00:49:31 -0800 X-CSE-ConnectionGUID: o+iOPzLvSBy235m+9YbFuQ== X-CSE-MsgGUID: CwnmZe5CQrW0Geei23RsoA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.12,228,1728975600"; d="scan'208";a="96589486" Received: from carterle-desk.ger.corp.intel.com (HELO [10.245.246.75]) ([10.245.246.75]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Dec 2024 00:49:27 -0800 Message-ID: <3c1cc9403eb50bc8c532d180f766eb7a429e8913.camel@linux.intel.com> Subject: Re: [PATCH 14/26] drm/xe/eudebug: implement userptr_vma access From: Thomas =?ISO-8859-1?Q?Hellstr=F6m?= 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 Date: Thu, 12 Dec 2024 09:49:24 +0100 In-Reply-To: References: <20241209133318.1806472-1-mika.kuoppala@linux.intel.com> <20241209133318.1806472-15-mika.kuoppala@linux.intel.com> Organization: Intel Sweden AB, Registration Number: 556189-6027 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.54.2 (3.54.2-1.fc41) MIME-Version: 1.0 X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: C8AF21A0012 X-Rspam-User: X-Stat-Signature: qqgqir14cg4yi48psmywtfwrtymou11h X-HE-Tag: 1733993345-172352 X-HE-Meta: U2FsdGVkX1/vG13nHCu5PUiXOaGGtSu/o0dSASmhTwnittJ/sI4NDalhW5qn0eYFhk6EI8fomdNjYaFIzDPLksrtlslm/ihIkSNsVmFs9Pm/1l9Jic4x3ylYcqnu/U+lWQcGu/8+gIW+PE1KZmxVhhE96QFkVJdmX2OyjxoxXohzPFGrP5eSL8gCQ6hTLJsYKKKN7z+bVBMK51Rety+mChsOvm4Tha1mL80Stq3B/uDxTRE9pIH/FPs3qhd6N2EA961AEDWtJtfLsNcXqKuUS869Rwrv4MF7ZJ89zxPn4O3P8/IunbxY0lVubnsV2dVEDNAmn7IOmCbXIlPBUlVVy9pf2KHeBpjnI+YDOwIApn++sM5WfOQA/StLaGg70q7sct81m3HFPiUB+xV6mPMQ5IE4daYKhgBGyC000oPyc8cUITYpraUVAHPmOFsXAqmp++qyBLBZhQcCCwoAHtUIMm1dgtUuNji12VWmmKiaCKVy34q5qgR/5te/DoODQPjxksSZ2dlVakKo52ul1HIS8lgJyGXoJhu2chBA3t8UroqwjFhgBZZzaPdc9Uo6QsTjfrzfqjLcJxInLg8n3WSjhfk7urjE1pwI2h48UKvbVCSISuiyGlW9bfwda5zeIPlroydgJL27XEaCBZOlrTEsO35MnJpFHUelKWgEm9fKUBAgsqtS/EwZWL3pP+FM6mL8Jsw7NJz9/ubrOWmbH09NQFHuGbp7/XJZEJJL7uiqie5DS0Cnf+bPsEl7qmSDlbJnnw/t7gYnF8oERZsRrUUbaCpqYWocQi8zlqdAdRKPfbdTeSdSycWk5ROhripPBBHe9nFLQrMpnWpRQ1DUWS4mI+R1aedvplqdIqg1XiFf51vAenTIdQzqR9OqABd3JpZ4rgr/brkxmI0NIPIThfd2lvGSslYtI1dVOmPkwuoyMVbaGDxkrGv9smpDbdcqAQK1TV/DWgBXfdnU8+8C+FD +UCqnJeK gIX9IZdnLkHAKU635U4jMnr5gTEIlHpiwVDyrEYEiMVaAxGBGQbB/TK2m1RNEmYHqpSUHprBDWOKnNMTb+f0+qQVcoD/7s3p6ph7AWun9UZLJajKDtmuhM6mhSdPZLu4/u0XbgqTiSDo0/H0lsxPgqytD2+DZfiBw7JBqkFVkP9ygFkO88TRH3o0elZcDMnty61QSGbaHbCy5imJuKRSOjVA9o/P08lzV23ylTo0pi0RhJzX1a4Oh5yJuNZ3Y/91GsIzSuTeX356IAAflhu3wxhdCoZo6pnvgml0xArImgYyPw8m9Ae0e0KBvng== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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, 2024-12-09 at 16:31 +0100, Simona Vetter wrote: > On Mon, Dec 09, 2024 at 03:03:04PM +0100, Christian K=C3=B6nig wrote: > > Am 09.12.24 um 14:33 schrieb Mika Kuoppala: > > > From: Andrzej Hajda > > >=20 > > > 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. > >=20 > > Oh, this implementation is extremely questionable as well. Adding > > the LKML > > and the MM list as well. > >=20 > > First of all hmm_range_fault() does *not* pin anything! > >=20 > > 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. >=20 > I think it's all there, holds the invalidation lock during the > critical > access/section, drops it when reacquiring pages, retries until it > works. >=20 > 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. >=20 > 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 > -Sima >=20 > >=20 > > > v2: pin pages vs notifier, move to vm.c (Matthew) > > > v3: - iterate over system pages instead of DMA, fixes iommu > > > enabled > > > =C2=A0=C2=A0=C2=A0=C2=A0 - s/xe_uvma_access/xe_vm_uvma_access/ (Matt) > > >=20 > > > Signed-off-by: Andrzej Hajda > > > Signed-off-by: Maciej Patelczyk > > > Signed-off-by: Mika Kuoppala > > > Reviewed-by: Jonathan Cavitt #v1 > > > --- > > > =C2=A0 drivers/gpu/drm/xe/xe_eudebug.c |=C2=A0 3 ++- > > > =C2=A0 drivers/gpu/drm/xe/xe_vm.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 47 > > > +++++++++++++++++++++++++++++++++ > > > =C2=A0 drivers/gpu/drm/xe/xe_vm.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2= =A0 3 +++ > > > =C2=A0 3 files changed, 52 insertions(+), 1 deletion(-) > > >=20 > > > 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, > > > =C2=A0=C2=A0 return ret; > > > =C2=A0=C2=A0 } > > > - return -EINVAL; > > > + return xe_vm_userptr_access(to_userptr_vma(vma), > > > offset_in_vma, > > > + =C2=A0=C2=A0=C2=A0 buf, bytes, write); > > > =C2=A0 } > > > =C2=A0 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) > > > =C2=A0=C2=A0 } > > > =C2=A0=C2=A0 kvfree(snap); > > > =C2=A0 } > > > + > > > +int xe_vm_userptr_access(struct xe_userptr_vma *uvma, u64 > > > offset, > > > + void *buf, u64 len, bool write) > > > +{ > > > + struct xe_vm *vm =3D xe_vma_vm(&uvma->vma); > > > + struct xe_userptr *up =3D &uvma->userptr; > > > + struct xe_res_cursor cur =3D {}; > > > + int cur_len, ret =3D 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 =3D xe_vma_userptr_pin_pages(uvma); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + if (!up->sg) { > > > + ret =3D -EINVAL; > > > + goto out_unlock_notifier; > > > + } > > > + > > > + for (xe_res_first_sg_system(up->sg, offset, len, &cur); > > > cur.remaining; > > > + =C2=A0=C2=A0=C2=A0=C2=A0 xe_res_next(&cur, cur_len)) { > > > + void *ptr =3D kmap_local_page(sg_page(cur.sgl)) + > > > cur.start; > >=20 > > The interface basically creates a side channel to access userptrs > > in the way > > an userspace application would do without actually going through > > userspace. > >=20 > > That is generally not something a device driver should ever do as > > far as I > > can see. > >=20 > > > + > > > + cur_len =3D min(cur.size, cur.remaining); > > > + if (write) > > > + memcpy(ptr, buf, cur_len); > > > + else > > > + memcpy(buf, ptr, cur_len); > > > + kunmap_local(ptr); > > > + buf +=3D cur_len; > > > + } > > > + ret =3D len; > > > + > > > +out_unlock_notifier: > > > + up_read(&vm->userptr.notifier_lock); > >=20 > > I just strongly hope that this will prevent the mapping from > > changing. > >=20 > > Regards, > > Christian. > >=20 > > > + 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); > > > =C2=A0 void xe_vm_snapshot_capture_delayed(struct xe_vm_snapshot > > > *snap); > > > =C2=A0 void xe_vm_snapshot_print(struct xe_vm_snapshot *snap, struct > > > drm_printer *p); > > > =C2=A0 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); > >=20 >=20