Re: [RFC PATCH 3/3] target/ppc: Implement reservation protection for larx/stcx

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

On 2/19/24 18:19, Nicholas Piggin wrote:
> +    env->access_type = ACCESS_RES;
> +    host = probe_access(env, addr, size, MMU_DATA_LOAD, mmu_idx, raddr);
> +    if (host) {
> +        cpu_set_llsc_prot(cs, qemu_ram_addr_from_host_nofail(host));
> +    } else {
> +        /* XXX: fault? */
> +        g_assert_not_reached();
> +    }

probe_access will not return a host address for lots of reasons, including watchpoints, 
plugins, etc.

> +
> +    if (unlikely(size == 16)) {
> +        Int128 val16;
> +        val16 = cpu_ld16_mmu(env, addr,
> +                     make_memop_idx(DEF_MEMOP(env, MO_128 | MO_ALIGN), mmu_idx),
> +                     raddr);
> +        env->gpr[reg] = int128_gethi(val16);
> +        env->gpr[reg + 1] = int128_getlo(val16);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 1:
> +        val = ldub_p(host);
> +        break;
> +    case 2:
> +        val = FIELD_EX64(env->msr, MSR, LE) ? lduw_le_p(host) : lduw_be_p(host);
> +        break;
> +    case 4:
> +        val = FIELD_EX64(env->msr, MSR, LE) ? ldl_le_p(host) : ldl_be_p(host);
> +        break;
> +    case 8:
> +        val = FIELD_EX64(env->msr, MSR, LE) ? ldq_le_p(host) : ldq_be_p(host);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +    env->gpr[reg] = val;

Passing in size and performing a switch on it is not the best organization.
You should use multiple helper functions with a common subroutine to handle the llsc 
setup.  You should pass in the whole MemOpIdx.

> +#define MEMOP_GET_SIZE(x)  (1 << ((x) & MO_SIZE))

This is memop_size().


r~