Re: [igt-dev] [PATCH 2/5] i915/perf: Add tests for mapped OA buffer

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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 <ashutosh.dixit@intel.com>

> 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 <sys/mman.h>'.

> +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);