Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 5 Jan 2023 09:20:19 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
Cc:     Yann Sionneau <ysionneau@kalray.eu>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        linux-kernel@vger.kernel.org,
        Guillaume Thouvenin <gthouvenin@kalray.eu>,
        Julian Vetter <jvetter@kalray.eu>,
        Marius Gligor <mgligor@kalray.eu>
Subject: Re: [RFC PATCH 21/25] kvx: Add support for ftrace
Message-ID: <20230105092019.4bc166b6@gandalf.local.home>
In-Reply-To: <20230105135526.0fc5f250@fixe.home>
References: <20230103164359.24347-1-ysionneau@kalray.eu>
        <20230103164359.24347-22-ysionneau@kalray.eu>
        <20230105135526.0fc5f250@fixe.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, 5 Jan 2023 13:55:26 +0100
Clément Léger <clement.leger@bootlin.com> wrote:

> > +/* The longest insns we check are for the far call: make + icall */
> > +#define MAX_SYLLABLES_TO_CHECK (KVX_INSN_MAKE_IMM64_SIZE + INSN_ICALL_SYLLABLE_SIZE)
> > +
> > +static int read_insns_and_check(u32 *insns, u8 insns_len, u32 *addr)
> > +{
> > +	u32 insns_read[MAX_SYLLABLES_TO_CHECK];
> > +	int syllables = insns_len / KVX_INSN_SYLLABLE_WIDTH;
> > +	int i;
> > +
> > +	if (syllables > MAX_SYLLABLES_TO_CHECK) {
> > +		pr_err("%s: shouldn't have more than %d syllables to check\n",
> > +		       __func__, MAX_SYLLABLES_TO_CHECK);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (kvx_insns_read(insns_read, insns_len, addr)) {
> > +		pr_err("%s: error when trying to read syllable\n", __func__);
> > +		return -EFAULT;
> > +	}
> > +
> > +	for (i = 0; i < syllables; i++) {
> > +		if (insns[i] != insns_read[i]) {
> > +			pr_err("%s: Instruction verification failed at PC 0x%lx\n",
> > +			       __func__,
> > +			       (unsigned long)addr + i * KVX_INSN_SYLLABLE_WIDTH);
> > +			pr_err("%s: \tExpect  0x%x\n", __func__, insns[i]);
> > +			pr_err("%s: \tRead    0x%x\n", __func__, insns_read[i]);
> > +			return -EFAULT;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> Hi Yann,
> 
> Is this still needed ? I'm guessing the instructions should always be
> correctly written no ? If not, something probably went horribly wrong ;)

I would definitely keep it. Code modifications can be quite fragile. Most
of the time things don't go wrong, but when they do, having these checks
makes it obvious to where the problem happened. Problems here is usually
some code mapping that got incorrectly flagged to be traced, when it
shouldn't be. Reporting these errors helps find what that was.

-- Steve


> 
> > +
> > +static int write_insns_and_check(u32 *insns, u8 insns_len, u32 *insn_addr)
> > +{
> > +	int ret;
> > +
> > +	ret = kvx_insns_write_nostop(insns, insns_len, insn_addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Check that what have been written is correct. */
> > +	return read_insns_and_check(insns, insns_len, insn_addr);
> > +}
> > +