Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: add 64-bit LoongArch support #356

Closed
wants to merge 5 commits into from

Conversation

yetist
Copy link
Contributor

@yetist yetist commented Nov 5, 2021

LoongArch is a new instruction set of Loongson 3A5000 CPU, you can read the documents or visit the development community to get more infomation.

Now I porting the libseccomp to support loongarch64, please review, thanks.

The test results of make check: make-check-main.log

@drakenclimber
Copy link
Member

I haven't had a chance to look through the patches yet; I'll likely get to them next week.

The DCO check is failing because the commits don't have a Signed-off-by: tag. See other commits to libseccomp for an example. Here's the thought process behind a signed-off tag if you haven't worked with them before:
https://developercertificate.org/

@coveralls
Copy link

coveralls commented Nov 5, 2021

Coverage Status

Coverage: 89.724% (+0.01%) from 89.71% when pulling c01c4fd on loongarch64:dev-main into 73be05e on seccomp:main.

@pcmoore pcmoore changed the title Add 64-bit LoongArch support RFE: add 64-bit LoongArch support Nov 5, 2021
@pcmoore
Copy link
Member

pcmoore commented Nov 5, 2021

Building upon what @drakenclimber already said, we have a CONTRIBUTING.md doc which describes the DCO and other important requirements for submitting patches to this project.

@yetist
Copy link
Contributor Author

yetist commented Nov 6, 2021

The results of make check:

Regression Test Summary
 tests run: 4693
 tests skipped: 121
 tests passed: 4693
 tests failed: 0
 tests errored: 0

The results of (cd tests; ./regression -T live):

Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 5
 tests failed: 6
 tests errored: 0

Failed tests:

Test 20-live-basic_die%%003-00001 result:   FAILURE 20-live-basic_die 1 ERRNO rc=38
Test 24-live-arg_allow%%001-00001 result:   FAILURE 24-live-arg_allow 1 ALLOW rc=161
Test 44-live-a2_order%%001-00001 result:   FAILURE 44-live-a2_order 1 ALLOW rc=1
Test 51-live-user_notification%%001-00001 result:   FAILURE 51-live-user_notification 5 ALLOW rc=14
Test 54-live-binary_tree%%001-00001 result:   FAILURE 54-live-binary_tree 1 ALLOW rc=161
Test 58-live-tsync_notify%%001-00001 result:   FAILURE 58-live-tsync_notify 6 ALLOW rc=14

@yetist
Copy link
Contributor Author

yetist commented Dec 3, 2021

After updating the kernel, the tests passed:

Regression Test Summary
 tests run: 4693
 tests skipped: 121
 tests passed: 4693
 tests failed: 0
 tests errored: 0
============================================================
PASS: regression
=============
1 test passed
=============
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

The full logs are as follows:

check-syntax.log
make-check.log
regression.log

@drakenclimber
Copy link
Member

Regression Test Summary
 tests run: 4693
 tests skipped: 121
 tests passed: 4693
 tests failed: 0
 tests errored: 0
============================================================
PASS: regression
=============
1 test passed
=============
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

@yetist, just to be clear - are these results from running the tests on loongarch?

@pcmoore
Copy link
Member

pcmoore commented Jan 5, 2022

Also, it doesn't appear that support for the LoongArch architecture has been accepted into Linus' tree yet, is this correct?

While we are always happy to add new processor architectures to libseccomp, the architecture must first be present in the mainline Linux Kernel.

@yetist
Copy link
Contributor Author

yetist commented Jan 6, 2022

@yetist, just to be clear - are these results from running the tests on loongarch?

Yes.

@yetist
Copy link
Contributor Author

yetist commented Jan 6, 2022

While we are always happy to add new processor architectures to libseccomp, the architecture must first be present in the mainline Linux Kernel.

Thank you for your reply, let's wait for the kernel.

@drakenclimber
Copy link
Member

Thanks, @yetist. I briefly looked through these changes last week, and they looked good to me.

Ping us once the LoongArch changes are in the kernel, and we can work on getting this into libseccomp.

@xen0n
Copy link
Contributor

xen0n commented Apr 25, 2022

Rebased on top of current main branch; tests passed locally, but @yetist please confirm they're still up to date as of 2.5.4.

xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 25, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 25, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
@pcmoore
Copy link
Member

pcmoore commented Apr 25, 2022

Just a note to myself, as of v5.18-rc4 LoongArch does not appear to be present in the upstream Linux Kernel.

xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 26, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 26, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 27, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

See: gentoo#25189
Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 28, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

See: gentoo#25189
Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 28, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

See: gentoo#25189
Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
xen0n added a commit to xen0n/gentoo that referenced this pull request Apr 28, 2022
The LoongArch patch is generated by diffing the original release
tarball with the dist tarball, made with the LoongArch support
PR [1] applied. Tests have passed on amd64 and loong.

[1]: seccomp/libseccomp#356

See: gentoo#25189
Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
@yetist
Copy link
Contributor Author

yetist commented Oct 28, 2022

@drakenclimber

Done.

Copy link
Member

@drakenclimber drakenclimber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your hard work and patience, @yetist. Looks good to me.

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

@drakenclimber
Copy link
Member

FYI - @yetist, there's still a small test failure in test 6:

Test 06-sim-actions%%005-00001 result:   FAILURE bpf_sim resulted in KILL

@yetist
Copy link
Contributor Author

yetist commented Nov 7, 2022

FYI - @yetist, there's still a small test failure in test 6:

