Skip to content

virt_mshv_vtl: add inspection support for cvm cpuid tables #1048

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
46 changes: 43 additions & 3 deletions openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use self::snp::SnpCpuidSupport;
use self::tdx::TdxCpuidInitializer;
use self::tdx::TdxCpuidSupport;
use core::arch::x86_64::CpuidResult;
use inspect::Inspect;
use masking::CpuidResultMask;
use snp::SnpCpuidInitializer;
use std::boxed::Box;
Expand Down Expand Up @@ -88,7 +89,7 @@ trait CpuidArchInitializer {
}

/// Architecture-specific behaviors for cpuid results during runtime
trait CpuidArchSupport: Sync + Send {
trait CpuidArchSupport: Sync + Send + Inspect {
/// Get the cpuid result of the leaf/subleaf but modified based on guest
/// context
fn process_guest_result(
Expand Down Expand Up @@ -178,8 +179,11 @@ pub struct ParsedCpuidEntry {
}

/// Prepares and caches the results that should be returned for hardware CVMs.
#[derive(Inspect)]
pub struct CpuidResults {
Copy link
Member

Choose a reason for hiding this comment

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

I still want this whole thing merged with the non-CVM cpuid handling. We shouldn't have two completely different table formats.

But anyway, that's certainly out of scope for this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree - it's really annoying that they're completely different.

#[inspect(with = "inspect_helpers::cpuid_table")]
results: HashMap<CpuidFunction, CpuidEntry>,
#[inspect(hex)]
max_extended_state: u64,
arch_support: Box<dyn CpuidArchSupport>,
vps_per_socket: u32,
Expand All @@ -188,9 +192,45 @@ pub struct CpuidResults {
type CpuidSubtable = HashMap<u32, CpuidResult>;

/// Entry in [`CpuidResults`] for caching leaf value or its subleaves.
#[derive(Inspect)]
#[inspect(tag = "type")]
enum CpuidEntry {
Leaf(CpuidResult),
Subtable(CpuidSubtable),
#[inspect(transparent)]
Leaf(#[inspect(with = "inspect_helpers::cpuid_result")] CpuidResult),
#[inspect(transparent)]
Subtable(#[inspect(with = "inspect_helpers::cpuid_subtable")] CpuidSubtable),
}

mod inspect_helpers {
use super::*;
use inspect::AsHex;
use inspect::Inspect;

pub(super) fn cpuid_result(result: &CpuidResult) -> impl Inspect + '_ {
inspect::adhoc(|req| {
req.respond()
.field("eax", AsHex(result.eax))
.field("ebx", AsHex(result.ebx))
.field("ecx", AsHex(result.ecx))
.field("edx", AsHex(result.edx));
})
}

pub(super) fn cpuid_table(table: &HashMap<CpuidFunction, CpuidEntry>) -> impl Inspect + '_ {
inspect::iter_by_key(
table
.iter()
.map(|(key, value)| (format!("{:?} ({:x})", key, key.0), value)),
Copy link
Member

Choose a reason for hiding this comment

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

Can we just make the key the hex value in both cases? I don't want spaces and parens and open_enum identifiers in keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we can just make it the hex key, i found it useful to have both (but that's because i just started looking at cpuid so i don't have the idents memorized)

)
}

pub(super) fn cpuid_subtable(table: &CpuidSubtable) -> impl Inspect + '_ {
inspect::iter_by_key(
table
.iter()
.map(|(key, value)| (format!("{:x?}", key), cpuid_result(value))),
Copy link
Member

Choose a reason for hiding this comment

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

Seems like it should be :#x and not :x?

)
}
}

/// Guest state needed to compute the cpuid result for a specific execution
Expand Down
7 changes: 7 additions & 0 deletions openhcl/virt_mshv_vtl/src/cvm_cpuid/snp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use super::ParsedCpuidEntry;
use super::TopologyError;
use super::ZERO_CPUID_RESULT;
use core::arch::x86_64::CpuidResult;
use inspect::Inspect;
use x86defs::cpuid;
use x86defs::cpuid::CpuidFunction;
use x86defs::snp::HvPspCpuidPage;
Expand Down Expand Up @@ -494,6 +495,12 @@ impl CpuidArchSupport for SnpCpuidSupport {
}
}

impl Inspect for SnpCpuidSupport {
fn inspect(&self, req: inspect::Request<'_>) {
req.respond().field("type", "snp-cpuid");
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
7 changes: 7 additions & 0 deletions openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use super::CpuidSubtable;
use super::ParsedCpuidEntry;
use super::TopologyError;
use core::arch::x86_64::CpuidResult;
use inspect::Inspect;
use x86defs::cpuid;
use x86defs::cpuid::CpuidFunction;
use x86defs::xsave;
Expand Down Expand Up @@ -252,3 +253,9 @@ impl CpuidArchSupport for TdxCpuidSupport {
// Nothing extra to do for TDX
}
}

impl Inspect for TdxCpuidSupport {
fn inspect(&self, req: inspect::Request<'_>) {
req.respond().field("type", "tdx-cpuid");
}
}
1 change: 0 additions & 1 deletion openhcl/virt_mshv_vtl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,6 @@ impl UhCvmVpState {
/// Partition-wide state for CVMs.
struct UhCvmPartitionState {
#[cfg(guest_arch = "x86_64")]
#[inspect(skip)]
cpuid: cvm_cpuid::CpuidResults,
/// VPs that have locked their TLB.
#[inspect(
Expand Down