Re: [PATCH i-g-t 2/4] lib/intel_vram: Add library for VRAM accesses

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 7/17/2025 11:08 AM, Lukasz Laguna wrote:

Hi Łukasz,

Thanks for the patch. I feel guilty as a co-author of the original code,
but since we are moving it into a library maybe we can do a few small
API tweaks to make it safer on 32-bit and easier to extend.

> Prepare a library with helpers to map and unmap VRAM BAR region and
> perform read/write operations.
> 
> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
> ---
>   lib/intel_vram.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
>   lib/intel_vram.h |  22 ++++++
>   lib/meson.build  |   1 +
>   3 files changed, 206 insertions(+)
>   create mode 100644 lib/intel_vram.c
>   create mode 100644 lib/intel_vram.h
> 
> diff --git a/lib/intel_vram.c b/lib/intel_vram.c
> new file mode 100644
> index 000000000..38e2ccf36
> --- /dev/null
> +++ b/lib/intel_vram.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +
> +#include "igt_core.h"
> +#include "igt_sriov_device.h"
> +#include "intel_vram.h"
> +
> +static int intel_vram_open_bar(int pf_fd, unsigned int vf_num, int mode)
> +{
> +	int sysfs, fd;
> +
> +	sysfs = igt_sriov_device_sysfs_open(pf_fd, vf_num);
> +	if (sysfs < 0) {
> +		igt_debug("Failed to open sysfs for VF%d: %s\n", vf_num, strerror(errno));
> +		return -1;
> +	}
> +
> +	fd = openat(sysfs, "resource2", mode | O_SYNC);

O_SYNC has no effect on mmap, can be dropped.

> +	if (fd < 0)
> +		igt_debug("Failed to open resource2 for VF%d: %s\n", vf_num, strerror(errno));
> +
> +	close(sysfs);
> +
> +	return fd;
> +}
> +
> +/**
> + * intel_vram_bar_size - Get the size of the VRAM BAR
> + * @pf_fd: PF device file descriptor
> + * @vf_num: VF number (1-based), or 0 for the PF
> + *
> + * Opens the VRAM BAR file descriptor for the specified device and retrieves
> + * its size by using fstat().
> + *
> + * Return: The size of the VRAM BAR in bytes on success,
> + *         or negative value on failure.
> + */
> +ssize_t intel_vram_bar_size(int pf_fd, unsigned int vf_num)
> +{
> +	int fd;
> +	struct stat st;
> +
> +	fd = intel_vram_open_bar(pf_fd, vf_num, O_RDONLY);
> +	if (fd < 0)
> +		return fd;
> +
> +	if (fstat(fd, &st)) {
> +		igt_debug("Failed to stat resource2 for VF%u: %s\n", vf_num, strerror(errno));
> +		close(fd);
> +		return -1;
> +	}
> +
> +	close(fd);
> +
> +	return st.st_size;
> +}

Using ssize_t may be problematic: on 32-bit it truncates BARs >2 GiB.
maybe return int and fill a uint64_t *out_size.

int intel_vram_bar_size(int pf_fd, unsigned int vf_num, uint64_t *out_size)
{
	int fd;

	if (!out_size)
		return -EINVAL;
	fd = intel_vram_open_bar(pf_fd, vf_num, O_RDONLY);
	if (fd < 0)
		return fd;

	struct stat st;
	if (fstat(fd, &st)) {
		int saved_err = errno;

		close(fd);
		return -saved_err;
	}
	close(fd);
	if (st.st_size < 0)
		return -EOVERFLOW;
	*out_size = (uint64_t)st.st_size;
	return 0;
}

> +
> +/**
> + * intel_vram_mmap - Map VRAM BAR region
> + * @pf_fd: PF device file descriptor
> + * @vf_num: VF number (1-based), or 0 for the PF
> + * @length: Number of bytes to map
> + * @prot: Memory protection flags
> + * @offset: Offset (in bytes) within the BAR to begin the mapping
> + *
> + * Maps a PF or VF VRAM BAR into user space using mmap().
> + *
> + * Return: A vram_mapping struct with the mapped address and size on success,
> + *         or NULL address and zero size on failure.
> + */
> +struct vram_mapping intel_vram_mmap(int pf_fd, unsigned int vf_num, size_t length, int prot,
> +				    off_t offset)

