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.

Sign up for GitHub

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

Jump to bottom

Improve SGX labels #130

Closed
mythi opened this issue May 28, 2018 · 12 comments · Fixed by #647
Closed

Improve SGX labels #130

mythi opened this issue May 28, 2018 · 12 comments · Fixed by #647
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@mythi
Copy link
Contributor

mythi commented May 28, 2018

Currently, the SGX label is added as true if the cpuid instruction says SGX is supported. However, it may be that the SGX leaf instructions are disabled from the feature control register making the label void (SXG=true but you cannot execute SGX enclaves).

Additionally, SGX Launch control feature bit is necessary as the label.
@mythi mythi mentioned this issue May 28, 2018
Closed
@okartau
Copy link
Contributor

okartau commented Jul 10, 2018

cpuid-SGX label is correct (note the cpuid part in label name).
"SGX-capable CPU" is not equal to "SGX can be used by SW".
This additional detection complexity is described in docs like https://en.wikipedia.org/wiki/Software_Guard_Extensions
(perhaps we can add a warning note for SGX case in NFD doc?).

The fact that use of SGX requires more (BIOS support, opt-in enabling), does not make cpuid-SGX label void or false.

If we want to see label "SGX can be used" then we need additional detection code somewhere.

Extending related CPUID-based flags:
Current klauspost/cpuid that is used by NFD, does not look further than SGX bit,
so if we want to see more on (Launch control, SGX1/SGX2 capability), we need to either push for additions on cpuid package side, or add own detection code (like we have on NFD/RDT side).

End goal, i.e. to get label "SGX can be used":
Can we rely on kernel cpu flag?
This flag gets added by SGX kernel driver, right?
(As I don't see it on a plain system (Xeon v6) where CPUID shows SGX capability).

@marquiz
Copy link
Contributor

marquiz commented Jul 10, 2018

I agree with @okartau here. The CPUID feature source advertises capabilities as returned by the cpuid instruction. I think we should not change that. It is just a bit unfortunate that the sgx flag from this source is not necessarily very useful for the user.

If this needs to be improved, we should probably implement a new feature source, e.g. using cpu flags (as suggested in #132) or something else.

@mythi
Copy link
Contributor Author

mythi commented Jul 16, 2018

IMO, the title of this issue is valid and the improvement is justified: cpuid-SGX alone cannot be use to determine the node can run enclaves.

@marquiz
Copy link
Contributor

marquiz commented Jul 16, 2018

Probably so. I just tried to point out that the CPUID SGX label is correct and should not be changed (possibly only masked out), and, we need a new feature source (plugin) if we want to have SGX detection.

@marquiz
Copy link
Contributor

marquiz commented Oct 2, 2018

@mythi: Is this feature still needed? What would be the use case for this?

@mythi
Copy link
Contributor Author

mythi commented Oct 2, 2018

@mythi: Is this feature still needed? What would be the use case for this?

I currently don't have the need for this. This was more of a reminder/note that the current cpuid based label for SGX may not give the truth about node SGX capabilities.

@marquiz marquiz added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 20, 2018
@marquiz marquiz mentioned this issue Apr 21, 2019
Merged
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 27, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 27, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mythi mythi mentioned this issue Jan 27, 2020
Merged
askervin added a commit to askervin/node-feature-discovery that referenced this issue Jan 29, 2020
@askervin
vendor: update klauspost/cpuid
d3d1334 
Update cpuid from v1.2.2 to v1.2.3. Brings in SGX improvements and
CPUID leaf 7 feature detection (VBMI2, VPOPCNTDQ, GFNI, VAES,
AVX512BITALG, VPCLMULQDQ, AVX512BF16, AVX512VP2INTERSECT). Blacklist
cpuid-SGX* (issue kubernetes-sigs#130).

Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
mythi added a commit to mythi/node-feature-discovery that referenced this issue Aug 11, 2020
@mythi
source: custom: add rules for cpuid and kconfig
856175a 
Closes: kubernetes-sigs#321
Related: kubernetes-sigs#130

Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
mythi added a commit to mythi/node-feature-discovery that referenced this issue Aug 13, 2020
@mythi
source: custom: add rules for cpuid and kconfig
651899a 
Closes: kubernetes-sigs#321
Related: kubernetes-sigs#130

Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
@mythi
Copy link
Contributor Author

mythi commented Nov 12, 2021

/reopen
/remove-lifecycle rotten

I'll be working on addressing this.

@k8s-ci-robot k8s-ci-robot reopened this Nov 12, 2021
@k8s-ci-robot
Copy link
Contributor

@mythi: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

I'll be working on addressing this.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 12, 2021
@mythi mythi mentioned this issue Nov 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

source/cpu: detect Intel SGX mythi/node-feature-discovery
5 participants
@marquiz @okartau @mythi @k8s-ci-robot @fejta-bot