On 2/19/24 06:12, Jonathan Cameron wrote:
I'm far from confident this handling here is correct. Hence
RFC. In particular not sure on what locks I should hold for this
to be even moderately safe.
The function already appears to be inconsistent in what it returns
as the CONFIG_ATOMIC64 block returns the endian converted 'eventual'
value of the cmpxchg whereas the TCG_OVERSIZED_GUEST case returns
the previous value.
Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
---
v2: Thanks Peter for reviewing.
- Handle the address space as in arm_ldq_ptw() - I should have looked
at the code immediately above :(
The result ends up a little more convoluted than I'd like. Could factor
this block of code out perhaps. I'm also not sure on the fault type
that is appropriate here.
- Switch to 'need_lock' as per Philippe's feedback on the x86 fixes.
likely() doesn't seem appropriate here though.
target/arm/ptw.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 5eb3577bcd..ba1a27ca2b 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -711,8 +711,68 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t
old_val,
void *host = ptw->out_host;
if (unlikely(!host)) {
- fi->type = ARMFault_UnsuppAtomicUpdate;
- return 0;
+ /* Page table in MMIO Memory Region */
+ CPUState *cs = env_cpu(env);
+ MemTxAttrs attrs = {
+ .space = ptw->out_space,
+ .secure = arm_space_is_secure(ptw->out_space),
+ };
+ AddressSpace *as = arm_addressspace(cs, attrs);
+ MemTxResult result = MEMTX_OK;
+ bool need_lock = !bql_locked();
+
+ if (need_lock) {
+ bql_lock();
+ }
+ if (ptw->out_be) {
+ cur_val = address_space_ldq_be(as, ptw->out_phys, attrs, &result);
+ if (unlikely(result != MEMTX_OK)) {
+ fi->type = ARMFault_SyncExternalOnWalk;
+ fi->ea = arm_extabort_type(result);
+ if (need_lock) {
+ bql_unlock();
+ }
+ return old_val;
+ }
Use BQL_LOCK_GUARD() and avoid all of the repeated unlocks at each return point.
You can merge all of the error paths, e.g.
cur_val = (ptw->out_be
? address_space_ldq_be(as, ptw->out_phys, attrs, &result)
: address_space_ldq_le(as, ptw->out_phys, attrs, &result));
if (result == MEMTX_OK && cur_val == old_val) {
if (ptw->out_be) {
address_space_stq_be(as, ptw->out_phys, new_val, attrs, &result);
} else {
address_space_stq_le(as, ptw->out_phys, new_val, attrs, &result);
}
}
if (unlikely(result != MEMTX_OK)) {
fi->type = ...
return old_val;
}
return cur_val;
r~
