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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On 15/01/2026 13:59, Jan Beulich wrote:
> 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
Hi Jan,

Thank you very much for your valuable input and involvement.

After further research and reconsidering all the points you raised, I 
have decided
to switch to using the ordered MMIO accessors (readb/readl and 
writeb/writel).

Here is my reasoning:

The concern you mentioned was valid: the use of __raw_read*/__raw_write* 
in the
helpers did not provide the ordering guarantees expected for MMIO 
copies, and the
header still referenced *_relaxed, even though the implementation no 
longer used
them. This could allow reordering around the copy and misrepresent the 
intended
semantics.

A few additional thoughts regarding your other questions:

Is it still Arm-specific?
Functionally, the logic could work on any architecture that supports 
8/32-bit MMIO
accesses and uses the same accessor semantics. However, this implementation
relies on Arm’s read*/write* ordering guarantees, as well as the 
Arm-specific
IS_ALIGNED check to avoid misaligned faults. Therefore, it remains 
Arm-specific
as written; other architectures would need their own implementations if they
have different alignment or granularity requirements.

Ordered accessors vs. raw accessors + trailing barrier:
I chose to use ordered accessors instead of raw accessors with a 
trailing barrier
because readb/readl and writeb/writel already provide the required 
device ordering
and barrier semantics, making an additional barrier unnecessary and 
solving potential
ordering issues.

Use for register access:
These helpers are suitable for MMIO buffers, shared memory, and 
registers that
tolerate 8-/32-bit accesses. They are not appropriate for registers that
require 16- or 64-bit accesses, or where side effects depend on the exact
access width. I'll update the header to document this limitation; 
drivers needing strict
register semantics should continue to use readl/writel
(or readw/writew/readq/writeq) directly, rather than memcpy_*io().

Summary:

I have made the following changes to the helper functions:

- switched to ordered accessors to address the ordering and barrier 
concerns.
- updated the documentation to match the implementation and explicitly state
the supported access sizes and granularity.

If this approach sounds good to you, I will proceed with the fix and 
submit a new version.

Best regards,
Oleksii