From: Tomoyuki Hirose <hrstmyk811m@gmail.com>
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 <hrstmyk811m@gmail.com>
Signed-off-by: CJ Chen <cjchen@igel.co.jp>
Tested-by: CJ Chen <cjchen@igel.co.jp>
Reported-by: Tomoyuki Hirose <hrstmyk811m@gmail.com>
---
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);
+
+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;
+ 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);
+
+ 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));
+ }
+
+ 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);
}
+
if (mr->dev && reentrancy_guard_applied) {
mr->dev->mem_reentrancy_guard.engaged_in_io = false;
}
@@ -1450,13 +1537,15 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
memory_region_read_accessor,
- mr, attrs);
+ memory_region_write_accessor,
+ false, mr, attrs);
} else {
return access_with_adjusted_size(addr, pval, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
memory_region_read_with_attrs_accessor,
- mr, attrs);
+ memory_region_write_with_attrs_accessor,
+ false, mr, attrs);
}
}
@@ -1544,15 +1633,17 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
return access_with_adjusted_size(addr, &data, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
- memory_region_write_accessor, mr,
- attrs);
+ memory_region_read_accessor,
+ memory_region_write_accessor,
+ true, mr, attrs);
} else {
return
access_with_adjusted_size(addr, &data, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
+ memory_region_read_with_attrs_accessor,
memory_region_write_with_attrs_accessor,
- mr, attrs);
+ true, mr, attrs);
}
}
diff --git a/system/physmem.c b/system/physmem.c
index a8a9ca309e..9c5f3fbef1 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2864,14 +2864,6 @@ int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
access_size_max = 4;
}
- /* Bound the maximum access by the alignment of the address. */
- if (!mr->ops->impl.unaligned) {
- unsigned align_size_max = addr & -addr;
- if (align_size_max != 0 && align_size_max < access_size_max) {
- access_size_max = align_size_max;
- }
- }
-
/* Don't attempt accesses larger than the maximum. */
if (l > access_size_max) {
l = access_size_max;
--
2.25.1On Fri, 22 Aug 2025 at 10:25, CJ Chen <cjchen@igel.co.jp> wrote:
>
> From: Tomoyuki Hirose <hrstmyk811m@gmail.com>
>
> 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 <hrstmyk811m@gmail.com>
> Signed-off-by: CJ Chen <cjchen@igel.co.jp>
> Tested-by: CJ Chen <cjchen@igel.co.jp>
> Reported-by: Tomoyuki Hirose <hrstmyk811m@gmail.com>
> ---
> 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
© 2016 - 2025 Red Hat, Inc.