Returning struct by value hides error codes and may be problematic to 
changes (ex. extending fields). As it's a library maybe better to return 
int (for error codes and let callers branch on cause, assert) and fill a 
pointer to struct. Also, offsets are unsigned and large, maybe use 
uint64_t for offset in the API, and cast down to off_t only at mmap().

> +{
> +	int open_flags = ((prot & PROT_WRITE) != 0) ? O_RDWR : O_RDONLY;
> +	struct vram_mapping m = { .addr = NULL, .size = 0 };
> +	ssize_t vram_bar_size;
> +	int vram_bar_fd;
> +	void *addr;
> +
> +	vram_bar_size = intel_vram_bar_size(pf_fd, vf_num);
> +	if (vram_bar_size < 0) {
> +		igt_debug("Failed to get VRAM BAR size for VF%u: %s\n", vf_num, strerror(errno));
> +		return m;
> +	}
> +
> +	if (vram_bar_size < length) {
> +		igt_debug("Mapping length (%zu) exceeds VRAM BAR size (%" PRIu64 ")\n",
> +			  length, (uint64_t)vram_bar_size);
> +		return m;
> +	}

Needs a proper range check: offset + length <= bar_size

> +
> +	vram_bar_fd = intel_vram_open_bar(pf_fd, vf_num, open_flags);
> +	if (vram_bar_fd < 0) {
> +		igt_debug("Failed to open VRAM BAR file for VF%u: %s\n", vf_num, strerror(errno));
> +		return m;
> +	}
> +
> +	addr = mmap(NULL, length, prot, MAP_SHARED, vram_bar_fd, offset);
> +	close(vram_bar_fd);
> +	if (addr == MAP_FAILED) {
> +		igt_debug("Failed to map VRAM BAR for VF%u: %s\n", vf_num, strerror(errno));
> +		return m;
> +	}
> +
> +	m.addr = addr;
> +	m.size = length;
> +	return m;
> +}

Proposed version:

int intel_vram_mmap(int pf_fd, unsigned int vf_num, uint64_t offset,
		    size_t length, int prot, struct vram_mapping *out)
{
	uint64_t bar_size, end;
	int fd, ret, saved_err;
	void *addr;

	if (!out)
		return -EINVAL;
	out->addr = NULL;
	out->size = 0;
	if (!length)
		return 0;

	ret = intel_vram_bar_size(pf_fd, vf_num, &bar_size);
	if (ret)
		return ret;

	end = offset + (uint64_t)length;
	if (end < offset || end > bar_size)
		return -EINVAL;

	fd = intel_vram_open_bar(pf_fd, vf_num,
							 (prot & PROT_WRITE) ? O_RDWR : O_RDONLY);
	if (fd < 0)
		return fd;

	addr = mmap(NULL, length, prot, MAP_SHARED, fd, (off_t)offset);
	saved_err = errno;
	close(fd);
	if (addr == MAP_FAILED)
		return -saved_err;

	out->addr = addr;
	out->size = length;
	return 0;
}

