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

Initial draft of TEE and TEE AIA SBI extensions #5

Merged

Conversation

atulkharerivos
Copy link
Contributor

Here's a first pass at transcribing the existing TEE and TEE AIA interfaces into a SBI format. Note that we'll have to revisit the implementation of sbi_tee_tvm_add_measured_pages() since it's likely not compatible with RISCV32.

specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
is a pointer to the `tvm_create_params` described below, and tvm_create_params_len is
the size of the structure in bytes.

Callers of this API should first invoke `sbi_tee_tsm_get_info()` to obtain information
Copy link
Contributor

Choose a reason for hiding this comment

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

I think sbi_tee_tsm_get_info should be FID #0 so that the reader is already aware of the function. There are multiple reference to tsm_info in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the question raised earlier as well. Should we renumber our existing FIDs so that they appear in a somewhat more chronological sequence? We initially ordered them by alphabetical order (of Rust enums), which is why the function IDs got shuffled around.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think chronological sequence more sense in the spec as it helps coherent reading. All other SBI extensions assign FIDs in that order as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken a stab at it, and have added more exposition...PTAL.

specification/sbi_atee.adoc Outdated Show resolved Hide resolved
unsigned long tvm_page_directory_addr;
/*
The base physical address of the confidential memory region to be used
to hold the TVM’s global state. Must be page-aligned and the number of
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the global state of the TVM ? It is not explained anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed global, and added some exposition...PTAL.

*/
unsigned long tvm_num_vcpus;
/* The base physical address of the confidential memory region to be used
to hold the TVM’s vCPU state. Must be page-aligned and must be at least
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment as above. It would be good to add some explanations about the vCPU & global state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the exposition to see if it helps. I have removed references to the global.

specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
The `tvm_guest_id` is the unique identifier returned by a previous call to`sbi_tee_create_tvm()`,
and the `tvm_vcpu_id` must have been previously added using `sbi_tee_tvm_vcpu_create`.

The `tvm_guest_id` must be in a "runnable" state. Note that the function does not
Copy link
Contributor

Choose a reason for hiding this comment

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

How many states do we have ?

Uninitialized --> Initializing -> Runnable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atishp04 Do we need anything beyond the following?

enum tvm_state {
/* The TVM has been created, but isn't yet ready to run /
TVM_INITIALIZING = 0,
/
The TVM is in a runnable state */
TVM_RUNNABLE = 1,
};

specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch 3 times, most recently from 36e17f8 to 8864703 Compare September 9, 2022 16:46
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
@rsahita rsahita self-assigned this Sep 9, 2022
@rsahita rsahita self-requested a review September 9, 2022 17:05
Copy link
Contributor

@atishp04 atishp04 left a comment

Choose a reason for hiding this comment

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

Overall looking good. Left mostly nit comments.
It would also be good to remove the big table with old function ID names at the end.
You can just have a table with all FIDs for both extensions.

specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch from 8864703 to a0638d7 Compare September 15, 2022 04:32
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch from c2cbeaf to af0593e Compare October 14, 2022 20:42
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Outdated Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch from af0593e to f6af5fe Compare October 18, 2022 22:11
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch 2 times, most recently from 06b31d4 to 1ebc2fd Compare October 20, 2022 17:55
@atulkharerivos atulkharerivos force-pushed the topic/tee_and_tee_aia_sbi_draft branch from 1ebc2fd to 2471ae1 Compare October 21, 2022 02:21
Copy link
Collaborator

@rsahita rsahita left a comment

Choose a reason for hiding this comment

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

thanks Atul - these flows are very good to understand the ABI interactions - some comments:

On the tsm_detection_and_tvm_creation flow:

*as part of the add vcpu loop - TSM should report the number of vcpus that have been added- if the relying party does not see the expected number it can disapprove attestation.
*should also show the other vcpus being entered - since the guest OS will typically maintain its own checks to register all TVM (v)cpus

On the tvm_runtime_execution flow:

  • on the fault/ecall exit - should note that TSM saves (and scrubs) TVM GPR and extended register state (V) before notifying host about TVM exit - similarly loads vhart saved state to resume
  • for demand fault in of confidential memory - should show that the TVM has to accept the GPA before it can use the mapping.
  • we should show a flow for secure interrupt handling - as well as debug and perfmon opt-in also in this runtime svg

On the tvm_destruction_and_memory_reclamation flow:

  • should make a top level note that the host may stop a TVM unilaterally (it does not have to wait for a TVM to exit from TVM-execution loop) - it can achieve that via injection of an interrupt which causes a trap into the TSM giving control to the VMM. (so should show an IPI sequence)
  • post the destroy TVM call - to reclaim confidential memory, should show the same sequence as before to synchronize TLBs to prevent stale mappings being held in a hart for a TVM (and to flush any tracking information that may being held in caches).

@rsahita rsahita marked this pull request as ready for review November 30, 2022 00:20
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
};
-------

== TEE Interrupt Extension (EID #0x54414949)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be updated for interrupt injection.

specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
specification/sbi_atee.adoc Show resolved Hide resolved
@rsahita rsahita merged commit 1534fff into riscv-non-isa:main Nov 30, 2022
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.

5 participants