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

[Florian Weimer <fweimer at redhat dot com>]

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.

> > > > You also need to check for XSAVE support, I think.  This is easy to miss,
> > > > and the proposed interface does not prevent this.
> > >
> > >
> > > That is why I added:
> > >
> > > @defmac HAS_ARCH_FEATURE(name)
> >
> >
> > There should only be a single feature to query, to avoid confusion.
>
> Good point.  I added more XXX_Usable definitions and updated
> document.

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.

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.

Thanks,
Florian