This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: V3 [PATCH] x86: Add <sys/platform/x86.h>


On Fri, Aug 31, 2018 at 8:25 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/31/2018 05:11 PM, H.J. Lu wrote:
>>
>> On Fri, Aug 31, 2018 at 7:08 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 08/30/2018 11:20 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Thu, Aug 30, 2018 at 12:47 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 08/29/2018 05:57 PM, H.J. Lu wrote:
>>>>>
>>>>>>> CPUID and XGETBV do not need relocations, so they will work where
>>>>>>> this
>>>>>>> approach will not.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Do you have a testcase to show that this claim is true?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I expect the effect to be similar to this bug:
>>>>>
>>>>>     <https://bugzilla.redhat.com/show_bug.cgi?id=1377895>
>>>>>
>>>>> And this one, which has more analysis:
>>>>>
>>>>>     <https://sourceware.org/bugzilla/show_bug.cgi?id=20019>
>>>>>
>>>>> This shows that the IFUNC resolvers inside libc itself (which use the
>>>>> proposed mechanism) are unsafe.
>>>>
>>>>
>>>>
>>>> This particular issue involves a shared library which references
>>>> libc.so,
>>>> but is not linked against libc.so.6.
>>>>
>>>> x86_get_arch_feature,  x86_get_cpu_kind and x86_get_cpuid_registers
>>>> are different since they are provided by ld.so.  They don't have any
>>>> dependency
>>>> issues.  I added 2 new testcases to verify that they can be used in
>>>> shared
>>>> libraries with or without lazy binding.
>>>
>>>
>>>
>>> Isn't there another problem?  The functions you listed have been fully
>>> relocated, but I don't see anything that ensures that the IFUNC resolver
>>> which calls these functions has been relocated when it is called.
>>>
>>> I think that, collectively, we are still undecided whether an IFUNC
>>> resolver
>>> may rely on run-time relocations.
>>
>>
>> This issue is orthogonal to <sys/platform/x86.h>.  Why should it block
>> <sys/platform/x86.h>?
>
>
> I think it's a replacement for <cpuid.h> and lots of manual checking of
> bits.  <cpuid.h> does not have this problem because everything is inlined.

The primary usage of <sys/platform/x86.h> is not for IFUNC resolver.

>>> To be frank, I think only the HAS_ARCH_FEATURE should exist. Programmers
>>> who
>>> need the actual CPUID bits can use <cpuid.h> from GCC.
>>
>>
>> Using <cpuid.h> requires many specific steps.  We have taken all these
>> steps and information is readily accessible by glibc.  I'd like to make it
>> available to programmers.  This is one of motivations for this header
>> file.
>
>
> Right, and programmers using the new interface want to know “can I execute
> an FMA3 instruction?”, which means support for the register file and the
> instruction itself.  So I think we should hide the CPUID details for this
> interface because they only add confusion and do not provide value.

One goal of <sys/platform/x86.h> is to make CPUID information available
to applications.  Programmers need to know how to use it.

>>> I also suggest to use a different interface for the implementation.
>>> Something like this:
>>>
>>>    unsigned int x86_feature_word (int idx)  __attribute__ ((__const__));
>>>
>>> This would return 32 feature bits in one call, and the caller would
>>> select
>>> the appropriate bit from the result.
>>>
>>> Or we could have a function call which returns the feature bit directly,
>>> but
>>> this could be less efficient if multiple features are queried.
>>>
>>> The advantage of doing it this way is that we do not need to add a new
>>> symbol version when backporting support for new CPU features.
>>> x86_feature_word would just return zero for unknown indexes.
>>
>>
>> My implementation doesn't require symbol version either:
>>
>> x86_get_arch_feature returns 0 if architecture index >=
>> FEATURE_INDEX_MAX.  This happens when the run-time glibc
>> doesn't support the newly added feature index.
>>
>> I don't see any significant difference other than the function name.
>
>
> I'm concerned about this bit, from the commit message:
>
> “
> 1. x86_get_cpu_kind will get a new version via symbol versioning if a
>  new CPU kind is added to cpu_features_kind.
> ”

I will remove it from <sys/platform/x86.h>.

> I think we can avoid that.
>
> And I'm also not convinced that CPUID is the arbiter of all things. It's
> really combination of several factors (initial CPUID probing, actual CPUID
> data fetch, maybe XGETBV), so a register-based interface strikes me as
> wrong.
>
> The interface also does not really support different probing mechanisms for
> different CPU vendors, even for features which are exactly the same
> otherwise.
>

<sys/platform/x86.h> provides raw information from CPUID with
HAS_CPU_FEATURE and additional information with
HAS_ARCH_FEATURE.   These are sufficient for glibc and should
be sufficient from most programmers.  If you have any specific requirements
which aren't provided by <sys/platform/x86.h>,  please let me know.

Thanks.

-- 
H.J.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]