Re: [PATCH 1/2] drm/xe: Add read/write debugfs helpers for GGTT node

["Lis, Tomasz" <tomasz.lis@intel.com>]

On 03.11.2024 21:16, Michal Wajdeczko wrote:
> In upcoming patch we want to allow copying and updating PTEs of
> some GGTT nodes over debugfs. Add simple helpers for that.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

No remarks other than two nitpicks, so:

Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>

> ---
>   drivers/gpu/drm/xe/xe_ggtt.c | 178 +++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_ggtt.h |   7 ++
>   2 files changed, 185 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 558fac8bb6fb..e003f5f51ac6 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -838,3 +838,181 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
>   
>   	return total;
>   }
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static size_t copy_iomem_to_user_by_chunk(void __user *to, const void __iomem *from,
> +					  size_t count, void *buf, size_t size)
> +{
> +	size_t chunk;
> +	size_t rem;
> +
> +	while (count) {
> +		chunk = umin(count, size);
> +		memcpy_fromio(buf, from, chunk);
> +		rem = copy_to_user(to, buf, chunk);
> +		count -= chunk - rem;
> +		if (rem)
> +			break;
> +		from += chunk;
> +		to += chunk;
> +	}
> +
> +	return count;
> +}
> +
> +static size_t copy_iomem_to_user(void __user *to, const void __iomem *from, size_t count)
> +{
> +	char chunk[64];
> +	size_t size;
> +	size_t rem;
> +	void *buf;

nit 1 - `chunk` and `buf` could be within an unnamed union. But that 
makes marginal difference, not sure if worth it.

nit 2 - instead of hard-coding 64, CACHELINE_BYTES define could be used.

-Tomasz

> +
> +	if (!count)
> +		return 0;
> +
> +	size = count > sizeof(chunk) ? umin(PAGE_SIZE, count) : 0;
> +	buf = size ? kmalloc(size, GFP_NOWAIT | __GFP_NORETRY) : NULL;
> +	if (buf) {
> +		rem = copy_iomem_to_user_by_chunk(to, from, count, buf, size);
> +		kfree(buf);
> +		return rem;
> +	}
> +
> +	return copy_iomem_to_user_by_chunk(to, from, count, chunk, sizeof(chunk));
> +}
> +
> +static ssize_t simple_read_from_iomem(void __user *to, size_t count, loff_t *ppos,
> +				      const void __iomem *from, size_t available)
> +{
> +	loff_t pos = *ppos;
> +	size_t rem;
> +
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= available || !count)
> +		return 0;
> +	if (count > available - pos)
> +		count = available - pos;
> +
> +	rem = copy_iomem_to_user(to, from + pos, count);
> +	if (rem == count)
> +		return -EFAULT;
> +
> +	count -= rem;
> +	*ppos = pos + count;
> +	return count;
> +}
> +
> +/**
> + * xe_ggtt_node_read() - Copy PTEs from the GGTT node to the user space buffer
> + * @node: the GGTT node to read from
> + * @buf: the user space buffer to read to
> + * @count: the maximum number of bytes to read
> + * @ppos: the current position
> + *
> + * Return: On success, the number of bytes read is returned and the offset
> + * @ppos is advanced by this number, or negative value is returned on error.
> + */
> +ssize_t xe_ggtt_node_read(struct xe_ggtt_node *node, char __user *buf,
> +			  size_t count, loff_t *ppos)
> +{
> +	if (!xe_ggtt_node_allocated(node))
> +		return 0;
> +
> +	xe_tile_assert(node->ggtt->tile, IS_ALIGNED(node->base.start, XE_PAGE_SIZE));
> +	xe_tile_assert(node->ggtt->tile, IS_ALIGNED(node->base.size, XE_PAGE_SIZE));
> +
> +	return simple_read_from_iomem(buf, count, ppos,
> +				      &node->ggtt->gsm[node->base.start / XE_PAGE_SIZE],
> +				      size_mul(sizeof(u64), node->base.size / XE_PAGE_SIZE));
> +}
> +
> +static size_t copy_iomem_from_user_by_chunk(void __iomem *to, const void __user *from,
> +					    size_t count, void *buf, size_t size)
> +{
> +	size_t chunk;
> +	size_t rem;
> +
> +	while (count) {
> +		chunk = umin(count, size);
> +		rem = copy_from_user(buf, from, chunk);
> +		memcpy_toio(to, buf, chunk - rem);
> +		count -= chunk - rem;
> +		if (rem)
> +			break;
> +		from += chunk;
> +		to += chunk;
> +	}
> +
> +	return count;
> +}
> +
> +static size_t copy_iomem_from_user(void __iomem *to, const void __user *from, size_t count)
> +{
> +	char chunk[64];
> +	size_t size;
> +	size_t rem;
> +	void *buf;
> +
> +	if (!count)
> +		return 0;
> +
> +	size = count > sizeof(chunk) ? umin(PAGE_SIZE, count) : 0;
> +	buf = size ? kmalloc(size, GFP_NOWAIT | __GFP_NORETRY) : NULL;
> +	if (buf) {
> +		rem = copy_iomem_from_user_by_chunk(to, from, count, buf, size);
> +		kfree(buf);
> +		return rem;
> +	}
> +
> +	return copy_iomem_from_user_by_chunk(to, from, count, chunk, sizeof(chunk));
> +}
> +
> +static ssize_t simple_write_to_iomem(void __iomem *to, size_t available, loff_t *ppos,
> +				     const void __user *from, size_t count)
> +{
> +	loff_t pos = *ppos;
> +	size_t rem;
> +
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= available || !count)
> +		return 0;
> +	if (count > available - pos)
> +		count = available - pos;
> +
> +	rem = copy_iomem_from_user(to + pos, from, count);
> +	if (rem == count)
> +		return -EFAULT;
> +
> +	count -= rem;
> +	*ppos = pos + count;
> +	return count;
> +}
> +
> +/**
> + * xe_ggtt_node_write() - Update PTEs of the GGTT node using data from the user space buffer
> + * @node: the GGTT node to write to
> + * @buf: the user space buffer to read from
> + * @count: the maximum number of bytes to read
> + * @ppos: the current position
> + *
> + * Return: On success, the number of bytes written is returned and the offset
> + * @ppos is advanced by this number, or negative value is returned on error.
> + */
> +ssize_t xe_ggtt_node_write(struct xe_ggtt_node *node, const char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	if (!xe_ggtt_node_allocated(node))
> +		return -ENXIO;
> +
> +	xe_tile_assert(node->ggtt->tile, IS_ALIGNED(node->base.start, XE_PAGE_SIZE));
> +	xe_tile_assert(node->ggtt->tile, IS_ALIGNED(node->base.size, XE_PAGE_SIZE));
> +
> +	return simple_write_to_iomem(&node->ggtt->gsm[node->base.start / XE_PAGE_SIZE],
> +				     size_mul(sizeof(u64), node->base.size / XE_PAGE_SIZE),
> +				     ppos, buf, count);
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 27e7d67de004..64746e23053e 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -34,6 +34,13 @@ u64 xe_ggtt_largest_hole(struct xe_ggtt *ggtt, u64 alignment, u64 *spare);
>   int xe_ggtt_dump(struct xe_ggtt *ggtt, struct drm_printer *p);
>   u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer *p);
>   
> +#ifdef CONFIG_DEBUG_FS
> +ssize_t xe_ggtt_node_read(struct xe_ggtt_node *node, char __user *buf,
> +			  size_t count, loff_t *pos);
> +ssize_t xe_ggtt_node_write(struct xe_ggtt_node *node, const char __user *buf,
> +			   size_t count, loff_t *pos);
> +#endif
> +
>   #ifdef CONFIG_PCI_IOV
>   void xe_ggtt_assign(const struct xe_ggtt_node *node, u16 vfid);
>   #endif