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
