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

Fix some nitpicks and letfovers from other PRs #2075

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkow
Copy link
Member

@mkow mkow commented Dec 13, 2024

Description of the changes

See the commits.

How to test this PR?

CI.


This change is Reviewable

mkow added 2 commits December 13, 2024 16:33
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 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: ITL) (waiting on @mkow)


pal/src/host/linux-sgx/host_framework.c line 19 at r1 (raw file):

        /* DCAP and upstreamed version used different paths in the past. */
        "/dev/sgx_enclave",
        "/dev/sgx/enclave",

This one probably shouldn't go, it's the one from original DCAP driver, but still compatible, and there's no reason to remove it. Unless we want to instruct people to add custom udev rule if they use old DCAP driver, that will SYMLINK+= it.


python/gramine-sgx-sigstruct-view line 52 at r1 (raw file):

        json.dump(sig_readable, output_txt, indent=4)
    else:
        # plaintext format imitates the legacy output of `gramine-sgx-get-token` tool

We've discussed it and resolved that it should remain, because it's still correct that this imitates this (now non-existent) tool: https://reviewable.io/reviews/gramineproject/gramine/2061#-OC6W8vl-J2dK3hV7s4S

Copy link
Contributor

@chiache chiache left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


pal/src/host/linux-sgx/host_framework.c line 19 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This one probably shouldn't go, it's the one from original DCAP driver, but still compatible, and there's no reason to remove it. Unless we want to instruct people to add custom udev rule if they use old DCAP driver, that will SYMLINK+= it.

To me, it's okay to remove this logic if we instruct people to use the new DCAP driver. Potentially, we can add a warning if "/dev/sgx_enclave" does not exist in the system, but "/dev/sgx/enclave" does.


pal/src/host/linux-sgx/host_framework.c line 35 at r1 (raw file):

    }
    log_error("Cannot open SGX driver device. Please make sure you're using an up-to-date kernel "
              "or the standalone Intel SGX kernel module is loaded.");

Why not keep this warning in case "/dev/sgx_enclave" does not exist? (i.e., ret = -ENOENT)? This warning is more informative and provide instructions to remedy the issue.

Copy link
Member Author

@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, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @chiache and @woju)


pal/src/host/linux-sgx/host_framework.c line 19 at r1 (raw file):

Previously, chiache (Chia-Che Tsai) wrote…

To me, it's okay to remove this logic if we instruct people to use the new DCAP driver. Potentially, we can add a warning if "/dev/sgx_enclave" does not exist in the system, but "/dev/sgx/enclave" does.

But I think we don't support kernels without the SGX driver built-in, which is incompatible with the DCAP driver?
Also, dropping the support has one more advantage IMO - we won't accidentally get bug reports for Gramine running on the older driver :)


pal/src/host/linux-sgx/host_framework.c line 35 at r1 (raw file):

Previously, chiache (Chia-Che Tsai) wrote…

Why not keep this warning in case "/dev/sgx_enclave" does not exist? (i.e., ret = -ENOENT)? This warning is more informative and provide instructions to remedy the issue.

But the error above has all this info, excluding the deprecated part? There's no mention about updating the kernel, because all the supported distros already have it.


python/gramine-sgx-sigstruct-view line 52 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

We've discussed it and resolved that it should remain, because it's still correct that this imitates this (now non-existent) tool: https://reviewable.io/reviews/gramineproject/gramine/2061#-OC6W8vl-J2dK3hV7s4S

Ah, I missed that discussion. But why would we keep this comment if this tool doesn't exist anymore? It doesn't give the reader any useful information?

@chiache chiache requested a review from woju December 23, 2024 19:58
chiache
chiache previously approved these changes Dec 23, 2024
Copy link
Contributor

@chiache chiache 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, 2 unresolved discussions (waiting on @woju)


pal/src/host/linux-sgx/host_framework.c line 19 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But I think we don't support kernels without the SGX driver built-in, which is incompatible with the DCAP driver?
Also, dropping the support has one more advantage IMO - we won't accidentally get bug reports for Gramine running on the older driver :)

I understand we don't support kernels without the SGX driver, but for usability, I think it's useful to provide warning message or hints to users who are using a wrong or incompatible kernel.


pal/src/host/linux-sgx/host_framework.c line 35 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But the error above has all this info, excluding the deprecated part? There's no mention about updating the kernel, because all the supported distros already have it.

I think for backward compatibility reasons, some users may be running on an older kernel that do not have this feature. In that case, they will at least know how to fix it, instead of contacting us in the channel.

Anyhow, I am not blocking this PR, but I think in general we can try to provide more warning or hint to users even if they are not exactly following our instructions.

Copy link
Member Author

@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: 5 of 6 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @woju)


pal/src/host/linux-sgx/host_framework.c line 19 at r1 (raw file):

Previously, chiache (Chia-Che Tsai) wrote…

I understand we don't support kernels without the SGX driver, but for usability, I think it's useful to provide warning message or hints to users who are using a wrong or incompatible kernel.

Ok, makes sense, I added more information to this error, please check now :)


pal/src/host/linux-sgx/host_framework.c line 35 at r1 (raw file):

Previously, chiache (Chia-Che Tsai) wrote…

I think for backward compatibility reasons, some users may be running on an older kernel that do not have this feature. In that case, they will at least know how to fix it, instead of contacting us in the channel.

Anyhow, I am not blocking this PR, but I think in general we can try to provide more warning or hint to users even if they are not exactly following our instructions.

Added, see above.

Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
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