RE: [dpdk-dev] [PATCH v2] doc: define qualification criteria for external library

[Morten Brørup]

> From: Jerin Jacob [mailto:jerinjac...@gmail.com]
> Sent: Friday, 17 November 2023 05.34
> 
> On Thu, Sep 28, 2023 at 11:10 AM  wrote:
> >
> > From: Jerin Jacob 
> >
> > Define qualification criteria for external library
> > based on a techboard meeting minutes [1] and past
> > learnings from mailing list discussion.
> >
> > [1]
> > http://mails.dpdk.org/archives/dev/2019-June/135847.html
> >
> > Signed-off-by: Jerin Jacob 
> 
> Ping for review and/or merge.
> 
> 
> > ---
> > v2:
> > - Added "Meson build integration" and "Code readability" sections.
> >
> >  doc/guides/contributing/index.rst |  1 +
> >  .../contributing/library_dependency.rst   | 23
> +++
> >  2 files changed, 24 insertions(+)
> >  create mode 100644 doc/guides/contributing/library_dependency.rst
> >
> > diff --git a/doc/guides/contributing/index.rst
> b/doc/guides/contributing/index.rst
> > index dcb9b1fbf0..e5a8c2b0a3 100644
> > --- a/doc/guides/contributing/index.rst
> > +++ b/doc/guides/contributing/index.rst
> > @@ -15,6 +15,7 @@ Contributor's Guidelines
> >  documentation
> >  unit_test
> >  new_library
> > +library_dependency
> >  patches
> >  vulnerability
> >  stable
> > diff --git a/doc/guides/contributing/library_dependency.rst
> b/doc/guides/contributing/library_dependency.rst
> > new file mode 100644
> > index 00..687a3b6cef
> > --- /dev/null
> > +++ b/doc/guides/contributing/library_dependency.rst
> > @@ -0,0 +1,23 @@
> > +.. SPDX-License-Identifier: BSD-3-Clause
> > +   Copyright(c) 2023 Marvell.
> > +
> > +Library dependency
> > +==
> > +
> > +This document defines the qualification criteria for external
> libraries that may be
> > +used as dependencies in DPDK drivers or libraries.
> > +
> > +- **Free availability**: The library must be freely available to
> build in either source or binary
> > +  form, with a preference for source form.

Suggest adding:

- **Free use and distribution license**: The library must be freely available 
to use and distribute without any attached conditions.

We must require a BSD-like license, to ensure that DPDK as a whole (including 
3rd party libraries) remains BSD licensed, and can be used in commercial 
(closed source) applications.

> > +
> > +- **Compiler compatibility**: The library must be able to compile
> with a DPDK supported compiler
> > +  for the given execution environment. For example, For Linux, the
> library must be able to compile

Typo (after "For example,"): For -> for

> > +  with GCC and/or clang.
> > +
> > +- **Documentation**: Must have adequate documentation for the steps
> to build it.
> > +
> > +- **Meson build integration**: The library must have standard method
> like ``pkg-config``
> > +  for seamless integration with DPDK's build environment.
> > +
> > +- **Code readability**: When the depended library is optional, use
> stubs to reduce the ``ifdef``
> > +  clutter to enable better code readability.

Why does everyone keep insisting that stubs make code more readable? Sometimes 
#ifdef is better.

Please use something like this instead:

- **Code readability**: When the depended library is optional, use either stubs 
or ``#ifdef`` consistently, not a mix of both, to ensure code readability.

RE: [PATCH v2 00/10] replace zero length arrays

[Morten Brørup]

> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Friday, 17 November 2023 05.03
> 
> Zero length arrays are a GNU extension that has been
> superseded by flex arrays.
> 
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> 
> These are places found by cocci/zero_length_array.cocci

Series-acked-by: Morten Brørup 

Suggest checkpatches to disallow this, if it doesn't already.

Re: [PATCH] net/txgbe: fix out of bound access

[Ferruh Yigit]

On 11/17/2023 2:45 AM, Jiawen Wu wrote:
> On Thursday, November 16, 2023 10:07 PM, ferruh.yi...@amd.com wrote:
>> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>>
>>  In function 'txgbe_host_interface_command',
>>  inlined from 'txgbe_host_interface_command'
>>  at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
>>  inlined from 'txgbe_hic_reset'
>>  at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
>>  ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
>> error: array subscript 2 is outside array bounds ofr
>>'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
>>145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>>  ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
>>  ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
>> note: at offset 8 into object 'reset_cmd' of size 8
>>331 | struct txgbe_hic_reset reset_cmd;
>>|^
>>
>> Access to buffer done based on command code, the case complained by
>> FW_RESET_CMD has short buffer but this code path only taken with command
>> 0x30, so this shouldn't be a problem.
>>
>> Adding a size check before accessing to the buffer, as this is control
>> plane code, additional check shouldn't hurt.
>>
>> [1]
>> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>>
>> [2]
>> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>>
>> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
>> Cc: sta...@dpdk.org
>>
>> Reported-by: Luca Boccassi 
>> Signed-off-by: Ferruh Yigit 
>> ---
>> Cc: jiawe...@trustnetic.com
>> Cc: jianw...@trustnetic.com
>>
>> @Luca, I am not sure if this additional check will satisfy the compiler,
>> can you please verify the patch?
>>
>> @Jiawen, there is a specific handling for command 0x30, from comment it
>> looks like it is Read Flash command, but it looks like this command is
>> not used by the driver, if this is correct can we remove the check
>> completely? Removing can be simpler way to fix the compiler error.
> 
> Thanks Ferruh. This command has been removed because flash can be read
> directly by the driver. The check can be simply removed.
> 

OK, I will send a new version for it.

>> ---
>>  drivers/net/txgbe/base/txgbe_mng.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/txgbe/base/txgbe_mng.c 
>> b/drivers/net/txgbe/base/txgbe_mng.c
>> index df7145094f84..9797b1b8b5da 100644
>> --- a/drivers/net/txgbe/base/txgbe_mng.c
>> +++ b/drivers/net/txgbe/base/txgbe_mng.c
>> @@ -147,6 +147,10 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 
>> *buffer,
>>   * two byes instead of one byte
>>   */
>>  if (resp->cmd == 0x30) {
>> +if (length < ((dword_len + 2) << 2)) {
>> +err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
>> +goto rel_out;
>> +}
>>  for (; bi < dword_len + 2; bi++)
>>  buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>>
>> --
>> 2.34.1
>>
> 

Re: [PATCH] app/test-pmd: fix L4 checksum with padding data

[Ferruh Yigit]

On 11/17/2023 3:28 AM, Stephen Hemminger wrote:
> On Fri, 17 Nov 2023 00:50:16 +
> Ferruh Yigit  wrote:
> 
 Hi Kaiwen,

 I am trying to understand the problem, what is the testcase that has
 checksum error?

 Are the received mbuf data_len & pkt_len wrong? Instead of trying to fix
 the mbuf during forwarding, can we fix where packet generated?  
>>>
>>> The root cause is that get_udptcp_cksum_mbuf is using m->pkt_len
>>> which maybe larger than the actual data. The real issue is there and
>>> in rte_ip.h checksum code. The correct fix would be to use l3_len instead.
>>>   
>>
>> I see, you are right.
>>
>> In 'rte_ipv4_udptcp_cksum_mbuf()',
>> as payload length "mbuf->pkt_len - l4_off" is used, which includes
>> padding and if padding is not zero it will end up producing wrong checksum.
>>
>>
>> I agree using 'l3_len' instead is correct fix.
>>
>> But this requires ABI/API change,
>> plus do we have any reason to keep the padding, discarding it as this
>> patch does is also simpler alternative.
> 
> 
> Possibly an API version to change the args would work to fix.
>

rte_ipv4_udptcp_cksum_mbuf() and rte_ipv6_udptcp_cksum_mbuf() are inline
functions, unfortunately we can't version them.

But those functions already gets IP header as parameter, can't we use IP
header to get the payload size? If so this can be fixed without updating
API.

RE: [EXT] [PATCH 2/2] test/crypto: add negative test cases for cipher buffer alignment

[Sivaramakrishnan, VenkatX]

Hi Akhil,


> -Original Message-
> From: Akhil Goyal 
> Sent: Monday, November 13, 2023 12:31 PM
> To: Sivaramakrishnan, VenkatX ; Fan
> Zhang 
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: RE: [EXT] [PATCH 2/2] test/crypto: add negative test cases for cipher
> buffer alignment
> 
> > add negative test cases for 3DES CBC and AES CBC cipher algorithms for
> > buffer misalignment
> >
> > Signed-off-by: Sivaramakrishnan Venkat
> > 
> > ---
> >  app/test/test_cryptodev.c  | 321 -
> >  app/test/test_cryptodev_aes_test_vectors.h | 119 
> >  app/test/test_cryptodev_blockcipher.c  |  20 +-
> >  app/test/test_cryptodev_blockcipher.h  |   1 +
> >  app/test/test_cryptodev_des_test_vectors.h |  38 +++
> >  5 files changed, 491 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index d2c4c6f8b5..12e0cf8044 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -1371,6 +1371,42 @@ negative_hmac_sha1_testsuite_setup(void)
> > return 0;
> >  }
> >
> > +static int
> > +negative_input_buffer_misalignment_testsuite_setup(void)
> > +{
> > +   struct crypto_testsuite_params *ts_params = &testsuite_params;
> > +   uint8_t dev_id = ts_params->valid_devs[0];
> > +   struct rte_cryptodev_info dev_info;
> > +   const enum rte_crypto_cipher_algorithm ciphers[] = {
> > +   RTE_CRYPTO_CIPHER_3DES_CBC,
> > +   RTE_CRYPTO_CIPHER_AES_CBC
> > +   };
> > +   const enum rte_crypto_auth_algorithm auths[] = {
> > +   RTE_CRYPTO_AUTH_SHA256,
> > +   RTE_CRYPTO_AUTH_SHA256,
> > +   };
> > +
> > +   rte_cryptodev_info_get(dev_id, &dev_info);
> > +
> > +   if (!(dev_info.feature_flags &
> > RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO) ||
> > +   ((global_api_test_type ==
> CRYPTODEV_RAW_API_TEST)
> > &&
> > +   !(dev_info.feature_flags &
> > RTE_CRYPTODEV_FF_SYM_RAW_DP))) {
> > +   RTE_LOG(INFO, USER1, "Feature flag requirements for
> Negative
> > "
> > +   "Input Buffer misalignment testsuite not
> > met\n");
> > +   return TEST_SKIPPED;
> > +   }
> > +
> > +   if (check_cipher_capabilities_supported(ciphers, RTE_DIM(ciphers))
> != 0
> > +   && check_auth_capabilities_supported(auths,
> > +   RTE_DIM(auths)) != 0) {
> > +   RTE_LOG(INFO, USER1, "Capability requirements for Negative
> "
> > +   "Input Buffer misalignment testsuite not
> > met\n");
> > +   return TEST_SKIPPED;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> >  static int
> >  dev_configure_and_start(uint64_t ff_disable)  { @@ -14469,6
> > +14505,192 @@ aes128cbc_hmac_sha1_test_vector = {
> > }
> >  };
> >
> > +static const struct test_crypto_vector
> > +aes128cbc_sha256_misalign_test_vector = {
> > +   .crypto_algo = RTE_CRYPTO_CIPHER_AES_CBC,
> > +   .cipher_offset = 0,
> > +   .cipher_len = 511,
> > +   .cipher_key = {
> > +   .data = {
> > +   0xE4, 0x23, 0x33, 0x8A, 0x35, 0x64, 0x61, 0xE2,
> > +   0x49, 0x03, 0xDD, 0xC6, 0xB8, 0xCA, 0x55, 0x7A
> > +   },
> > +   .len = 16
> > +   },
> > +   .iv = {
> > +   .data = {
> > +   0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
> > +   0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F
> > +   },
> > +   .len = 16
> > +   },
> > +   .plaintext = {
> > +   .data = plaintext_aes_common,
> > +   .len = 511
> > +   },
> > +   .ciphertext = {
> > +   .data = ciphertext512_aes128cbc,
> > +   .len = 511
> > +   },
> > +   .auth_algo = RTE_CRYPTO_AUTH_SHA256,
> > +   .auth_offset = 0,
> > +   .auth_key = {
> > +   .data = {
> > +   0x42, 0x1A, 0x7D, 0x3D, 0xF5, 0x82, 0x80, 0xF1,
> > +   0xF1, 0x35, 0x5C, 0x3B, 0xDD, 0x9A, 0x65, 0xBA,
> > +   0x58, 0x34, 0x85, 0x61, 0x1C, 0x42, 0x10, 0x76,
> > +   0x9A, 0x4F, 0x88, 0x1B, 0xB6, 0x8F, 0xD8, 0x60
> > +   },
> > +   .len = 32
> > +   },
> > +   .digest = {
> > +   .data = {
> > +   0xA8, 0xBC, 0xDB, 0x99, 0xAA, 0x45, 0x91, 0xA3,
> > +   0x2D, 0x75, 0x41, 0x92, 0x28, 0x01, 0x87, 0x5D,
> > +   0x45, 0xED, 0x49, 0x05, 0xD3, 0xAE, 0x32, 0x57,
> > +   0xB7, 0x79, 0x65, 0xFC, 0xFA, 0x6C, 0xFA, 0xDF
> > +   },
> > +   .len = 32
> > +   }
> > +};
> 
> Why are the vectors added in .c file?
> 
We will move the tests to blockcipher test vectors and put in test vector 
header files with others.
> 


> > +
> >  static const struct test_crypto_vector
> > aes128cbc_hmac_sha1_aad_test_vector = {
> > .crypto_algo = RTE_CRYPTO_CIPHER_AES_CBC, @@ -15058,7
> +15280,7 @@
> > test_authenticated_decryption_fail_when_corruption(
> >  }
> >
> >  static int
> > -test_authenticated_encrypt_

Re: [dpdk-dev] [PATCH v2] doc: define qualification criteria for external library

[Bruce Richardson]

On Fri, Nov 17, 2023 at 09:27:02AM +0100, Morten Brørup wrote:
> > From: Jerin Jacob [mailto:jerinjac...@gmail.com]
> > Sent: Friday, 17 November 2023 05.34
> > 
> > On Thu, Sep 28, 2023 at 11:10 AM  wrote:
> > >
> > > From: Jerin Jacob 
> > >
> > > Define qualification criteria for external library
> > > based on a techboard meeting minutes [1] and past
> > > learnings from mailing list discussion.
> > >
> > > [1]
> > > http://mails.dpdk.org/archives/dev/2019-June/135847.html
> > >
> > > Signed-off-by: Jerin Jacob 
> > 
> > Ping for review and/or merge.
> > 
> > 
> > > ---
> > > v2:
> > > - Added "Meson build integration" and "Code readability" sections.
> > >
> > >  doc/guides/contributing/index.rst |  1 +
> > >  .../contributing/library_dependency.rst   | 23
> > +++
> > >  2 files changed, 24 insertions(+)
> > >  create mode 100644 doc/guides/contributing/library_dependency.rst
> > >
> > > diff --git a/doc/guides/contributing/index.rst
> > b/doc/guides/contributing/index.rst
> > > index dcb9b1fbf0..e5a8c2b0a3 100644
> > > --- a/doc/guides/contributing/index.rst
> > > +++ b/doc/guides/contributing/index.rst
> > > @@ -15,6 +15,7 @@ Contributor's Guidelines
> > >  documentation
> > >  unit_test
> > >  new_library
> > > +library_dependency
> > >  patches
> > >  vulnerability
> > >  stable
> > > diff --git a/doc/guides/contributing/library_dependency.rst
> > b/doc/guides/contributing/library_dependency.rst
> > > new file mode 100644
> > > index 00..687a3b6cef
> > > --- /dev/null
> > > +++ b/doc/guides/contributing/library_dependency.rst
> > > @@ -0,0 +1,23 @@
> > > +.. SPDX-License-Identifier: BSD-3-Clause
> > > +   Copyright(c) 2023 Marvell.
> > > +
> > > +Library dependency
> > > +==
> > > +
> > > +This document defines the qualification criteria for external
> > libraries that may be
> > > +used as dependencies in DPDK drivers or libraries.
> > > +
> > > +- **Free availability**: The library must be freely available to
> > build in either source or binary
> > > +  form, with a preference for source form.
> 
> Suggest adding:
> 
> - **Free use and distribution license**: The library must be freely available 
> to use and distribute without any attached conditions.
> 
> We must require a BSD-like license, to ensure that DPDK as a whole (including 
> 3rd party libraries) remains BSD licensed, and can be used in commercial 
> (closed source) applications.
> 
I think the situation is a bit more complex. Firstly, we need to ensure
that there are no license incompatibilities. Beyond that though, the
importance of the library will depend on how strict we are going to be
about open-source licensing etc.

For example, for a particular driver - nic, crypto, whatever - we have in
the past allowed linking against non-opensource libraries in order to build
that component. That (thankfully) has not caused us any serious issues to
date, and I don't think we should change things by completely disallowing
it in future.

On the other hand, a library that becomes key for building more than just a
driver or rarely used library, e.g. one that adds key functionality to EAL,
would be held to a much higher standard. In that case we likely would look
for an open-source, appropriately licensed, version.

/Bruce

RE: [PATCH v7] app/test: secondary process passes allow parameters

[Huang, ZhiminX]

> -Original Message-
> From: Mingjin Ye 
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming ; Ye, MingjinX
> ; sta...@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
> 
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to the secondary
> process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Mingjin Ye 
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---

Verified pass for meson test:
dpdk-devbind.py -b vfio-pci 31:00.0 31:00.1 b1:00.0 b1:00.1
DPDK_TEST=eal_flags_file_prefix_autotest MALLOC_PERTURB_=192 
/root/dpdk/x86_64-native-linuxapp-gcc/app/test/dpdk-test 
--file-prefix=eal_flags_file_prefix_autotest -a 31:00.0 -a 31:00.1

Tested-by: Zhimin Huang 

[PATCH v2] net/txgbe: fix out of bound access

[Ferruh Yigit]

Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:

 In function 'txgbe_host_interface_command',
 inlined from 'txgbe_host_interface_command'
 at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
 inlined from 'txgbe_hic_reset'
 at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
 ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
error: array subscript 2 is outside array bounds ofr
   'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
   145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
 ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
 ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
note: at offset 8 into object 'reset_cmd' of size 8
   331 | struct txgbe_hic_reset reset_cmd;
   |^

Access to buffer done based on command code, the case complained by
FW_RESET_CMD has short buffer but this code path only taken with command
0x30, so this shouldn't be a problem.

Command 0x30 no more used, removing this exception check that cause
build error.

[1]
https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log

[2]
gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912

Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
Cc: sta...@dpdk.org

Reported-by: Luca Boccassi 
Signed-off-by: Ferruh Yigit 
---
Cc: jiawe...@trustnetic.com
Cc: jianw...@trustnetic.com

v2:
* Removed exception check for command 0x30
---
 drivers/net/txgbe/base/txgbe_mng.c | 16 +---
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/net/txgbe/base/txgbe_mng.c 
b/drivers/net/txgbe/base/txgbe_mng.c
index df7145094f84..029a0a1fe143 100644
--- a/drivers/net/txgbe/base/txgbe_mng.c
+++ b/drivers/net/txgbe/base/txgbe_mng.c
@@ -141,21 +141,7 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 
*buffer,
for (bi = 0; bi < dword_len; bi++)
buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
 
-   /*
-* If there is any thing in data position pull it in
-* Read Flash command requires reading buffer length from
-* two byes instead of one byte
-*/
-   if (resp->cmd == 0x30) {
-   for (; bi < dword_len + 2; bi++)
-   buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
-
-   buf_len = (((u16)(resp->cmd_or_resp.ret_status) << 3)
- & 0xF00) | resp->buf_len;
-   hdr_size += (2 << 2);
-   } else {
-   buf_len = resp->buf_len;
-   }
+   buf_len = resp->buf_len;
if (!buf_len)
goto rel_out;
 
-- 
2.34.1

Re: [PATCH] app/testpmd: fix set Tx offload command

[Ferruh Yigit]

On 11/17/2023 1:31 AM, lihuisong (C) wrote:

>> On 2023/11/17 1:21, Ferruh Yigit wrote:
>>> In command to set Tx offload:
>>> "port config  tx_offload  on|off",
>>>
>>> there is a defect in "on|off" comparison, so command does opposite of
>>> what is intended. Fixed comparison.
>>>
>>> Fixes: 6280fe565b44 ("app/testpmd: allow offload config for all ports")
>>>
>>> Signed-off-by: Ferruh Yigit 
>> 
>> Reviewed-by: Chengwen Feng 
>> 
>
> Acked-by: Huisong Li 
>

  Bugzilla ID: 1326

Applied to dpdk-next-net/main, thanks.

RE: [PATCH v7] app/test: secondary process passes allow parameters

[Ye, MingjinX]

Hi Richardson,

can you please take a look at this patch.

Thanks,
Mingjin


> -Original Message-
> From: Ye, MingjinX 
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming ; Ye, MingjinX
> ; sta...@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
> 
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to the
> secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Mingjin Ye 
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---
>  app/test/process.h | 46
> ++
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h index
> af7bc3e0de..06b2f8b192 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -17,6 +17,7 @@
>  #include 
> 
>  #include  /* strlcpy */
> +#include 
> 
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
> @@ -34,6 +35,34 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> 
> +#define PREFIX_ALLOW "--allow="
> +
> +static int
> +add_parameter_allow(char **argv, int max_capacity) {
> + struct rte_devargs *devargs;
> + int count = 0;
> +
> + RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> + if (strlen(devargs->name) == 0)
> + continue;
> +
> + if (devargs->data == NULL || strlen(devargs->data) == 0) {
> + if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> devargs->name) < 0)
> + break;
> + } else {
> + if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> +  devargs->name, devargs->data) < 0)
> + break;
> + }
> +
> + if (++count == max_capacity)
> + break;
> + }
> +
> + return count;
> +}
> +
>  /*
>   * launches a second copy of the test process using the given argv
> parameters,
>   * which should include argv[0] as the process name. To identify in the @@ -
> 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> process_dup(const char *const argv[], int numargs, const char *env_value)  {
> - int num;
> - char *argv_cpy[numargs + 1];
> + int num = 0;
> + char **argv_cpy;
> + int allow_num;
> + int argv_num;
>   int i, status;
>   char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,11 +89,18 @@ process_dup(const char *const argv[], int numargs,
> const char *env_value)
>   if (pid < 0)
>   return -1;
>   else if (pid == 0) {
> + allow_num =
> rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> + argv_num = numargs + allow_num + 1;
> + argv_cpy = calloc(argv_num, sizeof(char *));
> + if (!argv_cpy)
> + rte_panic("Memory allocation failed\n");
> +
>   /* make a copy of the arguments to be passed to exec */
>   for (i = 0; i < numargs; i++)
>   argv_cpy[i] = strdup(argv[i]);
> - argv_cpy[i] = NULL;
> - num = numargs;
> + if (allow_num > 0)
> + num = add_parameter_allow(&argv_cpy[i],
> allow_num);
> + num += numargs;
> 
>  #ifdef RTE_EXEC_ENV_LINUX
>   {
> --
> 2.25.1

RE: [PATCH] bus/vdev: fix devargs memory leak

[Ye, MingjinX]

Hi Burakov,

can you please take a look at this patch.

Thanks,
Mingjin

> > -Original Message-
> > From: Ling, WeiX 
> > Sent: Tuesday, September 12, 2023 5:08 PM
> > To: Ye, MingjinX ; dev@dpdk.org
> > Cc: Yang, Qiming ; Zhou, YidingX
> > ; Ye, MingjinX ;
> > sta...@dpdk.org; Burakov, Anatoly 
> > Subject: RE: [PATCH] bus/vdev: fix devargs memory leak
> >
> > > -Original Message-
> > > From: Mingjin Ye 
> > > Sent: Friday, September 1, 2023 3:24 PM
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming ; Zhou, YidingX
> > > ; Ye, MingjinX ;
> > > sta...@dpdk.org; Burakov, Anatoly 
> > > Subject: [PATCH] bus/vdev: fix devargs memory leak
> > >
> > > When a device is created by a secondary process, an empty devargs is
> > > temporarily generated and bound to it. This causes the device to not
> > > be associated with the correct devargs, and the empty devargs are
> > > not released when the resource is freed.
> > >
> > > This patch fixes the issue by matching the devargs when inserting a
> > > device in secondary process.
> > >
> > > Fixes: dda987315ca2 ("vdev: make virtual bus use its device struct")
> > > Fixes: a16040453968 ("eal: extract vdev infra")
> > > Cc: sta...@dpdk.org
> > >
> > > Signed-off-by: Mingjin Ye 
> > > ---
> >
> > Tested-by: Wei Ling 

Re: [PATCH] eal: fix alignment of RISCV xmm vector type

[Bruce Richardson]

On Thu, Nov 16, 2023 at 08:45:35AM +0100, Morten Brørup wrote:
> > From: Stanisław Kardach [mailto:k...@semihalf.com]
> > Sent: Thursday, 16 November 2023 00.21
> > 
> > On Wed, Nov 15, 2023 at 10:31 PM Morten Brørup
> >  wrote:
> > >
> > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com]
> > > > Sent: Wednesday, 15 November 2023 22.17
> > > >
> > > > Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.
> > > >
> > > > Fixes: f22e705ebf12 ("eal/riscv: support RISC-V architecture")
> > > > Cc: m...@semihalf.com
> > > > Cc: sta...@dpdk.org
> > > > Signed-off-by: Tyler Retzlaff 
> > > > ---
> > >
> > > Reviewed-by: Morten Brørup 
> > >
> > > As mentioned in the other thread:
> > >
> > > We need to urgently decide if this bug should live on in DPDK 23.11,
> > or if the fix should be included although we are very late in the
> > release process.
> > >
> > > Stanislaw, what do you think?
> > Good catch! As for backporting I'm not sure of the urgency given that
> > our examples still use scalar instructions for handling xmm_t. The
> > question is whether there is a platform in use which has vector
> > extensions enabled and that utilizes DPDK. I'm not that sure of it
> > though I'd be happy to be proven wrong.
> 
> Can we extrapolate this, and also conclude that postponing this bugfix until 
> the next ABI/API breaking release, DPDK 24.11, is not really going to hurt 
> anyone?
> 
> Stanislaw, please confirm?
> 
> Bruce, I don't feel 100 % confident in making this postponement 
> recommendation. Could you please provide a second opinion regarding the 
> timing of fixing this bug? Or rather: do you have any strong arguments 
> *against* postponing it to DPDK 24.11?
> 

