* [PATCH] x86: include OSXSAVE in x86-64-v3 level
@ 2022-12-09 10:36 Fabian Vogt
2022-12-09 17:14 ` Noah Goldstein
2022-12-09 20:25 ` H.J. Lu
0 siblings, 2 replies; 8+ messages in thread
From: Fabian Vogt @ 2022-12-09 10:36 UTC (permalink / raw)
To: libc-alpha; +Cc: hjl.tools, aurelien
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 10:36 [PATCH] x86: include OSXSAVE in x86-64-v3 level Fabian Vogt
@ 2022-12-09 17:14 ` Noah Goldstein
2022-12-09 23:53 ` Florian Weimer
2022-12-09 20:25 ` H.J. Lu
1 sibling, 1 reply; 8+ messages in thread
From: Noah Goldstein @ 2022-12-09 17:14 UTC (permalink / raw)
To: Fabian Vogt; +Cc: libc-alpha, hjl.tools, aurelien
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__`
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 10:36 [PATCH] x86: include OSXSAVE in x86-64-v3 level Fabian Vogt
2022-12-09 17:14 ` Noah Goldstein
@ 2022-12-09 20:25 ` H.J. Lu
2022-12-09 21:09 ` Noah Goldstein
1 sibling, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2022-12-09 20:25 UTC (permalink / raw)
To: Fabian Vogt; +Cc: libc-alpha, aurelien
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
>
>
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 20:25 ` H.J. Lu
@ 2022-12-09 21:09 ` Noah Goldstein
2022-12-09 22:52 ` H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: Noah Goldstein @ 2022-12-09 21:09 UTC (permalink / raw)
To: H.J. Lu; +Cc: Fabian Vogt, libc-alpha, aurelien
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 21:09 ` Noah Goldstein
@ 2022-12-09 22:52 ` H.J. Lu
2022-12-14 15:23 ` Fabian Vogt
0 siblings, 1 reply; 8+ messages in thread
From: H.J. Lu @ 2022-12-09 22:52 UTC (permalink / raw)
To: Noah Goldstein; +Cc: Fabian Vogt, libc-alpha, aurelien
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.
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 17:14 ` Noah Goldstein
@ 2022-12-09 23:53 ` Florian Weimer
0 siblings, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2022-12-09 23:53 UTC (permalink / raw)
To: Noah Goldstein via Libc-alpha
Cc: Fabian Vogt, Noah Goldstein, hjl.tools, aurelien
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-09 22:52 ` H.J. Lu
@ 2022-12-14 15:23 ` Fabian Vogt
2022-12-14 16:11 ` H.J. Lu
0 siblings, 1 reply; 8+ messages in thread
From: Fabian Vogt @ 2022-12-14 15:23 UTC (permalink / raw)
To: libc-alpha; +Cc: Noah Goldstein, H.J. Lu, aurelien
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: include OSXSAVE in x86-64-v3 level
2022-12-14 15:23 ` Fabian Vogt
@ 2022-12-14 16:11 ` H.J. Lu
0 siblings, 0 replies; 8+ messages in thread
From: H.J. Lu @ 2022-12-14 16:11 UTC (permalink / raw)
To: Fabian Vogt; +Cc: libc-alpha, Noah Goldstein, aurelien
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.
>
>
--
H.J.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-14 16:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 10:36 [PATCH] x86: include OSXSAVE in x86-64-v3 level Fabian Vogt
2022-12-09 17:14 ` Noah Goldstein
2022-12-09 23:53 ` Florian Weimer
2022-12-09 20:25 ` H.J. Lu
2022-12-09 21:09 ` Noah Goldstein
2022-12-09 22:52 ` H.J. Lu
2022-12-14 15:23 ` Fabian Vogt
2022-12-14 16:11 ` H.J. Lu
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).