From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [IPv6:2610:1c1:1:606c::19:2]) by sourceware.org (Postfix) with ESMTPS id 8FCD53858D38 for ; Mon, 10 Apr 2023 20:03:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8FCD53858D38 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4PwKfD2BxPz3SMp; Mon, 10 Apr 2023 20:03:16 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4PwKfD1RvKz48Bh; Mon, 10 Apr 2023 20:03:16 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1681156996; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5iAg2iWSz1Rwl20VAXJ2L6axTqwc9187wfkpNuSF89E=; b=snHyBo+lE5qHK1+T58CVypU+sF0sPd9rTsK36Glj2CToSaopVS4rqkqdJrtjZvk0x5JduI efiIReswCGU5kGNdhWrGCAlJP+bJGWg5U5gqfpIAbLsRt8K2+vTCOPjoaRXiQbYPhwIvnF GOxXeSrTf7A0u5fgU60E2wwbe76z178K1Jt8u0F90u/k1LzvrELP0UK3LaZKAuyQjfwm4O 6TCxZytYOvYfEJwNrWu2duVrT8+77TCX72cp0Y6sxajeil3wP2i8vXONjzZY0wE/WJlUHG YqWlvaF/bavUkzbJ3lfnZJthWdY0ImbKDIOEoAf+El+/wNu7cCEa1XgHWa111w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1681156996; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5iAg2iWSz1Rwl20VAXJ2L6axTqwc9187wfkpNuSF89E=; b=KNrLXWtDC/eM0kgTlUPBzDrmYblO5CqR5FEwlyRQ0zqRb75quWBDjvAAl/Zlwb9R2ii311 F/HqRdSj8CNV/HU2wuEoqfEFbnJ5U7TokCUHSRKk1sbaHU+Nj//SMD0QxBzn9H/cR4Yh9h LjX/F6PGlCoeMNoBZoai4UvAp3Pc5mDhcY/Y3lWgrZccpwsFV2JJRi+e1fZL/45Md7nGju AgRZ9pubfqXh+nLhABxECEARA2J1ffEOWvC0Ztc5Y/KcDRL0ofBuMxk4G2GpPFXqXAz95F gXAkQ6ksyBbCHyfn+RSDE3anvayqrAHMbXvOjA3ArkBRUqZi+PFDuLiEV7cTlQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1681156996; a=rsa-sha256; cv=none; b=kqtkStmrrtWntUsmrCHScJCDcd2hzYZxAOVhw+G9VK8Ii2/4t6OomgW1zXwqr4sEACtCyb g1CcfWXk1JAcRYm7VjU9GOvon6QG3sKKsqi5+OMx1rVMEFzsgJ0voSYCQAfv/uF+JvP/mF JtX1CpvIte0Gr9blP7h9E6EoBTTvSs7EkqnepXotPcPFc9Vv3vURxv1a5HvhGGup8kg/1f DuUbhSYKfsEFRE7Pvv/txTpyGL6rcYD62vDBUBgcIfVvNBFh9N5mt+sAd6DVCkTJJMHnzI dnHTsNw1ViJe6Yuthv8ErEdXJGQBlsFvW8bcVvQXNujuWc2pa1t25s2g7rOdUg== Received: from [IPV6:2601:648:8680:16b0:542a:22bd:2c8e:f2bd] (unknown [IPv6:2601:648:8680:16b0:542a:22bd:2c8e:f2bd]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4PwKfC4zlJzdnl; Mon, 10 Apr 2023 20:03:15 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <113146d6-48af-f0a3-0e87-4495a4529ca0@FreeBSD.org> Date: Mon, 10 Apr 2023 13:03:14 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org Cc: Aleksandar Paunovic References: <20230318010905.14294-1-jhb@FreeBSD.org> <20230318010905.14294-2-jhb@FreeBSD.org> From: John Baldwin Subject: Re: [PATCH v4 01/13] x86: Add an x86_xsave_layout structure to handle variable XSAVE layouts. In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_STATUS,NICE_REPLY_A,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: On 4/6/23 12:09 PM, Simon Marchi wrote: > On 3/17/23 21:08, John Baldwin wrote: >> The standard layout of the XSAVE extended state area consists of three >> regions. The first 512 bytes (legacy region) match the layout of the >> FXSAVE instruction including floating point registers, MMX registers, >> and SSE registers. The next 64 bytes (XSAVE header) contains a header >> with a fixed layout. The final region (extended region) contains zero >> or more optional state components. Examples of these include the >> upper 128 bits of YMM registers for AVX. >> >> These optional state components generally have an >> architecturally-fixed size, but they are not assigned architectural >> offsets in the extended region. Instead, processors provide >> additional CPUID leafs describing the size and offset of each >> component in the "standard" layout for a given CPU. (There is also a >> "compact" format which uses an alternate layout, but existing OS's >> currently export the "standard" layout when exporting XSAVE data via >> ptrace() and core dumps.) >> >> To date, GDB has assumed the layout used on current Intel processors >> for state components in the extended region and hardcoded those >> offsets in the tables in i387-tdep.c and i387-fp.cc. However, this >> fails on recent AMD processors which use a different layout. >> Specifically, AMD Ryzen 9 processors at least do not leave space for >> the MPX register set in between the AVX and AVX512 register sets. It >> is not known if other AMD processors are effected, but seems probable. >> >> To rectify this, add an x86_xsave_layout structure which contains the >> total size of the XSAVE extended state area as well as the offset of >> each known optional state component. This structure is stored in >> i386_gdbarch_tdep and is fetched from the current target in >> i386_gdbarch_init as a TARGET_OBJECT_X86_XSAVE_LAYOUT object. >> >> Subsequent commits will modify XSAVE parsing to use x86_xsave_layout. > > Thanks for the nice description, it helped me, since I was not familiar > with that feature. Just some minor comments and questions. > >> Co-authored-by: Aleksandar Paunovic >> --- >> gdb/i386-tdep.c | 36 +++++++++++++++++++++++++- >> gdb/i386-tdep.h | 4 +++ >> gdb/target.h | 2 ++ >> gdbsupport/x86-xstate.h | 57 +++++++++++++++++++++++++++++++++++------ >> 4 files changed, 90 insertions(+), 9 deletions(-) >> >> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c >> index 96c04c1a3d6..5c2d9e42d8d 100644 >> --- a/gdb/i386-tdep.c >> +++ b/gdb/i386-tdep.c >> @@ -8493,19 +8493,51 @@ i386_type_align (struct gdbarch *gdbarch, struct type *type) >> } >> >> >> +/* Fetch the XSAVE layout for the current target. */ >> + >> +static struct x86_xsave_layout > > Nit: drop the struct keyword whenever you use x86_xsave_layout. Will fix in the entire series. >> +i386_fetch_xsave_layout () >> +{ >> + struct x86_xsave_layout layout; >> + LONGEST len = target_read (current_inferior ()->top_target (), >> + TARGET_OBJECT_X86_XSAVE_LAYOUT, nullptr, >> + (gdb_byte *) &layout, 0, sizeof (layout)); >> + if (len != sizeof (layout)) >> + return {}; > > Do you have an idea of which conditions can make it so `len != sizeof > (layout)`? If I understand correctly, the contract for > TARGET_OBJECT_X86_XSAVE_LAYOUT is that the caller passes a pointer to an > x86_xsave_layout object and the callee fills it. This is all internal > to GDB. I don't imagine how a target could return something else than > ;`sizeof (layout)` (indicating success) or TARGET_XFER_E_IO (indicating > failure), other than being buggy. > > All of this to say, should it be an assert? Hmm, in the case of TARGET_XFER_E_IO this function needs to return an empty object (e.g. for native targets that don't support XSAVE at all). I should perhaps make it check for an error explicitly and then assert that otherwise they are equal? >> + >> + return layout; >> +} >> + >> /* Note: This is called for both i386 and amd64. */ >> >> static struct gdbarch * >> i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) >> { >> const struct target_desc *tdesc; >> + struct x86_xsave_layout xsave_layout; >> int mm0_regnum; >> int ymm0_regnum; >> int bnd0_regnum; >> int num_bnd_cooked; >> >> + xsave_layout = i386_fetch_xsave_layout (); > > Declare the variable where you initialize it. Fixed. >> + >> /* If there is already a candidate, use it. */ >> - arches = gdbarch_list_lookup_by_info (arches, &info); >> + for (arches = gdbarch_list_lookup_by_info (arches, &info); >> + arches != NULL; >> + arches = gdbarch_list_lookup_by_info (arches->next, &info)) >> + { >> + /* Check that the XSAVE layout of ARCHES matches the layout for >> + the current target. */ >> + i386_gdbarch_tdep *other_tdep >> + = (i386_gdbarch_tdep *) gdbarch_tdep (arches->gdbarch); > > This needs to be updated to gdbarch_tdep (...). Humm, I had fixed these when rebasing it, but I must have fixed this later in the series. I've fixed it here now. >> @@ -8762,6 +8794,8 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) >> gdbarch_free (gdbarch); >> return NULL; >> } >> + if (tdep->xcr0 != 0) >> + tdep->xsave_layout = xsave_layout; > > Just wondering if the "if" is really necessary. If nothing is > supported, I suppose the RHS xsave_layout will be a default one. And > the LHS is always a default one, having just been created. That's fair. >> @@ -145,6 +146,9 @@ struct i386_gdbarch_tdep : gdbarch_tdep_base >> /* Offset of XCR0 in XSAVE extended state. */ >> int xsave_xcr0_offset = 0; > > Does this represent the offset the saved value of xcr0 in the xsave > area? I haven't read that the xcr0 value would be saved there, but this > comment surely makes it sound like it. If so, does it belong inside > x86_xsave_layout? So both Linux and FreeBSD re-use a reserved field in the normal FXSAVE state area to store XCR0 in the XSAVE note saved in core dumps. The gdbarch_core_read_description methods then fetch the XCR0 mask from the first XSAVE core dump note to determine the XCR0 mask used for the core dump (and thus the XSAVE size and layout). Note that unlike the fields in xsave_layout (which are a description of architecture-specified behavior), this field is a software construct followed by Linux and FreeBSD. Note that the new core dump note discussed in earlier versions of this series would result in this field probably not being used for core dumps which include the new note. >> +/* Size and offsets of register states in the XSAVE area extended >> + region. Offsets are set to 0 to indicate the absence of the >> + associated registers. */ >> + >> +struct x86_xsave_layout >> +{ >> + int sizeof_xsave = 0; >> + int avx_offset = 0; >> + int bndregs_offset = 0; >> + int bndcfg_offset = 0; >> + int k_offset = 0; >> + int zmm_h_offset = 0; >> + int zmm_offset = 0; >> + int pkru_offset = 0; >> + >> + bool operator!= (const x86_xsave_layout &other) >> + { >> + return sizeof_xsave != other.sizeof_xsave >> + || avx_offset != other.avx_offset >> + || bndregs_offset != other.bndregs_offset >> + || bndcfg_offset != other.bndcfg_offset >> + || k_offset != other.k_offset >> + || zmm_h_offset != other.zmm_h_offset >> + || zmm_offset != other.zmm_offset >> + || pkru_offset != other.pkru_offset; >> + } > > Heh, would be nice to be able to use > > https://en.cppreference.com/w/cpp/language/default_comparisons Huh, I didn't know about that feature, but yes that would be nice to use eventually. -- John Baldwin