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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2C33BC4725D for ; Sat, 20 Jan 2024 03:11:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CC70E10EB00; Sat, 20 Jan 2024 03:11:25 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.14]) by gabe.freedesktop.org (Postfix) with ESMTPS id DC30010EB00 for ; Sat, 20 Jan 2024 03:11:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1705720284; x=1737256284; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=frWNhywjwnaism3qH/YNwEhur98/kDwiTtNPmOfMu9Q=; b=XL/URo5WG/nzrf4v9rSaPfIweY9B3+oso0Ez/hhdcUW7oIX8MrUC1gul a73UajUlCscryWTGgXwD5s+Z1oPv1jztOwtlQRve07xhRljopguL2WUMc Mf4BxBZHmsSthdTda+4Fhw+RwChs1/fLwh15ewIgRG7p8MJ3DNaOyQhal h3tAodk15KgcZe7Na9apL1SKBDPk+/I8BxQ89qui0trX+k5vEZdayV5Lj bxAy0cnxa0viAZpxPbY+KupGHNNPdMY5V5Lm4yXr/QO0rTGxFUcObkWnB OATbxQJo0A5TObhPuZ4FhgBlGSPDP5k1c3pcJP1SZP734LU2lSCVb5NAQ Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10957"; a="815683" X-IronPort-AV: E=Sophos;i="6.05,206,1701158400"; d="scan'208";a="815683" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orvoesa106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2024 19:11:24 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10957"; a="788516546" X-IronPort-AV: E=Sophos;i="6.05,206,1701158400"; d="scan'208";a="788516546" Received: from orsosgc001.jf.intel.com (HELO unerlige-ril.intel.com) ([10.165.21.138]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2024 19:11:22 -0800 Date: Fri, 19 Jan 2024 19:11:22 -0800 Message-ID: <85r0icbsx1.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa Subject: Re: [PATCH 15/17] drm/xe/oa/uapi: OA buffer mmap In-Reply-To: References: <20231208064329.2387604-1-ashutosh.dixit@intel.com> <20231208064329.2387604-16-ashutosh.dixit@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-redhat-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Fri, 22 Dec 2023 18:39:14 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Thu, Dec 07, 2023 at 10:43:27PM -0800, Ashutosh Dixit wrote: > > Allow the OA buffer to be mmap'd to userspace. This is needed for the MMIO > > trigger use case. Even otherwise, with whitelisted OA head/tail ptr > > registers, userspace can receive/interpret OA data from the mmap'd buffer > > without issuing read()'s on the OA stream fd. > > > > Suggested-by: Umesh Nerlige Ramappa > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/xe_oa.c | 53 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c > > index 42f32d4359f2c..97779cbb83ee8 100644 > > --- a/drivers/gpu/drm/xe/xe_oa.c > > +++ b/drivers/gpu/drm/xe/xe_oa.c > > @@ -898,6 +898,8 @@ static int xe_oa_alloc_oa_buffer(struct xe_oa_stream *stream) > > return PTR_ERR(bo); > > > > stream->oa_buffer.bo = bo; > > + /* mmap implementation requires OA buffer to be in system memory */ > > + xe_assert(stream->oa->xe, bo->vmap.is_iomem == 0); > > stream->oa_buffer.vaddr = bo->vmap.vaddr; > > return 0; > > } > > @@ -1174,6 +1176,9 @@ static int xe_oa_release(struct inode *inode, struct file *file) > > struct xe_oa_stream *stream = file->private_data; > > struct xe_gt *gt = stream->gt; > > > > + /* Zap mmap's */ > > + unmap_mapping_range(file->f_mapping, 0, -1, 1); > > + > > mutex_lock(>->oa.gt_lock); > > xe_oa_destroy_locked(stream); > > mutex_unlock(>->oa.gt_lock); > > @@ -1184,6 +1189,53 @@ static int xe_oa_release(struct inode *inode, struct file *file) > > return 0; > > } > > > > +static int xe_oa_mmap(struct file *file, struct vm_area_struct *vma) > > +{ > > + struct xe_oa_stream *stream = file->private_data; > > + struct xe_bo *bo = stream->oa_buffer.bo; > > + unsigned long start = vma->vm_start; > > + int i, ret; > > + > > + if (xe_perf_stream_paranoid && !perfmon_capable()) { > > + drm_dbg(&stream->oa->xe->drm, "Insufficient privilege to map OA buffer\n"); > > + return -EACCES; > > + } > > + > > + /* Can mmap the entire OA buffer or nothing (no partial OA buffer mmaps) */ > > + if (vma->vm_end - vma->vm_start != XE_OA_BUFFER_SIZE) { > > + drm_dbg(&stream->oa->xe->drm, "Wrong mmap size, must be OA buffer size\n"); > > + return -EINVAL; > > + } > > + > > + /* Only support VM_READ, enforce MAP_PRIVATE by checking for VM_MAYSHARE */ > > + if (vma->vm_flags & (VM_WRITE | VM_EXEC | VM_SHARED | VM_MAYSHARE)) { > > + drm_dbg(&stream->oa->xe->drm, "mmap must be read only\n"); > > + return -EINVAL; > > + } > > + > > + vm_flags_clear(vma, VM_MAYWRITE | VM_MAYEXEC); > > + > > + /* > > + * If the privileged parent forks and child drops root privilege, we do not want > > + * the child to retain access to the mapped OA buffer. Explicitly set VM_DONTCOPY > > + * to avoid such cases. > > + */ > > + vm_flags_set(vma, vma->vm_flags | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_DONTCOPY); > > Would help to just use the vm_flags_mod where you can specify both set and > clear flags. Yes, good idea, done. > > And then just to be paranoid about it, maybe add an assert to check that > the flags applied correctly. Don't think this is needed, code in vm_flags_mod is pretty clear. So skipping this. > Assuming you ran the existing mmap tests for this. Yes, existing IGT's have been ported to Xe. > I think we should also add an mremap case. I think that should fail with > EINVAL since this is a private mapping. Ah, mremap description says "mremap() expands (or shrinks) an existing memory mapping" so the relevant flag seems to be VM_DONTEXPAND rather than VM_SHARED/VM_MAYSHARE. I will see about adding a mremap test. > > > + > > + xe_assert(stream->oa->xe, bo->ttm.ttm->num_pages == > > + (vma->vm_end - vma->vm_start) >> PAGE_SHIFT); > > + for (i = 0; i < bo->ttm.ttm->num_pages; i++) { > > + ret = remap_pfn_range(vma, start, page_to_pfn(bo->ttm.ttm->pages[i]), > > + PAGE_SIZE, vma->vm_page_prot); > > vma->vm_page_prot is set to the state of vm_flags that existed at the > mmap_region() level. We have modified those flags here and we must update > the vma_page_prot with vm_get_page_prot(vma->vm_flags). If we need to do this I think the call to use would be vma_set_page_prot(). But, looking at vm_get_page_prot, vm_flags which affect vma->vm_page_prot are (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED). And note that in this function we have not modified any of these, if (VM_WRITE|VM_EXEC|VM_SHARED) is set, we just return error (vma->vm_page_prot seems to determine what we write to the PTE's and the flags we are modifying here are more like "software" vm_flags). Also we would see some test failures if we didn't do this correctly, and we are not seeing test failures. So I am a little torn about this, vma_set_page_prot() doesn't seem to be really needed, but may be nice to have, but it is also rarely used in the kernel. I am copying Thomas and let's see what he thinks. I'm skipping adding this for now but if Thomas or you say we should add it, I'll go ahead add it. Thanks. -- Ashutosh