On Fri, 22 Aug 2025 at 10:25, CJ Chen <[email protected]> wrote:
>
> From: Tomoyuki Hirose <[email protected]>
>
> The previous code ignored 'impl.unaligned' and handled unaligned
> accesses as-is. But this implementation could not emulate specific
> registers of some devices that allow unaligned access such as xHCI
> Host Controller Capability Registers.
>
> This commit emulates an unaligned access with multiple aligned
> accesses. Additionally, the overwriting of the max access size is
> removed to retrieve the actual max access size.
>
> Based-on-a-patch-by: Tomoyuki Hirose <[email protected]>
> Signed-off-by: CJ Chen <[email protected]>
> Tested-by: CJ Chen <[email protected]>
> Reported-by: Tomoyuki Hirose <[email protected]>
> ---
> system/memory.c | 147 ++++++++++++++++++++++++++++++++++++++---------
> system/physmem.c | 8 ---
> 2 files changed, 119 insertions(+), 36 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 63b983efcd..d6071b4414 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -509,27 +509,118 @@ static MemTxResult
> memory_region_write_with_attrs_accessor(MemoryRegion *mr,
> return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
> }
>
> +typedef MemTxResult (*MemoryRegionAccessFn)(MemoryRegion *mr,
> + hwaddr addr,
> + uint64_t *value,
> + unsigned size,
> + signed shift,
> + uint64_t mask,
> + MemTxAttrs attrs);
So we now have access_emulation and access_fastpath and
the function is_access_fastpath() to select between them.
Can we have a comment please that says what the two are
doing and what the criterion is that lets us pick the fast path ?
> +
> +static MemTxResult access_emulation(hwaddr addr,
> + uint64_t *value,
> + unsigned int size,
> + unsigned int access_size_min,
> + unsigned int access_size_max,
> + MemoryRegion *mr,
> + MemTxAttrs attrs,
> + MemoryRegionAccessFn access_fn_read,
> + MemoryRegionAccessFn access_fn_write,
> + bool is_write)
> +{
> + hwaddr a;
> + uint8_t *d;
> + uint64_t v;
> + MemTxResult r = MEMTX_OK;
> + bool is_big_endian = devend_big_endian(mr->ops->endianness);
> + void (*store)(void *, int, uint64_t) = is_big_endian ? stn_be_p :
> stn_le_p;
> + uint64_t (*load)(const void *, int) = is_big_endian ? ldn_be_p :
> ldn_le_p;
Please use a typedef for all function pointers: it makes it
much easier to read.
> + size_t access_size = MAX(MIN(size, access_size_max), access_size_min);
> + uint64_t access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> + hwaddr round_down = mr->ops->impl.unaligned && addr + size <= mr->size ?
> + 0 : addr % access_size;
> + hwaddr start = addr - round_down;
> + hwaddr tail = addr + size <= mr->size ? addr + size : mr->size;
> + uint8_t data[16] = {0};
> + g_assert(size <= 8);
There should be a blank line after the last variable definition
and before the g_assert() here.
> +
> + for (a = start, d = data, v = 0; a < tail;
> + a += access_size, d += access_size, v = 0) {
> + r |= access_fn_read(mr, a, &v, access_size, 0, access_mask,
> + attrs);
> + store(d, access_size, v);
> + }
> + if (is_write) {
> + stn_he_p(&data[round_down], size, load(value, size));
> + for (a = start, d = data; a < tail;
> + a += access_size, d += access_size) {
> + v = load(d, access_size);
> + r |= access_fn_write(mr, a, &v, access_size, 0, access_mask,
> + attrs);
> + }
> + } else {
> + store(value, size, ldn_he_p(&data[round_down], size));
> + }
This would be much easier to review if there were comments
that said what the intention/design of the code was.
> +
> + return r;
> +}
> +
> +static bool is_access_fastpath(hwaddr addr,
> + unsigned int size,
> + unsigned int access_size_min,
> + unsigned int access_size_max,
> + MemoryRegion *mr)
> +{
> + size_t access_size = MAX(MIN(size, access_size_max), access_size_min);
> + hwaddr round_down = mr->ops->impl.unaligned && addr + size <= mr->size ?
> + 0 : addr % access_size;
> +
> + return round_down == 0 && access_size <= size;
> +}
> +
> +static MemTxResult access_fastpath(hwaddr addr,
> + uint64_t *value,
> + unsigned int size,
> + unsigned int access_size_min,
> + unsigned int access_size_max,
> + MemoryRegion *mr,
> + MemTxAttrs attrs,
> + MemoryRegionAccessFn fastpath)
> +{
> + MemTxResult r = MEMTX_OK;
> + size_t access_size = MAX(MIN(size, access_size_max), access_size_min);
> + uint64_t access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> +
> + if (devend_big_endian(mr->ops->endianness)) {
> + for (size_t i = 0; i < size; i += access_size) {
> + r |= fastpath(mr, addr + i, value, access_size,
> + (size - access_size - i) * 8, access_mask, attrs);
> + }
> + } else {
> + for (size_t i = 0; i < size; i += access_size) {
> + r |= fastpath(mr, addr + i, value, access_size,
> + i * 8, access_mask, attrs);
> + }
> + }
> +
> + return r;
> +}
> +
> static MemTxResult access_with_adjusted_size(hwaddr addr,
> uint64_t *value,
> unsigned size,
> unsigned access_size_min,
> unsigned access_size_max,
> - MemTxResult (*access_fn)
> - (MemoryRegion *mr,
> - hwaddr addr,
> - uint64_t *value,
> - unsigned size,
> - signed shift,
> - uint64_t mask,
> - MemTxAttrs attrs),
> + MemoryRegionAccessFn access_fn_read,
> + MemoryRegionAccessFn access_fn_write,
> + bool is_write,
> MemoryRegion *mr,
> MemTxAttrs attrs)
> {
> - uint64_t access_mask;
> - unsigned access_size;
> - unsigned i;
> MemTxResult r = MEMTX_OK;
> bool reentrancy_guard_applied = false;
> + MemoryRegionAccessFn access_fn_fastpath =
> + is_write ? access_fn_write : access_fn_read;
>
> if (!access_size_min) {
> access_size_min = 1;
> @@ -551,20 +642,16 @@ static MemTxResult access_with_adjusted_size(hwaddr
> addr,
> reentrancy_guard_applied = true;
> }
>
> - /* FIXME: support unaligned access? */
> - access_size = MAX(MIN(size, access_size_max), access_size_min);
> - access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> - if (devend_big_endian(mr->ops->endianness)) {
> - for (i = 0; i < size; i += access_size) {
> - r |= access_fn(mr, addr + i, value, access_size,
> - (size - access_size - i) * 8, access_mask, attrs);
> - }
> + if (is_access_fastpath(addr, size, access_size_min, access_size_max,
> mr)) {
> + r |= access_fastpath(addr, value, size,
> + access_size_min, access_size_max, mr, attrs,
> + access_fn_fastpath);
> } else {
> - for (i = 0; i < size; i += access_size) {
> - r |= access_fn(mr, addr + i, value, access_size, i * 8,
> - access_mask, attrs);
> - }
> + r |= access_emulation(addr, value, size,
> + access_size_min, access_size_max, mr, attrs,
> + access_fn_read, access_fn_write, is_write);
> }
Because you've removed the loops from this function, we don't
any longer need to set r to MEMTX_OK and then OR in the
return value from access_whatever; we can just set r = ...
> +
> if (mr->dev && reentrancy_guard_applied) {
> mr->dev->mem_reentrancy_guard.engaged_in_io = false;
> }
thanks
-- PMM
