[dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
Neil Horman
nhorman at tuxdriver.com
Thu Mar 20 01:40:10 CET 2014
On Wed, Mar 19, 2014 at 08:44:46AM -0700, H. Peter Anvin wrote:
> On 03/19/2014 07:48 AM, Neil Horman wrote:
> > The recent conversion to build dpdk as a DSO has an error in
> > rte_cpu_get_features. When being build with -fpie, %ebx gets clobbered by the
> > cpuid instruction which is also the pic register. Therefore the inline asm
> > tries to save off %ebx, but does so incorrectly. It starts by loading
> > params.ebx to "D" which is %edi, but then the first instruction moves %ebx to
> > %edi, clobbering the input value. Then after the operation is complete, "D"
> > (%edi) is stored to the local ebx variable, but only after the xchgl instruction
> > has happened, which means ebx holds only the PIC pointer. This behavior was
> > causing strange segfults for me when running the cpuid instruction.
> >
> > The fix is pretty easy, split the asm into two separate directives, the first
> > saving ebx, and using it to grab the appropriate cpuid info (and correctly
> > listing %edi as a clobbered register in the process, and then a subsequent asm
> > directive preforming the reverse exchange (again, listing %edi as being
> > clobbered).
> >
> > Signed-off-by: Neil Horman <nhorman at tuxdriver.com>
> >
>
So after some discussion with hpa, I need to self NAK this again, apologies for
the noise. Theres some clean up to be done in this area, and I'm still getting
a segfault that is in some way related to this code, but I need to dig deeper to
understand it.
Neil
> Hi Neil :)
>
> If I'm reading this correctly, this is at the very best extremely
> dangerous (I'm confused why it would compile at all with PIC enabled),
> since it leaves the CPU state in an unexpected way between two asm
> statements, where the compiler is perfectly allowed to put code.
>
> Instead, I would do simple xchg/xchg, which is an idiom I have used for
> this particular purpose in a lot of code. The minimal patch is simply
> to change "mov" to "xchg" inside the asm statement.
>
> There is no fundamental reason to nail down the register to %edi,
> though; thus I would suggest instead:
>
> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c
> b/lib/librte_eal/common/eal_common_cpuflags.c
> index 1ebf78cc2a48..6b75992fec1a 100644
> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> @@ -206,16 +206,16 @@ rte_cpu_get_features(struct cpuid_parameters_t params)
> "d" (params.edx));
> #else
> asm volatile (
> - "mov %%ebx, %%edi\n"
> + "xchgl %%ebx, %1\n"
> "cpuid\n"
> - "xchgl %%ebx, %%edi;\n"
> + "xchgl %%ebx, %1;\n"
> : "=a" (eax),
> - "=D" (ebx),
> + "=r" (ebx),
> "=c" (ecx),
> "=d" (edx)
> /* input */
> : "a" (params.eax),
> - "D" (params.ebx),
> + "1" (params.ebx),
> "c" (params.ecx),
> "d" (params.edx));
> #endif
>
>
> > ---
> > Change notes
> >
> > v2) Fix constraints to ensure that ebx isn't overwritten before asm starts
> > ---
> > lib/librte_eal/common/eal_common_cpuflags.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
> > index 1ebf78c..75b505f 100644
> > --- a/lib/librte_eal/common/eal_common_cpuflags.c
> > +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> > @@ -190,7 +190,7 @@ static const struct feature_entry cpu_feature_table[] = {
> > static inline int
> > rte_cpu_get_features(struct cpuid_parameters_t params)
> > {
> > - int eax, ebx, ecx, edx; /* registers */
> > + int eax, ebx, ecx, edx, oldebx; /* registers */
> >
> > #ifndef __PIC__
> > asm volatile ("cpuid"
> > @@ -206,18 +206,21 @@ rte_cpu_get_features(struct cpuid_parameters_t params)
> > "d" (params.edx));
> > #else
> > asm volatile (
> > - "mov %%ebx, %%edi\n"
> > + "xchgl %%ebx, %%edi\n"
> > "cpuid\n"
> > - "xchgl %%ebx, %%edi;\n"
> > : "=a" (eax),
> > - "=D" (ebx),
> > + "=b" (ebx),
> > "=c" (ecx),
> > - "=d" (edx)
> > + "=d" (edx),
> > + "=D" (oldebx)
> > /* input */
> > : "a" (params.eax),
> > "D" (params.ebx),
> > "c" (params.ecx),
> > "d" (params.edx));
> > +
> > + asm volatile ("xchgl %%ebx, %%edi;\n"
> > + : : "D" (oldebx));
> > #endif
> >
> > switch (params.return_register) {
> >
>
>
More information about the dev
mailing list