Re: V2: [PATCH] x86: Install <sys/platform/x86.h> [BZ #26124]

[Florian Weimer <fweimer@redhat.com>]

* H. J. Lu via Libc-alpha:

> On Thu, Jun 18, 2020 at 1:45 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu via Libc-alpha:
>>
>> > <sys/platform/x86.h> exports only:
>> >
>> > struct cpu_features
>> > {
>> >   struct cpuid_registers cpuid[COMMON_CPUID_INDEX_MAX];
>> >   unsigned int feature[FEATURE_INDEX_MAX];
>> >   struct cpu_features_basic basic;
>> > };
>>
>> The struct cpu_features ABI does not appear to be stable, as I wrote on
>> the other thread:
>>
>>   <https://sourceware.org/pipermail/libc-alpha/2020-June/115161.html>
>>
>
> Here is the updated patch to use
>
> struct cpu_features
> {
>   struct cpu_features_basic basic;
>   unsigned int usable[USABLE_FEATURE_INDEX_MAX];
>   struct cpuid_registers cpuid[COMMON_CPUID_INDEX_MAX];
> };
>
> This should be backward compatible for both .o and .so files.

This is an improvement.  I still see issues with this interface, though.

Programmers will need something like this for IFUNC resolvers, but this
is not usable there if the relocation order does not match the explicit
ELF dependency order (e.g., due to symbol interposition).  This is a
generic problem with IFUNC resolvers.  Other architectures solve this by
adding arguments to IFUNC resolvers, so that the required can be
accessed without relocations.  __builtin_cpu_supports does not have this
issue.

The manual does not explain the difference between “available” and
“usable”.  I do not think we should expose both.  I don't see any
circumstances under which “available” would provide useful information.

struct cpu_features (even in its reduced form) is fairly large.  We will
never be able to reduce its size again if it becomes public ABI.

I'm not convinced that this interface conforms to the API contract for
the CPUID instruction.  Aren't the raw bits in the cpuid field subject
to interpretation based on family and model?  HAS_CPU_FEATURE does not
reflect this at all.

My preferred interface would look like this:

* The low-level function call ABI is:

  unsigned long long long int __x86_get_cpu_features (unsigned int word)
    __attribute__ ((const));

* IFUNC resolvers receive two arguments,

  unsigned long long long int *feature_words, size_t feature_words_length

* CPU_FEATURE_USABLE (NAME) would expand to something like

    ((__x86_get_cpu_features (NAME_word) & NAME_bitmask) != 0)

  The implementation of __x86_get_cpu_features simply returns 0 if the
  index is too large, which solves the extension problem.

* CPU_FEATURE_USABLE_ARRAY (WORDS, WORDS_LENGTH, NAME) would expand to this:

  (((NAME_word < WORDS_LENGTH ? WORDS[NAME_word] ? : 0) & NAME_bitmask) != 0)

  This version is expected to be used in IFUNC resolvers.  The macro
  name is just a strawman.

Only the “usable” bits are exposed, so the interface makes it clear that
the implementation performs some normalization.

Alternatively, the __builtin_cpu_supports interface could be enhanced to
cover all the features you need.

Thanks,
Florian