From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id C8EEF89856 for ; Mon, 23 Aug 2021 21:31:39 +0000 (UTC) Date: Mon, 23 Aug 2021 14:31:38 -0700 Message-ID: <87czq326h1.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210803200737.30843-2-umesh.nerlige.ramappa@intel.com> References: <20210803200737.30843-1-umesh.nerlige.ramappa@intel.com> <20210803200737.30843-2-umesh.nerlige.ramappa@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH 2/5] i915/perf: Add tests for mapped OA buffer List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Umesh Nerlige Ramappa Cc: igt-dev@lists.freedesktop.org, Lionel G Landwerlin List-ID: On Tue, 03 Aug 2021 13:07:34 -0700, Umesh Nerlige Ramappa wrote: > > For applications that need a faster way to access reports in the OA > buffer, i915 now provides a way to map the OA buffer to privileged user > space. Validate the mapped OA buffer. > > v2: Fail on forked-privileged access to mapped oa buffer (Chris) A few nits/questions below otherwise this is: Reviewed-by: Ashutosh Dixit > diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h > index a1c0030c..bb7d5e73 100644 > --- a/include/drm-uapi/i915_drm.h > +++ b/include/drm-uapi/i915_drm.h > @@ -2151,6 +2151,39 @@ struct drm_i915_perf_open_param { > */ > #define I915_PERF_IOCTL_CONFIG _IO('i', 0x2) > > +/* > + * Returns OA buffer properties to be used with mmap. > + * > + * This ioctl is available in perf revision 8. > + */ > +#define I915_PERF_IOCTL_GET_OA_BUFFER_INFO _IOWR('i', 0x3, struct drm_i915_perf_oa_buffer_info) > + > +/** > + * OA buffer size and offset. > + * > + * OA output buffer > + * type: 0 > + * flags: mbz > + * > + * After querying the info, pass (size,offset) to mmap(), > + * > + * mmap(0, info.size, PROT_READ, MAP_PRIVATE, perf_fd, info.offset). > + * > + * Note that only a private (not shared between processes, or across fork()) > + * read-only mmapping is allowed. > + * > + * Userspace must treat the incoming data as tainted, but it conforms to the OA What does tainted mean? > diff --git a/tests/i915/perf.c b/tests/i915/perf.c > index fa3840eb..4d4808ce 100644 > --- a/tests/i915/perf.c > +++ b/tests/i915/perf.c > @@ -5156,6 +5156,266 @@ static void test_oa_regs_whitelist(int paranoid) > intel_register_access_fini(&mmio_data); > } > > +#define OA_BUFFER_DATA(tail, head, oa_buffer_size) \ > + (((tail) - (head)) & ((oa_buffer_size) - 1)) > + > +#ifndef MAP_FAILED > +#define MAP_FAILED ((void *)-1) > +#endif Shouldn't need this, should just '#include '. > +static uint32_t oa_status_reg(void) > +{ > + uint32_t status; > + > + intel_register_access_init(&mmio_data, intel_get_pci_device(), > + 0, drm_fd); > + if (IS_HASWELL(devid)) > + status = intel_register_read(&mmio_data, 0x2346) & 0x7; > + else if (IS_GEN12(devid)) > + status = intel_register_read(&mmio_data, 0xdafc) & 0x7; > + else > + status = intel_register_read(&mmio_data, 0x2b08) & 0xf; OK, looks like these can be read directly after they are whitelisted by the kernel. > +static void try_invalid_access(void *vaddr) > +{ > + sighandler_t old_sigsegv; > + uint32_t dummy; > + > + old_sigsegv = signal(SIGSEGV, sigtrap); > + switch (sigsetjmp(jmp, SIGSEGV)) { > + case SIGSEGV: > + break; > + case 0: > + dummy = READ_ONCE(*((uint32_t *)vaddr + 1)); I would just read vaddr, not (vaddr + 1). > +static void *map_oa_buffer(uint32_t *size) > +{ > + struct drm_i915_perf_oa_buffer_info oa_buffer = { 0 }; > + void *vaddr; > + > + do_ioctl(stream_fd, I915_PERF_IOCTL_GET_OA_BUFFER_INFO, &oa_buffer); > + > + igt_debug("size = %llu\n", oa_buffer.size); > + igt_debug("offset = %llx\n", oa_buffer.offset); > + > + igt_assert_eq(oa_buffer.size & (oa_buffer.size - 1), 0); > + igt_assert_eq(oa_status_reg(), 0); > + > + vaddr = mmap(0, oa_buffer.size, PROT_READ, MAP_PRIVATE, stream_fd, oa_buffer.offset); > + igt_assert(vaddr != NULL); Should probably be: igt_assert(vaddr != MAP_FAILED && vaddr != NULL);