> +
> +/**
> + * intel_vram_munmap - Unmap previously mapped VRAM region
> + * @m: Pointer to a vram_mapping struct representing the mapped region
> + *
> + * Unmaps the user-space memory region previously mapped by intel_vram_mmap().
> + *
> + * Return: 0 on success, or negative value on failure.
> + */
> +int intel_vram_munmap(struct vram_mapping *m)
> +{
> +	int ret;
> +
> +	ret = munmap(m->addr, m->size);
> +	if (ret < 0)
> +		igt_debug("Failed munmap %p: %s\n", m->addr, strerror(errno));
> +
> +	return ret;
> +}
> +
> +/**
> + * intel_vram_read8 - Read 8-bit value from a mapped VRAM region
> + * @m: Pointer to a vram_mapping struct representing the mapped region
> + * @offset: Offset (in bytes) to read from
> + *
> + * Reads a single 8-bit value from the specified offset in the mapped VRAM.
> + *
> + * Return: The 8-bit value read from the given offset.
> + */
> +uint8_t intel_vram_read8(struct vram_mapping *m, size_t offset)
> +{
> +	igt_assert(offset < m->size);
> +
> +	return READ_ONCE(*((uint8_t *)m->addr + offset));
> +}
> +
> +/**
> + * intel_vram_write8 - Write 8-bit value to a mapped VRAM region
> + * @m: Pointer to a vram_mapping struct representing the mapped region
> + * @offset: Offset (in bytes) to write to
> + * @value: The 8-bit value to write
> + *
> + * Writes a single 8-bit value to the specified offset in the mapped VRAM.
> + */
> +void intel_vram_write8(struct vram_mapping *m, size_t offset, uint8_t value)
> +{
> +	igt_assert(offset < m->size);
> +
> +	WRITE_ONCE(*((uint8_t *)m->addr + offset), value);
> +}
> +
> +/**
> + * intel_vram_write_readback8 - Write and then read back an 8-bit value
> + * @m: Pointer to a vram_mapping struct representing the mapped region
> + * @offset: Offset (in bytes) to write to and read from
> + * @value: The 8-bit value to write
> + *
> + * Writes an 8-bit value to the specified offset in the mapped VRAM and
> + * reads it back to verify if the write was successful.
> + *
> + * Return: The 8-bit value read back from the given offset.
> + */
> +uint8_t intel_vram_write_readback8(struct vram_mapping *m, size_t offset, uint8_t value)
> +{
> +	intel_vram_write8(m, offset, value);
> +	return intel_vram_read8(m, offset);
> +}
> diff --git a/lib/intel_vram.h b/lib/intel_vram.h
> new file mode 100644
> index 000000000..b93f915e9
> --- /dev/null
> +++ b/lib/intel_vram.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#ifndef __INTEL_VRAM_H__
> +#define __INTEL_VRAM_H__
> +
> +struct vram_mapping {
> +	void *addr; /* Pointer to the mapped VRAM */
> +	size_t size; /* Size of the mapped VRAM region */
> +};
> +
> +ssize_t intel_vram_bar_size(int pf_fd, unsigned int vf_num);
> +struct vram_mapping intel_vram_mmap(int pf_fd, unsigned int vf_num, size_t length, int prot,
> +				    off_t offset);
> +int intel_vram_munmap(struct vram_mapping *m);
> +uint8_t intel_vram_read8(struct vram_mapping *m, size_t offset);
> +void intel_vram_write8(struct vram_mapping *m, size_t offset, uint8_t value);
> +uint8_t intel_vram_write_readback8(struct vram_mapping *m, size_t offset, uint8_t value);
> +

Proposed API

/*
  * Design choices:
  *
  * - Use uint64_t for BAR sizes/offsets (unsigned device semantics, no 
truncation on 32-bit).
  * - Functions return int (0 on success, -errno on failure).
  * - No internal alignment: if offset isn’t page aligned, mmap() will 
fail with EINVAL.
  */

int  intel_vram_bar_size(int pf_fd, unsigned int vf_num, uint64_t 
*out_size);
int  intel_vram_mmap(int pf_fd, unsigned int vf_num,
                      uint64_t offset, size_t length, int prot,
                      struct vram_mapping *out);
...

> +#endif	/* INTEL_VRAM_H */
> diff --git a/lib/meson.build b/lib/meson.build
> index 2eaca42a4..25c68011d 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -70,6 +70,7 @@ lib_sources = [
>   	'intel_mocs.c',
>   	'igt_multigpu.c',
>   	'intel_pat.c',
> +	'intel_vram.c',
>   	'ioctl_wrappers.c',
>   	'media_spin.c',
>   	'media_fill.c',