On Tue, 11 Oct 2022 at 04:43, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> Perform the atomic update for hardware management of the access flag.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> v4: Raise permission fault if pte read-only and atomic update reqd.
> Split out dirty bit portion.
> Prepare for a single update for AF + DB.
> +static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
> + uint64_t new_val, S1Translate *ptw,
> + ARMMMUFaultInfo *fi)
> +{
> + uint64_t cur_val;
> + void *host = ptw->out_host;
> +
> + if (unlikely(!host)) {
> + fi->type = ARMFault_UnsuppAtomicUpdate;
> + fi->s1ptw = true;
> + return 0;
> + }
> +
> + /*
> + * Raising a stage2 Protection fault for an atomic update to a read-only
> + * page is delayed until it is certain that there is a change to make.
> + */
> + if (unlikely(!ptw->out_rw)) {
> + int flags;
> + void *discard;
> +
> + env->tlb_fi = fi;
> + flags = probe_access_flags(env, ptw->out_virt, MMU_DATA_STORE,
> + arm_to_core_mmu_idx(ptw->in_ptw_idx),
> + true, &discard, 0);
> + env->tlb_fi = NULL;
> +
> + if (unlikely(flags & TLB_INVALID_MASK)) {
> + assert(fi->type != ARMFault_None);
> + fi->s2addr = ptw->out_virt;
> + fi->stage2 = true;
> + fi->s1ptw = true;
> + fi->s1ns = ptw->in_secure;
Shouldn't there be a ! here ? LHS is true-for-NS and RHS is true-for-S, I think.
> + return 0;
> + }
> +
> + /* In case CAS mismatches and we loop, remember writability. */
> + ptw->out_rw = true;
> + }
> +
> +#ifdef CONFIG_ATOMIC64
> + if (ptw->out_be) {
> + old_val = cpu_to_be64(old_val);
> + new_val = cpu_to_be64(new_val);
> + cur_val = qatomic_cmpxchg__nocheck((uint64_t *)host, old_val,
> new_val);
> + cur_val = be64_to_cpu(cur_val);
> + } else {
> + old_val = cpu_to_le64(old_val);
> + new_val = cpu_to_le64(new_val);
> + cur_val = qatomic_cmpxchg__nocheck((uint64_t *)host, old_val,
> new_val);
> + cur_val = le64_to_cpu(cur_val);
> + }
> +#else
> + /*
> + * We can't support the full 64-bit atomic cmpxchg on the host.
> + * Because this is only used for FEAT_HAFDBS, which is only for AA64,
> + * we know that TCG_OVERSIZED_GUEST is set, which means that we are
> + * running in round-robin mode and could only race with dma i/o.
> + */
> +#ifndef TCG_OVERSIZED_GUEST
> +# error "Unexpected configuration"
> +#endif
> + bool locked = qemu_mutex_iothread_locked();
> + if (!locked) {
> + qemu_mutex_lock_iothread();
> + }
> + if (ptw->out_be) {
> + cur_val = ldq_be_p(host);
> + if (cur_val == old_val) {
> + stq_be_p(host, new_val);
> + }
> + } else {
> + cur_val = ldq_le_p(host);
> + if (cur_val == old_val) {
> + stq_le_p(host, new_val);
> + }
> + }
> + if (!locked) {
> + qemu_mutex_unlock_iothread();
> + }
> +#endif
> +
> + return cur_val;
> +}
> @@ -1286,7 +1389,9 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> S1Translate *ptw,
> if (fi->type != ARMFault_None) {
> goto do_fault;
> }
> + new_descriptor = descriptor;
>
> + restart_atomic_update:
> if (!(descriptor & 1) || (!(descriptor & 2) && (level == 3))) {
> /* Invalid, or the Reserved level 3 encoding */
> goto do_translation_fault;
> @@ -1362,10 +1467,18 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> S1Translate *ptw,
> * Here descaddr is the final physical address, and attributes
> * are all in attrs.
> */
> - if ((attrs & (1 << 10)) == 0) {
> - /* Access flag */
> - fi->type = ARMFault_AccessFlag;
> - goto do_fault;
> + if (!(attrs & (1 << 10)) && !ptw->in_debug) {
> + /*
> + * Access flag.
> + * If HA is enabled, prepare to update the descriptor below.
> + * Otherwise, pass the access fault on to software.
> + */
> + if (param.ha) {
> + new_descriptor |= 1 << 10; /* AF */
> + } else {
> + fi->type = ARMFault_AccessFlag;
> + goto do_fault;
> + }
> }
>
> ap = extract32(attrs, 6, 2);
> @@ -1381,6 +1494,18 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> S1Translate *ptw,
> result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap, ns, xn, pxn);
> }
>
> + /* If FEAT_HAFDBS has made changes, update the PTE. */
> + if (new_descriptor != descriptor) {
> + new_descriptor = arm_casq_ptw(env, descriptor, new_descriptor, ptw,
> fi);
> + if (fi->type != ARMFault_None) {
> + goto do_fault;
> + }
> + if (new_descriptor != descriptor) {
I think we could probably usefully add a comment here:
/*
* I_YZSVV says that if the in-memory descriptor has changed, then we
* must use the information in that new value (which might include a
* different output address, different attributes, or generate a fault),
* Restart the handling of the descriptor value from scratch.
*/
Otherwise
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
thanks
-- PMM
