Re: [PATCH 00/11] Introduce support for RISC-V architecture

["Stanisław Kardach" <kda@semihalf.com>]

[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]

On Fri, May 6, 2022 at 11:13 AM David Marchand <david.marchand@redhat.com>
wrote:

> On Thu, May 5, 2022 at 7:30 PM Stanislaw Kardach <kda@semihalf.com> wrote:
> >
> > This patchset adds support for building and running DPDK on 64bit RISC-V
> > architecture. The initial support targets rv64gc (rv64imafdc) ISA and
> > was tested on SiFive Unmatched development board with the Freedom U740
> > SoC running Linux (freedom-u-sdk based kernel).
> > I have tested this codebase using DPDK unit and perf tests as well as
> > test-pmd, l2fwd and l3fwd examples.
> > The NIC attached to the DUT was Intel X520-DA2 which uses ixgbe PMD.
> > On the UIO side, since U740 does not have an IOMMU, I've used igb_uio,
> > uio_pci_generic and vfio-pci noiommu drivers.
> >
> > Commits 1-2 fix small issues which are encountered if a given platform
> >    does not support any vector operations (which is the case with U740).
> > Commit 3 introduces EAL and build system support for RISC-V architecture
> >    as well as documentation updates.
> > Commits 4-7 add missing defines and stubs to enable RISC-V operation in
> >    non-EAL parts.
> > Commit 8 adds RISC-V specific cpuflags test.
> > Commit 9 works around a bug in the current GCC in test_ring compiled
> >    with -O0 or -Og.
> > Commit 10 adds RISC-V testing to test-meson-builds.sh automatically
> >    iterating over cross-compile config files (currently present for
> >    generic rv64gc and SiFive U740).
> > Commit 11 extends hash r/w perf test by displaying both HTM and non-HTM
> >    measurements. This is an extraneous commit which is not directly
> >    needed for RISC-V support but was noticed when we have started
> >    gathering test results. If needed, I can submit it separately.
> >
> > I appreciate Your comments and feedback.
>
> Thanks for working on this!
>
Thanks for your review!

>
> Please add a cross compilation job to GHA, something like:
>
> https://github.com/david-marchand/dpdk/commit/4023e28f9050b85fb138eba14068bfe882036f01
> Which looks to run fine:
>
> https://github.com/david-marchand/dpdk/runs/6319625002?check_suite_focus=true

Will do in V2.

>
>
> Testing all riscv configs in test-meson-buils.sh seems too much to me.
> Is there a real value to test both current targets?
>
It's for sanity and compilation coverage testing. I.e. SiFive variant has a
specific build config which does not require extra barriers when reading
time and cycle registers for rte_rdtsc_precise(). I want to make sure that
if anyone changes some code based on configuration flags, it gets at least
compile-checked.
I believe similar thing is done for Aarch64 builds.

>
> About the new "Sponsored-by" tag, it should not raise warnings in the
> CI if we agree on its addition.
>
I'll modify it in V2 to be in form of:
  Sponsored by: StarFive Technology
  ...
  Signed-off-by: ...
This was suggested by Stephen Hemminger as having a precedent in Linux
kernel. Interestingly enough first use of this tag in kernel source was
this year in January.

>
> devtools/check-meson.py caught coding style issues.
>
Will fix in V2.

>
> In general, please avoid letting arch specific headers leak
> internal/non rte_ prefixed helpers out of them.
> For example, I noticed a RV64_CSRR macro that can be undefined after usage.
>
Thanks for noticing. I'l fix this one in V2.
There are 2 other symbols that leak but on purpose (out of a better
idea): vect_load_128() and vect_and(). Both are used in l3fwd_em to
simulate vector operations. Other platforms reference their intrinsics
straight in the l3fwd_em.c. As I don't have support for vector ops and I
wanted to indicate that xmm_t should be an isolated API, I've put both in
rte_vect.h. That said I'm not happy with this solution and am open to
suggestions on how to solve it neatly.

>
> Patch 3 is huge, not sure it is easy to split, did you consider doing so?
>
It seems to me the nature of a new EAL implementation, I have to include
all symbols, otherwise DPDK won't compile.
Alternatively I could have a huge initial patch with empty stubs that would
be filled in later commits. Downside of this approach is that it's hard to
verify each commit separately as tests will fail until all implementation
is there, so the division is only visual.

>
> The release notes update is verbose and some parts could be dropped,
> like the list of verifications that are fine in a series cover letter.
>
Will do. I'll move listed items to the cover letter.

>
> Please resubmit fixes separately from this series so that we can merge
> them sooner than this series.
>
Will do. Since at least 2 fixes are required for the RISC-V EAL to work or
compile, I'll put  Depends-on tag in the EAL commit.

>
>
> --
> David Marchand
>
>

[-- Attachment #2: Type: text/html, Size: 7032 bytes --]