From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2e.google.com (mail-oa1-x2e.google.com [IPv6:2001:4860:4864:20::2e]) by sourceware.org (Postfix) with ESMTPS id C94253858D1E for ; Fri, 4 Aug 2023 20:45:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C94253858D1E 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-oa1-x2e.google.com with SMTP id 586e51a60fabf-1bb575a6ed3so1875027fac.2 for ; Fri, 04 Aug 2023 13:45:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1691181941; x=1691786741; 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=6e0cmuyB57QGWgV6UkLD+VSBW1AvisPSPOYJmVOP/qA=; b=lRsdZJLNsae53mAUlLQwZoWsI5j39hfwBY0O1OgLyZbf8l//e5iIGylSIYjQT0eiFc F65/USBDzFCjWe4B9poACTbUEnMo9Bo3XZEgqU/6fMjrdncCZtlbpRUUS0ZgG+zaT++f gsR6yAB/SD0+hXyvx14tBr+J1QLPa/97vlG5zB7fsAKAdr4JNj+RC663mst8rKcte3j/ yskKqf6/lUfiMTdxmIPX1DEZo1uCWutydWiWAeCg8x7hM7NCBmqgbwyrVrEELuAgbsHi u27i13AMOmkQJHFoO1rbwjYzaWLFXpXg9waxd4kKRGFXxgfUJOd/BWgiAt4FCy7LOiZq gyeA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691181941; x=1691786741; 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=6e0cmuyB57QGWgV6UkLD+VSBW1AvisPSPOYJmVOP/qA=; b=OhGlLIo8pi0Ngd5fZCvQw5CicfFP+BwODtjXX1CjKIK7viT178mQRtZI1+H1UbLAYr F50mE1a+M+87eyxJpffZwH3z/KQ/z4UHrEbjLDriZRIwNK9BGo9D2FJaDweCGtNpEG/s /WIPclfVOWE8g3sNMTerVTZDXT4KmYlM5v7Vrw7jAByqoYvgVqdNw/MwVUj3tG69yGRF PPuuBlsvqxq2i1+9Q+MCtpKed9BJak5k/Aa7bKalHA1gPcroTy9K3/WsfpmymdXP+fp0 u4PBG0x3yZKXeRcygIgpmGTqJJ49r1HHu7Wv6pYDUJJBpyiUuyE08YHThhHVGONIbLDF E45w== X-Gm-Message-State: AOJu0Yxjvjeb/8sgDAj3HhyzgwGYKsSy8hCmwSioddQOtR9od1Q3R+Dt muSeBrOqBNSJIQr5Uc3XL1aS2w== X-Google-Smtp-Source: AGHT+IEBhTi+n2aTtSEN+ffweNkg0GaqGVNKiJEbvHENEV2/wa+YuKluZpqMOGrBnZjY+q2CJ1EPxA== X-Received: by 2002:a05:6870:5690:b0:1be:f1b0:fb9 with SMTP id p16-20020a056870569000b001bef1b00fb9mr3373745oao.16.1691181941023; Fri, 04 Aug 2023 13:45:41 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:b83b:a53e:3bb9:7f93]) by smtp.gmail.com with ESMTPSA id u4-20020a05687004c400b001bb83eca69asm1583097oam.46.2023.08.04.13.45.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Aug 2023 13:45:40 -0700 (PDT) References: <20230630134616.1238105-1-luis.machado@arm.com> <20230630134616.1238105-15-luis.machado@arm.com> <874jlh2d39.fsf@linaro.org> <4fec3ac7-1dcb-3cd3-4059-3b101b5fd1f2@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: <4fec3ac7-1dcb-3cd3-4059-3b101b5fd1f2@arm.com> Date: Fri, 04 Aug 2023 17:45:37 -0300 Message-ID: <871qgiilji.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 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 writes: > On 8/3/23 01:18, Thiago Jung Bauermann wrote: >> >> Luis Machado via Gdb-patches writes: >> >>> + /* 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. >> > > Did you mean s/SSE/SME? > > I'm not sure I understand. We need to process the SSVE entry first to make sure we have > the right values for SVCR, and > the SVE entry may be inactive. Could you please expand on your idea? I am assuming that if SVE is inactive, the buffer will at least have a header with sane values such as the dummy one written by collect_inactive_sve_regset. In this case, we could check whether the flags field has the SVE bit set: diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c index 0bd75daa994c..ce9cc4e32000 100644 --- a/gdb/aarch64-linux-tdep.c +++ b/gdb/aarch64-linux-tdep.c @@ -1034,11 +1034,14 @@ aarch64_linux_supply_sve_regset (const struct regset *regset, if (tdep->has_sme ()) { - ULONGEST svcr = 0; - regcache->raw_collect (tdep->sme_svcr_regnum, &svcr); + gdb_byte *header = (gdb_byte *) buf; + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); + uint16_t flags + = extract_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET, + SVE_HEADER_FLAGS_LENGTH, byte_order); /* Is streaming mode enabled? */ - if (svcr & SVCR_SM_BIT) + if (flags & SVE_HEADER_FLAG_SVE) /* If so, don't load SVE data from the SVE section. The data to be used is in the SSVE section. */ return; But if we can't assume dummy values for the SVE header, then my idea won't work. >>> + >>> + /* 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 (). >> > > That is a bit strange, but I don't see this function being called with a specified regnum > other than -1. > > For instance, in gcore-elf.c:gcore_elf_build_thread_register_notes, we fetch all > registers. > > Also, in corelow.c:core_target::get_core_register_section, supply is also explicitly > called with -1. > > Maybe this was updated at some point, and we don't fetch single registers from core files > anymore. > > Do you see any paths calling this with regnum != -1? Indeed you're right, I don't. I made this comment based on the documentation of regset and regcache functions, and existing regcache code that deals with regnum != -1. I see a few targets that seems to expect calling regset->collect_regset with regnum != -1 (e.g., fbsd_nat_target::store_register_set and ppc-linux-nat.c's store_regset) but nothing in generic code or aarch64-linux nat or tdep. >>> + >>> + /* 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? >> > > Yes. It will be invalidated and memset-ed to 0. Though that behavior might not be a strong > guarantee. Ok, thanks for the explanation. -- Thiago