Skip to content

Conversation

@mattkur
Copy link
Contributor

@mattkur mattkur commented Nov 4, 2025

This re-appplies #2215, now that we've figured out the CI issues (I hope).

Copilot AI review requested due to automatic review settings November 4, 2025 19:57
@mattkur mattkur requested review from a team as code owners November 4, 2025 19:57
@mattkur mattkur added the release-ci-required Add to a PR to trigger PR gates in release mode label Nov 4, 2025
@mattkur mattkur requested review from a team, chris-oo and justus-camp-microsoft November 4, 2025 19:57
@github-actions github-actions bot added the unsafe Related to unsafe code label Nov 4, 2025
@github-actions
Copy link

github-actions bot commented Nov 4, 2025

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces heuristics-based private pool memory allocation for OpenHCL's VTL2 DMA operations. The system now automatically calculates appropriate DMA pool sizes based on VP count and VTL2 memory size, eliminating the need for manual configuration in most cases.

  • Adds lookup tables with predefined DMA hint configurations for release and debug builds
  • Implements a new command-line interface OPENHCL_IGVM_VTL2_GPA_POOL_CONFIG supporting heuristics, explicit sizes, or disabling
  • Refactors existing tests to use a structured parameter approach and adds new test cases validating various configurations

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
openhcl/openhcl_boot/src/host_params/dt/dma_hint.rs New module implementing DMA hint calculation with lookup tables and extrapolation logic
openhcl/openhcl_boot/src/cmdline.rs Adds Vtl2GpaPoolConfig enum and parsing logic for the new configuration interface
openhcl/openhcl_boot/src/host_params/dt/mod.rs Integrates the new heuristics into the topology initialization path
openhcl/openhcl_dma_manager/src/lib.rs Adds logging for DMA manager initialization parameters
vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_uefi.rs Refactors tests to use NvmeRelayTestParams struct and adds multiple configuration test cases
vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct.rs Disables private pool for a specific test validating VTL2 RAM allocation
vm/loader/manifests/*.json Updates all manifest files to enable the new heuristics-based configuration
openhcl/openhcl_boot/Cargo.toml Adds test dependencies for tracing support in unit tests
petri/src/vm/mod.rs Adds Debug derive to ProcessorTopology struct

const ONE_MB: u64 = 1024 * 1024;

/// Maximum allowed memory size for DMA hint calculation (1 TiB).
const MAX_DMA_HINT_MEM_SIZE: u64 = 0xFFFFFFFF00000;
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

This magic number represents 1 TiB (as stated in the comment), but the hex value appears incorrect. 1 TiB = 0x10000000000 (1024^4). The current value 0xFFFFFFFF00000 equals ~256 TiB - 1 MiB. Either the comment or the constant should be corrected for clarity.

Suggested change
const MAX_DMA_HINT_MEM_SIZE: u64 = 0xFFFFFFFF00000;
const MAX_DMA_HINT_MEM_SIZE: u64 = 0x10000000000;

Copilot uses AI. Check for mistakes.
Comment on lines +293 to +294
const ONE_MB: u64 = 0x10_0000;

Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

This constant value is incorrect. 0x10_0000 equals 1,048,576 in decimal, which is 1 MiB, not 0x10_0000 which appears to be trying to represent 1 MB. However, this seems to be intentional based on usage. The issue is that this test-scoped ONE_MB shadows and contradicts the module-scoped ONE_MB at line 99 which correctly defines it as 1024 * 1024. Using different values for the same constant name in different scopes is confusing and error-prone.

Suggested change
const ONE_MB: u64 = 0x10_0000;

Copilot uses AI. Check for mistakes.
Comment on lines 24 to 26
#[derive(Default)]
struct NvmeRelayTestParams {
openhcl_cmdline: &'static str,
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

The Default derive will fail because &'static str does not have a sensible default value. This should use #[derive(Default)] with #[default] attributes on fields, or implement Default manually to provide an empty string.

Copilot uses AI. Check for mistakes.
Comment on lines +73 to +79
let num = arg.as_ref().parse::<u64>().unwrap_or(0);
// A size of 0 or failure to parse is treated as disabling
// the pool.
if num == 0 {
Vtl2GpaPoolConfig::Off
} else {
Vtl2GpaPoolConfig::Pages(num)
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

[nitpick] The parsing logic silently converts invalid numeric strings to 'Off' configuration. While documented in the comment, this behavior could mask configuration errors. Consider logging a warning when parse failures occur to help debug misconfigurations.

Suggested change
let num = arg.as_ref().parse::<u64>().unwrap_or(0);
// A size of 0 or failure to parse is treated as disabling
// the pool.
if num == 0 {
Vtl2GpaPoolConfig::Off
} else {
Vtl2GpaPoolConfig::Pages(num)
// Try to parse as u64, log a warning if parsing fails.
match arg.as_ref().parse::<u64>() {
Ok(num) => {
if num == 0 {
Vtl2GpaPoolConfig::Off
} else {
Vtl2GpaPoolConfig::Pages(num)
}
}
Err(_) => {
log!("Warning: Invalid VTL2 GPA pool config value '{}', falling back to 'off'.", arg.as_ref());
Vtl2GpaPoolConfig::Off
}

Copilot uses AI. Check for mistakes.
@mattkur mattkur merged commit b1e04a8 into microsoft:main Nov 4, 2025
58 checks passed
@mattkur mattkur deleted the reapply-heuristics branch November 4, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-ci-required Add to a PR to trigger PR gates in release mode unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants