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

[David Marchand <david.marchand@redhat.com>]

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.


>> 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