Message ID | 20210605135947.469959-1-hjl.tools@gmail.com |
---|---|
State | New |
Headers | show |
Series | x86: Install <bits/platform/x86.h> [BZ #27958] | expand |
On 6/5/21 9:59 AM, H.J. Lu via Libc-alpha wrote: > Install <bits/platform/x86.h> for <sys/platform/x86.h> which includes > <bits/platform/x86.h>. > > Fixes BZ #27958. The constants in bits/platform/x86.h are largely ABI given the behaviour of the cpuid instruction. Likewise we do a consistent mapping between the cpuid_array <-> usable_array without exposing internal details. The API in sys/platform/x86.h has already been reviewed, discussed, and exposes HAS_CPU_FEATURE(name) and CPU_FEATURE_USABLE(name). Given that we get one more chance at review let me ask a few final questions. (1) API prefixes in macros help developers remember names. Consistent prefix for APIs help developers remember. We use HAS_* but also CPU_* which requires the programmer remember two distinct naming strategies. Suggestion: CPU_FEATURE_PRESENT(), CPU_FEATURE_USABLE()? Note: We do this in the underlying name e.g. x86_cpu_* has_feature (could be is_present) vs. is_usable. (2) ABI testing? - How are we making sure we don't accidentally break ABI? - Do we need any further testing? - Do we have a decoupled test to ensure a refactor doesn't break things? - We have tst-cpu-features-cpuinfo.c, which should cover comparison to the decoupled cpuinfo. Notes: - We will not be able to avoid in-place-update failures, in that rpm will do an atomic rename that unlinks the old libc.so.6 with the new libc.so.6 and if ld.so is not yet updated or updated first then a process that starts will crash. This makes it error prone to update the ABI in downstream minor updates. > --- > sysdeps/x86/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > index 346ec491b3..567ea54243 100644 > --- a/sysdeps/x86/Makefile > +++ b/sysdeps/x86/Makefile > @@ -5,7 +5,7 @@ endif > ifeq ($(subdir),elf) > sysdep_routines += get-cpuid-feature-leaf > sysdep-dl-routines += dl-get-cpu-features > -sysdep_headers += sys/platform/x86.h > +sysdep_headers += sys/platform/x86.h bits/platform/x86.h > > CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector)
On Fri, Jul 9, 2021 at 1:01 PM Carlos O'Donell <carlos@redhat.com> wrote: > > On 6/5/21 9:59 AM, H.J. Lu via Libc-alpha wrote: > > Install <bits/platform/x86.h> for <sys/platform/x86.h> which includes > > <bits/platform/x86.h>. > > > > Fixes BZ #27958. > > The constants in bits/platform/x86.h are largely ABI given the behaviour > of the cpuid instruction. Likewise we do a consistent mapping between > the cpuid_array <-> usable_array without exposing internal details. > > The API in sys/platform/x86.h has already been reviewed, discussed, and > exposes HAS_CPU_FEATURE(name) and CPU_FEATURE_USABLE(name). > > Given that we get one more chance at review let me ask a few final questions. > > (1) API prefixes in macros help developers remember names. > > Consistent prefix for APIs help developers remember. > > We use HAS_* but also CPU_* which requires the programmer remember > two distinct naming strategies. > > Suggestion: CPU_FEATURE_PRESENT(), CPU_FEATURE_USABLE()? I will rename them to CPU_FEATURE_PRESENT and CPU_FEATURE_POSSIBLE. Given the current AMX support discussion, additional information may be needed from the Linux kernel to determine if a feature can be used. > Note: We do this in the underlying name e.g. x86_cpu_* > has_feature (could be is_present) vs. is_usable. I will change them to x86_cpu_present and x86_cpu_possible. I will also rename the "usable" field to "possible". > (2) ABI testing? > > - How are we making sure we don't accidentally break ABI? > > - Do we need any further testing? We only export struct cpuid_feature { unsigned int cpuid_array[4]; unsigned int possible_array[4]; }; extern const struct cpuid_feature *__x86_get_cpuid_feature_leaf (unsigned int) __attribute__ ((pure)); const struct cpuid_feature * __x86_get_cpuid_feature_leaf (unsigned int leaf) { static const struct cpuid_feature feature = {}; if (leaf < CPUID_INDEX_MAX) return ((const struct cpuid_feature *) &GLRO(dl_x86_cpu_features).features[leaf]); else return &feature; } As long as all new features are appended to the end, there should be no ABI issues. > - Do we have a decoupled test to ensure a refactor doesn't break > things? This shouldn't be a problem. > - We have tst-cpu-features-cpuinfo.c, which should cover > comparison to the decoupled cpuinfo. > > Notes: > > - We will not be able to avoid in-place-update failures, in that rpm > will do an atomic rename that unlinks the old libc.so.6 with the > new libc.so.6 and if ld.so is not yet updated or updated first then > a process that starts will crash. This makes it error prone to update > the ABI in downstream minor updates. > > > --- > > sysdeps/x86/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > > index 346ec491b3..567ea54243 100644 > > --- a/sysdeps/x86/Makefile > > +++ b/sysdeps/x86/Makefile > > @@ -5,7 +5,7 @@ endif > > ifeq ($(subdir),elf) > > sysdep_routines += get-cpuid-feature-leaf > > sysdep-dl-routines += dl-get-cpu-features > > -sysdep_headers += sys/platform/x86.h > > +sysdep_headers += sys/platform/x86.h bits/platform/x86.h > > > > CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector) > > > > -- > Cheers, > Carlos. > Thanks.
diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile index 346ec491b3..567ea54243 100644 --- a/sysdeps/x86/Makefile +++ b/sysdeps/x86/Makefile @@ -5,7 +5,7 @@ endif ifeq ($(subdir),elf) sysdep_routines += get-cpuid-feature-leaf sysdep-dl-routines += dl-get-cpu-features -sysdep_headers += sys/platform/x86.h +sysdep_headers += sys/platform/x86.h bits/platform/x86.h CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector)