| Message ID | 4789070.31r3eYUQgx@linux-e202.suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | x86: include OSXSAVE in x86-64-v3 level | expand |
On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt via Libc-alpha <libc-alpha@sourceware.org> wrote: > > For some reason the initial x86-64-v3 detection code was missing checks for > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > --- > sysdeps/x86/get-isa-level.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > index 5b4dd5f062..d62bf92cde 100644 > --- a/sysdeps/x86/get-isa-level.h > +++ b/sysdeps/x86/get-isa-level.h > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) > { > isa_level |= GNU_PROPERTY_X86_ISA_1_V3; > if (CPU_FEATURE_USABLE_P (cpu_features, AVX512F) > -- > 2.38.1 > > We should also update isa-level.h https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.h;h=06f6c9663e76d4a48efb8fbfdb707cc5045f47dc;hb=HEAD I think the compiler flag for it is `__XSAVE__`
On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt <fvogt@suse.de> wrote: > > For some reason the initial x86-64-v3 detection code was missing checks for > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > --- > sysdeps/x86/get-isa-level.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > index 5b4dd5f062..d62bf92cde 100644 > --- a/sysdeps/x86/get-isa-level.h > +++ b/sysdeps/x86/get-isa-level.h > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) If OSXSAVE isn't usable, all AVX/AVX512 features shouldn't be usable. Am I missing something? > { > isa_level |= GNU_PROPERTY_X86_ISA_1_V3; > if (CPU_FEATURE_USABLE_P (cpu_features, AVX512F) > -- > 2.38.1 > >
On Fri, Dec 9, 2022 at 12:26 PM H.J. Lu via Libc-alpha <libc-alpha@sourceware.org> wrote: > > On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt <fvogt@suse.de> wrote: > > > > For some reason the initial x86-64-v3 detection code was missing checks for > > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > > --- > > sysdeps/x86/get-isa-level.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > > index 5b4dd5f062..d62bf92cde 100644 > > --- a/sysdeps/x86/get-isa-level.h > > +++ b/sysdeps/x86/get-isa-level.h > > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) > > If OSXSAVE isn't usable, all AVX/AVX512 features shouldn't be usable. > Am I missing something? Seems like better to err on side of caution here. Skipping the extra check seems prone to bugs like BZ #29611. > > > { > > isa_level |= GNU_PROPERTY_X86_ISA_1_V3; > > if (CPU_FEATURE_USABLE_P (cpu_features, AVX512F) > > -- > > 2.38.1 > > > > > > > -- > H.J.
On Fri, Dec 9, 2022 at 1:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > On Fri, Dec 9, 2022 at 12:26 PM H.J. Lu via Libc-alpha > <libc-alpha@sourceware.org> wrote: > > > > On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt <fvogt@suse.de> wrote: > > > > > > For some reason the initial x86-64-v3 detection code was missing checks for > > > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > > > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > > > > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > > > --- > > > sysdeps/x86/get-isa-level.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > > > index 5b4dd5f062..d62bf92cde 100644 > > > --- a/sysdeps/x86/get-isa-level.h > > > +++ b/sysdeps/x86/get-isa-level.h > > > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > > > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > > > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > > > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > > > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > > > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > > > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) > > > > If OSXSAVE isn't usable, all AVX/AVX512 features shouldn't be usable. > > Am I missing something? > > Seems like better to err on side of caution here. We will have serious issues in many places if we don't get it correctly. CPU_FEATURE_USABLE_P is set according to the states supported by OSXSAVE. Checking OSXSAVE isn't needed here. > Skipping the extra check seems prone to bugs like BZ #29611.
* Noah Goldstein via Libc-alpha: > We should also update isa-level.h > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/isa-level.h;h=06f6c9663e76d4a48efb8fbfdb707cc5045f47dc;hb=HEAD > > I think the compiler flag for it is `__XSAVE__` I don't think so. We don't need XSAVE support, we need OSXSAVE support, and that's not actually a compiler or even purely a CPU thing. Thanks, Florian
Am Freitag, 9. Dezember 2022, 23:52:37 CET schrieb H.J. Lu: > On Fri, Dec 9, 2022 at 1:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > > > On Fri, Dec 9, 2022 at 12:26 PM H.J. Lu via Libc-alpha > > <libc-alpha@sourceware.org> wrote: > > > > > > On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt <fvogt@suse.de> wrote: > > > > > > > > For some reason the initial x86-64-v3 detection code was missing checks for > > > > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > > > > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > > > > > > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > > > > --- > > > > sysdeps/x86/get-isa-level.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > > > > index 5b4dd5f062..d62bf92cde 100644 > > > > --- a/sysdeps/x86/get-isa-level.h > > > > +++ b/sysdeps/x86/get-isa-level.h > > > > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > > > > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > > > > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > > > > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > > > > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > > > > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > > > > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) > > > > > > If OSXSAVE isn't usable, all AVX/AVX512 features shouldn't be usable. > > > Am I missing something? > > > > Seems like better to err on side of caution here. > > We will have serious issues in many places if we don't get it correctly. > CPU_FEATURE_USABLE_P is set according to the states supported > by OSXSAVE. Checking OSXSAVE isn't needed here. I see, you explicitly unset some flags if OSXSAVE is not available. In that case the patch is indeed a noop, but also won't hurt. What about adding a comment instead? > > Skipping the extra check seems prone to bugs like BZ #29611.
On Wed, Dec 14, 2022 at 7:23 AM Fabian Vogt <fvogt@suse.de> wrote: > > Am Freitag, 9. Dezember 2022, 23:52:37 CET schrieb H.J. Lu: > > On Fri, Dec 9, 2022 at 1:09 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > > > > > On Fri, Dec 9, 2022 at 12:26 PM H.J. Lu via Libc-alpha > > > <libc-alpha@sourceware.org> wrote: > > > > > > > > On Fri, Dec 9, 2022 at 2:36 AM Fabian Vogt <fvogt@suse.de> wrote: > > > > > > > > > > For some reason the initial x86-64-v3 detection code was missing checks for > > > > > BMI, BMI2 and OSXSAVE, which are all required for that level to be met. > > > > > BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. > > > > > > > > > > Signed-off-by: Fabian Vogt <fvogt@suse.de> > > > > > --- > > > > > sysdeps/x86/get-isa-level.h | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h > > > > > index 5b4dd5f062..d62bf92cde 100644 > > > > > --- a/sysdeps/x86/get-isa-level.h > > > > > +++ b/sysdeps/x86/get-isa-level.h > > > > > @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) > > > > > && CPU_FEATURE_USABLE_P (cpu_features, F16C) > > > > > && CPU_FEATURE_USABLE_P (cpu_features, FMA) > > > > > && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) > > > > > - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) > > > > > + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) > > > > > + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) > > > > > > > > If OSXSAVE isn't usable, all AVX/AVX512 features shouldn't be usable. > > > > Am I missing something? > > > > > > Seems like better to err on side of caution here. > > > > We will have serious issues in many places if we don't get it correctly. > > CPU_FEATURE_USABLE_P is set according to the states supported > > by OSXSAVE. Checking OSXSAVE isn't needed here. > > I see, you explicitly unset some flags if OSXSAVE is not available. > In that case the patch is indeed a noop, but also won't hurt. What about > adding a comment instead? Sure. Please submit a patch. > > > > Skipping the extra check seems prone to bugs like BZ #29611. > >
diff --git a/sysdeps/x86/get-isa-level.h b/sysdeps/x86/get-isa-level.h index 5b4dd5f062..d62bf92cde 100644 --- a/sysdeps/x86/get-isa-level.h +++ b/sysdeps/x86/get-isa-level.h @@ -52,7 +52,8 @@ get_isa_level (const struct cpu_features *cpu_features) && CPU_FEATURE_USABLE_P (cpu_features, F16C) && CPU_FEATURE_USABLE_P (cpu_features, FMA) && CPU_FEATURE_USABLE_P (cpu_features, LZCNT) - && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) + && CPU_FEATURE_USABLE_P (cpu_features, MOVBE) + && CPU_FEATURE_USABLE_P (cpu_features, OSXSAVE)) { isa_level |= GNU_PROPERTY_X86_ISA_1_V3; if (CPU_FEATURE_USABLE_P (cpu_features, AVX512F)
For some reason the initial x86-64-v3 detection code was missing checks for BMI, BMI2 and OSXSAVE, which are all required for that level to be met. BMI and BMI2 got added recently, but OSXSAVE was still missing. Add it. Signed-off-by: Fabian Vogt <fvogt@suse.de> --- sysdeps/x86/get-isa-level.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)