On Thu, 22 Feb 2024 at 21:21, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> 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.
I think this is a bug in the TCG_OVERSIZED_GUEST handling,
which we've never noticed because you only see that case
on a 32-bit host.
> > 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.
Is the BQL definitely sufficient to ensure atomicity here?
I guess that given that we know it's not backed by host
memory, any other vCPU trying to access the MMIO region
at the same time will have to take the BQL first, so it
seems like it ought to be good enough.
-- PMM
