From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id avbdIAg36l+KTAAAWB0awg (envelope-from ) for ; Mon, 28 Dec 2020 14:50:32 -0500 Received: by simark.ca (Postfix, from userid 112) id 6CC6D1F0AA; Mon, 28 Dec 2020 14:50:32 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 EDF661E552 for ; Mon, 28 Dec 2020 14:50:31 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 879B63846070; Mon, 28 Dec 2020 19:50:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 879B63846070 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609185031; bh=qzuxZI7V1ub9p3FxMiqD3Zj6MP1wbmGfv7CX3ae8BJw=; 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=GSt2dqUqgstQveTcVEUw2JXBUMYuSwI/aoMuy07CbOoiwfxZVlUjM5bOJ6kvX0Bgy rnrTmZDKXD/ua7kkhBKoIpyZHaueiPxRaqsRMF36KS2qrLTfWI2Rddyvv+Ii2zJa7a 2KgIj8X9acUIhOMCldncocWCocJNyEC2f+Pgx6L4= Received: from mail-qv1-xf35.google.com (mail-qv1-xf35.google.com [IPv6:2607:f8b0:4864:20::f35]) by sourceware.org (Postfix) with ESMTPS id 91AFE386101C for ; Mon, 28 Dec 2020 19:50:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 91AFE386101C Received: by mail-qv1-xf35.google.com with SMTP id 4so5448825qvh.1 for ; Mon, 28 Dec 2020 11:50:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=qzuxZI7V1ub9p3FxMiqD3Zj6MP1wbmGfv7CX3ae8BJw=; b=HqDlCpQYsb88lRtguxg61OxHljWnqcnblBq/UBftkijzzKdhLTU5JabousRB0sDQCo 8g3kT2Mj6ARxhF8jb0C5jFcZNH2N/kGIA8CaSHiIx+iDi7ZVhWvXY53IUIsvdpCo5wGt bZo0kbrA4isNrmpw1E1Pl3IYfVwDb7dsVNtiFkQBuUOpbodJiYEW/EXcE87T0OuRNQyM 9GxkA8cqpvsUWteSK9jMUZpE1gJinJEazd6u3E7gMpP9clYxSFER8upXTo/4vrUtCiRl eAmsBvbfrwTzKBPr/6Rpp+DlEb5GvB/CiNdlJ+zGK4nUP0f5KiaokDzZuXIf4e40k7Ta XF+Q== X-Gm-Message-State: AOAM530vgODm9tQGWRUDw90+zkNmUJWI3jQPA3xzpTy3ZNeCT+WyREX7 pkVps3tB2M/TfHiTWEGTTgwS5TvjMFEizQ== X-Google-Smtp-Source: ABdhPJy+lfDDgwTQBVGQNQ3Vz2MVMm6HwNO6vay3t0uXfmMXEfJuhrqdESVvkmNVvMJrrxlDrQjxgQ== X-Received: by 2002:a0c:fb0d:: with SMTP id c13mr48145383qvp.1.1609185028030; Mon, 28 Dec 2020 11:50:28 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:370e:19e7:527f:e109:2734? ([2804:7f0:8284:370e:19e7:527f:e109:2734]) by smtp.gmail.com with ESMTPSA id j142sm24965887qke.117.2020.12.28.11.50.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 28 Dec 2020 11:50:27 -0800 (PST) Subject: Re: [PATCH v3 12/24] AArch64: Implement memory tagging target methods for AArch64 To: Simon Marchi , gdb-patches@sourceware.org References: <20201109170435.15766-1-luis.machado@linaro.org> <20201109170435.15766-13-luis.machado@linaro.org> Message-ID: <8e54e291-ef36-c568-7a6a-15693df47c0a@linaro.org> Date: Mon, 28 Dec 2020 16:50:24 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Luis Machado via Gdb-patches Reply-To: Luis Machado Cc: david.spickett@linaro.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 12/25/20 9:33 PM, Simon Marchi wrote: > 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). > Indeed. Fixed now. >> + >> + /* 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. > All of these were supposed to return error codes. But right now we don't have a list of possible codes, therefore it is basically 0/1 for success/failure. I've switched to using bool. >> 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. > Done now. >> + >> +/* 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. > Done. >> +/* 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. > Attribute added. >> + >> +/* 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. > You're right. Removed now. >> 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. > That makes sense too. >> + >> +/* 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 > Thanks for the review and suggestions so far. They were very useful.