Re: [PATCH] Hexagon (target/hexagon) probe the stores in a packet at start of commit

[Richard Henderson]

On 9/22/21 11:35 AM, Taylor Simpson wrote:
> +static inline void probe_store(CPUHexagonState *env, int slot, int mmu_idx)
> +{
> +    if (!(env->slot_cancelled & (1 << slot))) {
> +        size1u_t width = env->mem_log_stores[slot].width;
> +        target_ulong va = env->mem_log_stores[slot].va;
> +        uintptr_t ra = GETPC();
> +        probe_write(env, va, width, mmu_idx, ra);
> +    }
> +}
> +
> +void HELPER(probe_pkt_stores)(CPUHexagonState *env, int has_st0, int has_st1,
> +                              int has_dczeroa, int mmu_idx)
> +{
> +    if (has_st0 && !has_dczeroa) {
> +        probe_store(env, 0, mmu_idx);
> +    }
> +    if (has_st1 && !has_dczeroa) {
> +        probe_store(env, 1, mmu_idx);
> +    }
> +    if (has_dczeroa) {
> +        /* Probe 32 bytes starting at (dczero_addr & ~0x1f) */
> +        target_ulong va = env->dczero_addr & ~0x1f;
> +        uintptr_t ra = GETPC();
> +        probe_write(env, va +  0, 8, mmu_idx, ra);
> +        probe_write(env, va +  8, 8, mmu_idx, ra);
> +        probe_write(env, va + 16, 8, mmu_idx, ra);
> +        probe_write(env, va + 24, 8, mmu_idx, ra);
> +    }
> +}

You know at translate time the value of all of these has_* variables.

Since has_dczeroa disables the other two probes, surely probe_pkt_dczeroa should be its 
own helper.

That said, if dczeroa (apparently) cannot be paired with other stores, why do you need to 
probe for it at all?  Since the operation is 32-byte aligned, surely the first real store 
will validate the write for the entire block.

Once you eliminate dczeroa from this helper, the only time it will be called is with both 
has_st0 and has_st1 true, at which point you don't need to pass the arguments in at all.


r~