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

DONTMERGE Rework of PR 1857 #1944

Closed
wants to merge 6 commits into from
Closed

DONTMERGE Rework of PR 1857 #1944

wants to merge 6 commits into from

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Jul 23, 2024

Description of the changes

It turned out to be easier to try to implement this myself.

How to test this PR?

I was trying Redis in Debug mode (works) and in Release mode (works).


This change is Reviewable

Dmitrii Kuvaiskii added 2 commits July 25, 2024 06:16
It turned out to be easier to try to implement this myself. Works in
both debug and release builds.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
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: 0 of 5 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), "DONTMERGE" and "fixup! " found in commit messages' one-liners


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

    #        RPC request objects, as the AEX flow here. It just so happens that the total size of
    #        allocated RPC request objects is less than 128B (red zone size), so I reuse this const.
    #        This double-stack-usage needs a proper fix.

These are the references:

Note how the red zone is accounted for in the preparation of the untrusted stack, and then the RPC request object is allocated from below that red zone. That's why we need two red zones in this workaround here.

This problem is unrelated to this PR really. Fundamentally this AEX handler and the OCALL logic re-use the same stack, and can do it at the same time. We were just lucky that the AEX handler didn't use the stack, or used the stack but only in DEBUG mode.

I feel like the OCALL stack should be a different memory region than the stack. Guess that's the correct solution.

Dmitrii Kuvaiskii added 3 commits July 25, 2024 10:52
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
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: 0 of 7 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! " and "DONTMERGE" found in commit messages' one-liners


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


    # Inform that we are in AEX profiling code
    movb $1, %gs:PAL_HOST_TCB_IN_AEX_PROF

I think the name of this variable/macro should be changed, dropping _PROF. Now AEX doesn't do only profiling but more.

Also, this asm line should be moved before the previous (lock inc) line.

@dimakuv
Copy link
Contributor Author

dimakuv commented Jul 29, 2024

Jenkins, test this please

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv
Copy link
Contributor Author

dimakuv commented Aug 5, 2024

All code was moved into the main PR #1857, so I'm closing this "proof of concept" PR

@dimakuv dimakuv closed this Aug 5, 2024
@dimakuv dimakuv deleted the dimakuv/sgx-stats-reset branch August 5, 2024 06:59
@dimakuv
Copy link
Contributor Author

dimakuv commented Oct 14, 2024

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

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

These are the references:

Note how the red zone is accounted for in the preparation of the untrusted stack, and then the RPC request object is allocated from below that red zone. That's why we need two red zones in this workaround here.

This problem is unrelated to this PR really. Fundamentally this AEX handler and the OCALL logic re-use the same stack, and can do it at the same time. We were just lucky that the AEX handler didn't use the stack, or used the stack but only in DEBUG mode.

I feel like the OCALL stack should be a different memory region than the stack. Guess that's the correct solution.

This comment is wrong and stale. The problem was in other two places, see the changes in this PR in files enclave_framework.c and enclave_ocalls.c.

Note that the latter place (actual bug) was fixed in a separate PR: #1955

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.

1 participant