[dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
On 03/19/2014 05:40 PM, Neil Horman wrote:
> 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
I still believe we should add the patch I posted in the previous email;
I should clean it up and put a proper header on it.
This is, if there is actually a need to feed %ebx and %edx into CPUID
(the native instruction is sensitive to %eax and %ecx, but not %ebx or
%edx.)
For reference, this is a version of CPUID I personally often use:
struct cpuid {
unsigned int eax, ecx, edx, ebx;
};
static inline void cpuid(unsigned int leaf, unsigned int subleaf,
struct cpuid *out)
{
#if defined(__i386__) && defined(__PIC__)
/* %ebx is a forbidden register */
asm volatile("movl %%ebx,%0 ; cpuid ; xchgl %%ebx,%0"
: "=r" (out->ebx),
"=a" (out->eax),
"=c" (out->ecx),
"=d" (out->edx)
: "a" (leaf), "c" (subleaf));
#else
asm volatile("cpuid"
: "=b" (out->ebx),
"=a" (out->eax),
"=c" (out->ecx),
"=d" (out->edx)
: "a" (leaf), "c" (subleaf));
#endif
}
... but that is a pretty significant API change.
Making it an inline lets gcc elide the entire memory structure, so that
is definitely useful.
>>
>> 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
>>
-hpa
[dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
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
> >
>
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) {
> >
>
>
[dpdk-dev] [PATCH] pcap: revert build patches
06/03/2014 11:11, David Marchand : > This reverts commit > a0cdfcf936bcccd15d08a22c9cf38013b6859ad2, > ef5b2363627dadacfee7da6ee0c7d1c2b40a30db and > 60191b8919ef630a0fd50c32a734dd04802ccbfc . > > These patches are creating more problems than solving the initial one (which > was a build error with too old pcap libraries). > Since old pcap librabries are not that common, just revert them. > > Reported-by: Meir Tseitlin > Reported-by: Mats Liljegren > Signed-off-by: David Marchand Acked and applied. If someone request to support an old version of libpcap, a slightly different solution should be provided. -- Thomas
[dpdk-dev] [PATCH] pcap: fix compilation error introduced by kvargs
02/03/2014 22:51, Olivier Matz : > Due to a merge conflict between commits 4c745617a1 and 9d5752d80, > rte_eth_pcap.c was not compiling with the following error: > > rte_eth_pcap.c: In function 'rte_pmd_init_internals': > rte_eth_pcap.c:559:30: error: dereferencing pointer to incomplete type > rte_eth_pcap.c:560:15: error: dereferencing pointer to incomplete type > rte_eth_pcap.c:561:18: error: dereferencing pointer to incomplete type > rte_eth_pcap.c:603:47: error: dereferencing pointer to incomplete type > rte_eth_pcap.c: In function 'rte_pmd_pcap_init': > rte_eth_pcap.c:732:73: error: 'dict' undeclared (first use in this > function) > rte_eth_pcap.c:732:73: note: each undeclared identifier is reported > only once for each function it appears in > > This commit replaces "struct args_dict" by "struct rte_kvargs" to fix > the compilation issue. > > By the way, it also removes the declaration of these functions from > the header file as no other file in DPDK references one of them. It > avoids to include in rte_eth_pcap.h. > > Pointed-out-by: Meir Tseitlin > Signed-off-by: Olivier Matz I missed it when applying the patches. Sorry for the inconvenience. It's now acked and applied. -- Thomas
[dpdk-dev] [PATCH] mk: fix dependencies when modifying config
If .config file is changed and .depdirs was built before, it must be updated. It is now done silently (-s) in checkconfig rule. Now .depdirs is not redone if .config is older. It has been tested by enabling PMD_PCAP after a default build. KVARGS dependency should appear in .depdirs. Signed-off-by: Thomas Monjalon --- mk/rte.sdkconfig.mk | 1 + mk/rte.sdkdepdirs.mk | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk index 3d05ba5..b4e45f2 100644 --- a/mk/rte.sdkconfig.mk +++ b/mk/rte.sdkconfig.mk @@ -114,6 +114,7 @@ checkconfig: fi $(Q)$(MAKE) -f $(RTE_SDK)/mk/rte.sdkconfig.mk \ headerconfig NODOTCONF=1 + $(Q)$(MAKE) -s depdirs .PHONY: FORCE FORCE: diff --git a/mk/rte.sdkdepdirs.mk b/mk/rte.sdkdepdirs.mk index 03dbdb1..bd3bac4 100644 --- a/mk/rte.sdkdepdirs.mk +++ b/mk/rte.sdkdepdirs.mk @@ -39,7 +39,8 @@ endif # use a "for" in a shell to process dependencies: we don't want this # task to be run in parallel. ..PHONY: depdirs -depdirs: +depdirs: $(RTE_OUTPUT)/.depdirs +$(RTE_OUTPUT)/.depdirs: $(RTE_OUTPUT)/.config @rm -f $(RTE_OUTPUT)/.depdirs ; \ for d in $(ROOTDIRS-y); do \ if [ -f $(RTE_SRCDIR)/$$d/Makefile ]; then \ -- 1.9.0
[dpdk-dev] patchwork for dpdk.org?
18/03/2014 16:31, Kyle Mestery: > stephen at networkplumber.org wrote: > > Anybody considered setting up patchwork to track patches on this list? > > > > If not familiar, patchwork is a back end which maintainers and developers > > can keep track of patches submitted and make sure of the status (accepted, > > rejected, review, etc). > > +1 for patchwork. I agree. Installing patchwork on dpdk.org is planned. I wasn't available last weeks so I'm starting to review the recent patches. By the way, it's good news that we now need such tools to manage contributions. -- Thomas
[dpdk-dev] Max possible packet burst size...
Hello friends, Does anyone know of a method, that would get me the maximum possible transmit burst size for a NIC ? Are there any methods to compute the check sum (for FCS, IPv4 etc) fields within Intel DPDK ? Thanks and regards
[dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
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
---
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) {
--
1.8.3.1
[dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
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
>
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) {
>
[dpdk-dev] IEEE 1588 support?
Hi David, Please notice that the default configuration doesn't enable 1588 support. Daniel On Tue, Mar 18, 2014 at 8:51 PM, Jayakumar, Muthurajan < muthurajan.jayakumar at intel.com> wrote: > Please find the code for 1588 support > http://www.dpdk.org/browse/dpdk/tree/app/test-pmd/ieee1588fwd.c > Thanks, > > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David P. Reed > Sent: Tuesday, March 18, 2014 10:50 AM > To: dev at dpdk.org > Subject: [dpdk-dev] IEEE 1588 support? > > Hi - I got the notice that DPDK is now supported for FreeBSD. That's > great - it could be useful. > > One question that I have is - does DPDK provide access to, and > performance/precision for, the support of IEEE 1588 at the hardware level > for 10 GigE NICs/switches that enable that? > > It's not obvious from the documentation scan I did. > > David P. Reed, Ph.D. > TidalScale, Inc. > > > > >
[dpdk-dev] [memnic PATCH v2] linux: fix build with kernel 3.3
Hi,
I missed it sorry.
> Subject: [memnic PATCH v2] linux: fix build with kernel 3.3
>
> Remove unused dev_ops functions.
>
> The API of some functions (memnic_vlan_rx_add_vid,
> memnic_vlan_rx_kill_vid) changed starting from 3.3 kernel. Instead of
> using a #ifdef to handle the compilation on any kernel, we can just
> remove these functions as they are not needed.
>
> Signed-off-by: Olivier Matz
Acked-by: Hiroshi Shimamoto
thanks,
Hiroshi
> ---
> linux/memnic_net.c | 33 -
> 1 file changed, 33 deletions(-)
>
> Hi Shimamoto-san,
>
> Here is a new version of the patch, I think we don't need the following
> functions so we can just remove them instead of keeping several dummy
> functions for different kernel versions.
>
> Let me know if you have any comment.
>
> Regards,
> Olivier
>
> diff --git a/linux/memnic_net.c b/linux/memnic_net.c
> index 747ae51..9019258 100644
> --- a/linux/memnic_net.c
> +++ b/linux/memnic_net.c
> @@ -235,16 +235,6 @@ drop:
> return NETDEV_TX_OK;
> }
>
> -static u16 memnic_select_queue(struct net_device *netdev,
> - struct sk_buff *skb)
> -{
> - return 0;
> -}
> -
> -static void memnic_set_rx_mode(struct net_device *netdev)
> -{
> -}
> -
> static int memnic_set_mac(struct net_device *netdev, void *p)
> {
> return 0;
> @@ -255,23 +245,6 @@ static int memnic_change_mtu(struct net_device *netdev,
> int new_mtu)
> return 0;
> }
>
> -static void memnic_tx_timeout(struct net_device *netdev)
> -{
> -}
> -
> -static void memnic_vlan_rx_add_vid(struct net_device *netdev, unsigned short
> vid)
> -{
> -}
> -
> -static void memnic_vlan_rx_kill_vid(struct net_device *netdev, unsigned
> short vid)
> -{
> -}
> -
> -static int memnic_ioctl(struct net_device *netdev, struct ifreq *req, int
> cmd)
> -{
> - return 0;
> -}
> -
> static struct net_device_stats *memnic_get_stats(struct net_device *netdev)
> {
> struct memnic_net *memnic = netdev_priv(netdev);
> @@ -283,15 +256,9 @@ static const struct net_device_ops memnic_netdev_ops = {
> .ndo_open = memnic_open,
> .ndo_stop = memnic_close,
> .ndo_start_xmit = memnic_start_xmit,
> - .ndo_select_queue = memnic_select_queue,
> - .ndo_set_rx_mode= memnic_set_rx_mode,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_set_mac_address= memnic_set_mac,
> .ndo_change_mtu = memnic_change_mtu,
> - .ndo_tx_timeout = memnic_tx_timeout,
> - .ndo_vlan_rx_add_vid= memnic_vlan_rx_add_vid,
> - .ndo_vlan_rx_kill_vid = memnic_vlan_rx_kill_vid,
> - .ndo_do_ioctl = memnic_ioctl,
> .ndo_get_stats = memnic_get_stats,
> };
>
> --
> 1.8.5.3