Not sure I'm any better placed to make an argument either way! However, I
would very much tend to say that we should include this in 23.11, on the
basis that if it turns out to be important we can't backport it later
without affecting ABI. Right now, the code looks broken to me, and I'm also
struggling to find circumstances where increasing the alignment will
actually stop something from working. There could well be performance
implications of having extra padding, but things should still work, AFAIK.
On the other hand, if we don't include the fix, it is possible that a
system (possibly a future one) could break and segfault due to incorrect
vector code. I'd take a possible slowdown over a segfault!

Is my assessment correct, or perhaps I'm missing some detail.

/Bruce

RE: [dpdk-dev] [PATCH v2] doc: define qualification criteria for external library

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> Sent: Friday, 17 November 2023 10.52
> 
> On Fri, Nov 17, 2023 at 09:27:02AM +0100, Morten Brørup wrote:
> > > From: Jerin Jacob [mailto:jerinjac...@gmail.com]
> > > Sent: Friday, 17 November 2023 05.34
> > >
> > > On Thu, Sep 28, 2023 at 11:10 AM  wrote:
> > > >
> > > > From: Jerin Jacob 
> > > >
> > > > Define qualification criteria for external library
> > > > based on a techboard meeting minutes [1] and past
> > > > learnings from mailing list discussion.
> > > >
> > > > [1]
> > > > http://mails.dpdk.org/archives/dev/2019-June/135847.html
> > > >
> > > > Signed-off-by: Jerin Jacob 
> > >
> > > Ping for review and/or merge.
> > >
> > >
> > > > ---
> > > > v2:
> > > > - Added "Meson build integration" and "Code readability"
> sections.
> > > >
> > > >  doc/guides/contributing/index.rst |  1 +
> > > >  .../contributing/library_dependency.rst   | 23
> > > +++
> > > >  2 files changed, 24 insertions(+)
> > > >  create mode 100644
> doc/guides/contributing/library_dependency.rst
> > > >
> > > > diff --git a/doc/guides/contributing/index.rst
> > > b/doc/guides/contributing/index.rst
> > > > index dcb9b1fbf0..e5a8c2b0a3 100644
> > > > --- a/doc/guides/contributing/index.rst
> > > > +++ b/doc/guides/contributing/index.rst
> > > > @@ -15,6 +15,7 @@ Contributor's Guidelines
> > > >  documentation
> > > >  unit_test
> > > >  new_library
> > > > +library_dependency
> > > >  patches
> > > >  vulnerability
> > > >  stable
> > > > diff --git a/doc/guides/contributing/library_dependency.rst
> > > b/doc/guides/contributing/library_dependency.rst
> > > > new file mode 100644
> > > > index 00..687a3b6cef
> > > > --- /dev/null
> > > > +++ b/doc/guides/contributing/library_dependency.rst
> > > > @@ -0,0 +1,23 @@
> > > > +.. SPDX-License-Identifier: BSD-3-Clause
> > > > +   Copyright(c) 2023 Marvell.
> > > > +
> > > > +Library dependency
> > > > +==
> > > > +
> > > > +This document defines the qualification criteria for external
> > > libraries that may be
> > > > +used as dependencies in DPDK drivers or libraries.
> > > > +
> > > > +- **Free availability**: The library must be freely available to
> > > build in either source or binary
> > > > +  form, with a preference for source form.
> >
> > Suggest adding:
> >
> > - **Free use and distribution license**: The library must be freely
> available to use and distribute without any attached conditions.
> >
> > We must require a BSD-like license, to ensure that DPDK as a whole
> (including 3rd party libraries) remains BSD licensed, and can be used
> in commercial (closed source) applications.
> >
> I think the situation is a bit more complex. Firstly, we need to ensure
> that there are no license incompatibilities. Beyond that though, the
> importance of the library will depend on how strict we are going to be
> about open-source licensing etc.

My point about the license was not related to source code availability, it was 
related to conditions for use and distribution of the library.

The license needs to allow unrestricted and unconditional use and distribution, 
like the BSD license does.

In principle, this requirement only applies to binary form; we don't need to be 
able to distribute the source code of external libraries, regardless if it is 
open source or NDA-protected "view only" source code or other restricted access 
source code (e.g. Microsoft's Shared Source Initiative).

> 
> For example, for a particular driver - nic, crypto, whatever - we have
> in
> the past allowed linking against non-opensource libraries in order to
> build
> that component. That (thankfully) has not caused us any serious issues
> to
> date, and I don't think we should change things by completely
> disallowing
> it in future.
> 
> On the other hand, a library that becomes key for building more than
> just a
> driver or rarely used library, e.g. one that adds key functionality to
> EAL,
> would be held to a much higher standard. In that case we likely would
> look
> for an open-source, appropriately licensed, version.

I agree that the required degree of open source principles should vary with 
DPDK's dependency of the library.

We can be more lenient with hardware PMDs, where end users ultimately can 
choose another hardware vendor if a dependent library is unacceptable for the 
end user.

There is no doubt we should keep allowing opaque binary BLOBs for hardware, 
such as on-board firmware and FPGA images.

Re: [PATCH] net/gve: add support for max_rx_bufsize

[Ferruh Yigit]

On 11/17/2023 1:35 AM, lihuisong (C) wrote:
> 
> 在 2023/11/17 6:16, Joshua Washington 写道:
>> The new max_rx_bufsize field in dev_info can be used to guide mbuf sizes
>> chosen by DPDK programs by ensuring that DPDK programs do not waste
>> memory by using an mbuf size too large for the maximum RX buffer size.
>> This patch adds support for this field in the GVE PMD.
>>
>> Signed-off-by: Joshua Washington 
>> Reviewed-by: Rushil Gupta 
> 
> Reviewed-by: Huisong Li 
>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v7] app/test: secondary process passes allow parameters

[Bruce Richardson]

On Fri, Nov 17, 2023 at 10:24:41AM +, Ye, MingjinX wrote:
> Hi Richardson,
> 
> can you please take a look at this patch.
> 
> Thanks,
> Mingjin
> 

Hi,

I acked v6. If nothing major has changed in the patch, then the ack can be
kept for v7. If there is something that has changed that you think I should
look at, please include it in the change-log, so I can check it out.

For now, assuming no major changes:

Acked-by: Bruce Richardson 

/Bruce

> 
> > -Original Message-
> > From: Ye, MingjinX 
> > Sent: Tuesday, November 14, 2023 6:28 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming ; Ye, MingjinX
> > ; sta...@dpdk.org
> > Subject: [PATCH v7] app/test: secondary process passes allow parameters
> > 
> > In EAL related test cases, the allow parameters are not passed to the
> > secondary process, resulting in unexpected NICs being loaded.
> > 
> > This patch fixes this issue by appending the allow parameters to the
> > secondary process.
> > 
> > Fixes: af75078fece3 ("first public release")
> > Cc: sta...@dpdk.org
> > 
> > Signed-off-by: Mingjin Ye 
> > ---
> > v5: Optimized.
> > ---
> > v6: Optimized.
> > ---
> > v7: Fix CI errors.
> > ---
> >  app/test/process.h | 46
> > ++
> >  1 file changed, 42 insertions(+), 4 deletions(-)
> > 
> > diff --git a/app/test/process.h b/app/test/process.h index
> > af7bc3e0de..06b2f8b192 100644
> > --- a/app/test/process.h
> > +++ b/app/test/process.h
> > @@ -17,6 +17,7 @@
> >  #include 
> > 
> >  #include  /* strlcpy */
> > +#include 
> > 
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >  #define self "curproc"
> > @@ -34,6 +35,34 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> > 
> > +#define PREFIX_ALLOW "--allow="
> > +
> > +static int
> > +add_parameter_allow(char **argv, int max_capacity) {
> > +   struct rte_devargs *devargs;
> > +   int count = 0;
> > +
> > +   RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> > +   if (strlen(devargs->name) == 0)
> > +   continue;
> > +
> > +   if (devargs->data == NULL || strlen(devargs->data) == 0) {
> > +   if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> > devargs->name) < 0)
> > +   break;
> > +   } else {
> > +   if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> > +devargs->name, devargs->data) < 0)
> > +   break;
> > +   }
> > +
> > +   if (++count == max_capacity)
> > +   break;
> > +   }
> > +
> > +   return count;
> > +}
> > +
> >  /*
> >   * launches a second copy of the test process using the given argv
> > parameters,
> >   * which should include argv[0] as the process name. To identify in the @@ 
> > -
> > 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> > process_dup(const char *const argv[], int numargs, const char *env_value)  {
> > -   int num;
> > -   char *argv_cpy[numargs + 1];
> > +   int num = 0;
> > +   char **argv_cpy;
> > +   int allow_num;
> > +   int argv_num;
> > int i, status;
> > char path[32];
> >  #ifdef RTE_LIB_PDUMP
> > @@ -58,11 +89,18 @@ process_dup(const char *const argv[], int numargs,
> > const char *env_value)
> > if (pid < 0)
> > return -1;
> > else if (pid == 0) {
> > +   allow_num =
> > rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> > +   argv_num = numargs + allow_num + 1;
> > +   argv_cpy = calloc(argv_num, sizeof(char *));
> > +   if (!argv_cpy)
> > +   rte_panic("Memory allocation failed\n");
> > +
> > /* make a copy of the arguments to be passed to exec */
> > for (i = 0; i < numargs; i++)
> > argv_cpy[i] = strdup(argv[i]);
> > -   argv_cpy[i] = NULL;
> > -   num = numargs;
> > +   if (allow_num > 0)
> > +   num = add_parameter_allow(&argv_cpy[i],
> > allow_num);
> > +   num += numargs;
> > 
> >  #ifdef RTE_EXEC_ENV_LINUX
> > {
> > --
> > 2.25.1
> 

RE: [PATCH] eal: fix alignment of RISCV xmm vector type

[Morten Brørup]

+CC Thomas, this patch is ready to be applied to 23.11.

> From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> Sent: Friday, 17 November 2023 11.55
> 
> On Thu, Nov 16, 2023 at 08:45:35AM +0100, Morten Brørup wrote:
> > > From: Stanisław Kardach [mailto:k...@semihalf.com]
> > > Sent: Thursday, 16 November 2023 00.21
> > >
> > > On Wed, Nov 15, 2023 at 10:31 PM Morten Brørup
> > >  wrote:
> > > >
> > > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com]
> > > > > Sent: Wednesday, 15 November 2023 22.17
> > > > >
> > > > > Fix the alignment for rte_xmm_t it should be 16 instead of 8
> bytes.
> > > > >
> > > > > Fixes: f22e705ebf12 ("eal/riscv: support RISC-V architecture")
> > > > > Cc: m...@semihalf.com
> > > > > Cc: sta...@dpdk.org
> > > > > Signed-off-by: Tyler Retzlaff 
> > > > > ---
> > > >
> > > > Reviewed-by: Morten Brørup 
> > > >
> > > > As mentioned in the other thread:
> > > >
> > > > We need to urgently decide if this bug should live on in DPDK
> 23.11,
> > > or if the fix should be included although we are very late in the
> > > release process.
> > > >
> > > > Stanislaw, what do you think?
> > > Good catch! As for backporting I'm not sure of the urgency given
> that
> > > our examples still use scalar instructions for handling xmm_t. The
> > > question is whether there is a platform in use which has vector
> > > extensions enabled and that utilizes DPDK. I'm not that sure of it
> > > though I'd be happy to be proven wrong.
> >
> > Can we extrapolate this, and also conclude that postponing this
> bugfix until the next ABI/API breaking release, DPDK 24.11, is not
> really going to hurt anyone?
> >
> > Stanislaw, please confirm?
> >
> > Bruce, I don't feel 100 % confident in making this postponement
> recommendation. Could you please provide a second opinion regarding the
> timing of fixing this bug? Or rather: do you have any strong arguments
> *against* postponing it to DPDK 24.11?
> >
> 
> Not sure I'm any better placed to make an argument either way!

Bruce, I picked you because of your experience with vector code.

> However, I
> would very much tend to say that we should include this in 23.11, on
> the
> basis that if it turns out to be important we can't backport it later
> without affecting ABI. Right now, the code looks broken to me, and I'm
> also
> struggling to find circumstances where increasing the alignment will
> actually stop something from working. There could well be performance
> implications of having extra padding, but things should still work,
> AFAIK.
> On the other hand, if we don't include the fix, it is possible that a
> system (possibly a future one) could break and segfault due to
> incorrect
> vector code. I'd take a possible slowdown over a segfault!

The risk of slowdown isn't a factor for me at this stage.

I'm trying to balance the risk of fixing the bug vs. breaking something this 
late in the 23.11 release cycle.

You have a strong point that we also need to consider the bugfix in the context 
of the total lifetime of DPDK 23.11 in the wild.
With RISC-V's current traction, that certainly speaks in favor of including it 
in 23.11.

> 
> Is my assessment correct, or perhaps I'm missing some detail.

Thank you for your valuable feedback, Bruce.

I was just being overly cautious... After all, 23.11 is still at a stage where 
bug fixes are accepted!

New conclusion: Let's get it into 23.11.

Re: [PATCH] cmdline-gen: fix error when command list has empty lines

[Bruce Richardson]

