From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) by sourceware.org (Postfix) with ESMTPS id 58F203858D35 for ; Wed, 3 Nov 2021 12:54:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 58F203858D35 Received: by mail-qk1-x72b.google.com with SMTP id bk22so2109643qkb.6 for ; Wed, 03 Nov 2021 05:54:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=hWJ5JKsWAon+p2w1kOHs1AJ9kBmFgZFB+lnQSKjm5BM=; b=ba3gD+kBdsaLcMgtmIQRQ9r3tDHob0KTLkIzROSpC2nAnZaJWnshkYRd8Vkkt7nLWQ hj8y7yh88m1o3U3bDal6mfF2U++VMj06aie/+VYl0c91j5B9YJlbdvdnASWAG/utH/br rlxhzLBEJMl+9bI21C6ydCtDIf6i2lMT8mByzX33XTsr4Hp1wqQZVYi7x0e1wt5kLq7Y thUUceOfxz2N7smvr61Z0h3JWCnHREp2gigh3UgH/PVO44Qzm8qbNYFqoYs4qTfVJh2Q 3r2Xw379JGfl54b7UpOe50Y35l7gEm3KKbr50j15kjuUhxM/2jiVQ7qM30H92SIjxZtx 51fQ== X-Gm-Message-State: AOAM533hLtBrD9snEk+jxj6cgpqt1dVNnwEQw3Iz7p9S9bngdXCS3XDQ CRuROM2sKQnPvYYqHIwU+tOegt6VfnRo1A== X-Google-Smtp-Source: ABdhPJx7I9ecKuHL178i88IDIjaoyzaOaEh0OTupcRMK5Sy14JBmAebPnUIgmHBy4zYv+4bzuiWUMA== X-Received: by 2002:a05:620a:132f:: with SMTP id p15mr32044813qkj.230.1635940319544; Wed, 03 Nov 2021 04:51:59 -0700 (PDT) Received: from ?IPv6:2804:7f0:4841:487c:a7:8cc5:d113:e3c4? ([2804:7f0:4841:487c:a7:8cc5:d113:e3c4]) by smtp.gmail.com with ESMTPSA id br20sm1339004qkb.104.2021.11.03.04.51.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Nov 2021 04:51:59 -0700 (PDT) Subject: Re: [PATCH] [AArch64] Make gdbserver register set selection dynamic To: Alan Hayward Cc: "gdb-patches\\@sourceware.org" , nd References: <20211101130913.1320649-1-luis.machado@linaro.org> From: Luis Machado Message-ID: Date: Wed, 3 Nov 2021 08:51:56 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 03 Nov 2021 12:54:03 -0000 On 11/3/21 5:50 AM, Alan Hayward wrote: > > >> On 1 Nov 2021, at 13:09, Luis Machado wrote: >> >> The current register set selection mechanism for AArch64 is static, based >> on a pre-populated array of register sets. >> >> This means that we might potentially probe register sets that are not >> available. This is OK if the kernel errors out during ptrace, but probing the >> tag_ctl register, for example, does not result in a ptrace error if the kernel >> supports the tagged address ABI but not MTE (PR 28355). >> >> Making the register set selection dynamic, based on feature checks, solves >> this and simplifies the code a bit. It allows us to list all of the register >> sets only once, and pick and choose based on HWCAP/HWCAP2 or other properties. >> > > This is all much better than the existing code. > The two sets was getting ugly now the number of optional features are building up. > > On first reading of the code, using size==0 to disable regsets feels like a hack. > Alternatively you could add a bool enabled flag. Having that you can now put the > sizes in the static declaration and simplify aarch64_adjust_register_sets. But, that’d > mean updating every target file, and it just adds to the memory used. So, it’s probably > better as you’ve done it. I agree the bool is a more elegant way of doing it. An improvement for the next release most likely. I didn't want to rework other targets because I also want to push this to GDB 11. > > Very minor nit below, otherwise looks good to me. > >> I plan to backport this fix to GDB 11 as well. >> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28355 >> --- >> gdb/arch/aarch64.h | 9 ++ >> gdbserver/linux-aarch64-low.cc | 184 ++++++++++++++++++--------------- >> 2 files changed, 109 insertions(+), 84 deletions(-) >> >> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h >> index 0eb702c5b5e..95edb664b55 100644 >> --- a/gdb/arch/aarch64.h >> +++ b/gdb/arch/aarch64.h >> @@ -22,6 +22,15 @@ >> >> #include "gdbsupport/tdesc.h" >> >> +/* Holds information on what architectural features are available. This is >> + used to select register sets. */ >> +struct aarch64_features >> +{ >> + bool sve = false; >> + bool pauth = false; >> + bool mte = false; >> +}; >> + >> /* Create the aarch64 target description. A non zero VQ value indicates both >> the presence of SVE and the Vector Quotient - the number of 128bit chunks in >> an SVE Z register. HAS_PAUTH_P indicates the presence of the PAUTH >> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc >> index daccfef746e..0ad9d01101a 100644 >> --- a/gdbserver/linux-aarch64-low.cc >> +++ b/gdbserver/linux-aarch64-low.cc >> @@ -196,16 +196,6 @@ is_64bit_tdesc (void) >> return register_size (regcache->tdesc, 0) == 8; >> } >> >> -/* Return true if the regcache contains the number of SVE registers. */ >> - >> -static bool >> -is_sve_tdesc (void) >> -{ >> - struct regcache *regcache = get_thread_regcache (current_thread, 0); >> - >> - return tdesc_contains_feature (regcache->tdesc, "org.gnu.gdb.aarch64.sve"); >> -} >> - >> static void >> aarch64_fill_gregset (struct regcache *regcache, void *buf) >> { >> @@ -680,40 +670,6 @@ aarch64_target::low_new_fork (process_info *parent, >> *child->priv->arch_private = *parent->priv->arch_private; >> } >> >> -/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h. */ >> -#define AARCH64_HWCAP_PACA (1 << 30) >> - >> -/* Implementation of linux target ops method "low_arch_setup". */ >> - >> -void >> -aarch64_target::low_arch_setup () >> -{ >> - unsigned int machine; >> - int is_elf64; >> - int tid; >> - >> - tid = lwpid_of (current_thread); >> - >> - is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine); >> - >> - if (is_elf64) >> - { >> - uint64_t vq = aarch64_sve_get_vq (tid); >> - unsigned long hwcap = linux_get_hwcap (8); >> - unsigned long hwcap2 = linux_get_hwcap2 (8); >> - bool pauth_p = hwcap & AARCH64_HWCAP_PACA; >> - /* MTE is AArch64-only. */ >> - bool mte_p = hwcap2 & HWCAP2_MTE; >> - >> - current_process ()->tdesc >> - = aarch64_linux_read_description (vq, pauth_p, mte_p); >> - } >> - else >> - current_process ()->tdesc = aarch32_linux_read_description (); >> - >> - aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread)); >> -} >> - >> /* Wrapper for aarch64_sve_regs_copy_to_reg_buf. */ >> >> static void >> @@ -730,20 +686,35 @@ aarch64_sve_regs_copy_from_regcache (struct regcache *regcache, void *buf) >> return aarch64_sve_regs_copy_from_reg_buf (regcache, buf); >> } >> >> +/* Array containing all the possible register sets for AArch64/Linux. During >> + architecture setup, these will be checked against the HWCAP/HWCAP2 bits for >> + validity and enabled/disabled accordingly. >> + >> + Their sizes are set to 0 here, but they will be adjusted later depending >> + on whether each register set is available or not. */ >> static struct regset_info aarch64_regsets[] = >> { >> + /* GPR registers. */ >> { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS, >> - sizeof (struct user_pt_regs), GENERAL_REGS, >> + 0, GENERAL_REGS, >> aarch64_fill_gregset, aarch64_store_gregset }, >> + /* Floating Point (FPU) registers. */ >> { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET, >> - sizeof (struct user_fpsimd_state), FP_REGS, >> + 0, FP_REGS, >> aarch64_fill_fpregset, aarch64_store_fpregset >> }, >> + /* Scalable Vector Extension (SVE) registers. */ >> + { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_SVE, >> + 0, EXTENDED_REGS, >> + aarch64_sve_regs_copy_from_regcache, aarch64_sve_regs_copy_to_regcache >> + }, >> + /* PAC registers. */ >> { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_PAC_MASK, >> - AARCH64_PAUTH_REGS_SIZE, OPTIONAL_REGS, >> - NULL, aarch64_store_pauthregset }, >> + 0, OPTIONAL_REGS, >> + nullptr, aarch64_store_pauthregset }, >> + /* Tagged address control / MTE registers. */ >> { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_TAGGED_ADDR_CTRL, >> - AARCH64_LINUX_SIZEOF_MTE, OPTIONAL_REGS, aarch64_fill_mteregset, >> + 0, OPTIONAL_REGS, aarch64_fill_mteregset, >> aarch64_store_mteregset }, > > Minor nit - the newlines in the PAC and MTE sets are inconsistent with the ones above. > Indeed. I'll get this adjusted. Thanks!