Test 06-sim-actions%%005-00001 result:   FAILURE bpf_sim resulted in KILL

Maybe fstatfs failed on the x86_64?

@yetist
Copy link
Contributor Author

yetist commented Nov 28, 2022

The test result on the LoongArch machine:

Regression Test Summary                                                          
 tests run: 5061                                                                 
 tests skipped: 123                                                              
 tests passed: 5061                                                              
 tests failed: 0                                                                 
 tests errored: 0                                                                
============================================================                     
PASS: regression                                                                 
=============                                                                    
1 test passed                                                                    
=============                                                                    

@yetist
Copy link
Contributor Author

yetist commented Nov 30, 2022

@pcmoore @drakenclimber

Please review this PR again, thanks.

@drakenclimber
Copy link
Member

The latest changes look good to me. Thanks for you patience, @yetist.

Acked-by: Tom Hromatka <tom.hromatka@oracle.com>

@RevySR
Copy link

RevySR commented Jan 31, 2023

When will this patch be merged?

@pcmoore
Copy link
Member

pcmoore commented Feb 1, 2023

My apologies, I have been very busy lately. I hope to get to this within the next week or two; I will update the comments when it it merged or if I have any additional comments/questions.

Thank you for your patience.

Copy link
Member

@pcmoore pcmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks pretty good, just two very minor questions - I think once we answer/resolve those this should be ready to merge.

src/arch-syscall-validate Outdated Show resolved Hide resolved
src/arch-syscall-validate Outdated Show resolved Hide resolved
Signed-off-by: Xiaotian Wu <wuxiaotian@loongson.cn>
Signed-off-by: WANG Xuerui <git@xen0n.name>
Signed-off-by: Xiaotian Wu <wuxiaotian@loongson.cn>
Signed-off-by: Xiaotian Wu <wuxiaotian@loongson.cn>
Signed-off-by: WANG Xuerui <git@xen0n.name>
Copy link
Member

@pcmoore pcmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yetist, thanks for making those changes! I sat down this afternoon expecting to merge this PR and while running the tests on x86_64 I saw a couple of failures :( It looks like CI wasn't running on this PR so you wouldn't have noticed them if you've only been testing on LoongArch, but I believe that is fixed now so future changes should be tested via the CI.

If you can fixup these two failures (see my comments for how to fix them) I think we'll be all set. Thanks for your patience and persistence with this PR!

@@ -12,7 +12,7 @@ test type: bpf-sim
06-sim-actions all write 1 0x856B008 N N N N ERRNO(1)
06-sim-actions all close 4 N N N N N TRAP
06-sim-actions all openat 0 0x856B008 4 N N N TRACE(1234)
06-sim-actions all fstat N N N N N N KILL_PROCESS
06-sim-actions all fstatfs 4 0x856B008 N N N N KILL_PROCESS
06-sim-actions all rt_sigreturn N N N N N N LOG
06-sim-actions x86 0-2 N N N N N N KILL
06-sim-actions x86 7-107 N N N N N N KILL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the test used fstat which has a syscall number of 108 on x86, hence the tests only go up to 107 on x86; with the test now using fstatfs, which is 100 on x86` this test line needs to be updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a similar issue below with x86_64 with fstat being 108 and fstatfs being 100.

tests/06-sim-actions.c Show resolved Hide resolved
[xen0n: LoongArch (and a few upcoming architectures / ABIs) does not
have fstat, so the fstat in 06-sim-actions is also being changed to
fstatfs for uniformity across the board.]

Signed-off-by: Xiaotian Wu <wuxiaotian@loongson.cn>
Signed-off-by: WANG Xuerui <git@xen0n.name>
Signed-off-by: Xiaotian Wu <wuxiaotian@loongson.cn>
@xen0n
Copy link
Contributor

xen0n commented Feb 13, 2023

@yetist is busy today so I'm helping to fix the tests after talking to him. Tests now pass on both x86_64 and loongarch64:

# x86_64 make check
Regression Test Summary
 tests run: 7871
 tests skipped: 84
 tests passed: 7871
 tests failed: 0
 tests errored: 0

# x86_64 live tests
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

# loongarch64 make check
Regression Test Summary
 tests run: 5061
 tests skipped: 124
 tests passed: 5061
 tests failed: 0
 tests errored: 0

# loongarch64 live tests
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

Thank you all for all the work and reviews!

@yetist
Copy link
Contributor Author

yetist commented Feb 14, 2023

@yetist is busy today so I'm helping to fix the tests after talking to him. Tests now pass on both x86_64 and loongarch64:

# x86_64 make check
Regression Test Summary
 tests run: 7871
 tests skipped: 84
 tests passed: 7871
 tests failed: 0
 tests errored: 0

# x86_64 live tests
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

# loongarch64 make check
Regression Test Summary
 tests run: 5061
 tests skipped: 124
 tests passed: 5061
 tests failed: 0
 tests errored: 0

# loongarch64 live tests
Regression Test Summary
 tests run: 11
 tests skipped: 0
 tests passed: 11
 tests failed: 0
 tests errored: 0

Thank you all for all the work and reviews!

Nice, thanks.

Copy link
Member

@pcmoore pcmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks again everyone!

@pcmoore
Copy link
Member

pcmoore commented Feb 19, 2023

I've merged this into the main branch at 7179ff3, thanks for all the work and patience that went into this!

@pcmoore pcmoore closed this Feb 19, 2023
@pcmoore pcmoore added this to the v2.6.0 milestone Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants