From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0kPwD+yE5l+TZAAAWB0awg (envelope-from ) for ; Fri, 25 Dec 2020 19:33:48 -0500 Received: by simark.ca (Postfix, from userid 112) id 3253B1F0AA; Fri, 25 Dec 2020 19:33:48 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E16BD1E965 for ; Fri, 25 Dec 2020 19:33:46 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5DC97385800D; Sat, 26 Dec 2020 00:33:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5DC97385800D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1608942826; bh=PK/XHLeOjE29wwALhrVc9MyI4adwnHs9Lwb3GyOl2GE=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=B9b5zNFmF+OQn5pl/7XOluZnmEKGxKXOQn7pJQoUKth9hqHTjnLUHS4cyz7tRPa2s aLBXNKRwPXp0xv40yX1OPdRgGLSZ5tjgsFTs81tVmK56EVBOvMQPHNPEu/2LnOfpLD LlfbkpXxRJfYlmeq3XH0KifBkhoeH4ginOKBabQc= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 1D3A0385802B for ; Sat, 26 Dec 2020 00:33:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1D3A0385802B Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 0BQ0XKgC021088 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 25 Dec 2020 19:33:25 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 0BQ0XKgC021088 Received: from [10.0.0.213] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 989731E965; Fri, 25 Dec 2020 19:33:20 -0500 (EST) Subject: Re: [PATCH v3 12/24] AArch64: Implement memory tagging target methods for AArch64 To: Luis Machado , gdb-patches@sourceware.org References: <20201109170435.15766-1-luis.machado@linaro.org> <20201109170435.15766-13-luis.machado@linaro.org> Message-ID: Date: Fri, 25 Dec 2020 19:33:20 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20201109170435.15766-13-luis.machado@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 26 Dec 2020 00:33:20 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Cc: david.spickett@linaro.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote: > Updates on v2: > > - Added type parameter to the target method implementations. > > -- > > The patch implements the memory tagging target hooks for AArch64, so we > can handle MTE. > > gdb/ChangeLog: > > YYYY-MM-DD Luis Machado > > * Makefile.in (ALL_64_TARGET_OBS): Add arch/aarch64-mte-linux.o. > (HFILES_NO_SRCDIR): Add arch/aarch64-mte-linux.h and > nat/aarch64-mte-linux-ptrace.h. > * aarch64-linux-nat.c: Include nat/aarch64-mte-linux-ptrace.h. > (aarch64_linux_nat_target) : New method > override. > : New method override. > : New method override. > (aarch64_linux_nat_target::supports_memory_tagging): New method. > (aarch64_linux_nat_target::fetch_memtags): New method. > (aarch64_linux_nat_target::store_memtags): New method. > * arch/aarch64-mte-linux.c: New file. > * arch/aarch64-mte-linux.h: Include gdbsupport/common-defs.h. > (MTE_GRANULE_SIZE): Define. > (get_tag_granules): New prototype. > * configure.nat (NATDEPFILES): Add nat/aarch64-mte-linux-ptrace.o. > * configure.tgt (aarch64*-*-linux*): Add arch/aarch64-mte-linux.o. > * nat/aarch64-mte-linux-ptrace.c: New file. > * nat/aarch64-mte-linux-ptrace.h: New file. > --- > gdb/Makefile.in | 1 + > gdb/aarch64-linux-nat.c | 50 ++++++++ > gdb/arch/aarch64-mte-linux.c | 34 +++++ > gdb/arch/aarch64-mte-linux.h | 10 ++ > gdb/configure.nat | 3 +- > gdb/configure.tgt | 1 + > gdb/nat/aarch64-mte-linux-ptrace.c | 200 +++++++++++++++++++++++++++++ > gdb/nat/aarch64-mte-linux-ptrace.h | 17 +++ > 8 files changed, 315 insertions(+), 1 deletion(-) > create mode 100644 gdb/arch/aarch64-mte-linux.c > create mode 100644 gdb/nat/aarch64-mte-linux-ptrace.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 4f08beea44..452e21b2b2 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -693,6 +693,7 @@ ALL_64_TARGET_OBS = \ > amd64-windows-tdep.o \ > arch/aarch64.o \ > arch/aarch64-insn.o \ > + arch/aarch64-mte-linux.o \ > arch/amd64.o \ > ia64-linux-tdep.o \ > ia64-tdep.o \ > diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c > index dea34da669..4edf5a0454 100644 > --- a/gdb/aarch64-linux-nat.c > +++ b/gdb/aarch64-linux-nat.c > @@ -52,6 +52,8 @@ > > #include "arch/aarch64-mte-linux.h" > > +#include "nat/aarch64-mte-linux-ptrace.h" > + > #ifndef TRAP_HWBKPT > #define TRAP_HWBKPT 0x0004 > #endif > @@ -102,6 +104,16 @@ class aarch64_linux_nat_target final : public linux_nat_target > override; > > struct gdbarch *thread_architecture (ptid_t) override; > + > + bool supports_memory_tagging () override; > + > + /* Read memory allocation tags from memory via PTRACE. */ > + int fetch_memtags (CORE_ADDR address, size_t len, > + gdb::byte_vector &tags, int type) override; > + > + /* Write allocation tags to memory via PTRACE. */ > + int store_memtags (CORE_ADDR address, size_t len, > + const gdb::byte_vector &tags, int type) override; > }; > > static aarch64_linux_nat_target the_aarch64_linux_nat_target; > @@ -1050,6 +1062,44 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid) > return gdbarch_find_by_info (info); > } > > +/* Implement the "supports_memory_tagging" target_ops method. */ > + > +bool > +aarch64_linux_nat_target::supports_memory_tagging () > +{ > + return (linux_get_hwcap2 (this) & HWCAP2_MTE) != 0; > +} > + > +/* Implement the "fetch_memtags" target_ops method. */ > + > +int > +aarch64_linux_nat_target::fetch_memtags (CORE_ADDR address, size_t len, > + gdb::byte_vector &tags, int type) > +{ > + int tid = inferior_ptid.lwp (); I believe this should use get_ptrace_pid (same in the store version). > + > + /* Allocation tags? */ > + if (type == 1) > + return aarch64_mte_fetch_memtags (tid, address, len, tags); > + > + return 1; What's the logic of this "return 1"? Does this mean "success", and is this what we want to do when it's an unhandled type? Oh, now that I read the doc for aarch64_mte_fetch_memtags, I see that 1 means failure. See comment below. > diff --git a/gdb/arch/aarch64-mte-linux.h b/gdb/arch/aarch64-mte-linux.h > index 4124e80543..e555f0af19 100644 > --- a/gdb/arch/aarch64-mte-linux.h > +++ b/gdb/arch/aarch64-mte-linux.h > @@ -20,6 +20,8 @@ > #ifndef ARCH_AARCH64_LINUX_H > #define ARCH_AARCH64_LINUX_H > > +#include "gdbsupport/common-defs.h" > + > /* Feature check for Memory Tagging Extension. */ > #ifndef HWCAP2_MTE > #define HWCAP2_MTE (1 << 18) > @@ -28,4 +30,12 @@ > /* The MTE regset consists of a single 64-bit register. */ > #define AARCH64_LINUX_SIZEOF_MTE 8 > > +/* We have one tag per 16 bytes of memory. */ > +#define MTE_GRANULE_SIZE 16 I'd suggest prefixing this, AARCH64_MTE_GRANULE_SIZE. > + > +/* Return the number of tag granules in the memory range > + [ADDR, ADDR + LEN) given GRANULE_SIZE. */ > +extern size_t get_tag_granules (CORE_ADDR addr, size_t len, > + size_t granule_size); And aarch64_mte_get_tag_granules. > +/* Helper function to display various possible errors when reading > + MTE tags. */ > + > +static void > +aarch64_mte_linux_peek_error (int error) > +{ > + switch (error) > + { > + case EIO: > + perror_with_name (_("PEEKMTETAGS not supported")); > + break; > + case EFAULT: > + perror_with_name (_("Couldn't fetch allocation tags")); > + break; > + case EOPNOTSUPP: > + perror_with_name (_("PROT_ME not enabled for requested address")); > + default: > + perror_with_name (_("Unknown MTE error")); > + break; > + } > +} > + > +/* Helper function to display various possible errors when writing > + MTE tags. */ > + > +static void > +aarch64_mte_linux_poke_error (int error) > +{ > + switch (error) > + { > + case EIO: > + perror_with_name (_("POKEMTETAGS not supported")); > + break; > + case EFAULT: > + perror_with_name (_("Couldn't store allocation tags")); > + break; > + case EOPNOTSUPP: > + perror_with_name (_("PROT_ME not enabled for requested address")); > + default: > + perror_with_name (_("Unknown MTE error")); > + break; > + } > +} These functions should probably be marked with ATTRIBUTE_NORETURN. > + > +/* Helper to prepare a vector of tags to be passed on to the kernel. The > + main purpose of this function is to optimize the number of calls to > + ptrace if we're writing too many tags at once, like a pattern fill > + request. > + > + Return a vector of tags of up to MAX_SIZE size, containing the tags that > + must be passed on to the kernel, extracted from TAGS, starting at POS. > + GRANULES is the number of tag granules to be modified. */ > + > +static gdb::byte_vector > +prepare_tag_vector (size_t granules, const gdb::byte_vector &tags, size_t pos, > + size_t max_size) > +{ > + gdb::byte_vector t; > + > + if (granules == 0) > + { > + t.clear (); That clear seems unnecessary. > diff --git a/gdb/nat/aarch64-mte-linux-ptrace.h b/gdb/nat/aarch64-mte-linux-ptrace.h > index 099b6440ca..7ba6f014f6 100644 > --- a/gdb/nat/aarch64-mte-linux-ptrace.h > +++ b/gdb/nat/aarch64-mte-linux-ptrace.h > @@ -30,4 +30,21 @@ > #define PTRACE_POKEMTETAGS 34 > #endif > > +/* Maximum number of tags to pass at once to the kernel. */ > +#define TAGS_MAX_SIZE 4096 I think this define should have a more scoped named, like AARCH64_MTE_TAGS_MAX_SIZE. > + > +/* Read the allocation tags from memory range [ADDRESS, ADDRESS + LEN) > + into TAGS. > + > + Return 0 if successful and non-zero otherwise. */ > +extern int aarch64_mte_fetch_memtags (int tid, CORE_ADDR address, size_t len, > + gdb::byte_vector &tags); > + > +/* Write the TAGS allocation tags to the memory range > + [ADDRESS, ADDRESS + LEN). > + > + Return 0 if successful and non-zero otherwise. */ > +extern int aarch64_mte_store_memtags (int tid, CORE_ADDR address, size_t len, > + const gdb::byte_vector &tags); IWBN to use bool for the return value, true for success and false for failure. And as mentioned in the first patch, the target methods should also use bool for the return value and be documented (although with bool, it's pretty obvious that "true" means success, unlike with ints where both ways are common). Otherwise, LGTM. Simon