Re: [RFC PATCH v2] arm/ptw: Handle atomic updates of page tables entries in MMIO during PTW.

[Richard Henderson <richard.henderson@linaro.org>]

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.Cameron@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~