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

Move usermode halt logic into backing-specific logic #114

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Move usermode halt logic into backing-specific logic #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc0dfae
Move usermode halt logic into backing-specific logic
smalis-msft Oct 17, 2024
072696b
Rebase fixup
smalis-msft Oct 21, 2024
d4ff8e9
fmt
smalis-msft Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions openhcl/virt_mshv_vtl/src/processor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ mod private {
stop: &mut StopVp<'_>,
) -> impl Future<Output = Result<(), VpHaltReason<UhRunVpError>>>;

/// Returns true if the VP is ready to run the given VTL, false if it is halted.
/// Process any pending APIC work.
fn poll_apic(
this: &mut UhProcessor<'_, Self>,
vtl: GuestVtl,
scan_irr: bool,
) -> Result<bool, UhRunVpError>;
) -> Result<(), UhRunVpError>;

/// Requests the VP to exit when an external interrupt is ready to be
/// delivered.
Expand All @@ -235,6 +235,13 @@ mod private {
/// the target VTL that will become active.
fn switch_vtl_state(this: &mut UhProcessor<'_, Self>, target_vtl: GuestVtl);

/// Returns whether this VP should be put to sleep in usermode, or
/// whether it's ready to proceed into the kernel.
fn halt_in_usermode(this: &mut UhProcessor<'_, Self>, target_vtl: GuestVtl) -> bool {
let _ = (this, target_vtl);
false
}

fn inspect_extra(_this: &mut UhProcessor<'_, Self>, _resp: &mut inspect::Response<'_>) {}
}
}
Expand Down Expand Up @@ -654,15 +661,13 @@ impl<'p, T: Backing> Processor for UhProcessor<'p, T> {
self.update_synic(GuestVtl::Vtl0, true);
}

// TODO CVM GUEST VSM: Split ready into two to track per-vtl
let mut ready = false;
for vtl in [GuestVtl::Vtl1, GuestVtl::Vtl0] {
// Process interrupts.
if self.hv(vtl).is_some() {
self.update_synic(vtl, false);
}

ready |= T::poll_apic(self, vtl, scan_irr[vtl] || first_scan_irr)
T::poll_apic(self, vtl, scan_irr[vtl] || first_scan_irr)
.map_err(VpHaltReason::Hypervisor)?;
}
first_scan_irr = false;
Expand All @@ -675,11 +680,12 @@ impl<'p, T: Backing> Processor for UhProcessor<'p, T> {
}
}

if ready {
// TODO WHP GUEST VSM: This should be next_vtl
if T::halt_in_usermode(self, GuestVtl::Vtl0) {
break Poll::Pending;
} else {
Comment on lines +684 to +686
Copy link
Contributor

@sluck-msft sluck-msft Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So for x64 the halt gets handled here, but in e.g. snp it gets handled in run_vp_snp? Do you know what the reasoning is for the difference in location? Wondering if we should try to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For x64 with apic emulation it gets handled here, which is only the case in test scenarios (WHP). For all other scenarios we call set_halted on the runner and let the kernel manage it for us, as that's more efficient.

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'm not entirely familiar with why we can't do that for WHP, I just tried to preserve existing behavior here.

return <Result<_, VpHaltReason<_>>>::Ok(()).into();
}

break Poll::Pending;
})
.await?;

Expand Down
15 changes: 3 additions & 12 deletions openhcl/virt_mshv_vtl/src/processor/mshv/apic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,13 +262,9 @@ impl UhApicState {

impl UhProcessor<'_, HypervisorBackedX86> {
/// Returns true if the VP is ready to run, false if it is halted.
pub(super) fn poll_apic(
&mut self,
vtl: GuestVtl,
scan_irr: bool,
) -> Result<bool, UhRunVpError> {
pub(super) fn poll_apic(&mut self, vtl: GuestVtl, scan_irr: bool) -> Result<(), UhRunVpError> {
let Some(lapics) = self.backing.lapics.as_mut() else {
return Ok(true);
return Ok(());
};

let lapic = &mut lapics[vtl];
Expand Down Expand Up @@ -309,12 +305,7 @@ impl UhProcessor<'_, HypervisorBackedX86> {
self.handle_sipi(vtl, vector)?;
}

let lapic = &self.backing.lapics.as_ref().unwrap()[vtl];
if lapic.halted || lapic.startup_suspend {
return Ok(false);
}

Ok(true)
Ok(())
}

fn handle_init(&mut self, vtl: GuestVtl) -> Result<(), UhRunVpError> {
Expand Down
4 changes: 2 additions & 2 deletions openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,8 @@ impl BackingPrivate for HypervisorBackedArm64 {
_this: &mut UhProcessor<'_, Self>,
_vtl: GuestVtl,
_scan_irr: bool,
) -> Result<bool, UhRunVpError> {
Ok(true)
) -> Result<(), UhRunVpError> {
Ok(())
}

fn request_extint_readiness(this: &mut UhProcessor<'_, Self>) {
Expand Down
11 changes: 10 additions & 1 deletion openhcl/virt_mshv_vtl/src/processor/mshv/x64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,19 @@ impl BackingPrivate for HypervisorBackedX86 {
this: &mut UhProcessor<'_, Self>,
vtl: GuestVtl,
scan_irr: bool,
) -> Result<bool, UhRunVpError> {
) -> Result<(), UhRunVpError> {
this.poll_apic(vtl, scan_irr)
}

fn halt_in_usermode(this: &mut UhProcessor<'_, Self>, target_vtl: GuestVtl) -> bool {
if let Some(lapics) = this.backing.lapics.as_ref() {
if lapics[target_vtl].halted || lapics[target_vtl].startup_suspend {
return true;
}
}
false
}

fn request_extint_readiness(this: &mut UhProcessor<'_, Self>) {
this.backing
.next_deliverability_notifications
Expand Down
7 changes: 2 additions & 5 deletions openhcl/virt_mshv_vtl/src/processor/snp/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ impl BackingPrivate for SnpBacked {
this: &mut UhProcessor<'_, Self>,
vtl: GuestVtl,
scan_irr: bool,
) -> Result<bool, UhRunVpError> {
) -> Result<(), UhRunVpError> {
// Check for interrupt requests from the host.
// TODO SNP GUEST VSM supporting VTL 1 proxy irrs requires kernel changes
if vtl == GuestVtl::Vtl0 {
Expand Down Expand Up @@ -390,10 +390,7 @@ impl BackingPrivate for SnpBacked {
}
}

// Return ready even if halted. `run_vp` will wait in the kernel when
// halted, which is necessary so that we are efficiently notified when
// more interrupts arrive.
Ok(true)
Ok(())
}

fn request_extint_readiness(_this: &mut UhProcessor<'_, Self>) {
Expand Down
7 changes: 2 additions & 5 deletions openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,7 @@ impl BackingPrivate for TdxBacked {
this: &mut UhProcessor<'_, Self>,
_vtl: GuestVtl,
scan_irr: bool,
) -> Result<bool, UhRunVpError> {
) -> Result<(), UhRunVpError> {
if !this.try_poll_apic(scan_irr)? {
tracing::info!("disabling APIC offload due to auto EOI");
let page = zerocopy::transmute_mut!(this.runner.tdx_apic_page_mut());
Expand All @@ -747,10 +747,7 @@ impl BackingPrivate for TdxBacked {
this.try_poll_apic(false)?;
}

// Return ready even if halted. `run_vp` will wait in the kernel when
// halted, which is necessary so that we are efficiently notified when
// more interrupts arrive.
Ok(true)
Ok(())
}

fn request_extint_readiness(_this: &mut UhProcessor<'_, Self>) {
Expand Down