From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5CE70A034C; Mon, 9 May 2022 14:24:39 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3671B407FF; Mon, 9 May 2022 14:24:38 +0200 (CEST) Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by mails.dpdk.org (Postfix) with ESMTP id BAC804068F for ; Mon, 9 May 2022 14:24:36 +0200 (CEST) Received: by mail-lf1-f48.google.com with SMTP id bq30so23484320lfb.3 for ; Mon, 09 May 2022 05:24:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nFCp1zutzfvnnDmlKlh7QMqFHMreTlYqQOap9tcFe7o=; b=2Y/E5YVxbGiKWg644rG04jEsgo/u68dMgoYXIUAcyzkTEnmaSWZ78OW3yDqclVKIdZ Q/sm7YITO4GVHG58XBE6JHr0Dc06C93PiOAVk0csc4OJVjecij6jbTv8xEnCNPx9MgBa Gcl5x7e9JHFVdRIAuGBPIR92ZmGFICdh7hd7LsVpdICj6B4NCMzqNOE86KKPAzKb2sSB CfJNsKBFo7V292HEU7xE02zzDOrqCQAsCz+Tz/LSeTKpZhE8j8P3AJTWw9OKgCZw+ANF OycR0LktHMglM2QHsMwm2CTsEhS1hnFHOoyJpg+Kb8NxxubjW+krUjsmE8rFg5OZL9w9 c0Vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nFCp1zutzfvnnDmlKlh7QMqFHMreTlYqQOap9tcFe7o=; b=Ambk8cJalsm8IQ2up768b/dFymfC0c8nX7zwqTerTdXnq09JtpUOo/cNq7j7reeybd XxAyvL/xtFFq6fFtWdZN8LU6xn3IfO0TNDIG05YSDwNzDcnp85oPZomxJhMLB+c5TtCS J47vxyXp0OheceCOURiNWZ48xR4U4KxWha1v5oMWTsVjMDS/hn1CYdKL9jezOS6q8iIE 4h716EL2HwpEi6kbY2z9OptMtbeYvE8mW9GASYrgGiTJStLQnhBLQfzLYT3zGLouRj8u Rqvr3IECmK2ysGWjZgfIFkX6rH+/VP+W3AzCOL4dC2O7X1ugvlKCQLaDbrr2Vg+Y9SjW poTQ== X-Gm-Message-State: AOAM532dShxO9K4/6fGx0XrZc3tYghFVJYSbp0NA3u3yEKKk8VU9QlI2 XbHFGpKhVQxOPHLWjfiRwbaQuCiaAN6Ws+t+f9VGVA== X-Google-Smtp-Source: ABdhPJy1GXQv1p5viOd0Ky73/QcCQZCgv9mm7zM9J4UP+c2PADP0KqeEcxSf9C4K/lOgVMAxDy6qaHXS1HPe5uMmf0s= X-Received: by 2002:a05:6512:2381:b0:472:5e75:13e with SMTP id c1-20020a056512238100b004725e75013emr12790921lfv.514.1652099076179; Mon, 09 May 2022 05:24:36 -0700 (PDT) MIME-Version: 1.0 References: <20220505173003.3242618-1-kda@semihalf.com> In-Reply-To: From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Mon, 9 May 2022 14:24:00 +0200 Message-ID: Subject: Re: [PATCH 00/11] Introduce support for RISC-V architecture To: David Marchand Cc: dev , Frank Zhao , Sam Grove , Marcin Wojtas , upstream@semihalf.com, Thomas Monjalon , Stephen Hemminger Content-Type: multipart/alternative; boundary="00000000000091dc3205de934ac1" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --00000000000091dc3205de934ac1 Content-Type: text/plain; charset="UTF-8" On Fri, May 6, 2022 at 11:13 AM David Marchand wrote: > On Thu, May 5, 2022 at 7:30 PM Stanislaw Kardach 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 > > --00000000000091dc3205de934ac1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, May 6, 2022 at 11:13 AM David Mar= chand <da= vid.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<= br> > 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<= br> > 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=
>=C2=A0 =C2=A0 does not support any vector operations (which is the case= with U740).
> Commit 3 introduces EAL and build system support for RISC-V architectu= re
>=C2=A0 =C2=A0 as well as documentation updates.
> Commits 4-7 add missing defines and stubs to enable RISC-V operation i= n
>=C2=A0 =C2=A0 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 >=C2=A0 =C2=A0 with -O0 or -Og.
> Commit 10 adds RISC-V testing to test-meson-builds.sh automatically >=C2=A0 =C2=A0 iterating over cross-compile config files (currently pres= ent for
>=C2=A0 =C2=A0 generic rv64gc and SiFive U740).
> Commit 11 extends hash r/w perf test by displaying both HTM and non-HT= M
>=C2=A0 =C2=A0 measurements. This is an extraneous commit which is not d= irectly
>=C2=A0 =C2=A0 needed for RISC-V support but was noticed when we have st= arted
>=C2=A0 =C2=A0 gathering test results. If needed, I can submit it separa= tely.
>
> I appreciate Your comments and feedback.

Thanks for working on this!
Thanks for your review!=C2= =A0

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/da= vid-marchand/dpdk/runs/6319625002?check_suite_focus=3Dtrue
=
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 bui= lds.

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:
=C2=A0 Sponsored by: StarFive Technology
=
=C2=A0 ...
=C2=A0 Signed-off-by: ...=C2=A0
T= his was suggested by=C2=A0Stephen Hemminger as having a precedent in Linux = kernel. Interestingly enough first use of this tag in kernel source was thi= s year in January.

devtools/check-meson.py caught coding style issues.
Wi= ll fix in V2.=C2=A0

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 i= dea):=C2=A0vect_load_128() and=C2=A0vect_and(). Both are used in l3fwd_em t= o simulate vector operations. Other platforms reference their intrinsics st= raight 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 t= o 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 har= d to verify each commit separately as tests will fail until all implementat= ion 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.
<= /blockquote>
Will do. I'll move listed items to the cover letter.= =C2=A0

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=C2= =A0 Depends-on tag in the EAL commit.


--
David Marchand

--00000000000091dc3205de934ac1--