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

feat(driver): Add loongarch64 kernel module support #1803

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

yzewei
Copy link
Contributor

@yzewei yzewei commented Apr 19, 2024

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:
Add loongarch64 kernel module support.
Temporarily does not involve bpf.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
I'm not sure what problems are there at present. If there is a problem, I hope to point out.

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: yzewei <yangzewei@loongson.cn>
Copy link

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

@yzewei
Copy link
Contributor Author

yzewei commented Apr 19, 2024

@FedeDP I'm not sure why the last PR was closed.
I'm very sorry, it may be caused by my operation error.、
feat(driver): Add loongarch64 kernel module support

@@ -99,14 +99,15 @@ void do___open_by_handle_atX_success(int *open_by_handle_fd, int *dirfd, char *f
}
}

#ifndef __loongarch64
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a small comment around these compilation guards? Eg: "fstat not available on loongarch64" is enough :)

#1654 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, this is what I should do, and I will add them one by one in other sections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be even better to just check for ifdef __NR_fstat ?
Sorry, i came up with that right now 😆
Since it is self explaining, then, we can avoid adding the comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea!

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

This LGTM! I'd also update the [Supported arch matrix] in the readme: https://github.com/falcosecurity/libs?tab=readme-ov-file#drivers-officially-supported-architectures together with the ARCHS label (at the beginning of the readme)

@FedeDP
Copy link
Contributor

FedeDP commented Apr 19, 2024

/milestone next-driver

Signed-off-by: yzewei <yangzewei@loongson.cn>
FedeDP
FedeDP previously approved these changes Apr 19, 2024
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Apr 19, 2024

LGTM label has been added.

Git tree hash: 38ee984291b5621cf9673e58a404cad5c32031dd

@FedeDP
Copy link
Contributor

FedeDP commented Apr 19, 2024

We can update the README later :)
#1803 (review)

@yzewei
Copy link
Contributor Author

yzewei commented Apr 20, 2024

This LGTM! I'd also update the [Supported arch matrix] in the readme: https://github.com/falcosecurity/libs?tab=readme-ov-file#drivers-officially-supported-architectures together with the ARCHS label (at the beginning of the readme)

We can update the README later :) #1803 (review)

Good Job! Thanks a bunch!

@FedeDP
Copy link
Contributor

FedeDP commented Apr 22, 2024

/cc @Andreagit97

@poiana poiana requested a review from Andreagit97 April 22, 2024 06:48
@@ -47,8 +47,8 @@ else()
ExternalProject_Add(protobuf
PREFIX "${PROJECT_BINARY_DIR}/protobuf-prefix"
DEPENDS zlib
URL "https://github.com/protocolbuffers/protobuf/releases/download/v3.17.3/protobuf-cpp-3.17.3.tar.gz"
URL_HASH "SHA256=51cec99f108b83422b7af1170afd7aeb2dd77d2bcbb7b6bad1f92509e9ccf8cb"
URL "https://github.com/protocolbuffers/protobuf/releases/download/v3.20.3/protobuf-cpp-3.20.3.tar.gz"
Copy link
Contributor

@FedeDP FedeDP Apr 22, 2024

Choose a reason for hiding this comment

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

Perhaps i already asked you; is this bump mandatory to build on loongarch64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, the main reason for the replacement is
protobuf 3.17.3 version cannot be compiled with loongarch, because config.guess and config.sub lack loongarch definition, so it needs to be updated to a higher version

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Co-authored-by: Andrea Terzolo <andreaterzolo3@gmail.com>
Signed-off-by: yzewei <141103849+yzewei@users.noreply.github.com>
@poiana poiana removed the lgtm label Apr 22, 2024
@poiana poiana requested a review from FedeDP April 22, 2024 09:28
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Apr 22, 2024
@poiana
Copy link
Contributor

poiana commented Apr 22, 2024

LGTM label has been added.

Git tree hash: 6c91bc4a01d8c9becbd9191239d6516e7fafd7a4

Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Apr 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, yzewei

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Andreagit97 Andreagit97 modified the milestones: next-driver, 7.1.0+driver Apr 22, 2024
@FedeDP
Copy link
Contributor

FedeDP commented Apr 22, 2024

/milestone 7.1.0+driver
/unhold

@poiana poiana merged commit af6bd4f into falcosecurity:master Apr 22, 2024
45 of 50 checks passed
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.

4 participants