From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 5000A5F36 for ; Thu, 22 Mar 2018 17:08:19 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Mar 2018 09:08:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,345,1517904000"; d="scan'208";a="40313063" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 22 Mar 2018 09:08:16 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.206]) by FMSMSX106.amr.corp.intel.com ([169.254.5.158]) with mapi id 14.03.0319.002; Thu, 22 Mar 2018 09:08:16 -0700 From: "Wiles, Keith" To: Aaron Conole CC: "dev@dpdk.org" , Timothy Redaelli , "Richardson, Bruce" , Jan Viktorin , Jianbo Liu , Jerin Jacob , Chao Zhu Thread-Topic: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector Thread-Index: AQHTwfOL5IFsqO9zPEmXpgnERGJ/3aPc4WmA Date: Thu, 22 Mar 2018 16:08:16 +0000 Message-ID: References: <20180322153549.17910-1-aconole@redhat.com> In-Reply-To: <20180322153549.17910-1-aconole@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.255.82.173] Content-Type: text/plain; charset="us-ascii" Content-ID: <17AA1303D3447A4AAA6C3274C5D2D890@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] eal: abstract away the auxiliary vector X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Mar 2018 16:08:20 -0000 > On Mar 22, 2018, at 10:35 AM, Aaron Conole wrote: >=20 > Rather than attempting to load the contents of the auxv directly, > prefer to use an exposed API - and if that doesn't exist then attempt > to load the vector. This is because on some systems, when a user > is downgraded, the /proc/self/auxv file retains the old ownership > and permissions. The original method of /proc/self/auxv is retained. >=20 > This also removes a potential abort() in the code when compiled with > NDEBUG. A quick parse of the code shows that many (if not all) of > the CPU flag parsing isn't used internally, so it should be okay. >=20 > Signed-off-by: Aaron Conole > Signed-off-by: Timothy Redaelli > --- > NOTE: These APIs aren't added to the map because I really want > them to be internal-only. No other component really needs > access to them. I couldn't find a good common internal > include file which is why they're in the rte_cpuflags.h - > but if there's a better one, we can spin another version. >=20 > lib/librte_eal/common/arch/arm/rte_cpuflags.c | 20 ++---- > lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c | 15 +---- > lib/librte_eal/common/eal_common_cpuflags.c | 76 +++++++++++++++++= +++++ > .../common/include/generic/rte_cpuflags.h | 13 ++++ > 4 files changed, 95 insertions(+), 29 deletions(-) >=20 > diff --git a/lib/librte_eal/common/arch/arm/rte_cpuflags.c b/lib/librte_e= al/common/arch/arm/rte_cpuflags.c > index 88f1cbe37..496d8b21a 100644 > --- a/lib/librte_eal/common/arch/arm/rte_cpuflags.c > +++ b/lib/librte_eal/common/arch/arm/rte_cpuflags.c > @@ -133,22 +133,10 @@ const struct feature_entry rte_cpu_feature_table[] = =3D { > static void > rte_cpu_get_features(hwcap_registers_t out) > { > - int auxv_fd; > - _Elfx_auxv_t auxv; > - > - auxv_fd =3D open("/proc/self/auxv", O_RDONLY); > - assert(auxv_fd !=3D -1); > - while (read(auxv_fd, &auxv, sizeof(auxv)) =3D=3D sizeof(auxv)) { > - if (auxv.a_type =3D=3D AT_HWCAP) { > - out[REG_HWCAP] =3D auxv.a_un.a_val; > - } else if (auxv.a_type =3D=3D AT_HWCAP2) { > - out[REG_HWCAP2] =3D auxv.a_un.a_val; > - } else if (auxv.a_type =3D=3D AT_PLATFORM) { > - if (!strcmp((const char *)auxv.a_un.a_val, PLATFORM_STR)) > - out[REG_PLATFORM] =3D 0x0001; > - } > - } > - close(auxv_fd); > + out[REG_HWCAP] =3D rte_cpu_getauxval(AT_HWCAP); > + out[REG_HWCAP2] =3D rte_cpu_getauxval(AT_HWCAP2); > + if (!rte_cpu_strcmp_auxval(AT_PLATFORM, PLATFORM_STR)) > + out[REG_PLATFORM] =3D 0x0001; I like the way you reduced this code here, which does make a lot of sense. > } >=20 > /* > diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c b/lib/librt= e_eal/common/arch/ppc_64/rte_cpuflags.c > index 970a61c5e..e7a82452b 100644 > --- a/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c > +++ b/lib/librte_eal/common/arch/ppc_64/rte_cpuflags.c > @@ -104,19 +104,8 @@ const struct feature_entry rte_cpu_feature_table[] = =3D { > static void > rte_cpu_get_features(hwcap_registers_t out) > { > - int auxv_fd; > - Elf64_auxv_t auxv; > - > - auxv_fd =3D open("/proc/self/auxv", O_RDONLY); > - assert(auxv_fd !=3D -1); > - while (read(auxv_fd, &auxv, > - sizeof(Elf64_auxv_t)) =3D=3D sizeof(Elf64_auxv_t)) { > - if (auxv.a_type =3D=3D AT_HWCAP) > - out[REG_HWCAP] =3D auxv.a_un.a_val; > - else if (auxv.a_type =3D=3D AT_HWCAP2) > - out[REG_HWCAP2] =3D auxv.a_un.a_val; > - } > - close(auxv_fd); > + out[REG_HWCAP] =3D rte_cpu_getauxval(AT_HWCAP); > + out[REG_HWCAP2] =3D rte_cpu_getauxval(AT_HWCAP2); > } >=20 > /* > diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal= /common/eal_common_cpuflags.c > index 3a055f7c7..87c9539ad 100644 > --- a/lib/librte_eal/common/eal_common_cpuflags.c > +++ b/lib/librte_eal/common/eal_common_cpuflags.c > @@ -2,11 +2,87 @@ > * Copyright(c) 2010-2014 Intel Corporation > */ >=20 > +#include > +#include > #include > +#include > +#include > +#include > +#include > + > +#if defined(__GLIBC__) && defined(__GLIBC_PREREQ) > +#if __GLIBC_PREREQ(2, 16) > +#include > +#define HAS_AUXV 1 > +#endif > +#endif >=20 > #include > #include >=20 > +#ifndef HAS_AUXV > +static unsigned long > +getauxval(unsigned long type) > +{ > + errno =3D ENOTSUP; > + return 0; > +} > +#endif > + > +#if RTE_ARCH_64 > +typedef Elf64_auxv_t Internal_Elfx_auxv_t; > +#else > +typedef Elf32_auxv_t Internal_Elfx_auxv_t; > +#endif > + > + > +/** > + * Provides a method for retrieving values from the auxiliary vector and > + * possibly running a string comparison. Need to add more docs here as zero is not the normal error return value, sa= me for header file. Please add the correct doxygen formats. > + */ > +static unsigned long > +_rte_cpu_getauxval(unsigned long type, int cmp_str, const char *str) Can we not remove cmp_str and just test for str !=3D NULL to compare string= s. This would be cleaner IMO. > +{ > + unsigned long val; > + > + errno =3D 0; > + val =3D getauxval(type); > + > + if (!val && (errno =3D=3D ENOTSUP || errno =3D=3D ENOENT)) { > + Internal_Elfx_auxv_t auxv; Should we set auxv =3D { 0 }; here, maybe not required. > + int auxv_fd =3D open("/proc/self/auxv", O_RDONLY); > + > + errno =3D ENOENT; > + if (auxv_fd =3D=3D -1) > + return 0; > + > + while (read(auxv_fd, &auxv, sizeof(auxv)) =3D=3D sizeof(auxv)) { > + if (auxv.a_type =3D=3D type) { > + errno =3D 0; > + val =3D auxv.a_un.a_val; > + if (cmp_str) > + val =3D strcmp((const char *)val, str); Does this need to be case sensitive or should we use strcasecmp() instead. > + break; > + } > + } > + close(auxv_fd); > + } > + > + return val; > +} > + > +unsigned long > +rte_cpu_getauxval(unsigned long type) > +{ > + return _rte_cpu_getauxval(type, 0, NULL); > +} > + > +int > +rte_cpu_strcmp_auxval(unsigned long type, const char *str) > +{ > + return _rte_cpu_getauxval(type, 1, str); > +} > + > /** > * Checks if the machine is adequate for running the binary. If it is not= , the > * program exits with status 1. > diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/l= ibrte_eal/common/include/generic/rte_cpuflags.h > index 8d31687d8..ffa3b744a 100644 > --- a/lib/librte_eal/common/include/generic/rte_cpuflags.h > +++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h > @@ -64,4 +64,17 @@ rte_cpu_check_supported(void); > int > rte_cpu_is_supported(void); >=20 > +/** > + * This function attempts to retrieve a value from the auxiliary vector. Document the return value here as it is not the normal -1 value on error, s= ame for the next one. And the doxygen formats too. > + */ > +unsigned long > +rte_cpu_getauxval(unsigned long type); > + > +/** > + * This function retrieves a value from the auxiliary vector, and compar= es it > + * as a string. > + */ > +int > +rte_cpu_strcmp_auxval(unsigned long type, const char *str); > + > #endif /* _RTE_CPUFLAGS_H_ */ > --=20 > 2.14.3 >=20 Regards, Keith