Hi Daniel,
On 2/6/26 21:08, Daniel Henrique Barboza wrote:
memory_region_access_valid() returns true and false and nothing else,
but it makes distintions between error types that we can see via each
error_log message: if mr->ops->valid.accepts fails the reason is
set to "rejected", reason is set to "unaligned" if addr is unaligned
and reason is set to "invalid size" if size is less that min_access_size
or greater than max_access_size.
We're particularly interested in the "reason: rejected" result. In
RISC-V this is thrown when the mem_op tries to access a protected page,
and we need to throw a special exception for this case.
memory_region_dispatch_read() (and write()) will default to
MEMTX_DECODE_ERROR because memory_region_access_valid() does not give
additional info when it fails, and back in the RISC-V side we're having
to make assumptions to determine that this MEMTX_DECODE_ERROR is in fact
a "rejected" error type.
Add a new MemTxResult arg in memory_region_access_valid() that the
funcion will use to set the reason why it failed. Based on the error
reasons (described above) and the definition of each MemTxResult, we're
setting the following results:
- result ok: MEMTX_OK
- rejected: MEMTX_ACCESS_ERROR
- all other errors: MEMTX_DECODE_ERROR
Orthogonal, in "hw/pci/pci_device.h":
/**
* pci_dma_rw: Read from or write to an address space from PCI device.
*
* Return a MemTxResult indicating whether the operation succeeded
* or failed (eg unassigned memory, device rejected the transaction,
* IOMMU fault).
This could deserve clarification as the mapping is not obvious to me.
Without looking at the code, I'd expect:
. unassigned memory -> MEMTX_DECODE_ERROR
. device rejected the transaction -> MEMTX_ACCESS_ERROR
. IOMMU fault -> MEMTX_ACCESS_ERROR
What confuses me I guess is having 3 descriptions for 2 enums.
Existing callers are unchanged. No behavioral change is intended for
now.
Signed-off-by: Daniel Henrique Barboza <[email protected]>
---
hw/s390x/s390-pci-inst.c | 2 +-
include/system/memory.h | 2 +-
system/memory.c | 20 +++++++++++++++++---
system/physmem.c | 3 ++-
4 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/include/system/memory.h b/include/system/memory.h
index 1417132f6d..d74fae52e3 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2602,7 +2602,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool
owner, bool disabled);
Please update the docstring.
bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
unsigned size, bool is_write,
- MemTxAttrs attrs);
+ MemTxAttrs attrs, MemTxResult *result);
/**
* memory_region_dispatch_read: perform a read directly to the specified
diff --git a/system/memory.c b/system/memory.c
index 739ba11da6..48348edb96 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1403,14 +1403,22 @@ bool memory_region_access_valid(MemoryRegion *mr,
hwaddr addr,
unsigned size,
bool is_write,
- MemTxAttrs attrs)
+ MemTxAttrs attrs,
+ MemTxResult *result)
{
+ if (result) {
+ *result = MEMTX_OK;
Per "exec/memattrs.h":
/* New-style MMIO accessors can indicate that the transaction failed.
* A zero (MEMTX_OK) response means success; anything else is a
* failure of some kind. The memory subsystem will bitwise-OR together
* results if it is synthesizing an operation from multiple smaller
* accesses.
*/
The caller should initialize to MEMTX_OK.
+ }
+
if (mr->ops->valid.accepts
&& !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
", size %u, region '%s', reason: rejected\n",
is_write ? "write" : "read",
addr, size, memory_region_name(mr));
+ if (result) {
+ *result = MEMTX_ACCESS_ERROR;
Here I'd OR.
+ }
return false;
}
@@ -1419,6 +1427,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
", size %u, region '%s', reason: unaligned\n",
is_write ? "write" : "read",
addr, size, memory_region_name(mr));
+ if (result) {
+ *result = MEMTX_DECODE_ERROR;
Ditto.
+ }
return false;
}
@@ -1436,6 +1447,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
addr, size, memory_region_name(mr),
mr->ops->valid.min_access_size,
mr->ops->valid.max_access_size);
+ if (result) {
+ *result = MEMTX_DECODE_ERROR;
Ditto.
+ }
return false;
}
return true;
@@ -1478,7 +1492,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
mr->alias_offset + addr,
pval, op, attrs);
}
- if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
+ if (!memory_region_access_valid(mr, addr, size, false, attrs, NULL)) {
Surely here you want to propagate the dispatched MR result.
*pval = unassigned_mem_read(mr, addr, size);
return MEMTX_DECODE_ERROR;
}
@@ -1527,7 +1541,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
mr->alias_offset + addr,
data, op, attrs);
}
- if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
+ if (!memory_region_access_valid(mr, addr, size, true, attrs, NULL)) {
unassigned_mem_write(mr, addr, data, size);
return MEMTX_DECODE_ERROR;
Also here.
}
diff --git a/system/physmem.c b/system/physmem.c
index 7bcbf87573..a0f77aae66 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3635,7 +3635,8 @@ static bool flatview_access_valid(FlatView *fv, hwaddr
addr, hwaddr len,
mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
if (!memory_access_is_direct(mr, is_write, attrs)) {
l = memory_access_size(mr, l, addr);
- if (!memory_region_access_valid(mr, xlat, l, is_write, attrs)) {
+ if (!memory_region_access_valid(mr, xlat, l, is_write, attrs,
+ NULL)) {
return false;
}
}
