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

cvm: Refactor ohcl/ovmm emulator for flexible register retrieval and update #482

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

babayet2
Copy link

@babayet2 babayet2 commented Dec 14, 2024

note: still a couple active discussions about this PR, but no longer in draft mode so we can see CI results

Motivation

  1. When an instruction is emulated for a CVM guest, the full guest register state is retrieved. For TDX, this dispatches an expensive TDCALL to read each segment register regardless of whether it is necessary for the emulation.
  2. The existing emulator code allowed arbitrary guest registers to be written: if the register is considered immutable, the update is silently dropped.

Changes

  • The emulator frontend no longer has a guest state struct. Instead, the frontend calls functions to read and update register state.
  • Each backend defines and implements its own register state struct. On a register read/write, the backend has the flexibility to choose whether a register should be read from this struct, or pass through directly to the backing. With the exception of segment registers for the TDX backing, the existing behavior has been preserved, e.g. reading XMM registers passes through to the backing, and GPs are read from the state struct.

@smmalis37
Copy link
Contributor

Take a look at #478 too, lets make sure we don't conflict too hard.

u128::from_ne_bytes(self.vp.runner.fx_state().xmm[index])
}

fn set_xmm(&mut self, index: usize, v: u128) -> Result<(), Self::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add some documentation on which registers get set immediately, and which only get set when you call flush.

Copy link
Contributor

Choose a reason for hiding this comment

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

And which are immutable i guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these docs would have to be per-backing.

@smalis-msft
Copy link
Contributor

Overall this looks really nice, and a great step in the right direction. Thanks!

@babayet2
Copy link
Author

babayet2 commented Jan 4, 2025

@smalis-msft taking this out of draft mode as several of the CI tests don't fail to run locally for me, I'd like to see the output of the full run

@babayet2 babayet2 changed the title [CVM][DRAFT] Refactor ohcl/ovmm emulator for flexible register retrieval and update cvm: Refactor ohcl/ovmm emulator for flexible register retrieval and update Jan 4, 2025
@babayet2 babayet2 marked this pull request as ready for review January 4, 2025 23:38
@babayet2 babayet2 requested a review from a team as a code owner January 4, 2025 23:38
@smalis-msft
Copy link
Contributor

smalis-msft commented Jan 6, 2025

This looks pretty good to me overall. I've asked @jstarks to also take a pass on it as well though. Also note that all of our windows-hosted vmm tests in this repo rely on virt_whp support, so they'll probably hit your todos. SNP/TDX/MSHV testing is done in an outer loop currently. (We know that situation isn't great and are actively working on making it better).

Comment on lines +919 to -920
guest_memory,
dev,
interruption_pending,
gva_valid,
) {
self.set_emulator_state(&state)
.map_err(VpHaltReason::Hypervisor)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a flush now right? Or is that handled inside emulate_mnf_write_fast_path?

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