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

[Taylor Simpson <tsimpson@quicinc.com>]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, September 23, 2021 12:05 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: f4bug@amsat.org; ale@rev.ng; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH] Hexagon (target/hexagon) probe the stores in a packet
> at start of commit
> 
> 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.

You are correct, dczeroa can't be in a packet with any other memory op, so it could be its own helper.

We also need to account for HVX stores in the other patch series under review.  With HVX, we could have an HVX store and a scalar store in the same packet.  I'll go ahead and make the changes you suggest here.  I'll create a more general helper in the HVX series.  For efficiency, I'd like to only call a single helper that will do all the probes.  So, we'll either end up with one for each possible combination or a one for scalar only and one for the other combinations with a mask.


Thanks,
Taylor