From: Akhil Goyal

quoted

Hi Matan,

quoted

quoted

quoted

quoted

quoted

+Prerequisites
+-------------
+
+- Mellanox OFED version: **5.3**
+  see :doc:`../../nics/mlx5` guide for more Mellanox OFED details.

Since the driver is by default compiled off due to the
dependency on external Libraries, I would recommend to add few
lines here as well for compilation.
Like to compile rdma-core and set PKG_CONFIG_LIBDIR.

Why? all Mellanox drivers has the same external dependencies.
I added here link for the doc explains it well.

This is a crypto PMD, not a NIC PMD. Somebody working on crypto
PMDs,

do

quoted

not really care about the NIC PMDs.
Hence it would be convenient to have compilation information here as

well.

quoted

You can refer to other document for details, but basic info should
be added here as well.

The link explains how to install OFED, this is only what the user need
to take from the link.
The basic is to install OFED.
I don't see a reason to duplicate doc section which are exactly the same.

As I compiled the PMD, it was not convenient to read the whole document.
And it is not needed to compile linux and everything.
I just needed rdma-core and set it in PKG_CONFIG_LIBDIR.

But compilation is not enough to run, you still cannot test if you break thigs
by compilation.
You need to install also the kernel modules.
That's what we explain in all our drivers.

For a person doing minor changes in the Lib, he cannot test each hardware.
He can only do compilation and that is what he is expected to do.

quoted

The reason I am insisting here is, when somebody do small changes in

Crypto

quoted

library, he may need to do subsequent changes in all PMDs.
For which compilation steps should be easily accessible in the PMD doc So
that the patch can be compiled properly.

Not enough.

quoted

Hence I just recommend to have 3-4 lines to enable the compilation In the
PMD doc.

We are not doing it in others mlx5 drivers.
If you insist, we will do.

quoted

quoted

quoted

quoted

quoted

And I do not see any updates to the test application for testing
this

driver.

quoted

quoted

You can see update to l2fwd_crypto, we tested with this example
for the first stage.
Everything looks ok there.

L2fwd-crypto is an app which only test data path with no packet

validation.

quoted

quoted

It does not tell if your encryption is correctly done as per standards or

not.

quoted

quoted

Did you test interoperability with l2fwd-crypto?
All basic configuration tests are also not done, like cleanup etc of the

PMD.

quoted

quoted

I haven't seen a driver getting merge without the unit test application

run.

quoted

quoted

Test app helps you comply with the way dpdk drivers are meant to be
written.

We adjusted the l2fwd-crypto to the dataunit feature and wrapped keys.
We validated data integrity from the packet returns back from the
crypto net port.
As I said, encryption\decryption with AES-XTS is working well.

Do you test interoperability here? Encryption by MLX5 and decryption By
another PMD/stack and vice-versa.

Compared to open-ssl results.

quoted

Test app is supposed to have test vectors which will work on any platform.
Hence data validation is done properly.

I think open-ssl is good standard to check too.

Yes it is good standard.
OK but what about the other basic cases that I listed below.

Did you mention any limitation that all these cases are not supported as of now.

quoted

quoted

Now, is too late to update the test application to the above features,
the driver code is here for a long time, no one ask about the test
adjustment until now.

I acked as maintainer of this PMD.

Is it really enough for the new PMD?

quoted

Can we defer to next release? I apologize for not asking it earlier. But this is
kind of obvious for somebody working in DPDK.
Please check that none of the PMD is merged without test app in the past 3-
4yrs.

This is not true.

quoted

quoted

We can add the adjustment to increase validity for the next release to
all the remaining crypto apps (test\test-crypto-perf).

For now, we have one validation with l2fwd-crypto And any user can run
it and see how to use mlx5 driver.

The user cannot be sure of the basic things of a crypto PMD are in order or
not.
As l2fwd-crypto does not test every basic thing.
For eg. Session deletion, PMD stop, PMD close, PMD restart, setting up
multiple sessions(l2fwd support single session).

Running datapath of a single use case is not sufficient for a PMD.
This is a POC and it need to comply with the environment.

There are only one\ two cases we need in this version and we tested them.

quoted

I hope the doubts are clear now and we are OK to defer to next release.

As I said, we don't agree, it is yours.

As I said, fix the issues, I can still merge today but not after that.

Regards,
Akhil