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

[PAL/Linux-SGX] Rename sgx_entry to sgx_do_host_ocall #1969

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Aug 12, 2024

Description of the changes

The file host_entry.S contains low-level code that executes ECALL/OCALL/AEX flows in the untrusted runtime of Gramine. Previously, this file contained the label sgx_entry that has a misleading name -- it doesn't "enter the SGX enclave" but instead it "performs an OCALL on the host", i.e. executes the syscall on the host. This commit renames this label to avoid confusion. Also, some explanations are added.

Requested by @mkow while reviewing #1857.

How to test this PR?

No functional changes.


This change is Reviewable

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


pal/src/host/linux-sgx/host_entry.S line 4 at r1 (raw file):

#
#     - Host-to-enclave normal-context flow (ECALL) -- sgx_ecall() function. There are three ECALLs
#       currently: ECALL_ENCLAVE_START, ECALL_THREAD_START and ECALL_THREAD_RESET.

I don't see a point of duplicating the ECALL list here, it's easy to get outdated.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

    .cfi_startproc

    # put entry address in RDX

-> exit target?

Code quote:

entry address

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


pal/src/host/linux-sgx/host_entry.S line 4 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't see a point of duplicating the ECALL list here, it's easy to get outdated.

Done.


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

-> exit target?

Done.

kailun-qin
kailun-qin previously approved these changes Aug 13, 2024
Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Isn't this ECALL exit target? Or OCALL entry?

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Isn't this ECALL exit target? Or OCALL entry?

Done.

That's not an ECALL exit target -- our ECALLs do not return (except for a special ECALL_THREAD_RESET, but it's a different story).

So here I'm talking about the OCALL-entry rip, i.e. where the enclave code will jump out of the enclave to perform an OCALL.

@kailun-qin kailun-qin requested a review from mkow August 14, 2024 00:15
Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

That's not an ECALL exit target -- our ECALLs do not return (except for a special ECALL_THREAD_RESET, but it's a different story).

So here I'm talking about the OCALL-entry rip, i.e. where the enclave code will jump out of the enclave to perform an OCALL.

Yeah, but isn't the term "exit target" for EEXIT? We mentioned similar things in multiple other places IIRC. Alternatively, what about keeping the original _entry suffix in the label, then the original comment would look fine?

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Yeah, but isn't the term "exit target" for EEXIT? We mentioned similar things in multiple other places IIRC. Alternatively, what about keeping the original _entry suffix in the label, then the original comment would look fine?

Done. Well, I removed the word "entry" :) Is this a compromise for everyone?

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Well, I removed the word "entry" :) Is this a compromise for everyone?

Thanks :) LGTM

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners


pal/src/host/linux-sgx/host_entry.S line 27 at r1 (raw file):

That's not an ECALL exit target -- our ECALLs do not return (except for a special ECALL_THREAD_RESET, but it's a different story).

It kinda is? Doing an OCALL is just a return from the ECALL.

@dimakuv
Copy link
Contributor Author

dimakuv commented Aug 14, 2024

Jenkins, retest Jenkins-SGX-20.04-apps please (network connectivity issue)

The file `host_entry.S` contains low-level code that executes
ECALL/OCALL/AEX flows in the untrusted runtime of Gramine. Previously,
this file contained the label `sgx_entry` that has a misleading name --
it doesn't "enter the SGX enclave" but instead it "performs an OCALL on
the host", i.e. executes the syscall on the host. This commit renames
this label to avoid confusion. Also, some explanations are added.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv dimakuv force-pushed the dimakuv/rename-host-entry-funcs branch from 3e3bda5 to 4a73025 Compare August 14, 2024 17:35
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow mkow merged commit 4a73025 into master Aug 15, 2024
18 checks passed
@mkow mkow deleted the dimakuv/rename-host-entry-funcs branch August 15, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants