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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2026 10:26, Jan Beulich wrote:
> On 14.01.2026 19:29, Oleksii Moisieiev wrote:
>> --- /dev/null
>> +++ b/xen/lib/arm/memcpy_fromio.c
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#include <asm/io.h>
>> +#include <xen/lib/io.h>
>> +
>> +/*
>> + * Use 32-bit raw IO operations for portability across ARM32/ARM64 where
>> + * 64-bit accessors may not be atomic and some devices only support 32-bit
>> + * aligned accesses.
>> + */
>> +
>> +void memcpy_fromio(void *to, const volatile void __iomem *from,
>> +               size_t count)
>> +{
>> +    while ( count && (!IS_ALIGNED((unsigned long)from, 4) ||
>> +                      !IS_ALIGNED((unsigned long)to, 4)) )
> 
> Nit: Xen style indentation (no hard tabs) please throughout.
> 
>> +    {
>> +            *(uint8_t *)to = __raw_readb(from);
>> +            from++;
>> +            to++;
>> +            count--;
>> +    }
>> +
>> +    while ( count >= 4 )
>> +    {
>> +            *(uint32_t *)to = __raw_readl(from);
>> +            from += 4;
>> +            to += 4;
>> +            count -= 4;
>> +    }
>> +
>> +    while ( count )
>> +    {
>> +            *(uint8_t *)to = __raw_readb(from);
>> +            from++;
>> +            to++;
>> +            count--;
>> +    }
>> +}
> 
> Barrier requirements on Arm aren't quite clear to me here: Is it really 
> correct
> to use __raw_read{b,w,l}() here, rather than read{b,w,l}()? If it was, 
> wouldn't
> a barrier then be needed at the end of the function?

Thinking about it, as the order of MMIO accesses needs to be guaranteed
(unless you have extra information about the area's properties, like it
being a video frame buffer), I'm pretty sure now that read{b,w,l}() needs
using here. In fact the comment in the header says that it would handle
"Memory ordering and barriers" when it doesn't look as if it did.

Note how Linux looks to have grown multiple flavors: Besides
memcpy_fromio() I can also spot at least fb_memcpy_fromio() and
sdio_memcpy_fromio().

> And then, if it was read{b,w,l}() that is to be used here, what about all of
> this would then still be Arm-specific? Hmm, I guess the IS_ALIGNED() on "to" 
> is,
> but that's Arm32-specific, with Arm64 not needing it? Plus then it's again not
> exactly Arm-specific, but specific to all architectures where misaligned
> accesses may fault.

There's a bigger issue here, with access granularity (despite the header
claiming "Implement alignment handling for devices requiring specific
access sizes"). MMIO can behave in interesting ways. The header comment
says nothing as to restrictions, i.e. when these functions may not be
used. Yet consider a device registers of which must be accessed in 32-bit
chunks. As long as the other pointer is suitably aligned, all would be
fine. But you handle the case where it isn't, and hence that case then
also needs to function correctly. At the same time accesses to a devices
requiring 16- or 64bit granularity wouldn't work at all, which for
required 8-bit granularity it would again only work partly.

How much of the above requires code adjustments and how much should be
dealt with by updating commentary I don't know, as I know nothing about
your particular use case, nor about possible future ones.

Also note that the header comment still references the ..._relaxed()
functions, when then implementation doesn't use those anymore.

Jan