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

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

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

On Mon, May 9, 2022 at 4:31 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, May 9, 2022 at 2:24 PM Stanisław Kardach <kda@semihalf.com> wrote:
> >> 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.
>
> As far as I experienced, building all those aarch64 combinations never
> revealed any specific platform compilation issue.
> It only consumes cpu, disk and our (maintainers) time.
> I proposed to Thomas to shrink aarch64 builds list not so long ago :-).
>
> The best would be for SiFive to provide a system for the CI to do
> those checks on their variant.
>
>
> >> 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.
>
> I don't have an opinion on the spelling.
>
> At the moment, the checks raise a warning:
> http://mails.dpdk.org/archives/test-report/2022-May/278580.html
>
> My point is that for this new tag, either checkpatch.pl in kernel
> handles it (which I don't think it is the case) or we need to disable
> the signature check in checkpatch.pl and something is added in dpdk
> checkpatches.sh to accept all known tags.
>
BAD_SIGN_OFF handles more than just tag names (in total there's 10 cases
checked). I'm not sure replicating this to checkpatches.sh is worth the
maintenance.
Alternatively I could ignore BAD_SIGN_OFF on initial checkpatch.pl run and
then run it again with just the BAD_SIGN_OFF type and filter out the result.
In that case, what would be the acceptable content of Sponsored-by tag? For
line:
  Sponsored-by: StarFive Technology
Current checkpatch.pl generates (used --terse for brevity):
  0001-eal-add-initial-support-for-RISC-V-architecture.patch:55:
WARNING:BAD_SIGN_OFF: Non-standard signature: Sponsored-by:
  0001-eal-add-initial-support-for-RISC-V-architecture.patch:55:
ERROR:BAD_SIGN_OFF: Unrecognized email address: 'StarFive Technology'

Using "Sponsored by:" does not trigger checks above (still feels like a
hack).

>
> >> 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.
>
> I'll try to have a look in the next revision.
>
>
> >>
> >>
> >> 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.
>
> If you are sure there is nothing that can be separated, let's keep it
> whole.
>
>
>
> --
> David Marchand
>
>

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