[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3}
From: |
Peter Maydell |
Subject: |
Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3} |
Date: |
Tue, 3 Dec 2019 13:42:05 +0000 |
On Fri, 11 Oct 2019 at 14:49, Richard Henderson
<address@hidden> wrote:
>
> Implements the rules of "PE generation of Checked and Unchecked
> accesses" which aren't already implied by TB_FLAGS_MTE_ACTIVE.
> Implements the rules of "PE handling of Tag Check Failure".
>
> Does not implement tag physical address space, so all operations
> reduce to unchecked so far.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> v2: Fix TFSR update.
> v3: Split helper_mte_check per {1,2} IAs; take tbi data from translate.
> v5: Split helper_mte_check3, the only one that needs a runtime check for tbi.
> ---
> target/arm/helper-a64.h | 4 +
> target/arm/mte_helper.c | 167 +++++++++++++++++++++++++++++++++++++
> target/arm/translate-a64.c | 15 +++-
> target/arm/Makefile.objs | 1 +
> 4 files changed, 186 insertions(+), 1 deletion(-)
> create mode 100644 target/arm/mte_helper.c
>
> diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
> index a915c1247f..a82e21f15a 100644
> --- a/target/arm/helper-a64.h
> +++ b/target/arm/helper-a64.h
> @@ -102,3 +102,7 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64,
> i64)
> DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
> DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
> DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
> +
> +DEF_HELPER_FLAGS_2(mte_check1, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_2(mte_check2, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_3(mte_check3, TCG_CALL_NO_WG, i64, env, i64, i32)
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> new file mode 100644
> index 0000000000..bbb90cbe86
> --- /dev/null
> +++ b/target/arm/mte_helper.c
> @@ -0,0 +1,167 @@
> +/*
> + * ARM v8.5-MemTag Operations
> + *
> + * Copyright (c) 2019 Linaro, Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "internals.h"
> +#include "exec/exec-all.h"
> +#include "exec/cpu_ldst.h"
> +#include "exec/helper-proto.h"
> +
> +
> +static int get_allocation_tag(CPUARMState *env, uint64_t ptr, uintptr_t ra)
> +{
> + /* Tag storage not implemented. */
> + return -1;
> +}
> +
> +static int allocation_tag_from_addr(uint64_t ptr)
> +{
> + ptr += 1ULL << 55; /* carry ptr[55] into ptr[59:56]. */
> + return extract64(ptr, 56, 4);
What's the carry-bit-55 logic for? The pseudocode
AArch64.AllocationTagFromAddress just returns bits [59:56].
> +}
> +
> +/*
> + * Perform a checked access for MTE.
> + * On arrival, TBI is known to enabled, as is allocation_tag_access_enabled.
"to be"
> + */
> +static uint64_t do_mte_check(CPUARMState *env, uint64_t dirty_ptr,
> + uint64_t clean_ptr, uint32_t select,
> + uintptr_t ra)
> +{
> + ARMMMUIdx stage1 = arm_stage1_mmu_idx(env);
> + int ptr_tag, mem_tag;
> +
> + /*
> + * If TCMA is enabled, then physical tag 0 is unchecked.
> + * Note the rules in D6.8.1 are written with logical tags, where
> + * the corresponding physical tag rule is simpler: equal to 0.
> + * We will need the physical tag below anyway.
> + */
This reads a bit oddly, because (in the final version of the spec)
physical and logical tags are identical (AArch64.PhysicalTag()
just returns bits [59:56] of the vaddr).
> + ptr_tag = allocation_tag_from_addr(dirty_ptr);
> + if (ptr_tag == 0) {
> + ARMVAParameters p = aa64_va_parameters(env, dirty_ptr, stage1, true);
> + if (p.tcma) {
> + return clean_ptr;
> + }
> + }
I don't think this logic gets the "regime has two address ranges"
case correct. For a two-address-range translation regime (where
TCR_ELx has TCMA0 and TCMA1 bits, rather than just a single TCMA bit),
then the 'select' argument to this function needs to be involved,
because we should do a tag-unchecked access if:
* addr[59:55]==0b00000 (ie select == 0 and ptr_tag == 0)
and TCR_ELx.TCMA0 is set
* addr[59:55]==0b11111 (ie select == 1 and ptr_tag == 0xf)
and TCR_ELx.TCMA1 is set
(the pseudocode for this is in AArch64.AccessTagIsChecked(),
and the TCR_EL1.TCMA[01] bit definitions agree; the text in
D6.8.1 appears to be confused.)
> +
> + /*
> + * If an access is made to an address that does not provide tag
> + * storage, the result is IMPLEMENTATION DEFINED. We choose to
> + * treat the access as unchecked.
> + * This is similar to MemAttr != Tagged, which are also unchecked.
> + */
> + mem_tag = get_allocation_tag(env, clean_ptr, ra);
> + if (mem_tag < 0) {
> + return clean_ptr;
> + }
> +
> + /* If the tags do not match, the tag check operation fails. */
> + if (unlikely(ptr_tag != mem_tag)) {
> + int el, regime_el, tcf;
> + uint64_t sctlr;
> +
> + el = arm_current_el(env);
> + regime_el = (el ? el : 1); /* TODO: ARMv8.1-VHE EL2&0 regime */
We could write this as "regime_el(env, stage1)" if that function
wasn't local to helper.c, right ?
> + sctlr = env->cp15.sctlr_el[regime_el];
> + if (el == 0) {
> + tcf = extract64(sctlr, 38, 2);
> + } else {
> + tcf = extract64(sctlr, 40, 2);
> + }
> +
> + switch (tcf) {
> + case 1:
> + /*
> + * Tag check fail causes a synchronous exception.
> + *
> + * In restore_state_to_opc, we set the exception syndrome
> + * for the load or store operation. Do that first so we
> + * may overwrite that with the syndrome for the tag check.
> + */
> + cpu_restore_state(env_cpu(env), ra, true);
> + env->exception.vaddress = dirty_ptr;
> + raise_exception(env, EXCP_DATA_ABORT,
> + syn_data_abort_no_iss(el != 0, 0, 0, 0, 0, 0x11),
> + exception_target_el(env));
> + /* noreturn; fall through to assert anyway */
hopefully this fallthrough comment syntax doesn't confuse any
of our compilers/static analyzers...
> +
> + case 0:
> + /*
> + * Tag check fail does not affect the PE.
> + * We eliminate this case by not setting MTE_ACTIVE
> + * in tb_flags, so that we never make this runtime call.
> + */
> + g_assert_not_reached();
> +
> + case 2:
> + /* Tag check fail causes asynchronous flag set. */
> + env->cp15.tfsr_el[regime_el] |= 1 << select;
Won't this incorrectly accumulate tagfails for EL0 into
TFSR_EL1 rather than TFSRE0_EL1 ? I think you want "[el]".
> + break;
> +
> + default:
> + /* Case 3: Reserved. */
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "Tag check failure with SCTLR_EL%d.TCF "
> + "set to reserved value %d\n", regime_el, tcf);
Technically this message is going to be wrong for the
case of el==0 (where it's SCTLR_EL1.TCF0, not .TCF, that's
been mis-set).
> + break;
> + }
> + }
> +
> + return clean_ptr;
> +}
> +
> +/*
> + * Perform check in translation regime w/single IA range.
> + * It is known that TBI is enabled on entry.
> + */
> +uint64_t HELPER(mte_check1)(CPUARMState *env, uint64_t dirty_ptr)
> +{
> + uint64_t clean_ptr = extract64(dirty_ptr, 0, 56);
> + return do_mte_check(env, dirty_ptr, clean_ptr, 0, GETPC());
> +}
> +
> +/*
> + * Perform check in translation regime w/two IA ranges.
> + * It is known that TBI is enabled on entry.
> + */
> +uint64_t HELPER(mte_check2)(CPUARMState *env, uint64_t dirty_ptr)
> +{
> + uint32_t select = extract64(dirty_ptr, 55, 1);
> + uint64_t clean_ptr = sextract64(dirty_ptr, 0, 56);
> + return do_mte_check(env, dirty_ptr, clean_ptr, select, GETPC());
> +}
> +
> +/*
> + * Perform check in translation regime w/two IA ranges.
> + * The TBI argument is the concatenation of TBI1:TBI0.
> + */
> +uint64_t HELPER(mte_check3)(CPUARMState *env, uint64_t dirty_ptr, uint32_t
> tbi)
> +{
> + uint32_t select = extract64(dirty_ptr, 55, 1);
> + uint64_t clean_ptr = sextract64(dirty_ptr, 0, 56);
> +
> + if ((tbi >> select) & 1) {
> + return do_mte_check(env, dirty_ptr, clean_ptr, select, GETPC());
> + } else {
> + /* TBI is disabled; the access is unchecked. */
> + return dirty_ptr;
> + }
> +}
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 62bdf50796..8e4fea6b4c 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -214,7 +214,20 @@ static void gen_a64_set_pc(DisasContext *s, TCGv_i64 src)
> static TCGv_i64 clean_data_tbi(DisasContext *s, TCGv_i64 addr)
> {
> TCGv_i64 clean = new_tmp_a64(s);
> - gen_top_byte_ignore(s, clean, addr, s->tbid);
> +
> + /* Note that s->mte_active already includes a check for s->tbid != 0. */
> + if (!s->mte_active) {
> + gen_top_byte_ignore(s, clean, addr, s->tbid);
> + } else if (!regime_has_2_ranges(s->mmu_idx)) {
> + gen_helper_mte_check1(clean, cpu_env, addr);
> + } else if (s->tbid == 3) {
> + /* Both TBI1:TBI0 are set; no need to check at runtime. */
> + gen_helper_mte_check2(clean, cpu_env, addr);
> + } else {
> + TCGv_i32 tbi = tcg_const_i32(s->tbid);
> + gen_helper_mte_check3(clean, cpu_env, addr, tbi);
> + tcg_temp_free_i32(tbi);
> + }
> return clean;
> }
>
> diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs
> index cf26c16f5f..8fd7d086c8 100644
> --- a/target/arm/Makefile.objs
> +++ b/target/arm/Makefile.objs
> @@ -67,3 +67,4 @@ obj-$(CONFIG_SOFTMMU) += psci.o
> obj-$(TARGET_AARCH64) += translate-a64.o helper-a64.o
> obj-$(TARGET_AARCH64) += translate-sve.o sve_helper.o
> obj-$(TARGET_AARCH64) += pauth_helper.o
> +obj-$(TARGET_AARCH64) += mte_helper.o
> --
> 2.17.1
thanks
-- PMM
- Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3},
Peter Maydell <=