Re: [PATCH v8 3/5] lib/arm: Add I/O memory copy helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2026 19:43, Oleksii Moisieiev wrote:
> This commit introduces two helper functions, `memcpy-fromio` and
> `memcpy-toio`, to provide a robust mechanism for copying data between
> standard memory and memory-mapped I/O (MMIO) space for the ARM
> architecture.

No helpers of this name are being introduced, as what's spelled out above aren't
even identifiers. Also instead of the quoting we've been trying to uniformly
identify functions in descriptions by adding parentheses: memcpy_fromio(). Plus,
nit: Please don't use "This commit ..." or alike in descriptions.

> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -10,6 +10,7 @@ endif
>  obj-y += firmware/
>  obj-$(CONFIG_TEE) += tee/
>  obj-$(CONFIG_HAS_VPCI) += vpci.o
> +obj-y += lib/

Yes, sorting in this section was already screwed. But why make the problem 
worse?

> --- a/xen/arch/arm/arch.mk
> +++ b/xen/arch/arm/arch.mk
> @@ -2,6 +2,7 @@
>  # arm-specific definitions
>  
>  ARCH_LIBS-y += arch/arm/$(ARCH)/lib/lib.a
> +ALL_LIBS-y += arch/arm/lib/lib.a

Conceivable generic helpers of the same names could be introduced. In that case
this choice of yours would lead to them being used, instead of the Arm ones. IOW
I think you want to add to ARCH_LIBS here.

> --- /dev/null
> +++ b/xen/arch/arm/lib/memcpy-fromio.c
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#include <asm/io.h>
> +#include <xen/lib/io.h>

Preferably the other way around, and with a blank line between them. (But see 
below
as to the header being generic; if it wasn't, this remark wouldn't apply 
anymore.)

> --- /dev/null
> +++ b/xen/include/xen/lib/io.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Generic I/O memory copy function prototypes.
> + *
> + * These functions provide low-level implementation for copying data between
> + * regular memory and I/O memory regions. Each architecture must provide its
> + * own implementation based on the specific requirements of the 
> architecture's
> + * memory model and I/O access patterns. An architecture may supply these as
> + * functions or as macros in its own headers before including this file.
> + *
> + * Architecture-specific implementations:
> + * =====================================
> + * Each architecture should implement these functions in xen/lib/<arch>/io.c
> + * (or define them as macros) based on their hardware requirements. See
> + * xen/lib/arm/io.c for an example using explicit I/O accessors.
> + */

The file name referenced is unhelpful and actually wrong for the Arm functions
you add here.

> +#ifndef _XEN_LIB_IO_H
> +#define _XEN_LIB_IO_H
> +
> +#include <xen/types.h>
> +
> +/*
> + * memcpy_fromio - Copy data from I/O memory space to regular memory
> + * @to: Destination buffer in regular memory
> + * @from: Source address in I/O memory space (must be marked __iomem)
> + * @count: Number of bytes to copy
> + *
> + * This function handles copying from memory-mapped I/O regions using
> + * architecture-appropriate I/O accessor functions (e.g. readb/readl on Arm)
> + * that already impose the required ordering for device accesses. Typical
> + * implementations may:
> + * - Handle leading/trailing unaligned bytes with 8-bit accesses

This is either imprecise, or the implementation is wrong: From context, this
ought to be talking solely of the MMIO side of the operation. Yet if src and
dst are misaligned with one another, you'd do the entire operation in 8-bit
chunks. For devices requiring aligned 32-bit accesses that won't work at all.

> + * - Use the widest safe aligned access size supported by the target (often
> + *   32-bit on Arm where 64-bit MMIO may not be atomic)
> + * - Rely on MMIO accessors to provide the needed barriers
> + *
> + * Limitations:
> + * - Only suitable for devices that tolerate 8-bit and 32-bit accesses; it is
> + *   not valid for devices that require strictly 16-bit or 64-bit access 
> sizes.
> + * - Callers must ensure the target MMIO region is mapped with appropriate
> + *   device attributes.
> + */

The description is now valid for the Arm implementation you supply, but the
header we're in is a generic one. Imo, generic constraints should be reduced
as much as possible, like dealing with leading / trailing sub-32-bit items
by doing at most one 8-bit access followed by at most one 16-bit one (the
other way around for the trailing part). Or else the header should be Arm-
only as well (more strict constraints on Arm would make these functions
potentially unusable from generic code, after all).

Along these lines, "device attributes" is Arm terminology, aiui.

Also, if indeed a generic header, why xen/lib/io.h and not xen/io.h (which
already exists)?

Jan