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

TSM capabilities and minor editorial changes #82

Merged
merged 17 commits into from
Aug 13, 2024

Conversation

wojciechozga
Copy link
Contributor

We propose to extend the COVH get_tsm_info() call with additional information called TSM capabilities to simplify implementation of a hypervisor that supports different CoVE deployment models. Specifically, TSM informs the hypervisor via the tsm_info structure what capabilities is supports, such as support for single- or mult-step TVM creation, local and/or remote attestation, legacy interrupt handling or AIA, static or dynamic memory allocation.

Additionally, we provide a set of minor editorial changes that clarify: (1) how interrupts are injected if AIA is not supported, (2) what properties local attestation can offer for embedded platforms, (3) how TVM enables or disables injection of all interrupts (wildcard), (4) how MMIO regions are specified, (5) how TVM retrieves local attestation secret.

@rsahita rsahita self-requested a review July 16, 2024 16:52
src/sbi_cove.adoc Outdated Show resolved Hide resolved
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.

Lgtm - thanks for the updates from the 7-16 meeting review

src/sbi_cove.adoc Outdated Show resolved Hide resolved
src/sbi_cove.adoc Outdated Show resolved Hide resolved
/*
* CoVE features supported by the running TSM.
* Enabled bit `i` indicates that the TSM and underly supports capability `i` and there
* is hardware support.
Copy link
Contributor

@atishp04 atishp04 Jul 30, 2024

Choose a reason for hiding this comment

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

The comment sounds bit ambiguous to me w.r.t "there is hardware support". I think something along the lines is sufficient.
"A bitmask of CoVE features supported by the running TSM." - We can refer to the below capability table for additional clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I wrote it like this is that there are cases when software (TSM) and hardware support different set of features. The TSM must report to OS/VMM only features that it can use, i.e., the ones that are supported by software and hardware.

For example, take a platform with hardware support for AIA. If someone runs there a TSM that does not implement support for AIA, OS/VMM would have to use only TSM's legacy interrupt handling.

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 added a reference to the table as suggested.

@atishp04
Copy link
Contributor

Apart from those nit comments, looks good to me.

@wojciechozga
Copy link
Contributor Author

Thanks Atish and Ravi for feedback and valuable comments. I have addressed them and pushed already changes.

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.

LGTM

@rsahita rsahita merged commit ce567b0 into riscv-non-isa:main Aug 13, 2024
2 checks passed
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.

4 participants