On Thu, Nov 16, 2023 at 12:18:13PM +0100, Robin Jarry wrote:
> Fix the following error when a command list file contains empty lines:
> 
> Traceback (most recent call last):
>   File "buildtools/dpdk-cmdline-gen.py", line 202, in 
> main()
>   File "buildtools/dpdk-cmdline-gen.py", line 184, in main
> process_commands(args.infile, sys.stdout, None, args.context_name)
>   File "buildtools/dpdk-cmdline-gen.py", line 141, in process_commands
> cmd_inst, h_out, c_out = process_command(lineno, tokens.strip().spl…
>  ^^…
>   File "buildtools/dpdk-cmdline-gen.py", line 36, in process_command
> if tokens[0].startswith("<"):
>~~^^^
> IndexError: list index out of range
> 
> Use shlex.split() to properly split each line arguments into tokens and
> strip comments.
> 
> If there are no tokens, ignore the line.
> 
> Fixes: 3791e9ed ("buildtools: add a tool to generate cmdline boilerplate")
> 
> Cc: Bruce Richardson 
> Signed-off-by: Robin Jarry 
> ---

LGTM, thanks.

Acked-by: Bruce Richardson 

RE: [PATCH] app/test-pmd: fix L4 checksum with padding data

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
> Sent: Friday, 17 November 2023 10.30
> 
> On 11/17/2023 3:28 AM, Stephen Hemminger wrote:
> > On Fri, 17 Nov 2023 00:50:16 +
> > Ferruh Yigit  wrote:
> >
>  Hi Kaiwen,
> 
>  I am trying to understand the problem, what is the testcase that
> has
>  checksum error?
> 
>  Are the received mbuf data_len & pkt_len wrong? Instead of trying
> to fix
>  the mbuf during forwarding, can we fix where packet generated?
> >>>
> >>> The root cause is that get_udptcp_cksum_mbuf is using m->pkt_len
> >>> which maybe larger than the actual data. The real issue is there
> and
> >>> in rte_ip.h checksum code. The correct fix would be to use l3_len
> instead.
> >>>
> >>
> >> I see, you are right.
> >>
> >> In 'rte_ipv4_udptcp_cksum_mbuf()',
> >> as payload length "mbuf->pkt_len - l4_off" is used, which includes
> >> padding and if padding is not zero it will end up producing wrong
> checksum.
> >>
> >>
> >> I agree using 'l3_len' instead is correct fix.
> >>
> >> But this requires ABI/API change,
> >> plus do we have any reason to keep the padding, discarding it as
> this
> >> patch does is also simpler alternative.
> >
> >
> > Possibly an API version to change the args would work to fix.
> >
> 
> rte_ipv4_udptcp_cksum_mbuf() and rte_ipv6_udptcp_cksum_mbuf() are
> inline
> functions, unfortunately we can't version them.
> 
> But those functions already gets IP header as parameter, can't we use
> IP
> header to get the payload size? If so this can be fixed without
> updating
> API.

If rte_ipv4_udptcp_cksum_mbuf() - or any other function in the DPDK Network 
Headers library - includes Ethernet padding (which may be non-zero) when 
calculating the TCP/UDP checksum of an IPv4 packet, it is a bug, and must be 
fixed there.

Our test cases should use random padding to catch bugs like this.

And I just realized that Ethernet padding may be added to any IP packet, so 
don't assume that this bug only applies to small packets.

[PATCH v7 0/4] PCI Dev and SG copy support

[Gowrishankar Muthukrishnan]

Improve dma-perf application to support PCI dev and SG copy,
along with additional supports below:
 - validate copied memory
 - skip tests if not opted.

v7:
 - PCI patch updated.

Amit Prakash Shukla (2):
  app/dma-perf: add skip support
  app/dma-perf: add PCI device support

Gowrishankar Muthukrishnan (2):
  app/dma-perf: validate copied memory
  app/dma-perf: add SG copy support

 app/test-dma-perf/benchmark.c | 383 +++---
 app/test-dma-perf/config.ini  |  56 +
 app/test-dma-perf/main.c  | 136 +++-
 app/test-dma-perf/main.h  |  12 +-
 4 files changed, 545 insertions(+), 42 deletions(-)

-- 
2.25.1

[PATCH v7 1/4] app/dma-perf: add skip support

[Gowrishankar Muthukrishnan]

From: Amit Prakash Shukla 

Add support to skip running a dma-perf test-case.

Signed-off-by: Amit Prakash Shukla 
Acked-by: Anoob Joseph 
---
 app/test-dma-perf/config.ini |  2 ++
 app/test-dma-perf/main.c | 23 +++
 app/test-dma-perf/main.h |  1 +
 3 files changed, 26 insertions(+)

diff --git a/app/test-dma-perf/config.ini b/app/test-dma-perf/config.ini
index b550f4b23f..4d59234b2a 100644
--- a/app/test-dma-perf/config.ini
+++ b/app/test-dma-perf/config.ini
@@ -36,6 +36,8 @@
 ; If you do not specify a result file, one will be generated with the same 
name as the configuration
 ; file, with the addition of "_result.csv" at the end.
 
+; "skip" To skip a test-case set skip to 1.
+
 [case1]
 type=DMA_MEM_COPY
 mem_size=10
diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 5f8bab8f45..c74f1d81bd 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -320,6 +320,7 @@ load_configs(const char *path)
const char *case_type;
const char *lcore_dma;
const char *mem_size_str, *buf_size_str, *ring_size_str, 
*kick_batch_str;
+   const char *skip;
int args_nr, nb_vp;
bool is_dma;
 
@@ -339,6 +340,13 @@ load_configs(const char *path)
for (i = 0; i < nb_sections; i++) {
snprintf(section_name, CFG_NAME_LEN, "case%d", i + 1);
test_case = &test_cases[i];
+
+   skip = rte_cfgfile_get_entry(cfgfile, section_name, "skip");
+   if (skip && (atoi(skip) == 1)) {
+   test_case->is_skip = true;
+   continue;
+   }
+
case_type = rte_cfgfile_get_entry(cfgfile, section_name, 
"type");
if (case_type == NULL) {
printf("Error: No case type in case %d, the test will 
be finished here.\n",
@@ -523,6 +531,21 @@ main(int argc, char *argv[])
 
printf("Running cases...\n");
for (i = 0; i < case_nb; i++) {
+   if (test_cases[i].is_skip) {
+   printf("Test case %d configured to be skipped.\n\n", i 
+ 1);
+   snprintf(output_str[0], MAX_OUTPUT_STR_LEN, "Skip the 
test-case %d\n",
+i + 1);
+
+   fd = fopen(rst_path_ptr, "a");
+   if (!fd) {
+   printf("Open output CSV file error.\n");
+   return 0;
+   }
+   output_csv(true);
+   fclose(fd);
+   continue;
+   }
+
if (!test_cases[i].is_valid) {
printf("Invalid test case %d.\n\n", i + 1);
snprintf(output_str[0], MAX_OUTPUT_STR_LEN, "Invalid 
case %d\n", i + 1);
diff --git a/app/test-dma-perf/main.h b/app/test-dma-perf/main.h
index 62085e6e8f..32670151af 100644
--- a/app/test-dma-perf/main.h
+++ b/app/test-dma-perf/main.h
@@ -40,6 +40,7 @@ struct lcore_dma_map_t {
 
 struct test_configure {
bool is_valid;
+   bool is_skip;
uint8_t test_type;
const char *test_type_str;
uint16_t src_numa_node;
-- 
2.25.1

[PATCH v7 2/4] app/dma-perf: add PCI device support

[Gowrishankar Muthukrishnan]

From: Amit Prakash Shukla 

Add support to test performance for "device to memory" and
"memory to device" data transfer.

Signed-off-by: Amit Prakash Shukla 
Acked-by: Anoob Joseph 
---
v7:
 - changed cfg->raddr type to uintptr_t to fix 32 bit compilation.
---
 app/test-dma-perf/benchmark.c | 108 +++---
 app/test-dma-perf/config.ini  |  37 
 app/test-dma-perf/main.c  |  67 +
 app/test-dma-perf/main.h  |   6 ++
 4 files changed, 209 insertions(+), 9 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index 9b1f58c78c..eaed224c67 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -127,17 +127,54 @@ cache_flush_buf(__rte_unused struct rte_mbuf **array,
 #endif
 }
 
+static int
+vchan_data_populate(uint32_t dev_id, struct rte_dma_vchan_conf *qconf,
+   struct test_configure *cfg)
+{
+   struct rte_dma_info info;
+
+   qconf->direction = cfg->transfer_dir;
+
+   rte_dma_info_get(dev_id, &info);
+   if (!(RTE_BIT64(qconf->direction) & info.dev_capa))
+   return -1;
+
+   qconf->nb_desc = cfg->ring_size.cur;
+
+   switch (qconf->direction) {
+   case RTE_DMA_DIR_MEM_TO_DEV:
+   qconf->dst_port.pcie.vfen = 1;
+   qconf->dst_port.port_type = RTE_DMA_PORT_PCIE;
+   qconf->dst_port.pcie.coreid = cfg->dcoreid;
+   qconf->dst_port.pcie.vfid = cfg->vfid;
+   qconf->dst_port.pcie.pfid = cfg->pfid;
+   break;
+   case RTE_DMA_DIR_DEV_TO_MEM:
+   qconf->src_port.pcie.vfen = 1;
+   qconf->src_port.port_type = RTE_DMA_PORT_PCIE;
+   qconf->src_port.pcie.coreid = cfg->scoreid;
+   qconf->src_port.pcie.vfid = cfg->vfid;
+   qconf->src_port.pcie.pfid = cfg->pfid;
+   break;
+   case RTE_DMA_DIR_MEM_TO_MEM:
+   case RTE_DMA_DIR_DEV_TO_DEV:
+   break;
+   }
+
+   return 0;
+}
+
 /* Configuration of device. */
 static void
-configure_dmadev_queue(uint32_t dev_id, uint32_t ring_size)
+configure_dmadev_queue(uint32_t dev_id, struct test_configure *cfg)
 {
uint16_t vchan = 0;
struct rte_dma_info info;
struct rte_dma_conf dev_config = { .nb_vchans = 1 };
-   struct rte_dma_vchan_conf qconf = {
-   .direction = RTE_DMA_DIR_MEM_TO_MEM,
-   .nb_desc = ring_size
-   };
+   struct rte_dma_vchan_conf qconf = { 0 };
+
+   if (vchan_data_populate(dev_id, &qconf, cfg) != 0)
+   rte_exit(EXIT_FAILURE, "Error with vchan data populate.\n");
 
if (rte_dma_configure(dev_id, &dev_config) != 0)
rte_exit(EXIT_FAILURE, "Error with dma configure.\n");
@@ -159,7 +196,6 @@ configure_dmadev_queue(uint32_t dev_id, uint32_t ring_size)
 static int
 config_dmadevs(struct test_configure *cfg)
 {
-   uint32_t ring_size = cfg->ring_size.cur;
struct lcore_dma_map_t *ldm = &cfg->lcore_dma_map;
uint32_t nb_workers = ldm->cnt;
uint32_t i;
@@ -176,7 +212,7 @@ config_dmadevs(struct test_configure *cfg)
}
 
ldm->dma_ids[i] = dev_id;
-   configure_dmadev_queue(dev_id, ring_size);
+   configure_dmadev_queue(dev_id, cfg);
++nb_dmadevs;
}
 
@@ -302,13 +338,22 @@ do_cpu_mem_copy(void *p)
return 0;
 }
 
+static void
+dummy_free_ext_buf(void *addr, void *opaque)
+{
+   RTE_SET_USED(addr);
+   RTE_SET_USED(opaque);
+}
+
 static int
 setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
struct rte_mbuf ***dsts)
 {
+   static struct rte_mbuf_ext_shared_info *ext_buf_info;
unsigned int buf_size = cfg->buf_size.cur;
unsigned int nr_sockets;
uint32_t nr_buf = cfg->nr_buf;
+   uint32_t i;
 
nr_sockets = rte_socket_count();
if (cfg->src_numa_node >= nr_sockets ||
@@ -361,16 +406,47 @@ setup_memory_env(struct test_configure *cfg, struct 
rte_mbuf ***srcs,
return -1;
}
 
+   if (cfg->transfer_dir == RTE_DMA_DIR_DEV_TO_MEM ||
+   cfg->transfer_dir == RTE_DMA_DIR_MEM_TO_DEV) {
+   ext_buf_info = rte_malloc(NULL, sizeof(struct 
rte_mbuf_ext_shared_info), 0);
+   if (ext_buf_info == NULL) {
+   printf("Error: ext_buf_info malloc failed.\n");
+   return -1;
+   }
+   }
+
+   if (cfg->transfer_dir == RTE_DMA_DIR_DEV_TO_MEM) {
+   ext_buf_info->free_cb = dummy_free_ext_buf;
+   ext_buf_info->fcb_opaque = NULL;
+   for (i = 0; i < nr_buf; i++) {
+   /* Using mbuf structure to hold remote iova address. */
+   rte_pktmbuf_attach_extbuf((*srcs)[i], (void 
*)cfg->raddr,
+ (rte_iova_t)cfg->rad

[PATCH v7 3/4] app/dma-perf: validate copied memory

[Gowrishankar Muthukrishnan]

Validate copied memory to ensure DMA copy did not fail.

Signed-off-by: Gowrishankar Muthukrishnan 
Acked-by: Anoob Joseph 
---
 app/test-dma-perf/benchmark.c | 21 -
 app/test-dma-perf/main.c  | 16 +++-
 app/test-dma-perf/main.h  |  2 +-
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index eaed224c67..034461da4e 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "main.h"
 
@@ -406,6 +407,11 @@ setup_memory_env(struct test_configure *cfg, struct 
rte_mbuf ***srcs,
return -1;
}
 
+   for (i = 0; i < nr_buf; i++) {
+   memset(rte_pktmbuf_mtod((*srcs)[i], void *), rte_rand(), 
buf_size);
+   memset(rte_pktmbuf_mtod((*dsts)[i], void *), 0, buf_size);
+   }
+
if (cfg->transfer_dir == RTE_DMA_DIR_DEV_TO_MEM ||
cfg->transfer_dir == RTE_DMA_DIR_MEM_TO_DEV) {
ext_buf_info = rte_malloc(NULL, sizeof(struct 
rte_mbuf_ext_shared_info), 0);
@@ -440,7 +446,7 @@ setup_memory_env(struct test_configure *cfg, struct 
rte_mbuf ***srcs,
return 0;
 }
 
-void
+int
 mem_copy_benchmark(struct test_configure *cfg, bool is_dma)
 {
uint32_t i;
@@ -458,6 +464,7 @@ mem_copy_benchmark(struct test_configure *cfg, bool is_dma)
uint32_t avg_cycles_total;
float mops, mops_total;
float bandwidth, bandwidth_total;
+   int ret = 0;
 
if (setup_memory_env(cfg, &srcs, &dsts) < 0)
goto out;
@@ -531,6 +538,16 @@ mem_copy_benchmark(struct test_configure *cfg, bool is_dma)
 
rte_eal_mp_wait_lcore();
 
+   for (i = 0; i < (nr_buf / nb_workers) * nb_workers; i++) {
+   if (memcmp(rte_pktmbuf_mtod(srcs[i], void *),
+  rte_pktmbuf_mtod(dsts[i], void *),
+  cfg->buf_size.cur) != 0) {
+   printf("Copy validation fails for buffer number %d\n", 
i);
+   ret = -1;
+   goto out;
+   }
+   }
+
mops_total = 0;
bandwidth_total = 0;
avg_cycles_total = 0;
@@ -596,4 +613,6 @@ mem_copy_benchmark(struct test_configure *cfg, bool is_dma)
rte_dma_stop(ldm->dma_ids[i]);
}
}
+
+   return ret;
 }
diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 4671ca5335..4dbba255ed 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -87,20 +87,24 @@ output_header(uint32_t case_id, struct test_configure 
*case_cfg)
output_csv(true);
 }
 
-static void
+static int
 run_test_case(struct test_configure *case_cfg)
 {
+   int ret = 0;
+
switch (case_cfg->test_type) {
case TEST_TYPE_DMA_MEM_COPY:
-   mem_copy_benchmark(case_cfg, true);
+   ret = mem_copy_benchmark(case_cfg, true);
break;
case TEST_TYPE_CPU_MEM_COPY:
-   mem_copy_benchmark(case_cfg, false);
+   ret = mem_copy_benchmark(case_cfg, false);
break;
default:
printf("Unknown test type. %s\n", case_cfg->test_type_str);
break;
}
+
+   return ret;
 }
 
 static void
@@ -145,8 +149,10 @@ run_test(uint32_t case_id, struct test_configure *case_cfg)
case_cfg->scenario_id++;
printf("\nRunning scenario %d\n", case_cfg->scenario_id);
 
-   run_test_case(case_cfg);
-   output_csv(false);
+   if (run_test_case(case_cfg) < 0)
+   printf("\nTest fails! skipping this scenario.\n");
+   else
+   output_csv(false);
 
if (var_entry->op == OP_ADD)
var_entry->cur += var_entry->incr;
diff --git a/app/test-dma-perf/main.h b/app/test-dma-perf/main.h
index 8ac3270fba..7dcaa166f2 100644
--- a/app/test-dma-perf/main.h
+++ b/app/test-dma-perf/main.h
@@ -65,6 +65,6 @@ struct test_configure {
uintptr_t raddr;
 };
 
-void mem_copy_benchmark(struct test_configure *cfg, bool is_dma);
+int mem_copy_benchmark(struct test_configure *cfg, bool is_dma);
 
 #endif /* MAIN_H */
-- 
2.25.1

[PATCH v7 4/4] app/dma-perf: add SG copy support

[Gowrishankar Muthukrishnan]

Add SG copy support.

Signed-off-by: Gowrishankar Muthukrishnan 
Acked-by: Anoob Joseph 
---
 app/test-dma-perf/benchmark.c | 274 +-
 app/test-dma-perf/config.ini  |  19 ++-
 app/test-dma-perf/main.c  |  34 -
 app/test-dma-perf/main.h  |   5 +-
 4 files changed, 292 insertions(+), 40 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index 034461da4e..4530bd98ce 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -46,6 +46,10 @@ struct lcore_params {
uint16_t test_secs;
struct rte_mbuf **srcs;
struct rte_mbuf **dsts;
+   struct rte_dma_sge *src_sges;
+   struct rte_dma_sge *dst_sges;
+   uint8_t src_ptrs;
+   uint8_t dst_ptrs;
volatile struct worker_info worker_info;
 };
 
@@ -86,21 +90,31 @@ calc_result(uint32_t buf_size, uint32_t nr_buf, uint16_t 
nb_workers, uint16_t te
 }
 
 static void
-output_result(uint8_t scenario_id, uint32_t lcore_id, char *dma_name, uint16_t 
ring_size,
-   uint16_t kick_batch, uint64_t ave_cycle, uint32_t 
buf_size, uint32_t nr_buf,
-   float memory, float bandwidth, float mops, bool is_dma)
+output_result(struct test_configure *cfg, struct lcore_params *para,
+   uint16_t kick_batch, uint64_t ave_cycle, uint32_t 
buf_size,
+   uint32_t nr_buf, float memory, float bandwidth, float 
mops)
 {
-   if (is_dma)
-   printf("lcore %u, DMA %s, DMA Ring Size: %u, Kick Batch Size: 
%u.\n",
-   lcore_id, dma_name, ring_size, kick_batch);
-   else
+   uint16_t ring_size = cfg->ring_size.cur;
+   uint8_t scenario_id = cfg->scenario_id;
+   uint32_t lcore_id = para->lcore_id;
+   char *dma_name = para->dma_name;
+
+   if (cfg->is_dma) {
+   printf("lcore %u, DMA %s, DMA Ring Size: %u, Kick Batch Size: 
%u", lcore_id,
+  dma_name, ring_size, kick_batch);
+   if (cfg->is_sg)
+   printf(" DMA src ptrs: %u, dst ptrs: %u",
+  para->src_ptrs, para->dst_ptrs);
+   printf(".\n");
+   } else {
printf("lcore %u\n", lcore_id);
+   }
 
printf("Average Cycles/op: %" PRIu64 ", Buffer Size: %u B, Buffer 
Number: %u, Memory: %.2lf MB, Frequency: %.3lf Ghz.\n",
ave_cycle, buf_size, nr_buf, memory, 
rte_get_timer_hz()/10.0);
printf("Average Bandwidth: %.3lf Gbps, MOps: %.3lf\n", bandwidth, mops);
 
-   if (is_dma)
+   if (cfg->is_dma)
snprintf(output_str[lcore_id], MAX_OUTPUT_STR_LEN, 
CSV_LINE_DMA_FMT,
scenario_id, lcore_id, dma_name, ring_size, kick_batch, 
buf_size,
nr_buf, memory, ave_cycle, bandwidth, mops);
@@ -167,7 +181,7 @@ vchan_data_populate(uint32_t dev_id, struct 
rte_dma_vchan_conf *qconf,
 
 /* Configuration of device. */
 static void
-configure_dmadev_queue(uint32_t dev_id, struct test_configure *cfg)
+configure_dmadev_queue(uint32_t dev_id, struct test_configure *cfg, uint8_t 
ptrs_max)
 {
uint16_t vchan = 0;
struct rte_dma_info info;
@@ -190,6 +204,10 @@ configure_dmadev_queue(uint32_t dev_id, struct 
test_configure *cfg)
rte_exit(EXIT_FAILURE, "Error, no configured queues reported on 
device id. %u\n",
dev_id);
 
+   if (info.max_sges < ptrs_max)
+   rte_exit(EXIT_FAILURE, "Error, DMA ptrs more than supported by 
device id %u.\n",
+   dev_id);
+
if (rte_dma_start(dev_id) != 0)
rte_exit(EXIT_FAILURE, "Error with dma start.\n");
 }
@@ -202,8 +220,12 @@ config_dmadevs(struct test_configure *cfg)
uint32_t i;
int dev_id;
uint16_t nb_dmadevs = 0;
+   uint8_t ptrs_max = 0;
char *dma_name;
 
+   if (cfg->is_sg)
+   ptrs_max = RTE_MAX(cfg->src_ptrs, cfg->dst_ptrs);
+
for (i = 0; i < ldm->cnt; i++) {
dma_name = ldm->dma_names[i];
dev_id = rte_dma_get_dev_id_by_name(dma_name);
@@ -213,7 +235,7 @@ config_dmadevs(struct test_configure *cfg)
}
 
ldm->dma_ids[i] = dev_id;
-   configure_dmadev_queue(dev_id, cfg);
+   configure_dmadev_queue(dev_id, cfg, ptrs_max);
++nb_dmadevs;
}
 
@@ -253,7 +275,7 @@ do_dma_submit_and_poll(uint16_t dev_id, uint64_t *async_cnt,
 }
 
 static inline int
-do_dma_mem_copy(void *p)
+do_dma_plain_mem_copy(void *p)
 {
struct lcore_params *para = (struct lcore_params *)p;
volatile struct worker_info *worker_info = &(para->worker_info);
@@ -306,6 +328,65 @@ do_dma_mem_copy(void *p)
return 0;
 }
 
+static inline int
+do_dma_sg_mem_copy(void *p)
+{
+   struct lcore_params *para = (struct lcore_params *)p;
+   

[PATCH v2] crypto/qat: fix block cipher misalignment for AES CBC and 3DES CBC

[Sivaramakrishnan Venkat]

check cipher length alignment for 3DES CBC and AES CBC
to change it to NULL service type for buffer misalignment.

Fixes: def38073ac90 ("crypto/qat: check cipher buffer alignment")
Cc: sta...@dpdk.org

Signed-off-by: Sivaramakrishnan Venkat 
Acked-by: Kai Ji 
---
v2:
Dropped new tests patch.
Removed settings reponse header directly.
Cleared cookie status instead of setting to SUCCESS.
---
 drivers/crypto/qat/dev/qat_crypto_pmd_gens.h | 27 +++-
 drivers/crypto/qat/dev/qat_sym_pmd_gen1.c| 12 -
 drivers/crypto/qat/qat_sym.h |  8 ++
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h 
b/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
index eebf2e6eb8..b8ddf42d6f 100644
--- a/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
+++ b/drivers/crypto/qat/dev/qat_crypto_pmd_gens.h
@@ -618,7 +618,8 @@ static __rte_always_inline void
 enqueue_one_cipher_job_gen1(struct qat_sym_session *ctx,
struct icp_qat_fw_la_bulk_req *req,
struct rte_crypto_va_iova_ptr *iv,
-   union rte_crypto_sym_ofs ofs, uint32_t data_len)
+   union rte_crypto_sym_ofs ofs, uint32_t data_len,
+   struct qat_sym_op_cookie *cookie)
 {
struct icp_qat_fw_la_cipher_req_params *cipher_param;
 
@@ -629,6 +630,15 @@ enqueue_one_cipher_job_gen1(struct qat_sym_session *ctx,
cipher_param->cipher_offset = ofs.ofs.cipher.head;
cipher_param->cipher_length = data_len - ofs.ofs.cipher.head -
ofs.ofs.cipher.tail;
+
+   if (AES_OR_3DES_MISALIGNED) {
+   QAT_LOG(DEBUG,
+ "Input cipher buffer misalignment detected and change job as NULL 
operation");
+   struct icp_qat_fw_comn_req_hdr *header = &req->comn_hdr;
+   header->service_type = ICP_QAT_FW_COMN_REQ_NULL;
+   header->service_cmd_id = ICP_QAT_FW_NULL_REQ_SERV_ID;
+   cookie->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
+   }
 }
 
 static __rte_always_inline void
@@ -685,7 +695,8 @@ enqueue_one_chain_job_gen1(struct qat_sym_session *ctx,
struct rte_crypto_va_iova_ptr *cipher_iv,
struct rte_crypto_va_iova_ptr *digest,
struct rte_crypto_va_iova_ptr *auth_iv,
-   union rte_crypto_sym_ofs ofs, uint32_t data_len)
+   union rte_crypto_sym_ofs ofs, uint32_t data_len,
+   struct qat_sym_op_cookie *cookie)
 {
struct icp_qat_fw_la_cipher_req_params *cipher_param;
struct icp_qat_fw_la_auth_req_params *auth_param;
@@ -722,11 +733,13 @@ enqueue_one_chain_job_gen1(struct qat_sym_session *ctx,
 * error detected.
 */
if (AES_OR_3DES_MISALIGNED) {
-   QAT_LOG(ERR, "Input cipher length alignment error detected.\n");
-   ctx->qat_cipher_alg = ICP_QAT_HW_CIPHER_ALGO_NULL;
-   ctx->qat_hash_alg = ICP_QAT_HW_AUTH_ALGO_NULL;
-   cipher_param->cipher_length = 0;
-   auth_param->auth_len = 0;
+   QAT_LOG(DEBUG,
+ "Input cipher buffer misalignment detected and change job as NULL 
operation");
+   struct icp_qat_fw_comn_req_hdr *header = &req->comn_hdr;
+   header->service_type = ICP_QAT_FW_COMN_REQ_NULL;
+   header->service_cmd_id = ICP_QAT_FW_NULL_REQ_SERV_ID;
+   cookie->status = RTE_CRYPTO_OP_STATUS_INVALID_ARGS;
+   return -1;
}
 
switch (ctx->qat_hash_alg) {
diff --git a/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c 
b/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
index e4bcfa59e7..208b7e0ba6 100644
--- a/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
+++ b/drivers/crypto/qat/dev/qat_sym_pmd_gen1.c
@@ -248,7 +248,7 @@ qat_sym_build_op_cipher_gen1(void *in_op, struct 
qat_sym_session *ctx,
return -EINVAL;
}
 
-   enqueue_one_cipher_job_gen1(ctx, req, &cipher_iv, ofs, total_len);
+   enqueue_one_cipher_job_gen1(ctx, req, &cipher_iv, ofs, total_len, 
op_cookie);
 
qat_sym_debug_log_dump(req, ctx, in_sgl.vec, in_sgl.num, &cipher_iv,
NULL, NULL, NULL);
@@ -383,7 +383,7 @@ qat_sym_build_op_chain_gen1(void *in_op, struct 
qat_sym_session *ctx,
 
enqueue_one_chain_job_gen1(ctx, req, in_sgl.vec, in_sgl.num,
out_sgl.vec, out_sgl.num, &cipher_iv, &digest, &auth_iv,
-   ofs, total_len);
+   ofs, total_len, cookie);
 
qat_sym_debug_log_dump(req, ctx, in_sgl.vec, in_sgl.num, &cipher_iv,
&auth_iv, NULL, &digest);
@@ -507,7 +507,7 @@ qat_sym_dp_enqueue_single_cipher_gen1(void *qp_data, 
uint8_t *drv_ctx,
if (unlikely(data_len < 0))
return -1;
 
-   enqueue_one_cipher_job_gen1(ctx, req, iv, ofs, (uint32_t)data_len);
+   enqueue_one_cipher_job_gen1(ctx, req, iv, ofs, (uint32_t)data_len, 
cookie);
 
qat_sym_debug_log_dump(req, ctx, data, n_data_vecs, iv,
 

Re: [PATCH v3 5/7] Section 5: Appendix

[Bruce Richardson]

On Wed, Nov 15, 2023 at 08:28:55PM -0500, Dave Young wrote:
>Bruce,
>Is the following Linux update correct per your feedback?
>Linux
>-
>To run DPDK applications without root privileges on Linux, perform the
>following steps:
>1. **Create a DPDK User Group**: Create a new user group for DPDK and
>add the desired user to this group.
>2. **Set Up Hugepages**: Configure hugepages for the user.
>3. **Bind the NIC to a User-Space Driver**: Use the DPDK tool
>``dpdk-devbind.py`` to bind the NIC to a user-space driver like
>``vfio-pci`` or ``igb_uio``

These three steps don't need to be covered in the docs. We already
described elsewhere about configuring hugepages and binding the device.
Also, to run as non-root, I would expect there to be a user a/c already
available on the system that the deployer of the app wants to use.

>4. **Adjust Permissions for Specific Files and Directories**:
> 
>   - VFIO entries in ``/dev``, such as ``/dev/vfio/``, where 
>is the VFIO group to which a device used by DPDK belongs.
>   - The hugepage mount directory, typically ``/dev/hugepages`` on many
>distributions, or any alternative mount point configured by the user,
>e.g., ``/mnt/huge``, ``/mnt/huge_1G``.
>   Note: Running DPDK as non-root on Linux requires IOMMU support
>through vfio.

This note needs to be explicitly called out to the top of the instructions.

The list of files to change the permissions on is key. The action -
changing permissions - is common between Linux and BSD, but the specific
files to adjust will be different.

>5. **Run the DPDK Application**: Run the desired DPDK application as
>the user who has been added to the DPDK group.
>FreeBSD
>---
>- The userspace-io device files in ``/dev``, for example,
>``/dev/uio0``, ``/dev/uio1``, and so on
>- The userspace contiguous memory device: ``/dev/contigmem``

Regards,
/Bruce

[RFC 0/3] Detect superfluous newline in logs

[David Marchand]

Getting readable and consistent logs is important when running a DPDK
application, especially when troubleshooting.
A common issue with logs is when a DPDK change do not add (or on the
contrary add too many \n) in the format string.

This issue would only get noticed when actually hitting this log (which
may be something difficult to do).

This series proposes to introduce a new RTE_LOG helper that is
responsible for logging a one line message and spews a build error (with
gcc) if any \n is part of the format string.


Note:
- the first patch is intentionnally sent as a single block: splitting it
  into per library commits with correct Fixes: tags is a tedious work.
  I would split it for a non RFC series. For now, it is enough to show
  case the idea.
- the last patch shows how an existing log macro is converted,


-- 
David Marchand

David Marchand (3):
  lib: remove redundant newline from logs
  log: add a per line log helper
  lib: use per line logging

 drivers/crypto/ipsec_mb/ipsec_mb_ops.c |   2 +-
 lib/bbdev/rte_bbdev.c  |   9 +-
 lib/cfgfile/rte_cfgfile.c  |  18 ++--
 lib/compressdev/rte_compressdev_internal.h |   5 +-
 lib/compressdev/rte_compressdev_pmd.c  |   4 +-
 lib/cryptodev/rte_cryptodev.c  |   5 +-
 lib/cryptodev/rte_cryptodev.h  |  16 ++--
 lib/dispatcher/rte_dispatcher.c|  12 +--
 lib/dmadev/rte_dmadev.c|   8 +-
 lib/eventdev/eventdev_pmd.h|  14 +--
 lib/eventdev/rte_event_crypto_adapter.c|  12 +--
 lib/eventdev/rte_event_dma_adapter.c   |  18 ++--
 lib/eventdev/rte_event_eth_rx_adapter.c|  30 +++---
 lib/eventdev/rte_event_eth_tx_adapter.c|   2 +-
 lib/eventdev/rte_event_timer_adapter.c |  21 +++--
 lib/eventdev/rte_eventdev.c|  10 +-
 lib/gpudev/gpudev.c|   6 +-
 lib/graph/graph_private.h  |   5 +-
 lib/log/rte_log.h  |  21 +
 lib/metrics/rte_metrics_telemetry.c|   6 +-
 lib/mldev/rte_mldev.c  | 102 ++---
 lib/mldev/rte_mldev.h  |   5 +-
 lib/net/rte_net_crc.c  |  14 +--
 lib/node/ethdev_rx.c   |   4 +-
 lib/node/ip4_lookup.c  |   2 +-
 lib/node/ip6_lookup.c  |   2 +-
 lib/node/kernel_rx.c   |   8 +-
 lib/node/kernel_tx.c   |   4 +-
 lib/node/node_private.h|   6 +-
 lib/rawdev/rte_rawdev_pmd.h|   4 +-
 lib/rcu/rte_rcu_qsbr.c |   4 +-
 lib/rcu/rte_rcu_qsbr.h |  17 ++--
 lib/stack/rte_stack.c  |   8 +-
 lib/stack/stack_pvt.h  |   4 +-
 34 files changed, 220 insertions(+), 188 deletions(-)

-- 
2.41.0

[RFC 1/3] lib: remove redundant newline from logs

[David Marchand]

Fix places where two newline characters may be logged.
Also fix some direct calls to printf or RTE_LOG when a dedicated log
helper for a library existed.

Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
---
 drivers/crypto/ipsec_mb/ipsec_mb_ops.c  |   2 +-
 lib/bbdev/rte_bbdev.c   |   6 +-
 lib/cfgfile/rte_cfgfile.c   |  14 ++--
 lib/compressdev/rte_compressdev_pmd.c   |   4 +-
 lib/cryptodev/rte_cryptodev.c   |   5 +-
 lib/dispatcher/rte_dispatcher.c |  12 +--
 lib/dmadev/rte_dmadev.c |   2 +-
 lib/eventdev/eventdev_pmd.h |   6 +-
 lib/eventdev/rte_event_crypto_adapter.c |  12 +--
 lib/eventdev/rte_event_dma_adapter.c|  18 ++---
 lib/eventdev/rte_event_eth_rx_adapter.c |  30 +++
 lib/eventdev/rte_event_eth_tx_adapter.c |   2 +-
 lib/eventdev/rte_event_timer_adapter.c  |   4 +-
 lib/eventdev/rte_eventdev.c |  10 +--
 lib/metrics/rte_metrics_telemetry.c |   2 +-
 lib/mldev/rte_mldev.c   | 102 
 lib/net/rte_net_crc.c   |   6 +-
 lib/node/ethdev_rx.c|   4 +-
 lib/node/ip4_lookup.c   |   2 +-
 lib/node/ip6_lookup.c   |   2 +-
 lib/node/kernel_rx.c|   8 +-
 lib/node/kernel_tx.c|   4 +-
 lib/rcu/rte_rcu_qsbr.c  |   4 +-
 lib/rcu/rte_rcu_qsbr.h  |   8 +-
 lib/stack/rte_stack.c   |   8 +-
 25 files changed, 138 insertions(+), 139 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c 
b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
index 30f919cd40..2a5599b7d8 100644
--- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
+++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
@@ -406,7 +406,7 @@ ipsec_mb_ipc_request(const struct rte_mp_msg *mp_msg, const 
void *peer)
resp_param->result = ipsec_mb_qp_release(dev, qp_id);
break;
default:
-   CDEV_LOG_ERR("invalid mp request type\n");
+   CDEV_LOG_ERR("invalid mp request type");
}
 
 out:
diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
index cfebea09c7..e09bb97abb 100644
--- a/lib/bbdev/rte_bbdev.c
+++ b/lib/bbdev/rte_bbdev.c
@@ -1106,12 +1106,12 @@ rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t 
queue_id, int epfd, int op,
 
intr_handle = dev->intr_handle;
if (intr_handle == NULL) {
-   rte_bbdev_log(ERR, "Device %u intr handle unset\n", dev_id);
+   rte_bbdev_log(ERR, "Device %u intr handle unset", dev_id);
return -ENOTSUP;
}
 
if (queue_id >= RTE_MAX_RXTX_INTR_VEC_ID) {
-   rte_bbdev_log(ERR, "Device %u queue_id %u is too big\n",
+   rte_bbdev_log(ERR, "Device %u queue_id %u is too big",
dev_id, queue_id);
return -ENOTSUP;
}
@@ -1120,7 +1120,7 @@ rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t 
queue_id, int epfd, int op,
ret = rte_intr_rx_ctl(intr_handle, epfd, op, vec, data);
if (ret && (ret != -EEXIST)) {
rte_bbdev_log(ERR,
-   "dev %u q %u int ctl error op %d epfd %d vec 
%u\n",
+   "dev %u q %u int ctl error op %d epfd %d vec 
%u",
dev_id, queue_id, op, epfd, vec);
return ret;
}
diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
index eefba6e408..2f9cc0722a 100644
--- a/lib/cfgfile/rte_cfgfile.c
+++ b/lib/cfgfile/rte_cfgfile.c
@@ -137,7 +137,7 @@ rte_cfgfile_check_params(const struct 
rte_cfgfile_parameters *params)
unsigned int i;
 
if (!params) {
-   CFG_LOG(ERR, "missing cfgfile parameters\n");
+   CFG_LOG(ERR, "missing cfgfile parameters");
return -EINVAL;
}
 
@@ -150,7 +150,7 @@ rte_cfgfile_check_params(const struct 
rte_cfgfile_parameters *params)
}
 
if (valid_comment == 0) {
-   CFG_LOG(ERR, "invalid comment characters %c\n",
+   CFG_LOG(ERR, "invalid comment characters %c",
   params->comment_character);
return -ENOTSUP;
}
@@ -188,7 +188,7 @@ rte_cfgfile_load_with_params(const char *filename, int 
flags,
lineno++;
if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
CFG_LOG(ERR, " line %d - no \\n found on string. "
-   "Check if line too long\n", lineno);
+   "Check if line too long", lineno);
goto error1;
}
/* skip parsing if comment character found */
@@ -209,7 +209,7 @@ rte_cfgfile_load_with_params(const char *filename, int 
flags,
char *end = memchr(buffer, ']', len);
if (end == NULL) {
 

[RFC 2/3] log: add a per line log helper

[David Marchand]

gcc builtin __builtin_strchr can be used as a static assertion to check
whether passed format strings contain a \n.
This can be useful to detect double \n in log messages.

Signed-off-by: David Marchand 
---
 lib/log/rte_log.h | 21 +
 1 file changed, 21 insertions(+)

diff --git a/lib/log/rte_log.h b/lib/log/rte_log.h
index f7a8405de9..d6db699a07 100644
--- a/lib/log/rte_log.h
+++ b/lib/log/rte_log.h
@@ -17,6 +17,7 @@
 extern "C" {
 #endif
 
+#include 
 #include 
 #include 
 #include 
@@ -358,6 +359,26 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char 
*format, va_list ap)
 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) : \
 0)
 
+#ifdef RTE_TOOLCHAIN_GCC
+#define RTE_LOG_CHECK_NO_NEWLINE(fmt) \
+   static_assert(!__builtin_strchr(fmt, '\n'), \
+   "This log format string contains a \\n")
+#else
+#define RTE_LOG_CHECK_NO_NEWLINE(...)
+#endif
+
+#define RTE_LOG_LINE(l, t, ...) do { \
+   RTE_LOG_CHECK_NO_NEWLINE(RTE_FMT_HEAD(__VA_ARGS__,)); \
+   RTE_LOG(l, t, RTE_FMT(RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_FMT_TAIL(__VA_ARGS__ ,))); \
+} while (0)
+
+#define RTE_LOG_DP_LINE(l, t, ...) do { \
+   RTE_LOG_CHECK_NO_NEWLINE(RTE_FMT_HEAD(__VA_ARGS__,)); \
+   RTE_LOG_DP(l, t, RTE_FMT(RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_FMT_TAIL(__VA_ARGS__ ,))); \
+} while (0)
+
 #define RTE_LOG_REGISTER_IMPL(type, name, level)   \
 int type;  \
 RTE_INIT(__##type) \
-- 
2.41.0

[RFC 3/3] lib: use per line logging

[David Marchand]

Use RTE_LOG_LINE in existing macros that append a \n.

Signed-off-by: David Marchand 
---
 lib/bbdev/rte_bbdev.c  |  3 ++-
 lib/cfgfile/rte_cfgfile.c  |  4 ++--
 lib/compressdev/rte_compressdev_internal.h |  5 +++--
 lib/cryptodev/rte_cryptodev.h  | 16 
 lib/dmadev/rte_dmadev.c|  6 --
 lib/eventdev/eventdev_pmd.h|  8 
 lib/eventdev/rte_event_timer_adapter.c | 17 ++---
 lib/gpudev/gpudev.c|  6 --
 lib/graph/graph_private.h  |  5 +++--
 lib/metrics/rte_metrics_telemetry.c|  4 ++--
 lib/mldev/rte_mldev.h  |  5 +++--
 lib/net/rte_net_crc.c  |  8 
 lib/node/node_private.h|  6 --
 lib/rawdev/rte_rawdev_pmd.h|  4 ++--
 lib/rcu/rte_rcu_qsbr.h |  9 -
 lib/stack/stack_pvt.h  |  4 ++--
 16 files changed, 61 insertions(+), 49 deletions(-)

diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
index e09bb97abb..a61aa599aa 100644
--- a/lib/bbdev/rte_bbdev.c
+++ b/lib/bbdev/rte_bbdev.c
@@ -28,10 +28,11 @@
 
 /* BBDev library logging ID */
 RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE);
+#define RTE_LOGTYPE_BBDEV bbdev_logtype
 
 /* Helper macro for logging */
 #define rte_bbdev_log(level, fmt, ...) \
-   rte_log(RTE_LOG_ ## level, bbdev_logtype, fmt "\n", ##__VA_ARGS__)
+   RTE_LOG_LINE(level, BBDEV, fmt , ##__VA_ARGS__)
 
 #define rte_bbdev_log_debug(fmt, ...) \
rte_bbdev_log(DEBUG, RTE_STR(__LINE__) ":%s() " fmt, __func__, \
diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
index 2f9cc0722a..6a5e4fd942 100644
--- a/lib/cfgfile/rte_cfgfile.c
+++ b/lib/cfgfile/rte_cfgfile.c
@@ -29,10 +29,10 @@ struct rte_cfgfile {
 
 /* Setting up dynamic logging 8< */
 RTE_LOG_REGISTER_DEFAULT(cfgfile_logtype, INFO);
+#define RTE_LOGTYPE_CFGFILE cfgfile_logtype
 
 #define CFG_LOG(level, fmt, args...)   \
-   rte_log(RTE_LOG_ ## level, cfgfile_logtype, "%s(): " fmt "\n",  \
-   __func__, ## args)
+   RTE_LOG_LINE(level, CFGFILE, "%s(): " fmt, __func__, ## args)
 /* >8 End of setting up dynamic logging */
 
 /** when we resize a file structure, how many extra entries
diff --git a/lib/compressdev/rte_compressdev_internal.h 
b/lib/compressdev/rte_compressdev_internal.h
index b3b193e3ee..34d6d95649 100644
--- a/lib/compressdev/rte_compressdev_internal.h
+++ b/lib/compressdev/rte_compressdev_internal.h
@@ -21,9 +21,10 @@ extern "C" {
 
 /* Logging Macros */
 extern int compressdev_logtype;
+#define RTE_LOGTYPE_COMPRESSDEV compressdev_logtype
+
 #define COMPRESSDEV_LOG(level, fmt, args...) \
-   rte_log(RTE_LOG_ ## level, compressdev_logtype, "%s(): " fmt "\n", \
-   __func__, ##args)
+   RTE_LOG_LINE(level, COMPRESSDEV, "%s(): " fmt, __func__, ##args)
 
 /**
  * Dequeue processed packets from queue pair of a device.
diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index aaeaf294e6..5131d2d947 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -31,23 +31,23 @@ extern const char **rte_cyptodev_names;
 /* Logging Macros */
 
 #define CDEV_LOG_ERR(...) \
-   RTE_LOG(ERR, CRYPTODEV, \
-   RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_LOG_LINE(ERR, CRYPTODEV, \
+   RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,), \
__func__, __LINE__, RTE_FMT_TAIL(__VA_ARGS__,)))
 
 #define CDEV_LOG_INFO(...) \
-   RTE_LOG(INFO, CRYPTODEV, \
-   RTE_FMT(RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_LOG_LINE(INFO, CRYPTODEV, \
+   RTE_FMT(RTE_FMT_HEAD(__VA_ARGS__,), \
RTE_FMT_TAIL(__VA_ARGS__,)))
 
 #define CDEV_LOG_DEBUG(...) \
-   RTE_LOG(DEBUG, CRYPTODEV, \
-   RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_LOG_LINE(DEBUG, CRYPTODEV, \
+   RTE_FMT("%s() line %u: " RTE_FMT_HEAD(__VA_ARGS__,), \
__func__, __LINE__, RTE_FMT_TAIL(__VA_ARGS__,)))
 
 #define CDEV_PMD_TRACE(...) \
-   RTE_LOG(DEBUG, CRYPTODEV, \
-   RTE_FMT("[%s] %s: " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
+   RTE_LOG_LINE(DEBUG, CRYPTODEV, \
+   RTE_FMT("[%s] %s: " RTE_FMT_HEAD(__VA_ARGS__,), \
dev, __func__, RTE_FMT_TAIL(__VA_ARGS__,)))
 
 /**
diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 009a21849a..c1a166858c 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -32,9 +32,11 @@ static struct {
 } *dma_devices_shared_data;
 
 RTE_LOG_REGISTER_DEFAULT(rte_dma_logtype, INFO);
+#define RTE_LOGTYPE_DMA rte_dma_logtype
+
 #define RTE_DMA_LOG(level, ...) \
-   rte_log(RTE_LOG_ ## level, rte_dma_logtype, RTE_FMT("dma: " \

Re: [RFC 0/3] Detect superfluous newline in logs

[Bruce Richardson]

On Fri, Nov 17, 2023 at 02:18:21PM +0100, David Marchand wrote:
> Getting readable and consistent logs is important when running a DPDK
> application, especially when troubleshooting.
> A common issue with logs is when a DPDK change do not add (or on the
> contrary add too many \n) in the format string.
> 
> This issue would only get noticed when actually hitting this log (which
> may be something difficult to do).
> 
> This series proposes to introduce a new RTE_LOG helper that is
> responsible for logging a one line message and spews a build error (with
> gcc) if any \n is part of the format string.
> 
> 
> Note:
> - the first patch is intentionnally sent as a single block: splitting it
>   into per library commits with correct Fixes: tags is a tedious work.
>   I would split it for a non RFC series. For now, it is enough to show
>   case the idea.
> - the last patch shows how an existing log macro is converted,
> 
> 
very nice. I definitely think this should be implemented for 24.03

Thanks,
/Bruce

PS: I'm not even sure that the first patch needs to be split. I think it's
fairly clear as-is.

RE: [RFC 0/3] Detect superfluous newline in logs

[Morten Brørup]

> From: David Marchand [mailto:david.march...@redhat.com]
> Sent: Friday, 17 November 2023 14.18
> 
> Getting readable and consistent logs is important when running a DPDK
> application, especially when troubleshooting.
> A common issue with logs is when a DPDK change do not add (or on the
> contrary add too many \n) in the format string.
> 
> This issue would only get noticed when actually hitting this log (which
> may be something difficult to do).
> 
> This series proposes to introduce a new RTE_LOG helper that is
> responsible for logging a one line message and spews a build error
> (with gcc) if any \n is part of the format string.
> 

The new helper's name is RTE_LOG_LINE, not RTE_LOG.

As far as I can see, RTE_LOG continues working as before - allowing one line of 
log message to span multiple lines of RTE_LOG() calls with \n in the last of 
them. Which is good.

Anyway, I like the concept. And it solves a real problem.

If you want other names, e.g. RTE_LOG for a complete line (appending \n in the 
macro itself), and RTE_LOG_PART (or similar) for an incomplete line, I wouldn't 
object. But that would probably break the API.

> 
> Note:
> - the first patch is intentionnally sent as a single block: splitting
> it
>   into per library commits with correct Fixes: tags is a tedious work.
>   I would split it for a non RFC series. For now, it is enough to show
>   case the idea.
> - the last patch shows how an existing log macro is converted,

Re: [RFC 0/3] Detect superfluous newline in logs

[David Marchand]

On Fri, Nov 17, 2023 at 2:27 PM Bruce Richardson
 wrote:
>
> On Fri, Nov 17, 2023 at 02:18:21PM +0100, David Marchand wrote:
> > Getting readable and consistent logs is important when running a DPDK
> > application, especially when troubleshooting.
> > A common issue with logs is when a DPDK change do not add (or on the
> > contrary add too many \n) in the format string.
> >
> > This issue would only get noticed when actually hitting this log (which
> > may be something difficult to do).
> >
> > This series proposes to introduce a new RTE_LOG helper that is
> > responsible for logging a one line message and spews a build error (with
> > gcc) if any \n is part of the format string.
> >
> >
> > Note:
> > - the first patch is intentionnally sent as a single block: splitting it
> >   into per library commits with correct Fixes: tags is a tedious work.
> >   I would split it for a non RFC series. For now, it is enough to show
> >   case the idea.
> > - the last patch shows how an existing log macro is converted,
> >
> >
> very nice. I definitely think this should be implemented for 24.03

I am still wondering how this idea should evolve...

Some points I have in mind but for which I am not sure what is the best.

Some log helpers were exposed to applications and had no explicit
requirement wrt \n.
Applications may have used those helpers with multiline messages.
So maybe existing *exposed* helpers should be left untouched... and a
new helper would need to be introduced.
IOW with an example, cryptodev (missing a RTE_ prefix) CDEV_LOG_ERR
macro is publicly exposed.
A CDEV_LOG_LINE_ERR may be needed to avoid breaking external users.


There are a lot of other log macros that let it to the callers to add
a trailing \n.
Should we convert them?
Converting the *whole* DPDK code to the new helper (with some
exceptions for people who like multiline logs..) would be nice to
close this topic once and for all.
But it would likely be a nightmare for later fixes that contain logs
and which could introduce regressions in such logs if the backport
does not take care of re-adding a \n.


-- 
David Marchand

Re: [RFC 0/3] Detect superfluous newline in logs

[David Marchand]

On Fri, Nov 17, 2023 at 2:47 PM Morten Brørup  
wrote:
>
> > From: David Marchand [mailto:david.march...@redhat.com]
> > Sent: Friday, 17 November 2023 14.18
> >
> > Getting readable and consistent logs is important when running a DPDK
> > application, especially when troubleshooting.
> > A common issue with logs is when a DPDK change do not add (or on the
> > contrary add too many \n) in the format string.
> >
> > This issue would only get noticed when actually hitting this log (which
> > may be something difficult to do).
> >
> > This series proposes to introduce a new RTE_LOG helper that is
> > responsible for logging a one line message and spews a build error
> > (with gcc) if any \n is part of the format string.
> >
>
> The new helper's name is RTE_LOG_LINE, not RTE_LOG.

Sorry, wrong completion.

>
> As far as I can see, RTE_LOG continues working as before - allowing one line 
> of log message to span multiple lines of RTE_LOG() calls with \n in the last 
> of them. Which is good.

Indeed, we can't break / change RTE_LOG api.
This API is too old, that would be a nightmare.

>
> Anyway, I like the concept. And it solves a real problem.
>
> If you want other names, e.g. RTE_LOG for a complete line (appending \n in 
> the macro itself), and RTE_LOG_PART (or similar) for an incomplete line, I 
> wouldn't object. But that would probably break the API.

No API breaking allowed :-).


-- 
David Marchand

Re: [RFC 0/3] Detect superfluous newline in logs

[Bruce Richardson]

On Fri, Nov 17, 2023 at 02:48:25PM +0100, David Marchand wrote:
> On Fri, Nov 17, 2023 at 2:27 PM Bruce Richardson
>  wrote:
> >
> > On Fri, Nov 17, 2023 at 02:18:21PM +0100, David Marchand wrote:
> > > Getting readable and consistent logs is important when running a DPDK
> > > application, especially when troubleshooting.
> > > A common issue with logs is when a DPDK change do not add (or on the
> > > contrary add too many \n) in the format string.
> > >
> > > This issue would only get noticed when actually hitting this log (which
> > > may be something difficult to do).
> > >
> > > This series proposes to introduce a new RTE_LOG helper that is
> > > responsible for logging a one line message and spews a build error (with
> > > gcc) if any \n is part of the format string.
> > >
> > >
> > > Note:
> > > - the first patch is intentionnally sent as a single block: splitting it
> > >   into per library commits with correct Fixes: tags is a tedious work.
> > >   I would split it for a non RFC series. For now, it is enough to show
> > >   case the idea.
> > > - the last patch shows how an existing log macro is converted,
> > >
> > >
> > very nice. I definitely think this should be implemented for 24.03
> 
> I am still wondering how this idea should evolve...
> 
> Some points I have in mind but for which I am not sure what is the best.
> 
> Some log helpers were exposed to applications and had no explicit
> requirement wrt \n.
> Applications may have used those helpers with multiline messages.
> So maybe existing *exposed* helpers should be left untouched... and a
> new helper would need to be introduced.

And the existing helpers deprecated. Internal log helpers should not be
exposed to apps.

> IOW with an example, cryptodev (missing a RTE_ prefix) CDEV_LOG_ERR
> macro is publicly exposed.
> A CDEV_LOG_LINE_ERR may be needed to avoid breaking external users.
> 

RTE_CDEV_LOG_ERR should be available, though, right? :-)

> 
> There are a lot of other log macros that let it to the callers to add
> a trailing \n.
> Should we convert them?

I would think that, ideally, yes we should.

> Converting the *whole* DPDK code to the new helper (with some
> exceptions for people who like multiline logs..) would be nice to
> close this topic once and for all.
> But it would likely be a nightmare for later fixes that contain logs
> and which could introduce regressions in such logs if the backport
> does not take care of re-adding a \n.

Good point. I wonder how many backports add new logs, or modify existing
ones? Are there any automated checks, or a checklist for backports to
enable us to catch these sort of things?
Naming could be a problem, but we could look to move over existing logs by
using new log macro names, which should help us to catch these. Even if the
log names are a bit awkward, if they are kept internal-only, it shouldn't
matter much. [Plus we would always be free to do a mass-rename back to the
original name in future, once the backport issues are reduced].

/Bruce

RE: [RFC 0/3] Detect superfluous newline in logs

[Morten Brørup]

> From: David Marchand [mailto:david.march...@redhat.com]
> Sent: Friday, 17 November 2023 15.10
> 
> On Fri, Nov 17, 2023 at 2:47 PM Morten Brørup
>  wrote:
> >
> > > From: David Marchand [mailto:david.march...@redhat.com]
> > > Sent: Friday, 17 November 2023 14.18
> > >
> > > Getting readable and consistent logs is important when running a
> DPDK
> > > application, especially when troubleshooting.
> > > A common issue with logs is when a DPDK change do not add (or on
> the
> > > contrary add too many \n) in the format string.
> > >
> > > This issue would only get noticed when actually hitting this log
> (which
> > > may be something difficult to do).
> > >
> > > This series proposes to introduce a new RTE_LOG helper that is
> > > responsible for logging a one line message and spews a build error
> > > (with gcc) if any \n is part of the format string.
> > >
> >
> > The new helper's name is RTE_LOG_LINE, not RTE_LOG.
> 
> Sorry, wrong completion.
> 
> >
> > As far as I can see, RTE_LOG continues working as before - allowing
> one line of log message to span multiple lines of RTE_LOG() calls with
> \n in the last of them. Which is good.
> 
> Indeed, we can't break / change RTE_LOG api.
> This API is too old, that would be a nightmare.
> 
> >
> > Anyway, I like the concept. And it solves a real problem.
> >
> > If you want other names, e.g. RTE_LOG for a complete line (appending
> \n in the macro itself), and RTE_LOG_PART (or similar) for an
> incomplete line, I wouldn't object. But that would probably break the
> API.
> 
> No API breaking allowed :-).

OK, then:

Series-acked-by: Morten Brørup 

Re: [RFC 0/3] Detect superfluous newline in logs

[David Marchand]

On Fri, Nov 17, 2023 at 3:11 PM Bruce Richardson
 wrote:
>
> On Fri, Nov 17, 2023 at 02:48:25PM +0100, David Marchand wrote:
> > On Fri, Nov 17, 2023 at 2:27 PM Bruce Richardson
> >  wrote:
> > >
> > > On Fri, Nov 17, 2023 at 02:18:21PM +0100, David Marchand wrote:
> > > > Getting readable and consistent logs is important when running a DPDK
> > > > application, especially when troubleshooting.
> > > > A common issue with logs is when a DPDK change do not add (or on the
> > > > contrary add too many \n) in the format string.
> > > >
> > > > This issue would only get noticed when actually hitting this log (which
> > > > may be something difficult to do).
> > > >
> > > > This series proposes to introduce a new RTE_LOG helper that is
> > > > responsible for logging a one line message and spews a build error (with
> > > > gcc) if any \n is part of the format string.
> > > >
> > > >
> > > > Note:
> > > > - the first patch is intentionnally sent as a single block: splitting it
> > > >   into per library commits with correct Fixes: tags is a tedious work.
> > > >   I would split it for a non RFC series. For now, it is enough to show
> > > >   case the idea.
> > > > - the last patch shows how an existing log macro is converted,
> > > >
> > > >
> > > very nice. I definitely think this should be implemented for 24.03
> >
> > I am still wondering how this idea should evolve...
> >
> > Some points I have in mind but for which I am not sure what is the best.
> >
> > Some log helpers were exposed to applications and had no explicit
> > requirement wrt \n.
> > Applications may have used those helpers with multiline messages.
> > So maybe existing *exposed* helpers should be left untouched... and a
> > new helper would need to be introduced.
>
> And the existing helpers deprecated. Internal log helpers should not be
> exposed to apps.

I agree.


>
> > IOW with an example, cryptodev (missing a RTE_ prefix) CDEV_LOG_ERR
> > macro is publicly exposed.
> > A CDEV_LOG_LINE_ERR may be needed to avoid breaking external users.
> >
>
> RTE_CDEV_LOG_ERR should be available, though, right? :-)

Err, yes, but if we make it private, we don't need the RTE_ prefix ? :-)

>
> >
> > There are a lot of other log macros that let it to the callers to add
> > a trailing \n.
> > Should we convert them?
>
> I would think that, ideally, yes we should.
>
> > Converting the *whole* DPDK code to the new helper (with some
> > exceptions for people who like multiline logs..) would be nice to
> > close this topic once and for all.
> > But it would likely be a nightmare for later fixes that contain logs
> > and which could introduce regressions in such logs if the backport
> > does not take care of re-adding a \n.
>
> Good point. I wonder how many backports add new logs, or modify existing
> ones? Are there any automated checks, or a checklist for backports to

Touching logs in backport happens often afaics.

(to simplify I added a vXX.11.0 tag pointing at vXX.11)

$ for i in $(seq 0 4); do echo v21.11.${i}..v21.11.$((i + 1)) $(git
diff v21.11.${i}..v21.11.$((i + 1)) | grep -c ^\+.*LOG); done
v21.11.0..v21.11.1 122
v21.11.1..v21.11.2 40
v21.11.2..v21.11.3 43
v21.11.3..v21.11.4 30
v21.11.4..v21.11.5 24

$ for i in $(seq 0 2); do echo v22.11.${i}..v22.11.$((i + 1)) $(git
diff v22.11.${i}..v22.11.$((i + 1)) | grep -c ^\+.*LOG); done
v22.11.0..v22.11.1 0
v22.11.1..v22.11.2 37
v22.11.2..v22.11.3 106

I don't think there are checks on this topic (and to be fair, I don't
see which check could be done).


> enable us to catch these sort of things?
> Naming could be a problem, but we could look to move over existing logs by
> using new log macro names, which should help us to catch these. Even if the
> log names are a bit awkward, if they are kept internal-only, it shouldn't
> matter much. [Plus we would always be free to do a mass-rename back to the
> original name in future, once the backport issues are reduced].

This is the way I had in mind.


-- 
David Marchand

RE: [PATCH v2] crypto/qat: fix block cipher misalignment for AES CBC and 3DES CBC

[Power, Ciara]

> -Original Message-
> From: Sivaramakrishnan Venkat 
> Sent: Friday, November 17, 2023 12:38 PM
> To: dev@dpdk.org
> Cc: sta...@dpdk.org; Ji, Kai ; gak...@marvell.com;
> Sivaramakrishnan, VenkatX 
> Subject: [PATCH v2] crypto/qat: fix block cipher misalignment for AES CBC and
> 3DES CBC
> 
> check cipher length alignment for 3DES CBC and AES CBC to change it to NULL
> service type for buffer misalignment.
> 
> Fixes: def38073ac90 ("crypto/qat: check cipher buffer alignment")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Sivaramakrishnan Venkat
> 
> Acked-by: Kai Ji 
> ---
> v2:
> Dropped new tests patch.
> Removed settings reponse header directly.
> Cleared cookie status instead of setting to SUCCESS.

Acked-by: Ciara Power 

[PATCH v3 00/10] replace uses of zero length array

[Stephen Hemminger]

Zero length arrays are a GNU extension that has been
superseded by flex arrays.

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

These are places found by cocci/zero_length_array.cocci

Series-acked-by: Morten Brørup 

v3 - fix misuse of flex array in dpaxx found by clang

v2 - rebased on 23.11-rc3
 more places started using zero length array

Stephen Hemminger (10):
  member: replace zero length array with flex array
  cryptodev: replace zero length array with flex array
  security: replace zero length array with flex array
  pipeline: replace zero length array with flex array
  net/nfp: replace zero length array with flex array
  net/enic: replace zero length array with flex array
  net/mlx5: replace zero length array with flex array
  pdcp: replace zero length array with flex array
  net/cpfl: replace zero length array with flex array
  common/dpaxx: remove zero length array

 drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++
 drivers/common/mlx5/mlx5_prm.h | 2 +-
 drivers/net/cpfl/cpfl_flow_engine_fxp.c| 2 +-
 drivers/net/enic/base/vnic_devcmd.h| 2 +-
 drivers/net/mlx5/mlx5.h| 4 ++--
 drivers/net/mlx5/mlx5_flow.h   | 2 +-
 drivers/net/mlx5/mlx5_tx.h | 2 +-
 drivers/net/nfp/flower/nfp_flower_cmsg.h   | 2 +-
 lib/cryptodev/cryptodev_pmd.h  | 2 +-
 lib/member/rte_member_heap.h   | 2 +-
 lib/pdcp/pdcp_entity.h | 2 +-
 lib/pipeline/rte_swx_pipeline_internal.h   | 2 +-
 lib/security/rte_security_driver.h | 2 +-
 13 files changed, 15 insertions(+), 17 deletions(-)

-- 
2.42.0

[PATCH v3 01/10] member: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 lib/member/rte_member_heap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/member/rte_member_heap.h b/lib/member/rte_member_heap.h
index 9c4a01aebe95..ab6319bc2de4 100644
--- a/lib/member/rte_member_heap.h
+++ b/lib/member/rte_member_heap.h
@@ -26,7 +26,7 @@ struct hash {
uint16_t bkt_cnt;
uint16_t num_item;
uint32_t seed;
-   struct hash_bkt buckets[0];
+   struct hash_bkt buckets[];
 };
 
 struct node {
-- 
2.42.0

[PATCH v3 02/10] cryptodev: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 lib/cryptodev/cryptodev_pmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
index 3bb3d95c1338..0732b356883c 100644
--- a/lib/cryptodev/cryptodev_pmd.h
+++ b/lib/cryptodev/cryptodev_pmd.h
@@ -153,7 +153,7 @@ struct rte_cryptodev_sym_session {
 
RTE_MARKER cacheline1 __rte_cache_min_aligned;
/**< Second cache line - start of the driver session data */
-   uint8_t driver_priv_data[0];
+   uint8_t driver_priv_data[];
/**< Driver specific session data, variable size */
 };
 
-- 
2.42.0

[PATCH v3 03/10] security: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 lib/security/rte_security_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/security/rte_security_driver.h 
b/lib/security/rte_security_driver.h
index 62664dacdbb4..faa4074f1965 100644
--- a/lib/security/rte_security_driver.h
+++ b/lib/security/rte_security_driver.h
@@ -33,7 +33,7 @@ struct rte_security_session {
/**< session private data IOVA address */
 
RTE_MARKER cacheline1 __rte_cache_min_aligned;
-   uint8_t driver_priv_data[0];
+   uint8_t driver_priv_data[];
/**< Private session material, variable size (depends on driver) */
 };
 
-- 
2.42.0

[PATCH v3 04/10] pipeline: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 lib/pipeline/rte_swx_pipeline_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pipeline/rte_swx_pipeline_internal.h 
b/lib/pipeline/rte_swx_pipeline_internal.h
index a67b6e965de7..8ec12263b989 100644
--- a/lib/pipeline/rte_swx_pipeline_internal.h
+++ b/lib/pipeline/rte_swx_pipeline_internal.h
@@ -213,7 +213,7 @@ TAILQ_HEAD(rss_tailq, rss);
 
 struct rss_runtime {
uint32_t key_size; /* key size in bytes. */
-   uint8_t key[0]; /* key. */
+   uint8_t key[]; /* key. */
 };
 
 /*
-- 
2.42.0

[PATCH v3 05/10] net/nfp: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Acked-by: Niklas Söderlund 
Reviewed-by: Tyler Retzlaff 
---
 drivers/net/nfp/flower/nfp_flower_cmsg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/flower/nfp_flower_cmsg.h 
b/drivers/net/nfp/flower/nfp_flower_cmsg.h
index c2938fb6f63c..f00d2e838d8f 100644
--- a/drivers/net/nfp/flower/nfp_flower_cmsg.h
+++ b/drivers/net/nfp/flower/nfp_flower_cmsg.h
@@ -73,7 +73,7 @@ struct nfp_flower_cmsg_mac_repr {
uint8_t info;
uint8_t nbi_port;
uint8_t phys_port;
-   } ports[0];
+   } ports[];
 };
 
 /*
-- 
2.42.0

[PATCH v3 06/10] net/enic: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Acked-by: John Daley 
Reviewed-by: Tyler Retzlaff 
---
 drivers/net/enic/base/vnic_devcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/enic/base/vnic_devcmd.h 
b/drivers/net/enic/base/vnic_devcmd.h
index 253a791c3f65..f91cc3078d63 100644
--- a/drivers/net/enic/base/vnic_devcmd.h
+++ b/drivers/net/enic/base/vnic_devcmd.h
@@ -765,7 +765,7 @@ struct vnic_devcmd_notify {
 struct vnic_devcmd_provinfo {
uint8_t oui[3];
uint8_t type;
-   uint8_t data[0];
+   uint8_t data[];
 };
 
 /*
-- 
2.42.0

[PATCH v3 07/10] net/mlx5: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 drivers/common/mlx5/mlx5_prm.h | 2 +-
 drivers/net/mlx5/mlx5.h| 4 ++--
 drivers/net/mlx5/mlx5_flow.h   | 2 +-
 drivers/net/mlx5/mlx5_tx.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
index 9e22dce6da13..932b89bd79d3 100644
--- a/drivers/common/mlx5/mlx5_prm.h
+++ b/drivers/common/mlx5/mlx5_prm.h
@@ -5181,7 +5181,7 @@ struct mlx5_ifc_flow_context_bits {
u8 reserved_at_e0[0x40];
u8 encrypt_decrypt_obj_id[0x20];
u8 reserved_at_140[0x16c0];
-   union mlx5_ifc_dest_format_flow_counter_list_auto_bits destination[0];
+   union mlx5_ifc_dest_format_flow_counter_list_auto_bits destination[];
 };
 
 struct mlx5_ifc_set_fte_in_bits {
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index f0d63a0ba5f5..89d13900fd3c 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -1308,7 +1308,7 @@ struct mlx5_aso_ct_pool {
};
struct mlx5_aso_sq *sq; /* Async ASO SQ. */
struct mlx5_aso_sq *shared_sq; /* Shared ASO SQ. */
-   struct mlx5_aso_ct_action actions[0];
+   struct mlx5_aso_ct_action actions[];
/* CT action structures bulk. */
 };
 
@@ -1325,7 +1325,7 @@ struct mlx5_aso_ct_pools_mng {
rte_spinlock_t ct_sl; /* The ASO CT free list lock. */
rte_rwlock_t resize_rwl; /* The ASO CT pool resize lock. */
struct aso_ct_list free_cts; /* Free ASO CT objects list. */
-   struct mlx5_aso_sq aso_sqs[0]; /* ASO queue objects. */
+   struct mlx5_aso_sq aso_sqs[]; /* ASO queue objects. */
 };
 
 #ifdef PEDANTIC
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 6dde9de688b9..b35079b30a6e 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -1257,7 +1257,7 @@ struct rte_flow_hw {
cnt_id_t cnt_id;
uint32_t mtr_id;
uint32_t rule_idx;
-   uint8_t rule[0]; /* HWS layer data struct. */
+   uint8_t rule[]; /* HWS layer data struct. */
 } __rte_packed;
 
 #ifdef PEDANTIC
diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
index e59ce37667ba..2045e5174e6d 100644
--- a/drivers/net/mlx5/mlx5_tx.h
+++ b/drivers/net/mlx5/mlx5_tx.h
@@ -171,7 +171,7 @@ struct mlx5_txq_data {
struct mlx5_txq_stats stats; /* TX queue counters. */
struct mlx5_txq_stats stats_reset; /* stats on last reset. */
struct mlx5_uar_data uar_data;
-   struct rte_mbuf *elts[0];
+   struct rte_mbuf *elts[];
/* Storage for queued packets, must be the last field. */
 } __rte_cache_aligned;
 
-- 
2.42.0

[PATCH v3 08/10] pdcp: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
Acked-by: Anoob Joseph 
---
 lib/pdcp/pdcp_entity.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pdcp/pdcp_entity.h b/lib/pdcp/pdcp_entity.h
index 4fc6342a5ced..f854192e98dc 100644
--- a/lib/pdcp/pdcp_entity.h
+++ b/lib/pdcp/pdcp_entity.h
@@ -185,7 +185,7 @@ struct entity_priv_dl_part {
/** Reorder packet buffer */
struct pdcp_reorder reorder;
/** Bitmap memory region */
-   uint8_t bitmap_mem[0];
+   uint8_t bitmap_mem[];
 };
 
 struct entity_priv_ul_part {
-- 
2.42.0

[PATCH v3 09/10] net/cpfl: replace zero length array with flex array

[Stephen Hemminger]

Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
Reviewed-by: Tyler Retzlaff 
---
 drivers/net/cpfl/cpfl_flow_engine_fxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/cpfl/cpfl_flow_engine_fxp.c 
b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
index 8a4e1419b4db..39a281fa61ee 100644
--- a/drivers/net/cpfl/cpfl_flow_engine_fxp.c
+++ b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
@@ -53,7 +53,7 @@ struct cpfl_rule_info_meta {
uint32_t pr_num;/* number of pattern rules */
uint32_t mr_num;/* number of modification rules 
*/
uint32_t rule_num;  /* number of all rules */
-   struct cpfl_rule_info rules[0];
+   struct cpfl_rule_info rules[];
 };
 
 static uint32_t cpfl_fxp_mod_idx_alloc(struct cpfl_adapter_ext *ad);
-- 
2.42.0

[PATCH v3 10/10] common/dpaxx: remove zero length array

[Stephen Hemminger]

There is a place holder zero length array in this driver.
But since the structure is embedded in other structures,
it could not have been safely used anyway.
There doesn't appear to be any uses of it in the current code.

Signed-off-by: Stephen Hemminger 
---
 drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h 
b/drivers/common/dpaax/caamflib/desc/ipsec.h
index 95fc3ea5ba3b..9d59b93292f9 100644
--- a/drivers/common/dpaax/caamflib/desc/ipsec.h
+++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
@@ -336,7 +336,6 @@ struct ipsec_encap_gcm {
  * @ip_hdr_len: optional IP Header length (in bytes)
  *  reserved - 16b
  *  Opt. IP Hdr Len - 16b
- * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
  */
 struct ipsec_encap_pdb {
uint32_t options;
@@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
};
uint32_t spi;
uint32_t ip_hdr_len;
-   uint8_t ip_hdr[0];
 };
 
 static inline unsigned int
@@ -776,7 +774,7 @@ cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool ps, bool 
swap,
PROGRAM_SET_36BIT_ADDR(p);
phdr = SHR_HDR(p, share, hdr, 0);
__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-   COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
SET_LABEL(p, hdr);
pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
if (authdata->keylen)
@@ -913,7 +911,7 @@ cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t *descbuf,
PROGRAM_CNTXT_INIT(p, descbuf, 0);
phdr = SHR_HDR(p, share, hdr, 0);
__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-   COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
SET_LABEL(p, hdr);
pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD | SELF);
/*
-- 
2.42.0

Re: [PATCH] app/test-pmd: fix L4 checksum with padding data

[Stephen Hemminger]

On Fri, 17 Nov 2023 09:29:41 +
Ferruh Yigit  wrote:

> >> I agree using 'l3_len' instead is correct fix.
> >>
> >> But this requires ABI/API change,
> >> plus do we have any reason to keep the padding, discarding it as this
> >> patch does is also simpler alternative.  
> > 
> > 
> > Possibly an API version to change the args would work to fix.
> >  
> 
> rte_ipv4_udptcp_cksum_mbuf() and rte_ipv6_udptcp_cksum_mbuf() are inline
> functions, unfortunately we can't version them.
> 
> But those functions already gets IP header as parameter, can't we use IP
> header to get the payload size? If so this can be fixed without updating
> API.

Inlines are easier. Just make a fixed new function and make sure the old
one is not used.  They shouldn't have been inline in the first place.

Re: [PATCH] app/test-pmd: fix L4 checksum with padding data

[Stephen Hemminger]

On Fri, 17 Nov 2023 13:11:50 +0100
Morten Brørup  wrote:

> > rte_ipv4_udptcp_cksum_mbuf() and rte_ipv6_udptcp_cksum_mbuf() are
> > inline
> > functions, unfortunately we can't version them.
> > 
> > But those functions already gets IP header as parameter, can't we use
> > IP
> > header to get the payload size? If so this can be fixed without
> > updating
> > API.  
> 
> If rte_ipv4_udptcp_cksum_mbuf() - or any other function in the DPDK Network 
> Headers library - includes Ethernet padding (which may be non-zero) when 
> calculating the TCP/UDP checksum of an IPv4 packet, it is a bug, and must be 
> fixed there.
> 
> Our test cases should use random padding to catch bugs like this.
> 
> And I just realized that Ethernet padding may be added to any IP packet, so 
> don't assume that this bug only applies to small packets.

Agree. And test code needs lots more header checks it is way too trusting that 
mbuf is valid.

[PATCH v7 1/5] pdump: fix setting rte_errno on mp error

[Stephen Hemminger]

The response from MP server sets err_value to negative
on error. The convention for rte_errno is to use a positive
value on error. This makes errors like duplicate registration
show up with the correct error value.

Fixes: 660098d61f57 ("pdump: use generic multi-process channel")
Signed-off-by: Stephen Hemminger 
Acked-by: Morten Brørup 
---
 lib/pdump/rte_pdump.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
index 80b90c6f7d03..e94f49e21250 100644
--- a/lib/pdump/rte_pdump.c
+++ b/lib/pdump/rte_pdump.c
@@ -564,9 +564,10 @@ pdump_prepare_client_request(const char *device, uint16_t 
queue,
if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0) {
mp_rep = &mp_reply.msgs[0];
resp = (struct pdump_response *)mp_rep->param;
-   rte_errno = resp->err_value;
-   if (!resp->err_value)
+   if (resp->err_value == 0)
ret = 0;
+   else
+   rte_errno = -resp->err_value;
free(mp_reply.msgs);
}
 
-- 
2.42.0

[PATCH v7 0/5] dumpcap and pcapng fixes

[Stephen Hemminger]

It fixes issues related to timestamping. The design choices are
to maximize performance in the primary process; and do
all the time adjustment in the secondary (dumpcap) since
the dumpcap needs to system calls anyway to write the result.

This patches set changes where the adjustment is calculated
into the pcapng portion that opens the output file.
All details of the format of timestamp are contained inside
pcapng (data hiding).

v7 - no change, rebase there were some apply failures by CI
v6 - make sure all steps compile
v5 - fix format of getpid in capture name
v4 - incorporate review feedback
v3 - don't use alloca() since can have VLA type issues

Stephen Hemminger (5):
  pdump: fix setting rte_errno on mp error
  dumpcap: allow multiple invocations
  pcapng: modify timestamp calculation
  pcapng: avoid using alloca()
  test: cleanups to pcapng test

 app/dumpcap/main.c  |  53 ++---
 app/test/meson.build|   2 +-
 app/test/test_pcapng.c  | 417 +++-
 lib/graph/graph_pcap.c  |   2 +-
 lib/pcapng/rte_pcapng.c | 156 ++-
 lib/pcapng/rte_pcapng.h |  19 +-
 lib/pdump/rte_pdump.c   |   9 +-
 7 files changed, 373 insertions(+), 285 deletions(-)

-- 
2.42.0

[PATCH v7 2/5] dumpcap: allow multiple invocations

[Stephen Hemminger]

If dumpcap is run twice with each instance pointing a different
interface, it would fail because of overlap in ring a pool names.
Fix by putting process id in the name.

It is still not allowed to do multiple invocations on the same
interface because only one callback is allowed and only one copy
of mbuf is done. Dumpcap will fail with error in this case:

   pdump_prepare_client_request(): client request for pdump enable/disable 
failed
   EAL: Error - exiting with code: 1
 Cause: Packet dump enable on 0:net_null0 failed File exists

Fixes: cbb44143be74 ("app/dumpcap: add new packet capture application")
Reported-by: Isaac Boukris 
Signed-off-by: Stephen Hemminger 
---
 app/dumpcap/main.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 4f581bd341d8..d05dddac0071 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -44,7 +44,6 @@
 #include 
 #include 
 
-#define RING_NAME "capture-ring"
 #define MONITOR_INTERVAL  (500 * 1000)
 #define MBUF_POOL_CACHE_SIZE 32
 #define BURST_SIZE 32
@@ -647,6 +646,7 @@ static void dpdk_init(void)
 static struct rte_ring *create_ring(void)
 {
struct rte_ring *ring;
+   char ring_name[RTE_RING_NAMESIZE];
size_t size, log2;
 
/* Find next power of 2 >= size. */
@@ -660,28 +660,28 @@ static struct rte_ring *create_ring(void)
ring_size = size;
}
 
-   ring = rte_ring_lookup(RING_NAME);
-   if (ring == NULL) {
-   ring = rte_ring_create(RING_NAME, ring_size,
-   rte_socket_id(), 0);
-   if (ring == NULL)
-   rte_exit(EXIT_FAILURE, "Could not create ring :%s\n",
-rte_strerror(rte_errno));
-   }
+   /* Want one ring per invocation of program */
+   snprintf(ring_name, sizeof(ring_name),
+"dumpcap-%d", getpid());
+
+   ring = rte_ring_create(ring_name, ring_size,
+  rte_socket_id(), 0);
+   if (ring == NULL)
+   rte_exit(EXIT_FAILURE, "Could not create ring :%s\n",
+rte_strerror(rte_errno));
+
return ring;
 }
 
 static struct rte_mempool *create_mempool(void)
 {
const struct interface *intf;
-   static const char pool_name[] = "capture_mbufs";
+   char pool_name[RTE_MEMPOOL_NAMESIZE];
size_t num_mbufs = 2 * ring_size;
struct rte_mempool *mp;
uint32_t data_size = 128;
 
-   mp = rte_mempool_lookup(pool_name);
-   if (mp)
-   return mp;
+   snprintf(pool_name, sizeof(pool_name), "capture_%d", getpid());
 
/* Common pool so size mbuf for biggest snap length */
TAILQ_FOREACH(intf, &interfaces, next) {
@@ -826,7 +826,7 @@ static void enable_pdump(struct rte_ring *r, struct 
rte_mempool *mp)
rte_exit(EXIT_FAILURE,
"Packet dump enable on %u:%s failed %s\n",
intf->port, intf->name,
-   rte_strerror(-ret));
+   rte_strerror(rte_errno));
}
 
if (intf->opts.promisc_mode) {
-- 
2.42.0

[PATCH v7 3/5] pcapng: modify timestamp calculation

[Stephen Hemminger]

The computation of timestamp is best done in the part of
pcapng library that is in secondary process.
The secondary process is already doing a bunch of system
calls which makes it not performance sensitive.
This does change the rte_pcapng_copy()
and rte_pcapng_write_stats() experimental API's.

Simplify the computation of nanoseconds from TSC to a two
step process which avoids numeric overflow issues. The previous
code was not thread safe as well.

Fixes: c882eb544842 ("pcapng: fix timestamp wrapping in output files")
Signed-off-by: Stephen Hemminger 
Acked-by: Morten Brørup 
---
 app/dumpcap/main.c  |  25 +++--
 app/test/test_pcapng.c  |   7 +--
 lib/graph/graph_pcap.c  |   2 +-
 lib/pcapng/rte_pcapng.c | 119 +++-
 lib/pcapng/rte_pcapng.h |  19 ++-
 lib/pdump/rte_pdump.c   |   4 +-
 6 files changed, 62 insertions(+), 114 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index d05dddac0071..fc28e2d7027a 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -66,13 +66,13 @@ static bool print_stats;
 
 /* capture limit options */
 static struct {
-   uint64_t  duration; /* nanoseconds */
+   time_t  duration;   /* seconds */
unsigned long packets;  /* number of packets in file */
size_t size;/* file size (bytes) */
 } stop;
 
 /* Running state */
-static uint64_t start_time, end_time;
+static time_t start_time;
 static uint64_t packets_received;
 static size_t file_size;
 
@@ -197,7 +197,7 @@ static void auto_stop(char *opt)
if (*value == '\0' || *endp != '\0' || interval <= 0)
rte_exit(EXIT_FAILURE,
 "Invalid duration \"%s\"\n", value);
-   stop.duration = NSEC_PER_SEC * interval;
+   stop.duration = interval;
} else if (strcmp(opt, "filesize") == 0) {
stop.size = get_uint(value, "filesize", 0) * 1024;
} else if (strcmp(opt, "packets") == 0) {
@@ -511,15 +511,6 @@ static void statistics_loop(void)
}
 }
 
-/* Return the time since 1/1/1970 in nanoseconds */
-static uint64_t create_timestamp(void)
-{
-   struct timespec now;
-
-   clock_gettime(CLOCK_MONOTONIC, &now);
-   return rte_timespec_to_ns(&now);
-}
-
 static void
 cleanup_pdump_resources(void)
 {
@@ -589,9 +580,8 @@ report_packet_stats(dumpcap_out_t out)
ifdrop = pdump_stats.nombuf + pdump_stats.ringfull;
 
if (use_pcapng)
-   rte_pcapng_write_stats(out.pcapng, intf->port, NULL,
-  start_time, end_time,
-  ifrecv, ifdrop);
+   rte_pcapng_write_stats(out.pcapng, intf->port,
+  ifrecv, ifdrop, NULL);
 
if (ifrecv == 0)
percent = 0;
@@ -983,7 +973,7 @@ int main(int argc, char **argv)
mp = create_mempool();
out = create_output();
 
-   start_time = create_timestamp();
+   start_time = time(NULL);
enable_pdump(r, mp);
 
if (!quiet) {
@@ -1005,11 +995,10 @@ int main(int argc, char **argv)
break;
 
if (stop.duration != 0 &&
-   create_timestamp() - start_time > stop.duration)
+   time(NULL) - start_time > stop.duration)
break;
}
 
-   end_time = create_timestamp();
disable_primary_monitor();
 
if (rte_eal_primary_proc_alive(NULL))
diff --git a/app/test/test_pcapng.c b/app/test/test_pcapng.c
index b8429a02f160..21131dfa0c5e 100644
--- a/app/test/test_pcapng.c
+++ b/app/test/test_pcapng.c
@@ -146,7 +146,7 @@ test_write_packets(void)
struct rte_mbuf *mc;
 
mc = rte_pcapng_copy(port_id, 0, orig, mp, pkt_len,
-   rte_get_tsc_cycles(), 0, NULL);
+RTE_PCAPNG_DIRECTION_UNKNOWN, NULL);
if (mc == NULL) {
fprintf(stderr, "Cannot copy packet\n");
return -1;
@@ -174,8 +174,7 @@ test_write_stats(void)
 
/* write a statistics block */
len = rte_pcapng_write_stats(pcapng, port_id,
-NULL, 0, 0,
-NUM_PACKETS, 0);
+UINT64_MAX, UINT64_MAX, NULL);
if (len <= 0) {
fprintf(stderr, "Write of statistics failed\n");
return -1;
@@ -262,7 +261,7 @@ test_write_over_limit_iov_max(void)
struct rte_mbuf *mc;
 
mc = rte_pcapng_copy(port_id, 0, orig, mp, pkt_len,
-   rte_get_tsc_cycles(), 0, NULL);
+RTE_PCAPNG_DIRECTION_UNKNOWN, NULL);
if (mc == NULL) {
fprintf(stderr, "Cannot copy

[PATCH v7 4/5] pcapng: avoid using alloca()

[Stephen Hemminger]

The function alloca() like VLA's has problems if the caller
passes a large value. Instead use a fixed size buffer (2K)
which will be more than sufficient for the info related blocks
in the file. Add bounds checks as well.

Signed-off-by: Stephen Hemminger 
Acked-by: Morten Brørup 
---
 lib/pcapng/rte_pcapng.c | 37 -
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
index 13fd2b97fb80..f74ec939a9f8 100644
--- a/lib/pcapng/rte_pcapng.c
+++ b/lib/pcapng/rte_pcapng.c
@@ -33,6 +33,9 @@
 /* conversion from DPDK speed to PCAPNG */
 #define PCAPNG_MBPS_SPEED 100ull
 
+/* upper bound for section, stats and interface blocks */
+#define PCAPNG_BLKSIZ  2048
+
 /* Format of the capture file handle */
 struct rte_pcapng {
int  outfd; /* output file */
@@ -140,9 +143,8 @@ pcapng_section_block(rte_pcapng_t *self,
 {
struct pcapng_section_header *hdr;
struct pcapng_option *opt;
-   void *buf;
+   uint8_t buf[PCAPNG_BLKSIZ];
uint32_t len;
-   ssize_t cc;
 
len = sizeof(*hdr);
if (hw)
@@ -158,8 +160,7 @@ pcapng_section_block(rte_pcapng_t *self,
len += pcapng_optlen(0);
len += sizeof(uint32_t);
 
-   buf = calloc(1, len);
-   if (!buf)
+   if (len > sizeof(buf))
return -1;
 
hdr = (struct pcapng_section_header *)buf;
@@ -193,10 +194,7 @@ pcapng_section_block(rte_pcapng_t *self,
/* clone block_length after option */
memcpy(opt, &hdr->block_length, sizeof(uint32_t));
 
-   cc = write(self->outfd, buf, len);
-   free(buf);
-
-   return cc;
+   return write(self->outfd, buf, len);
 }
 
 /* Write an interface block for a DPDK port */
@@ -213,7 +211,7 @@ rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
struct pcapng_option *opt;
const uint8_t tsresol = 9;  /* nanosecond resolution */
uint32_t len;
-   void *buf;
+   uint8_t buf[PCAPNG_BLKSIZ];
char ifname_buf[IF_NAMESIZE];
char ifhw[256];
uint64_t speed = 0;
@@ -267,8 +265,7 @@ rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
len += pcapng_optlen(0);
len += sizeof(uint32_t);
 
-   buf = alloca(len);
-   if (!buf)
+   if (len > sizeof(buf))
return -1;
 
hdr = (struct pcapng_interface_block *)buf;
@@ -296,17 +293,16 @@ rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t 
port,
opt = pcapng_add_option(opt, PCAPNG_IFB_HARDWARE,
 ifhw, strlen(ifhw));
if (filter) {
-   /* Encoding is that the first octet indicates string vs BPF */
size_t len;
-   char *buf;
 
len = strlen(filter) + 1;
-   buf = alloca(len);
-   *buf = '\0';
-   memcpy(buf + 1, filter, len);
+   opt->code = PCAPNG_IFB_FILTER;
+   opt->length = len;
+   /* Encoding is that the first octet indicates string vs BPF */
+   opt->data[0] = 0;
+   memcpy(opt->data + 1, filter, strlen(filter));
 
-   opt = pcapng_add_option(opt, PCAPNG_IFB_FILTER,
-   buf, len);
+   opt = (struct pcapng_option *)((uint8_t *)opt + 
pcapng_optlen(len));
}
 
opt = pcapng_add_option(opt, PCAPNG_OPT_END, NULL, 0);
@@ -333,7 +329,7 @@ rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
uint64_t start_time = self->offset_ns;
uint64_t sample_time;
uint32_t optlen, len;
-   uint8_t *buf;
+   uint8_t buf[PCAPNG_BLKSIZ];
 
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -353,8 +349,7 @@ rte_pcapng_write_stats(rte_pcapng_t *self, uint16_t port_id,
optlen += pcapng_optlen(0);
 
len = sizeof(*hdr) + optlen + sizeof(uint32_t);
-   buf = alloca(len);
-   if (buf == NULL)
+   if (len > sizeof(buf))
return -1;
 
hdr = (struct pcapng_statistics *)buf;
-- 
2.42.0

[PATCH v7 5/5] test: cleanups to pcapng test

[Stephen Hemminger]

Overhaul of the pcapng test:
  - promote it to be a fast test so it gets regularly run.
  - create null device and use i.
  - use UDP discard packets that are valid so that for debugging
the resulting pcapng file can be looked at with wireshark.
  - do basic checks on resulting pcap file that lengths and
timestamps are in range.
  - add test for interface options

Signed-off-by: Stephen Hemminger 
---
 app/test/meson.build   |   2 +-
 app/test/test_pcapng.c | 416 +++--
 2 files changed, 281 insertions(+), 137 deletions(-)

diff --git a/app/test/meson.build b/app/test/meson.build
index 4183d66b0e9c..dcc93f4a43b4 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -128,7 +128,7 @@ source_file_deps = {
 'test_metrics.c': ['metrics'],
 'test_mp_secondary.c': ['hash', 'lpm'],
 'test_net_ether.c': ['net'],
-'test_pcapng.c': ['ethdev', 'net', 'pcapng'],
+'test_pcapng.c': ['ethdev', 'net', 'pcapng', 'bus_vdev'],
 'test_pdcp.c': ['eventdev', 'pdcp', 'net', 'timer', 'security'],
 'test_pdump.c': ['pdump'] + sample_packet_forward_deps,
 'test_per_lcore.c': [],
diff --git a/app/test/test_pcapng.c b/app/test/test_pcapng.c
index 21131dfa0c5e..89535efad096 100644
--- a/app/test/test_pcapng.c
+++ b/app/test/test_pcapng.c
@@ -6,25 +6,34 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 
 #include 
 
 #include "test.h"
 
-#define NUM_PACKETS10
-#define DUMMY_MBUF_NUM 3
+#define PCAPNG_TEST_DEBUG 0
+
+#define TOTAL_PACKETS  4096
+#define MAX_BURST  64
+#define MAX_GAP_US 10
+#define DUMMY_MBUF_NUM 3
 
-static rte_pcapng_t *pcapng;
 static struct rte_mempool *mp;
 static const uint32_t pkt_len = 200;
 static uint16_t port_id;
-static char file_name[] = "/tmp/pcapng_test_XX.pcapng";
+static const char null_dev[] = "net_null0";
 
 /* first mbuf in the packet, should always be at offset 0 */
 struct dummy_mbuf {
@@ -61,6 +70,7 @@ mbuf1_prepare(struct dummy_mbuf *dm, uint32_t plen)
struct {
struct rte_ether_hdr eth;
struct rte_ipv4_hdr ip;
+   struct rte_udp_hdr udp;
} pkt = {
.eth = {
.dst_addr.addr_bytes = "\xff\xff\xff\xff\xff\xff",
@@ -68,148 +78,200 @@ mbuf1_prepare(struct dummy_mbuf *dm, uint32_t plen)
},
.ip = {
.version_ihl = RTE_IPV4_VHL_DEF,
-   .total_length = rte_cpu_to_be_16(plen),
-   .time_to_live = IPDEFTTL,
-   .next_proto_id = IPPROTO_RAW,
+   .time_to_live = 1,
+   .next_proto_id = IPPROTO_UDP,
.src_addr = rte_cpu_to_be_32(RTE_IPV4_LOOPBACK),
.dst_addr = rte_cpu_to_be_32(RTE_IPV4_BROADCAST),
-   }
+   },
+   .udp = {
+   .dst_port = rte_cpu_to_be_16(9), /* Discard port */
+   },
};
 
memset(dm, 0, sizeof(*dm));
dummy_mbuf_prep(&dm->mb[0], dm->buf[0], sizeof(dm->buf[0]), plen);
 
rte_eth_random_addr(pkt.eth.src_addr.addr_bytes);
-   memcpy(rte_pktmbuf_mtod(dm->mb, void *), &pkt, RTE_MIN(sizeof(pkt), 
plen));
+   plen -= sizeof(struct rte_ether_hdr);
+
+   pkt.ip.total_length = rte_cpu_to_be_16(plen);
+   pkt.ip.hdr_checksum = rte_ipv4_cksum(&pkt.ip);
+
+   plen -= sizeof(struct rte_ipv4_hdr);
+   pkt.udp.src_port = rte_rand();
+   pkt.udp.dgram_len = rte_cpu_to_be_16(plen);
+
+   memcpy(rte_pktmbuf_mtod(dm->mb, void *), &pkt, sizeof(pkt));
 }
 
 static int
 test_setup(void)
 {
-   int tmp_fd;
-
-   port_id = rte_eth_find_next(0);
-   if (port_id >= RTE_MAX_ETHPORTS) {
-   fprintf(stderr, "No valid Ether port\n");
-   return -1;
-   }
+   port_id = rte_eth_dev_count_avail();
 
-   tmp_fd = mkstemps(file_name, strlen(".pcapng"));
-   if (tmp_fd == -1) {
-   perror("mkstemps() failure");
-   return -1;
-   }
-   printf("pcapng: output file %s\n", file_name);
-
-   /* open a test capture file */
-   pcapng = rte_pcapng_fdopen(tmp_fd, NULL, NULL, "pcapng_test", NULL);
-   if (pcapng == NULL) {
-   fprintf(stderr, "rte_pcapng_fdopen failed\n");
-   close(tmp_fd);
-   return -1;
-   }
-
-   /* Add interface to the file */
-   if (rte_pcapng_add_interface(pcapng, port_id,
-NULL, NULL, NULL) != 0) {
-   fprintf(stderr, "can not add port %u\n", port_id);
-   return -1;
+   /* Make a dummy null device to snoop on */
+   if (rte_vdev_init(null_dev, NULL) != 0) {
+   fprintf(stderr, "Failed to create vdev '%s'\n", null_dev);
+   goto fail;
}
 

Re: [PATCH v3 7/7] dts: allow configuring MTU of ports

[Jeremy Spewock]

Both good points, I'll be sure to add/adjust those docstrings.

On Thu, Nov 16, 2023 at 1:05 PM Juraj Linkeš 
wrote:

> On Mon, Nov 13, 2023 at 9:28 PM  wrote:
> >
> > From: Jeremy Spewock 
> >
> > Adds methods in both os_session and linux session to allow for setting
> > MTU of port interfaces in an OS agnostic way.
> >
> > Signed-off-by: Jeremy Spewock 
> > ---
> >  dts/framework/remote_session/linux_session.py | 7 +++
> >  dts/framework/remote_session/os_session.py| 9 +
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/dts/framework/remote_session/linux_session.py
> b/dts/framework/remote_session/linux_session.py
> > index a3f1a6bf3b..dab68d41b1 100644
> > --- a/dts/framework/remote_session/linux_session.py
> > +++ b/dts/framework/remote_session/linux_session.py
> > @@ -196,6 +196,13 @@ def configure_port_ip_address(
> >  verify=True,
> >  )
> >
> > +def configure_port_mtu(self, mtu: int, port: Port) -> None:
>
> This is missing a docstring.
> The way I've decided to document these overridden abstract methods is
> to just to link to the superclass's method, used heavily in
>
> https://patches.dpdk.org/project/dpdk/patch/20231115130959.39420-17-juraj.lin...@pantheon.tech/
> :
> """Overrides :meth:`~.os_session.OSSession.configure_port_mtu`."""
>
> The docstring checker complains if the docstring is missing. There may
> be better ways, such as with @typing.override (but that requires
> Python 3.12 or typing_extensions, but there's a bug in Pylama that
> prevents us from using that solution:
> https://github.com/klen/pylama/pull/247).
>
> > +self.send_command(
> > +f"ip link set dev {port.logical_name} mtu {mtu}",
> > +privileged=True,
> > +verify=True,
> > +)
> > +
> >  def configure_ipv4_forwarding(self, enable: bool) -> None:
> >  state = 1 if enable else 0
> >  self.send_command(f"sysctl -w net.ipv4.ip_forward={state}",
> privileged=True)
> > diff --git a/dts/framework/remote_session/os_session.py
> b/dts/framework/remote_session/os_session.py
> > index 8a709eac1c..c038f78b79 100644
> > --- a/dts/framework/remote_session/os_session.py
> > +++ b/dts/framework/remote_session/os_session.py
> > @@ -277,6 +277,15 @@ def configure_port_ip_address(
> >  Configure (add or delete) an IP address of the input port.
> >  """
> >
> > +@abstractmethod
> > +def configure_port_mtu(self, mtu: int, port: Port) -> None:
> > +"""Configure MTU on a given port.
> > +
> > +Args:
> > +mtu: Desired MTU value.
> > +port: Port to set the MTU on.
> > +"""
>
> I've compiled the rules for composing docstrings here:
>
> https://patches.dpdk.org/project/dpdk/patch/20231115130959.39420-4-juraj.lin...@pantheon.tech/
> .
>
> The relevant part here is:
>
> When referencing a parameter of a function or a method in their
> docstring, don't use
> any articles and put the parameter into single backticks. This mimics
> the style of
> `Python's documentation `_.
>
> Both the mtu and the port parameters are mentioned, so they should be
> without articles and in backticks.
>
> > +
> >  @abstractmethod
> >  def configure_ipv4_forwarding(self, enable: bool) -> None:
> >  """
> > --
> > 2.42.0
> >
>

Re: [PATCH v3 6/7] dts: add pci addresses to EAL parameters

[Jeremy Spewock]

On Thu, Nov 16, 2023 at 1:10 PM Juraj Linkeš 
wrote:

> On Mon, Nov 13, 2023 at 9:28 PM  wrote:
> >
> > From: Jeremy Spewock 
> >
> > Added allow list to the EAL parameters created in DTS to ensure that
> > only the relevant PCI devices are considered when launching DPDK
> > applications.
> >
> > Signed-off-by: Jeremy Spewock 
> > ---
> >  dts/framework/testbed_model/sut_node.py | 9 +
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> > index bcac939e72..f9c7bd9bf3 100644
> > --- a/dts/framework/testbed_model/sut_node.py
> > +++ b/dts/framework/testbed_model/sut_node.py
> > @@ -20,6 +20,7 @@
> >  from framework.utils import MesonArgs
> >
> >  from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice
> > +from .hw.port import Port
> >  from .node import Node
> >
> >
> > @@ -31,6 +32,7 @@ def __init__(
> >  prefix: str,
> >  no_pci: bool,
> >  vdevs: list[VirtualDevice],
> > +ports: list[Port],
>
> Please add this to the docstring.
>
> This overlaps with the docstrings patch. I guess we'll need to modify
> one of the patches when the other one gets merged.
>

Right, this would probably get a little weird with the conflict if one gets
merged before the other. Another option could be copying your docstring
over into this patch as well which might clean it up, but it would sort of
be out of place in this patch. I'll add in the current format for now and
we can change one or the other depending on the order to make sure it
applies.


> >  other_eal_param: str,
> >  ):
> >  """
> > @@ -56,6 +58,7 @@ def __init__(
> >  self._prefix = f"--file-prefix={prefix}"
> >  self._no_pci = "--no-pci" if no_pci else ""
> >  self._vdevs = " ".join(f"--vdev {vdev}" for vdev in vdevs)
> > +self._ports = " ".join(f"-a {port.pci}" for port in ports)
> >  self._other_eal_param = other_eal_param
> >
> >  def __str__(self) -> str:
> > @@ -65,6 +68,7 @@ def __str__(self) -> str:
> >  f"{self._prefix} "
> >  f"{self._no_pci} "
> >  f"{self._vdevs} "
> > +f"{self._ports} "
> >  f"{self._other_eal_param}"
> >  )
> >
> > @@ -308,6 +312,7 @@ def create_eal_parameters(
> >  append_prefix_timestamp: bool = True,
> >  no_pci: bool = False,
> >  vdevs: list[VirtualDevice] = None,
> > +ports: list[Port] | None = None,
>
> Same here.
>

Good catch, I will do the same here.


>
> >  other_eal_param: str = "",
> >  ) -> "EalParameters":
> >  """
> > @@ -350,12 +355,16 @@ def create_eal_parameters(
> >  if vdevs is None:
> >  vdevs = []
> >
> > +if ports is None:
> > +ports = self.ports
> > +
> >  return EalParameters(
> >  lcore_list=lcore_list,
> >  memory_channels=self.config.memory_channels,
> >  prefix=prefix,
> >  no_pci=no_pci,
> >  vdevs=vdevs,
> > +ports=ports,
> >  other_eal_param=other_eal_param,
> >  )
> >
> > --
> > 2.42.0
> >
>

Re: [PATCH v3 5/7] dts: add optional packet filtering to scapy sniffer

[Jeremy Spewock]

On Thu, Nov 16, 2023 at 1:34 PM Juraj Linkeš 
wrote:

> As I'm thinking about the filtering, it's not a trivial manner. For
> now, I'd like to pass a class instead of multiple parameters, as that
> will be easier to extend if we need to add to the filter. The class
> could be a dataclass holding the various booleans. Then the capturing
> traffic generators would need to implement a method that would
> translate the dataclass into a TG-specific filters. I'm not sure we
> want to introduce an abstract method for this, as the return value may
> differ for different TGs - this needs more thought.
>

I agree that making it a dataclass and having a method within Scapy for
interpreting it for now would work. You're right there there are some other
considerations to be had like if the return types could be different. I
would think that for the most part a string is what would be used, but it
could be different depending on what traffic generator you use. I know that
tcpdump supports BPFs as they are shown here, so if we went down the route
of using that for collecting packets with other traffic generators we would
be fine on that front, but that might need more discussion as well when we
add those traffic generators.

If we needed this to be more generic as well, it could be possible to
assign each traffic generator a filter type and then we could make
subclasses of the dataclass for each filter type. This way, in the filter
subclasses, they could specify what each of the parameters meant and how to
create the filter. This would always return the right data type for the
filter and allow us to implement methods for some of the more generic
filters (like the one I use here, a BPF) rather than implementing them on
the traffic generator that uses the filter. Then we could just use a
TypeVar to generically create the filters based on what the capturing
traffic generator class says it would need.

I think another way we could go about this problem however is just not
filtering out packets that are generally just noise like these and instead
just check the packets we receive when capturing and only return ones that
are relevant (like ones that have the same layers as the packet we sent for
example). However, I think because of only having one traffic generator it
would be fine to just create a method i nthe scapy capturing traffic
generator that knows how to create BPF filters and I can do that for the
next version if that is preferred.


>
> On Mon, Nov 13, 2023 at 9:28 PM  wrote:
> >
> > From: Jeremy Spewock 
> >
> > Added the options to filter out LLDP and ARP packets when
> > sniffing for packets with scapy. This was done using BPF filters to
> > ensure that the noise these packets provide does not interfere with test
> > cases.
> >
> > Signed-off-by: Jeremy Spewock 
> > ---
> >  dts/framework/test_suite.py | 13 +++--
> >  .../testbed_model/capturing_traffic_generator.py| 12 +++-
> >  dts/framework/testbed_model/scapy.py| 11 ++-
> >  dts/framework/testbed_model/tg_node.py  |  4 +++-
> >  4 files changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> > index 3b890c0451..3222f172f3 100644
> > --- a/dts/framework/test_suite.py
> > +++ b/dts/framework/test_suite.py
> > @@ -152,7 +152,11 @@ def _configure_ipv4_forwarding(self, enable: bool)
> -> None:
> >  self.sut_node.configure_ipv4_forwarding(enable)
> >
> >  def send_packet_and_capture(
> > -self, packet: Packet, duration: float = 1
> > +self,
> > +packet: Packet,
> > +duration: float = 1,
> > +no_lldp: bool = True,
> > +no_arp: bool = True,
>
> The parameters of this method are not documented in the docstring, but
> we should add these new parameters to the docstring. The same goes for
> the other methods (and other parameters of other methods) if they're
> not overriding an abstract method.
>
> The default should be False if these are meant to be optional, but I
> like these defaulting to True, so I'd change the wording from optional
> to configurable.
>

That is a good catch, I didn't think about the wording that way but that
makes a lot of sense. I will make this change.


>
> >  ) -> list[Packet]:
> >  """
> >  Send a packet through the appropriate interface and
> > @@ -162,7 +166,12 @@ def send_packet_and_capture(
> >  """
> >  packet = self._adjust_addresses(packet)
> >  return self.tg_node.send_packet_and_capture(
> > -packet, self._tg_port_egress, self._tg_port_ingress,
> duration
> > +packet,
> > +self._tg_port_egress,
> > +self._tg_port_ingress,
> > +duration,
> > +no_lldp,
> > +no_arp,
> >  )
> >
> >  def get_expected_packet(self, packet: Packet) -> Packet:
> > diff --git a/dts/framework/testbed_model/capturing_tra

Re: [PATCH v3 4/7] dts: allow passing parameters into interactive apps

[Jeremy Spewock]

On Thu, Nov 16, 2023 at 1:53 PM Juraj Linkeš 
wrote:

> On Mon, Nov 13, 2023 at 9:28 PM  wrote:
> >
> > From: Jeremy Spewock 
> >
> > Modified interactive applications to allow for the ability to pass
> > parameters into the app on start up. Also modified the way EAL
> > parameters are handled so that the trailing "--" separator is added be
> > default after all EAL parameters.
> >
> > Signed-off-by: Jeremy Spewock 
> > ---
> >  dts/framework/remote_session/remote/testpmd_shell.py |  2 +-
> >  dts/framework/testbed_model/sut_node.py  | 11 +++
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py
> b/dts/framework/remote_session/remote/testpmd_shell.py
> > index 3ea16c7ab3..3f6a86aa35 100644
> > --- a/dts/framework/remote_session/remote/testpmd_shell.py
> > +++ b/dts/framework/remote_session/remote/testpmd_shell.py
> > @@ -32,7 +32,7 @@ def _start_application(
> >  self, get_privileged_command: Callable[[str], str] | None
> >  ) -> None:
> >  """See "_start_application" in InteractiveShell."""
> > -self._app_args += " -- -i"
> > +self._app_args += " -i"
> >  super()._start_application(get_privileged_command)
> >
> >  def get_devices(self) -> list[TestPmdDevice]:
> > diff --git a/dts/framework/testbed_model/sut_node.py
> b/dts/framework/testbed_model/sut_node.py
> > index 4161d3a4d5..bcac939e72 100644
> > --- a/dts/framework/testbed_model/sut_node.py
> > +++ b/dts/framework/testbed_model/sut_node.py
> > @@ -377,7 +377,8 @@ def create_interactive_shell(
> >  shell_cls: Type[InteractiveShellType],
> >  timeout: float = SETTINGS.timeout,
> >  privileged: bool = False,
> > -eal_parameters: EalParameters | str | None = None,
> > +eal_parameters: EalParameters | str = "",
>
> I think it makes more sense if the type is EalParameters | None. With
> this change, the str type of eal_parameters moved to app_parameters.
>
> > +app_parameters: str = "",
> >  ) -> InteractiveShellType:
> >  """Factory method for creating a handler for an interactive
> session.
> >
> > @@ -392,12 +393,14 @@ def create_interactive_shell(
> >  eal_parameters: List of EAL parameters to use to launch the
> app. If this
> >  isn't provided or an empty string is passed, it will
> default to calling
> >  create_eal_parameters().
> > +app_parameters: Additional arguments to pass into the
> application on the
> > +command-line.
> >  Returns:
> >  Instance of the desired interactive application.
> >  """
> > -if not eal_parameters:
> > +if not eal_parameters and shell_cls.dpdk_app:
> >  eal_parameters = self.create_eal_parameters()
> > -
> > +eal_parameters = f"{eal_parameters} --"
>
> I think this change is more complicated than it seems at first glance.
>
> Before we either passed EalParameters (meant for DPDK apps) or a
> string (meant for other apps). There was no additional check for these
> assumptions.
> Now that we've split it, I see some cases which seem to be not covered.
>
> 1. The app is a DPDK app and we pass EalParameters. The two hyphens
> are not added.
> 2. The app is not a DPDK app and we pass EalParameters. Similarly to
> current behavior (without the patch), we kinda assume that the caller
> won't do this, but now that we're modifying the behavior, let's add a
> check that won't allow EalParameters with non-DPDK apps.
>
>
That is a good point that not all DPDk apps need the two hyphens, I can
make that just specific to testpmd and change it instead so that we don't
pass EalParameters into DPDK applications and I think that should cover
these cases.



> >  # We need to append the build directory for DPDK apps
> >  if shell_cls.dpdk_app:
> >  shell_cls.path = self.main_session.join_remote_path(
> > @@ -405,7 +408,7 @@ def create_interactive_shell(
> >  )
> >
> >  return super().create_interactive_shell(
> > -shell_cls, timeout, privileged, str(eal_parameters)
> > +shell_cls, timeout, privileged, f"{eal_parameters}
> {app_parameters}"
> >  )
> >
> >  def bind_ports_to_driver(self, for_dpdk: bool = True) -> None:
> > --
> > 2.42.0
> >
>

Re: [PATCH v3 2/7] dts: add waiting for port up in testpmd

[Jeremy Spewock]

On Thu, Nov 16, 2023 at 2:05 PM Juraj Linkeš 
wrote:

> On Mon, Nov 13, 2023 at 9:28 PM  wrote:
> >
> > From: Jeremy Spewock 
> >
> > Added a method within the testpmd interactive shell that polls the
> > status of ports and verifies that the link status on a given port is
> > "up." Polling will continue until either the link comes up, or the
> > timeout is reached.
> >
> > Signed-off-by: Jeremy Spewock 
> > ---
> >  .../remote_session/remote/testpmd_shell.py| 29 +++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/dts/framework/remote_session/remote/testpmd_shell.py
> b/dts/framework/remote_session/remote/testpmd_shell.py
> > index 1455b5a199..3ea16c7ab3 100644
> > --- a/dts/framework/remote_session/remote/testpmd_shell.py
> > +++ b/dts/framework/remote_session/remote/testpmd_shell.py
> > @@ -1,9 +1,12 @@
> >  # SPDX-License-Identifier: BSD-3-Clause
> >  # Copyright(c) 2023 University of New Hampshire
> >
> > +import time
> >  from pathlib import PurePath
> >  from typing import Callable
> >
> > +from framework.settings import SETTINGS
> > +
> >  from .interactive_shell import InteractiveShell
> >
> >
> > @@ -47,3 +50,29 @@ def get_devices(self) -> list[TestPmdDevice]:
> >  if "device name:" in line.lower():
> >  dev_list.append(TestPmdDevice(line))
> >  return dev_list
> > +
> > +def wait_link_status_up(self, port_id: int,
> timeout=SETTINGS.timeout) -> bool:
> > +"""Wait until the link status on the given port is "up".
> > +
> > +Arguments:
> > +port_id: Port to check the link status on.
> > +timeout: time to wait for the link to come up.
> > +
> > +Returns:
> > +If the link came up in time or not.
> > +"""
>
> Again with the docstrings - the new thing here is the usage of
> SETTINGS. Here's an example:
>
> The YAML test run configuration file is specified in the
> :option:`--config-file` command line
> argument or the :envvar:`DTS_CFG_FILE` environment variable.
>
> The rule is to first mention the cmdline arg, then the env var.
>
>
Good catch, I'll change this.


> > +time_to_stop = time.time() + timeout
> > +while time.time() < time_to_stop:
> > +port_info = self.send_command(f"show port info {port_id}")
> > +if "Link status: up" in port_info:
> > +break
> > +time.sleep(0.5)
> > +else:
> > +self._logger.error(
> > +f"The link for port {port_id} did not come up in the
> given timeout."
> > +)
> > +return "Link status: up" in port_info
> > +
> > +def close(self) -> None:
> > +self.send_command("exit", "")
> > +return super().close()
> > --
> > 2.42.0
> >
>

Re: [PATCH v3] net/axgbe: support TSO

[Ferruh Yigit]

On 11/16/2023 4:03 PM, Jesna K E wrote:
> Added TSO support for axgbe PMD.
> 
> Initial Implementation for the TSO feature support
> Currently only headers transmitted to
> tester receiver side
> 
> Signed-off-by: Jesna K E 
> ---
>  doc/guides/nics/features/axgbe.ini |  1 +
>  drivers/net/axgbe/axgbe_common.h   | 12 
>  drivers/net/axgbe/axgbe_dev.c  | 13 +
>  drivers/net/axgbe/axgbe_ethdev.c   |  3 +
>  drivers/net/axgbe/axgbe_ethdev.h   |  1 +
>  drivers/net/axgbe/axgbe_rxtx.c | 88 +-
>  6 files changed, 104 insertions(+), 14 deletions(-)
> 
> diff --git a/doc/guides/nics/features/axgbe.ini 
> b/doc/guides/nics/features/axgbe.ini
> index 5e2d6498e5..5c30c967bc 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -7,6 +7,7 @@
>  Speed capabilities   = Y
>  Link status  = Y
>  Scattered Rx = Y
> +TSO   = Y
>  Promiscuous mode = Y
>  Allmulticast mode= Y
>  RSS hash = Y
> diff --git a/drivers/net/axgbe/axgbe_common.h 
> b/drivers/net/axgbe/axgbe_common.h
> index a5d11c5832..c30efe4c02 100644
> --- a/drivers/net/axgbe/axgbe_common.h
> +++ b/drivers/net/axgbe/axgbe_common.h
> @@ -161,6 +161,10 @@
>  #define DMA_CH_CARBR_LO  0x5c
>  #define DMA_CH_SR0x60
>  
> +/* Setting MSS register entry bit positions and sizes for TSO */
> +#define DMA_CH_CR_MSS_INDEX 0
> +#define DMA_CH_CR_MSS_WIDTH 14
> +
>  /* DMA channel register entry bit positions and sizes */
>  #define DMA_CH_CR_PBLX8_INDEX16
>  #define DMA_CH_CR_PBLX8_WIDTH1
> @@ -1232,6 +1236,14 @@
>  #define TX_CONTEXT_DESC3_VT_INDEX0
>  #define TX_CONTEXT_DESC3_VT_WIDTH16
>  
> +/* TSO related register entry bit positions and sizes*/
> +#define TX_NORMAL_DESC3_TPL_INDEX   0
> +#define TX_NORMAL_DESC3_TPL_WIDTH   18
> +#define TX_NORMAL_DESC3_THL_INDEX   19
> +#define TX_NORMAL_DESC3_THL_WIDTH   4
> +#define TX_CONTEXT_DESC3_OSTC_INDEX 27
> +#define TX_CONTEXT_DESC3_OSTC_WIDTH 1
> +
>  #define TX_NORMAL_DESC2_HL_B1L_INDEX 0
>  #define TX_NORMAL_DESC2_HL_B1L_WIDTH 14
>  #define TX_NORMAL_DESC2_IC_INDEX 31
> diff --git a/drivers/net/axgbe/axgbe_dev.c b/drivers/net/axgbe/axgbe_dev.c
> index 6a7fddffca..eef453fab0 100644
> --- a/drivers/net/axgbe/axgbe_dev.c
> +++ b/drivers/net/axgbe/axgbe_dev.c
> @@ -808,6 +808,18 @@ int axgbe_write_rss_lookup_table(struct axgbe_port 
> *pdata)
>   return 0;
>  }
>  
> +static void xgbe_config_tso_mode(struct axgbe_port *pdata)
> +{
> + unsigned int i;
> + struct axgbe_tx_queue *txq;
> +
> + for (i = 0; i < pdata->eth_dev->data->nb_tx_queues; i++) {
> + txq = pdata->eth_dev->data->tx_queues[i];
> + AXGMAC_DMA_IOWRITE_BITS(txq, DMA_CH_TCR, TSE, 1);
> + AXGMAC_DMA_IOWRITE_BITS(txq, DMA_CH_CR, MSS, 800);
> + }
> +}
> +
>  static int axgbe_enable_rss(struct axgbe_port *pdata)
>  {
>   int ret;
> @@ -1314,6 +1326,7 @@ static int axgbe_init(struct axgbe_port *pdata)
>   axgbe_config_rx_pbl_val(pdata);
>   axgbe_config_rx_buffer_size(pdata);
>   axgbe_config_rss(pdata);
> + xgbe_config_tso_mode(pdata);
>

Driver namespace is 'axgbe', all other functions/variables starts with
it, but new additions start with 'xgbe', what do you think rename them
as 'axgbe' for consistency.


>   wrapper_tx_desc_init(pdata);
>   ret = wrapper_rx_desc_init(pdata);
>   if (ret)
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c 
> b/drivers/net/axgbe/axgbe_ethdev.c
> index 3717166384..0a4901aabc 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -12,6 +12,8 @@
>  
>  #include "eal_filesystem.h"
>  
> +#include 
> +
>  #ifdef RTE_ARCH_X86
>  #include 
>  #else
> @@ -1237,6 +1239,7 @@ axgbe_dev_info_get(struct rte_eth_dev *dev, struct 
> rte_eth_dev_info *dev_info)
>   RTE_ETH_TX_OFFLOAD_IPV4_CKSUM  |
>   RTE_ETH_TX_OFFLOAD_MULTI_SEGS  |
>   RTE_ETH_TX_OFFLOAD_UDP_CKSUM   |
> + RTE_ETH_TX_OFFLOAD_TCP_TSO |
>   RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
>  
>   if (pdata->hw_feat.rss) {
> diff --git a/drivers/net/axgbe/axgbe_ethdev.h 
> b/drivers/net/axgbe/axgbe_ethdev.h
> index 7f19321d88..31a583c2c6 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.h
> +++ b/drivers/net/axgbe/axgbe_ethdev.h
> @@ -583,6 +583,7 @@ struct axgbe_port {
>   unsigned int tx_osp_mode;
>   unsigned int tx_max_fifo_size;
>   unsigned int multi_segs_tx;
> + unsigned int tso_tx;
>  
>   /* Rx settings */
>   unsigned int rx_sf_mode;
> diff --git a/drivers/net/axgbe/axgbe_rxtx.c b/drivers/net/axgbe/axgbe_rxtx.c
> index a9ff291cef..b0cafcbdda 100644
> --- a/drivers/net/axgbe/axgbe_rxtx.c
> +++ b/drivers/net/axgbe/axgbe_rxtx.c
> @@ -627,6 +

[Bug 1328] af_xdp driver not built on Fedora 39

[bugzilla]

https://bugs.dpdk.org/show_bug.cgi?id=1328

Bug ID: 1328
   Summary: af_xdp driver not built on Fedora 39
   Product: DPDK
   Version: 23.11
  Hardware: All
OS: All
Status: UNCONFIRMED
  Severity: normal
  Priority: Normal
 Component: core
  Assignee: dev@dpdk.org
  Reporter: step...@networkplumber.org
  Target Milestone: ---

On Fedora 39, doing meson setup reports that net/af_xdp is n ot built.

  net/af_xdp:  missing dependency, "libxdp >= 1.2.2" and "libbpf"

$ dnf list libxdp libbpf
Last metadata expiration check: 0:06:05 ago on Fri 17 Nov 2023 10:46:20 AM PST.
Installed Packages
libbpf.x86_64   2:1.1.0-4.fc39  
@fedora-modular
libxdp.x86_64   1.4.1-1.fc39 @updates   
Available Packages
libbpf.i686 2:1.1.0-4.fc39   fedora 
libxdp.i686 1.4.1-1.fc39 updates

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [PATCH v2 00/10] replace zero length arrays

[Tyler Retzlaff]

On Fri, Nov 17, 2023 at 08:18:23AM -0800, Stephen Hemminger wrote:
> On Fri, 17 Nov 2023 09:31:11 +0100
> Morten Brørup  wrote:
> 
> > Series-acked-by: Morten Brørup 
> > 
> > Suggest checkpatches to disallow this, if it doesn't already.
> 
> Unfortunately, matching this with awk is too hard. Coccinelle can do it
> but don't want to introduce dependency on that.
> 
> Maybe a compiler flag would be possible, but could not find one.

-pedantic will warn, but to use it without clobbering the build with
warnings we would probably need to qualify all use of non-standard
extensions with the __extension__ keyword.


```
struct foo {
int a;
int b;
char buffer[0];
};
```
zhora ~> gcc -Wall -pedantic oink.c
oink.c:7:14: warning: ISO C forbids zero-size array ‘buffer’ [-Wpedantic]
7 | char buffer[0];

[PATCH] doc: document basic MSVC build requirements

[Tyler Retzlaff]

Document the basic requirements for download and add an option
describing how to build with MSVC.

Signed-off-by: Tyler Retzlaff 
---
 doc/guides/windows_gsg/build_dpdk.rst | 33 -
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/doc/guides/windows_gsg/build_dpdk.rst 
b/doc/guides/windows_gsg/build_dpdk.rst
index 29f2b38..406e28f 100644
--- a/doc/guides/windows_gsg/build_dpdk.rst
+++ b/doc/guides/windows_gsg/build_dpdk.rst
@@ -10,8 +10,12 @@ System Requirements
 Building the DPDK and its applications requires one of the following
 environments:
 
-* The Clang-LLVM C compiler and Microsoft MSVC linker.
+* LLVM 14.0.0 (or later) and Microsoft MSVC linker.
 * The MinGW-w64 toolchain (either native or cross).
+* Microsoft Visual Studio 2022 (any edition).
+  - note Microsoft Visual Studio 2022 does not currently build enough
+of DPDK to produce a working DPDK application but may be used to
+validate that changes are portable between toolchains.
 
 The Meson Build system is used to prepare the sources for compilation
 with the Ninja backend.
@@ -54,6 +58,12 @@ Any thread model (POSIX or Win32) can be chosen, DPDK does 
not rely on it.
 Install to a folder without spaces in its name, like ``C:\MinGW``.
 This path is assumed for the rest of this guide.
 
+Option 3. Microsoft Visual Studio Toolset (MSVC)
+
+
+Install any edition of Microsoft Visual Studio 2022 from the Visual Studio
+website https://visualstudio.microsoft.com/downloads/
+
 
 Install the Build System
 
@@ -64,7 +74,7 @@ A good option to choose is the MSI installer for both meson 
and ninja together::
 

http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer%22
 
-Recommended version is Meson 0.57.
+Required version is Meson 0.57.
 
 Versions starting from 0.58 are unusable with LLVM toolchain
 because of a `Meson issue `_.
@@ -83,8 +93,8 @@ Build the code
 The build environment is setup to build the EAL and the helloworld example by
 default.
 
-Option 1. Native Build on Windows
-~
+Option 1. Native Build on Windows using LLVM
+
 
 When using Clang-LLVM, specifying the compiler might be required to complete
 the meson command:
@@ -105,7 +115,7 @@ To compile the examples, the flag ``-Dexamples`` is 
required.
 
 cd C:\Users\me\dpdk
 meson setup -Dexamples=helloworld build
-ninja -C build
+meson compile -C build
 
 Option 2. Cross-Compile with MinGW-w64
 ~~
@@ -117,3 +127,16 @@ Depending on the distribution, paths in this file may need 
adjustments.
 
 meson setup --cross-file config/x86/cross-mingw -Dexamples=helloworld build
 ninja -C build
+
+Option 3. Native Build on Windows using MSVC
+
+
+Open a 'Developer PowerShell for VS 2022' prompt from the start menu. The
+developer prompt will configure the environment to select the appropriate
+compiler, linker and SDK paths required to build with Visual Studio 2022.
+
+.. code-block:: console
+
+cd C:\Users\me\dpdk
+meson setup -Denable_stdatomic=true build
+meson compile -C build
-- 
1.8.3.1

Community Lab Maintenance 11/18/23

[Patrick Robb]

Hello,

We will be doing some network maintenance at the Community Lab tomorrow,
11/18, which will result in CI testing being offline from approximately
12:00 UTC to 8:00 UTC. Testing will resume as normal tomorrow evening, and
if there are any patchseries which are submitted during the downtime, we
will automatically pick those up once we're back online.

Thank you for your understanding.

-Community Lab team