From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com [IPv6:2607:f8b0:4864:20::22f]) by sourceware.org (Postfix) with ESMTPS id 8D9703858D33 for ; Thu, 3 Aug 2023 00:18:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8D9703858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oi1-x22f.google.com with SMTP id 5614622812f47-3a5ad6087a1so215915b6e.2 for ; Wed, 02 Aug 2023 17:18:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1691021887; x=1691626687; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=W8ZnhJg/mceO/wk4bi0kzLLB39SDM8lN46hyok1uXwE=; b=idr05i/mahwZm/GopuwTGdsf2LaXYB+nfr3GCZqkXoBH40OSy8x+qAzdU/AwisDYjq 20lo7AGGBxiIkCixkj/puawzhsxUlmRpYzeyLCLRDdKS5k8LPtGnCKTNp6UxH89EPNuA i668MbnF4kwo/v1SxA2IdDgUrF5aU5AMQM9wYWs/OQApQl1hrbE6gr1no4m7So4TyyfH hGzLdGqLvPRBH6BXRkUwPR6wo7QXyh9ashX6BwVQtTEYfVTM9vqKpeKZ8a+ZZW9SBLBV 8WoOZwW89eB3bqrzRz6PubDqr8TR2YlQp70VagYJolEawMA/btH/470LXhkAXmaHgDhc UwKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691021887; x=1691626687; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=W8ZnhJg/mceO/wk4bi0kzLLB39SDM8lN46hyok1uXwE=; b=KvHkfwHjLc52J/3GQ/0pWqv8wwnEfc2aUInqd9uHLDj88qC1TGfU2WiGmXoJ3YtYfD TRGifAJmn3VMOnciScJQlCjt50o27GuHaFEJCwFTKbuNb7SaBdwsDrXIg96m7tj4oAGs nw1HEXpo0GjfoNgsOPEELGe9+ww4iYSyIl3VHpGxXKA+K0eC9GTBWUrFtPcGDjJaPjDr +l/gqJxozYWGRzNX0fSSCIagGQxzaeayUis5gpGkDpXklfWtQCN1ioAycpF6WzJ8mrjY 9POZ1XKGwUPPIqvpx3V6lQZ1DVmyFZnOt46YA27F0qShV4RNl87aJpvkgeYb/tog4LYe xAKQ== X-Gm-Message-State: ABy/qLYK26Y9fwljLAnzdqxJhvu1jwU8wniU/Ij9/ubEN5lBapJ2JKgA bFoFCsM4TXwurwuUbPwE8Bu9cg== X-Google-Smtp-Source: APBJJlFgDLx55pOpUJVS9xUm12KjfzXkOGBw9JDakz2Nr+66FrcdMbZjYLAkQecNgGy1F7w50BGbxQ== X-Received: by 2002:a05:6870:14c7:b0:19e:fa1f:fc2f with SMTP id l7-20020a05687014c700b0019efa1ffc2fmr16247400oab.38.1691021886576; Wed, 02 Aug 2023 17:18:06 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:35ac:e9c7:9887:93de]) by smtp.gmail.com with ESMTPSA id dt2-20020a0568705a8200b001b03fbfa0c5sm7015713oab.39.2023.08.02.17.18.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Aug 2023 17:18:05 -0700 (PDT) References: <20230630134616.1238105-1-luis.machado@arm.com> <20230630134616.1238105-15-luis.machado@arm.com> User-agent: mu4e 1.10.5; emacs 28.2 From: Thiago Jung Bauermann To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 14/16] [gdb/aarch64] sme: Core file support for Linux In-reply-to: <20230630134616.1238105-15-luis.machado@arm.com> Date: Wed, 02 Aug 2023 21:18:02 -0300 Message-ID: <874jlh2d39.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Luis Machado via Gdb-patches writes: > This patch enables dumping SME state via gdb's gcore command and also > enables gdb to read SME state from a core file generated by the Linux > Kernel. > > Regression-tested on aarch64-linux Ubuntu 22.04/20.04. > --- > gdb/aarch64-linux-tdep.c | 532 ++++++++++++++++++++++++++++-- > gdb/arch/aarch64-scalable-linux.c | 34 ++ > gdb/arch/aarch64-scalable-linux.h | 15 + > 3 files changed, 548 insertions(+), 33 deletions(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 7ce34ee6846..0bd75daa994 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -57,6 +57,10 @@ > > #include "elf/common.h" > #include "elf/aarch64.h" > +#include "arch/aarch64-insn.h" > + > +/* For std::sqrt */ s/sqrt/pow/ > +#include > @@ -853,14 +901,89 @@ aarch64_linux_supply_sve_regset (const struct regset *regset, > } > } > > +/* Collect an inactive SVE register set state. This is equivalent to a > + fpsimd layout. > + > + Collect the data from REGCACHE to BUF, using the register > + map in REGSET. */ > + > +static void > +collect_inactive_sve_regset (const struct regcache *regcache, > + void *buf, size_t size, int vg_regnum) > +{ > + gdb_byte *header = (gdb_byte *) buf; > + struct gdbarch *gdbarch = regcache->arch (); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + > + gdb_assert (buf != nullptr); > + gdb_assert (size > SVE_HEADER_SIZE); This assert should check for SVE_CORE_DUMMY_SIZE. > + > + /* Zero out everything first. */ > + memset ((gdb_byte *) buf, 0, SVE_CORE_DUMMY_SIZE); > + > + /* BUF starts with a SVE header prior to the register dump. */ > + > + /* Dump the default size of an empty SVE payload. */ > + uint32_t real_size = SVE_CORE_DUMMY_SIZE; > + store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET, > + SVE_HEADER_SIZE_LENGTH, byte_order, real_size); > + > + /* Dump a dummy max size. */ > + uint32_t max_size = SVE_CORE_DUMMY_MAX_SIZE; > + store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET, > + SVE_HEADER_MAX_SIZE_LENGTH, byte_order, max_size); > + > + /* Dump the vector length. */ > + ULONGEST vg = 0; > + regcache->raw_collect (vg_regnum, &vg); > + uint16_t vl = sve_vl_from_vg (vg); > + store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH, > + byte_order, vl); > + > + /* Dump the standard maximum vector length. */ > + uint16_t max_vl = SVE_CORE_DUMMY_MAX_VL; > + store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET, > + SVE_HEADER_MAX_VL_LENGTH, byte_order, > + max_vl); > + > + /* The rest of the fields is zero. */ s/is/are/ > + uint16_t flags = SVE_CORE_DUMMY_FLAGS; > + store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET, > + SVE_HEADER_FLAGS_LENGTH, byte_order, > + flags); > + uint16_t reserved = SVE_CORE_DUMMY_RESERVED; > + store_unsigned_integer (header + SVE_HEADER_RESERVED_OFFSET, > + SVE_HEADER_RESERVED_LENGTH, byte_order, reserved); > + > + /* We are done with the header part of it. Now dump the register state > + in the FPSIMD format. */ > + > + /* Dump the first 128 bits of each of the Z registers. */ > + header += AARCH64_SVE_CONTEXT_REGS_OFFSET; > + for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++) > + regcache->raw_collect_part (AARCH64_SVE_Z0_REGNUM + i, 0, V_REGISTER_SIZE, > + header + V_REGISTER_SIZE * i); > + > + /* Dump FPSR and FPCR. */ > + header += 32 * V_REGISTER_SIZE; > + regcache->raw_collect (AARCH64_FPSR_REGNUM, header); > + regcache->raw_collect (AARCH64_FPCR_REGNUM, header); Header needs to be incremented before writing FPCR, otherwise raw_collect() will just overwrite FPSR. > + > + /* Dump two reserved empty fields of 4 bytes. */ > + header += 8; > + memset (header, 0, 8); > + > + /* We should have a FPSIMD-formatted register dump now. */ > +} > +/* Supply register REGNUM from BUF to REGCACHE, using the register map > + in REGSET. If REGNUM is -1, do this for all registers in REGSET. > + If BUF is NULL, set the registers to "unavailable" status. */ > + > +static void > +aarch64_linux_supply_sve_regset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *buf, size_t size) > +{ > + struct gdbarch *gdbarch = regcache->arch (); > + aarch64_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > + if (tdep->has_sme ()) > + { > + ULONGEST svcr = 0; > + regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); Instead of relying on parsing the SSVE section before the SVE section to ensure that the SVCR register is valid by the time we arrive here, isn't it more robust to read the flags field from the SVE header in buf, as is done in suply_sve_regset()? Also, it's what the SSE version does. > + > + /* Is streaming mode enabled? */ > + if (svcr & SVCR_SM_BIT) > + /* If so, don't load SVE data from the SVE section. The data to be > + used is in the SSVE section. */ > + return; > + } > + /* If streaming mode is not enabled, load the SVE regcache data from the SVE > + section. */ > + supply_sve_regset (regset, regcache, regnum, buf, size); > +} > + > +/* Collect register REGNUM from REGCACHE to BUF, using the register > + map in REGSET. If REGNUM is -1, do this for all registers in > + REGSET. */ > + > +static void > +aarch64_linux_collect_sve_regset (const struct regset *regset, > + const struct regcache *regcache, > + int regnum, void *buf, size_t size) > +{ > + struct gdbarch *gdbarch = regcache->arch (); > + aarch64_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + bool streaming_mode = false; > + > + if (tdep->has_sme ()) > + { > + ULONGEST svcr = 0; > + regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); > + > + /* Is streaming mode enabled? */ > + if (svcr & SVCR_SM_BIT) > + { > + /* If so, don't dump SVE regcache data to the SVE section. The SVE > + data should be dumped to the SSVE section. Dump an empty SVE > + block instead. */ > + streaming_mode = true; > + } > + } > + > + /* If streaming mode is not enabled or there is no SME support, dump the > + SVE regcache data to the SVE section. */ > + > + /* Check if we have an active SVE state (non-zero Z/P/FFR registers). > + If so, then we need to dump registers in the SVE format. > + > + Otherwise we should dump the registers in the FPSIMD format. */ > + if (sve_state_is_empty (regcache) || streaming_mode) > + collect_inactive_sve_regset (regcache, buf, size, AARCH64_SVE_VG_REGNUM); If regnum != -1, collect_inactive_sve_regset () will still dump the entirety of the fpsimd regset. Shouldn't it get the regnum as an argument and dump just the requested register? Same comment in aarch64_linux_collect_ssve_regset (). > + else > + collect_sve_regset (regset, regcache, regnum, buf, size); > +} > +/* Collect register REGNUM from REGCACHE to BUF, using the register > + map in REGSET. If REGNUM is -1, do this for all registers in > + REGSET. */ > + > +static void > +aarch64_linux_collect_ssve_regset (const struct regset *regset, > + const struct regcache *regcache, > + int regnum, void *buf, size_t size) > +{ > + struct gdbarch *gdbarch = regcache->arch (); > + aarch64_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + ULONGEST svcr = 0; > + regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); > + > + /* Is streaming mode enabled? */ > + if (svcr & SVCR_SM_BIT) > + { > + /* If so, dump SVE regcache data to the SSVE section. */ > + collect_sve_regset (regset, regcache, regnum, buf, size); > + } > + else > + { > + /* Otherwise dump an empty SVE block to the SSVE section with the > + streaming vector length. */ > + collect_inactive_sve_regset (regcache, buf, size, tdep->sme_svg_regnum); Same argument here about regnum != -1 as in aarch64_linux_collect_sve_regset(). > + } > +} > + > +/* Supply register REGNUM from BUF to REGCACHE, using the register map > + in REGSET. If REGNUM is -1, do this for all registers in REGSET. > + If BUF is NULL, set the registers to "unavailable" status. */ > + > +static void > +aarch64_linux_supply_za_regset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *buf, size_t size) > +{ > + gdb_byte *header = (gdb_byte *) buf; > + struct gdbarch *gdbarch = regcache->arch (); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + > + /* Handle an empty buffer. */ > + if (buf == nullptr) > + return regcache->supply_regset (regset, regnum, nullptr, size); > + > + if (size < SVE_HEADER_SIZE) > + warning (_("ZA state header size (%s) invalid. Should be at least %s."), > + pulongest (size), pulongest (SVE_HEADER_SIZE)); If the header is truncated, I don't think the rest of this function works correctly. I'd suggest either changing the warning to an error, or treating this case in the same way as the buf == nullptr case above. > + > + /* The ZA register note in a core file can have a couple of states: > + > + 1 - Just the header without the payload. This means that there is no > + ZA data, and we should populate only SVCR and SVG registers on GDB's > + side. The ZA data should be marked as unavailable. > + > + 2 - The header with an additional data payload. This means there is > + actual ZA data, and we should populate ZA, SVCR and SVG. */ > + > + aarch64_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > + /* Populate SVG. */ > + ULONGEST svg > + = sve_vg_from_vl (extract_unsigned_integer (header + SVE_HEADER_VL_OFFSET, > + SVE_HEADER_VL_LENGTH, > + byte_order)); > + regcache->raw_supply (tdep->sme_svg_regnum, &svg); > + > + size_t data_size > + = extract_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET, > + SVE_HEADER_SIZE_LENGTH, byte_order) > + - SVE_HEADER_SIZE; > + > + /* Populate SVCR. */ > + bool has_za_payload = (data_size > 0); > + ULONGEST svcr; > + regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); > + > + /* If we have a ZA payload, enable bit 2 of SVCR, otherwise clear it. This > + register gets updated by the SVE/SSVE-handling functions as well, as they > + report the SM bit 1. */ > + if (has_za_payload) > + svcr |= SVCR_ZA_BIT; > + else > + svcr &= ~SVCR_ZA_BIT; > + > + /* Update SVCR in the register buffer. */ > + regcache->raw_supply (tdep->sme_svcr_regnum, &svcr); > + > + /* Populate the register cache with ZA register contents, if we have any. */ > + buf = has_za_payload ? (gdb_byte *) buf + SVE_HEADER_SIZE : nullptr; > + > + /* Update ZA in the register buffer. */ > + if (has_za_payload) > + regcache->raw_supply (tdep->sme_za_regnum, buf); Do we need to make sure that buf is big enough to supply the whole contents of the ZA register? Otherwise, the memcpy() in raw_supply() will copy garbage. > + else > + { > + size_t za_bytes = std::pow (sve_vl_from_vg (svg), 2); > + gdb_byte za_zeroed[za_bytes]; > + memset (za_zeroed, 0, za_bytes); > + regcache->raw_supply (tdep->sme_za_regnum, za_zeroed); > + } > +} > + > +/* Collect register REGNUM from REGCACHE to BUF, using the register > + map in REGSET. If REGNUM is -1, do this for all registers in > + REGSET. */ > + > +static void > +aarch64_linux_collect_za_regset (const struct regset *regset, > + const struct regcache *regcache, > + int regnum, void *buf, size_t size) > +{ > + gdb_assert (buf != nullptr); > + > + if (size < SVE_HEADER_SIZE) > + warning (_("ZA state header size (%s) invalid. Should be at least %s."), > + pulongest (size), pulongest (SVE_HEADER_SIZE)); Here too, if size < SVE_HEADER_SIZE I don't think it makes sense to try to continue. Perhaps put this condition in a gdb_assert()? > + > + /* The ZA register note in a core file can have a couple of states: > + > + 1 - Just the header without the payload. This means that there is no > + ZA data, and we should dump just the header. > + > + 2 - The header with an additional data payload. This means there is > + actual ZA data, and we should dump both the header and the ZA data > + payload. */ > + > + aarch64_gdbarch_tdep *tdep > + = gdbarch_tdep (regcache->arch ()); > + > + /* Determine if we have ZA state from the SVCR register ZA bit. */ > + ULONGEST svcr; > + regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); > + > + /* Check the ZA payload. */ > + bool has_za_payload = (svcr & SVCR_ZA_BIT) != 0; > + size = has_za_payload ? size : SVE_HEADER_SIZE; > + > + /* Write the size and max_size fields. */ > + gdb_byte *header = (gdb_byte *) buf; > + enum bfd_endian byte_order = gdbarch_byte_order (regcache->arch ()); > + store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET, > + SVE_HEADER_SIZE_LENGTH, byte_order, size); > + > + uint32_t max_size > + = SVE_HEADER_SIZE + std::pow (sve_vl_from_vq (tdep->sme_svq), 2); > + store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET, > + SVE_HEADER_MAX_SIZE_LENGTH, byte_order, max_size); > + > + /* Output the other fields of the ZA header (vl, max_vl, flags and > + reserved). */ > + uint64_t svq = tdep->sme_svq; > + store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH, > + byte_order, sve_vl_from_vq (svq)); > + > + uint16_t max_vl = SVE_CORE_DUMMY_MAX_VL; > + store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET, > + SVE_HEADER_MAX_VL_LENGTH, byte_order, > + max_vl); > + > + uint16_t flags = SVE_CORE_DUMMY_FLAGS; > + store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET, > + SVE_HEADER_FLAGS_LENGTH, byte_order, flags); > + > + uint16_t reserved = SVE_CORE_DUMMY_RESERVED; > + store_unsigned_integer (header + SVE_HEADER_RESERVED_OFFSET, > + SVE_HEADER_RESERVED_LENGTH, byte_order, reserved); > + > + buf = has_za_payload ? (gdb_byte *) buf + SVE_HEADER_SIZE : nullptr; > + > + /* Dump the register cache contents for the ZA register to the buffer. */ > + regcache->collect_regset (regset, regnum, (gdb_byte *) buf, > + size - SVE_HEADER_SIZE); Calling collect with buf == nullptr will invalidate the regset. Is that the intention? > +} -- Thiago