From: "H.J. Lu" <hjl.tools@gmail.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: GNU C Library <libc-alpha@sourceware.org>
Subject: Re: [PATCH] x86: Copy IBT and SHSTK usable only if CET is enabled
Date: Wed, 23 Jun 2021 17:40:20 -0700 [thread overview]
Message-ID: <CAMe9rOpJkfpqC8hzk0S2nRUkjmx8a7YjrS0F1WBFN8ArNnt-8A@mail.gmail.com> (raw)
In-Reply-To: <89312fbc-0fb1-8523-2c1c-c99025bb787e@linaro.org>
On Wed, Jun 23, 2021 at 5:30 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 23/06/2021 18:36, H.J. Lu wrote:
> > On Wed, Jun 23, 2021 at 2:15 PM Adhemerval Zanella
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 23/06/2021 17:41, H.J. Lu wrote:
> >>>> @@ -216,12 +233,15 @@ do_test (int argc, char **argv)
> >>>> fails += CHECK_PROC (sgx, SGX);
> >>>> fails += CHECK_PROC (sgx_lc, SGX_LC);
> >>>> fails += CHECK_PROC (sha_ni, SHA);
> >>>> - fails += CHECK_PROC (shstk, SHSTK);
> >>>> + fails += CHECK_PROC_OPTIN (shstk, SHSTK);
> >>>
> >>> Why do you need this? If kernel doesn't support SHSTK, it will be
> >>> turned off:
> >>>
> >>> /* Check CET status. */
> >>> unsigned int cet_status = get_cet_status ();
> >>>
> >>> if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_IBT) == 0)
> >>> CPU_FEATURE_UNSET (cpu_features, IBT)
> >>> if ((cet_status & GNU_PROPERTY_X86_FEATURE_1_SHSTK) == 0)
> >>> CPU_FEATURE_UNSET (cpu_features, SHSTK)
> >>
> >> The problem is this is only enabled for CET_ENABLED, the configuration I am using
> >> does not define __CET__. So the CPU I am using does support SHSTK, but the bit
> >> ended up not being cleared by glibc.
> >
> > IBT and SHSTK usable bits are copied from CPUID feature bits and later
> > cleared if kernel doesn't support CET. Copy IBT and SHSTK usable only
> > if CET is enabled so that they aren't set on CET capable processors
> > with non-CET enabled glibc.
> >
> > Can you try this?
>
> It fixes the SHSTK issue, but it still shows the following failures on
> the Ryzen 9 cpu:
>
> Checking HAS_CPU_FEATURE (IBRS_IBPB):
> HAS_CPU_FEATURE (IBRS_IBPB): 0
> cpuinfo (ibrs): 1
> *** failure ***
>
> Checking HAS_CPU_FEATURE (SSBD):
> HAS_CPU_FEATURE (SSBD): 0
> cpuinfo (ssbd): 1
> *** failure ***
>
> Checking HAS_CPU_FEATURE (STIBP):
> HAS_CPU_FEATURE (STIBP): 0
> cpuinfo (stibp): 1
> *** failure ***
>
> Below I update my patch, your look ok thanks!
>
> ---
>
> [PATCH v2] x86: Fix tst-cpu-features-cpuinfo on Ryzen 9 (BZ #27873)
>
> AMD define different flags for IRPB, IBRS, and STIPBP [1], so new
> x86_64_cpu are added and IBRS_IBPB is only tested for Intel.
>
> The SSDB is also defined and implemented different on AMD [2],
> and also a new AMD_SSDB flag is added. It should map to the
> cpuinfo 'ssdb' on recent AMD cpus.
>
> It fixes tst-cpu-features-cpuinfo and tst-cpu-features-cpuinfo-static
> on recent AMD cpus.
>
> Checked on x86_64-linux-gnu on AMD Ryzen 9 5900X.
>
> [1] https://developer.amd.com/wp-content/resources/Architecture_Guidelines_Update_Indirect_Branch_Control.pdf
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=199889
> ---
> sysdeps/x86/bits/platform/x86.h | 4 ++++
> sysdeps/x86/include/cpu-features.h | 12 ++++++++++++
> sysdeps/x86/tst-cpu-features-cpuinfo.c | 22 ++++++++++++++++++----
> 3 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/sysdeps/x86/bits/platform/x86.h b/sysdeps/x86/bits/platform/x86.h
> index fe08d8a1b6..26e3b67ede 100644
> --- a/sysdeps/x86/bits/platform/x86.h
> +++ b/sysdeps/x86/bits/platform/x86.h
> @@ -278,6 +278,10 @@ enum
> + cpuid_register_index_ebx * 8 * sizeof (unsigned int)),
>
> x86_cpu_WBNOINVD = x86_cpu_index_80000008_ebx + 9,
> + x86_cpu_AMD_IBPB = x86_cpu_index_80000008_ebx + 12,
> + x86_cpu_AMD_IBRS = x86_cpu_index_80000008_ebx + 14,
> + x86_cpu_AMD_STIBP = x86_cpu_index_80000008_ebx + 15,
> + x86_cpu_AMD_SSBD = x86_cpu_index_80000008_ebx + 24,
>
> x86_cpu_index_7_ecx_1_eax
> = (CPUID_INDEX_7_ECX_1 * 8 * 4 * sizeof (unsigned int)
> diff --git a/sysdeps/x86/include/cpu-features.h b/sysdeps/x86/include/cpu-features.h
> index d042a2ebef..4f1c4ee402 100644
> --- a/sysdeps/x86/include/cpu-features.h
> +++ b/sysdeps/x86/include/cpu-features.h
> @@ -289,6 +289,10 @@ enum
>
> /* EBX. */
> #define bit_cpu_WBNOINVD (1u << 9)
> +#define bit_cpu_AMD_IBPB (1u << 12)
> +#define bit_cpu_AMD_IBRS (1u << 14)
> +#define bit_cpu_AMD_STIBP (1u << 15)
> +#define bit_cpu_AMD_SSBD (1u << 24)
>
> /* CPUID_INDEX_7_ECX_1. */
>
> @@ -519,6 +523,10 @@ enum
>
> /* EBX. */
> #define index_cpu_WBNOINVD CPUID_INDEX_80000008
> +#define index_cpu_AMD_IBPB CPUID_INDEX_80000008
> +#define index_cpu_AMD_IBRS CPUID_INDEX_80000008
> +#define index_cpu_AMD_STIBP CPUID_INDEX_80000008
> +#define index_cpu_AMD_SSBD CPUID_INDEX_80000008
>
> /* CPUID_INDEX_7_ECX_1. */
>
> @@ -749,6 +757,10 @@ enum
>
> /* EBX. */
> #define reg_WBNOINVD ebx
> +#define reg_AMD_IBPB ebx
> +#define reg_AMD_IBRS ebx
> +#define reg_AMD_STIBP ebx
> +#define reg_AMD_SSBD ebx
>
> /* CPUID_INDEX_7_ECX_1. */
>
> diff --git a/sysdeps/x86/tst-cpu-features-cpuinfo.c b/sysdeps/x86/tst-cpu-features-cpuinfo.c
> index 75e7eb9352..f457e8677b 100644
> --- a/sysdeps/x86/tst-cpu-features-cpuinfo.c
> +++ b/sysdeps/x86/tst-cpu-features-cpuinfo.c
> @@ -16,10 +16,11 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -#include <sys/platform/x86.h>
> +#include <cpu-features.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <stdbool.h>
>
> static char *cpu_flags;
>
> @@ -99,6 +100,7 @@ static int
> do_test (int argc, char **argv)
> {
> int fails = 0;
> + const struct cpu_features *cpu_features = __get_cpu_features ();
>
> get_cpuinfo ();
> fails += CHECK_PROC (acpi, ACPI);
> @@ -159,7 +161,17 @@ do_test (int argc, char **argv)
> fails += CHECK_PROC (hle, HLE);
> fails += CHECK_PROC (ht, HTT);
> fails += CHECK_PROC (hybrid, HYBRID);
> - fails += CHECK_PROC (ibrs, IBRS_IBPB);
> + if (cpu_features->basic.kind == arch_kind_intel)
> + {
> + fails += CHECK_PROC (ibrs, IBRS_IBPB);
> + fails += CHECK_PROC (stibp, STIBP);
> + }
> + else if (cpu_features->basic.kind == arch_kind_amd)
> + {
> + fails += CHECK_PROC (ibpb, AMD_IBPB);
> + fails += CHECK_PROC (ibrs, AMD_IBRS);
> + fails += CHECK_PROC (stibp, AMD_STIBP);
> + }
> fails += CHECK_PROC (ibt, IBT);
> fails += CHECK_PROC (invariant_tsc, INVARIANT_TSC);
> fails += CHECK_PROC (invpcid, INVPCID);
> @@ -221,7 +233,10 @@ do_test (int argc, char **argv)
> fails += CHECK_PROC (smep, SMEP);
> fails += CHECK_PROC (smx, SMX);
> fails += CHECK_PROC (ss, SS);
> - fails += CHECK_PROC (ssbd, SSBD);
> + if (cpu_features->basic.kind == arch_kind_intel)
> + fails += CHECK_PROC (ssbd, SSBD);
> + else if (cpu_features->basic.kind == arch_kind_amd)
> + fails += CHECK_PROC (ssbd, AMD_SSBD);
> fails += CHECK_PROC (sse, SSE);
> fails += CHECK_PROC (sse2, SSE2);
> fails += CHECK_PROC (pni, SSE3);
> @@ -229,7 +244,6 @@ do_test (int argc, char **argv)
> fails += CHECK_PROC (sse4_2, SSE4_2);
> fails += CHECK_PROC (sse4a, SSE4A);
> fails += CHECK_PROC (ssse3, SSSE3);
> - fails += CHECK_PROC (stibp, STIBP);
> fails += CHECK_PROC (svm, SVM);
> #ifdef __x86_64__
> /* NB: SYSCALL_SYSRET is 64-bit only. */
> --
> 2.30.2
LGTM.
Reviewed-by: H.J. Lu <hjl.tools@gmail.com>
Thanks.
--
H.J.
next prev parent reply other threads:[~2021-06-24 0:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-23 14:54 [PATCH] x86: Fix tst-cpu-features-cpuinfo on Ryzen 9 (BZ #27873) Adhemerval Zanella
2021-06-23 20:41 ` H.J. Lu
2021-06-23 21:15 ` Adhemerval Zanella
2021-06-23 21:36 ` [PATCH] x86: Copy IBT and SHSTK usable only if CET is enabled H.J. Lu
2021-06-24 0:30 ` Adhemerval Zanella
2021-06-24 0:40 ` H.J. Lu [this message]
2021-06-24 12:17 ` Adhemerval Zanella
2021-06-24 12:27 ` H.J. Lu
2021-06-24 12:55 ` Adhemerval Zanella
2021-06-24 7:56 ` Florian Weimer
2021-06-24 12:07 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMe9rOpJkfpqC8hzk0S2nRUkjmx8a7YjrS0F1WBFN8ArNnt-8A@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).