Hi Taylor,
On 9/30/21 23:16, Taylor Simpson wrote:
> When a packet has 2 stores, either both commit or neither commit.
> At the beginning of gen_commit_packet, we check for multiple stores.
> If there are multiple stores, call a helper that will probe each of
> them before proceeding with the commit.
>
> Note that we don't call the probe helper for packets with only one
> store. Therefore, we call process_store_log before anything else
> involved in committing the packet.
>
> Test case added in tests/tcg/hexagon/hex_sigsegv.c
>
> Signed-off-by: Taylor Simpson <tsimp...@quicinc.com>
>
> *** Changes in v2 ***
> Address feedback from Richard Henderson <richard.hender...@linaro.org>
> - Since we know the value of all the mask at translation time, call
> specialized helper
> - dczeroa has to be the only store operation in a packet, so we go
> ahead and process that first
> - When there are two scalar stores, we probe the one in slot 0 - the
> call to process_store_log will do slot 1 first, so we don't need
> to probe
> ---
> target/hexagon/helper.h | 2 +
> target/hexagon/op_helper.c | 16 ++++++
> target/hexagon/translate.c | 32 +++++++++++-
> tests/tcg/hexagon/hex_sigsegv.c | 106
> ++++++++++++++++++++++++++++++++++++++
> tests/tcg/hexagon/Makefile.target | 1 +
> 5 files changed, 155 insertions(+), 2 deletions(-)
> create mode 100644 tests/tcg/hexagon/hex_sigsegv.c
>
> diff --git a/target/hexagon/helper.h b/target/hexagon/helper.h
> index ca201fb..89de2a3 100644
> --- a/target/hexagon/helper.h
> +++ b/target/hexagon/helper.h
> @@ -89,3 +89,5 @@ DEF_HELPER_4(sffms_lib, f32, env, f32, f32, f32)
>
> DEF_HELPER_3(dfmpyfix, f64, env, f64, f64)
> DEF_HELPER_4(dfmpyhh, f64, env, f64, f64, f64)
> +
> +DEF_HELPER_2(probe_pkt_scalar_store_s0, void, env, int)
> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 61d5cde..af32de4 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -377,6 +377,22 @@ int32_t HELPER(vacsh_pred)(CPUHexagonState *env,
> return PeV;
> }
>
> +static 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);
> + }
Matter of taste probably:
if (env->slot_cancelled & (1 << slot) {
return;
}
probe_write(env, env->mem_log_stores[slot].va,
env->mem_log_stores[slot].width, mmu_idx, GETPC());
> +}
> +
> +/* Called during packet commit when there are two scalar stores */
> +void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx)
> +{
> + probe_store(env, 0, mmu_idx);
> +}
> +
> /*
> * mem_noshuf
> * Section 5.5 of the Hexagon V67 Programmer's Reference Manual
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index 6fb4e68..8fc2c83 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -471,10 +471,38 @@ static void update_exec_counters(DisasContext *ctx,
> Packet *pkt)
>
> static void gen_commit_packet(DisasContext *ctx, Packet *pkt)
> {
> + /*
> + * If there is more than one store in a packet, make sure they are all OK
> + * before proceeding with the rest of the packet commit.
> + *
> + * dczeroa has to be the only store operation in the packet, so we go
> + * ahead and process that first.
> + *
> + * When there are two scalar stores, we probe the one in slot 0.
> + *
> + * Note that we don't call the probe helper for packets with only one
> + * store. Therefore, we call process_store_log before anything else
> + * involved in committing the packet.
> + */
> + bool has_store_s0 = pkt->pkt_has_store_s0;
> + bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed);
> + if (pkt->pkt_has_dczeroa) {
> + /*
> + * The dczeroa will be the store in slot 0, check that we don't have
> + * a store in slot 1.
> + */
> + g_assert(has_store_s0 && !has_store_s1);
> + process_dczeroa(ctx, pkt);
> + } else if (has_store_s0 && has_store_s1) {
> + TCGv mem_idx = tcg_const_tl(ctx->mem_idx);
> + gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
> + tcg_temp_free(mem_idx);
The index is read-only, so you can avoid the temporary:
gen_helper_probe_pkt_scalar_store_s0(cpu_env,
tcg_constant_tl(ctx->mem_idx));
Maybe add a (better) comment here:
} else {
/* default path, all constraints OK, we are good */
> + }
> +
> + process_store_log(ctx, pkt);
> +
> gen_reg_writes(ctx);
> gen_pred_writes(ctx, pkt);
> - process_store_log(ctx, pkt);
> - process_dczeroa(ctx, pkt);
> update_exec_counters(ctx, pkt);
> if (HEX_DEBUG) {
> TCGv has_st0 =
> diff --git a/tests/tcg/hexagon/hex_sigsegv.c b/tests/tcg/hexagon/hex_sigsegv.c
> new file mode 100644
> index 0000000..dc2b349
> --- /dev/null
> +++ b/tests/tcg/hexagon/hex_sigsegv.c
hex_sigsegv is a generic test name ...
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright(c) 2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * Test the VLIW semantics of two stores in a packet
... but you are testing a very specific case.
Maybe rename as "multi_pkt_stores" (or better)?
> + *
> + * When a packet has 2 stores, either both commit or neither commit.
> + * We test this with a packet that does stores to both NULL and a global
> + * variable, "should_not_change". After the SIGSEGV is caught, we check
> + * that the "should_not_change" value is the same.
> + */
Otherwise LGTM.
Regards,
Phil.
