diff --git a/.github/workflows/dep_rust.yml b/.github/workflows/dep_rust.yml index 389f24faa..07c9f3615 100644 --- a/.github/workflows/dep_rust.yml +++ b/.github/workflows/dep_rust.yml @@ -31,6 +31,49 @@ defaults: shell: bash jobs: + code-checks: + if: ${{ inputs.docs_only == 'false' }} + timeout-minutes: 60 + strategy: + fail-fast: true + matrix: + hypervisor: ['hyperv-ws2025', kvm] + config: [debug, release] + runs-on: ${{ fromJson( + format('["self-hosted", "{0}", "X64", "1ES.Pool=hld-{1}-amd"]', + (matrix.hypervisor == 'hyperv-ws2025') && 'Windows' || 'Linux', + matrix.hypervisor == 'hyperv-ws2025' && 'win2025' || 'kvm')) }} + steps: + - uses: actions/checkout@v4 + + - uses: hyperlight-dev/ci-setup-workflow@v1.5.0 + with: + rust-toolchain: "1.85" + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Does not check for updated Cargo.lock files for test rust guests as this causes an issue with this checkwhen deoendabot updates dependencies in common crates + - name: Ensure up-to-date Cargo.lock + run: | + cargo fetch --locked + + - name: fmt + run: just fmt-check + + - name: clippy + if: ${{ (runner.os == 'Windows' )}} + run: | + just clippy ${{ matrix.config }} + just clippy-guests ${{ matrix.config }} + + - name: clippy exhaustive check + if: ${{ (runner.os == 'Linux' )}} + run: | + just clippy-exhaustive ${{ matrix.config }} + + - name: Verify MSRV + run: ./dev/verify-msrv.sh hyperlight-host hyperlight-guest hyperlight-guest-bin hyperlight-common + build: if: ${{ inputs.docs_only == 'false' }} timeout-minutes: 60 @@ -61,19 +104,6 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - name: fmt - run: just fmt-check - - - name: clippy - run: | - just clippy ${{ matrix.config }} - just clippy-guests ${{ matrix.config }} - - # Does not check for updated Cargo.lock files for test rust guests as this causes an issue with this checkwhen deoendabot updates dependencies in common crates - - name: Ensure up-to-date Cargo.lock - run: | - cargo fetch --locked - - name: Get gh action service name if: ${{ (runner.os == 'Windows' )}} run: (Get-Service actions.runner.*) | Foreach { $_.Name, $_.UserName, $_.ServiceType } @@ -93,9 +123,6 @@ jobs: - name: Build run: just build ${{ matrix.config }} - - name: Verify MSRV - run: ./dev/verify-msrv.sh hyperlight-host hyperlight-guest hyperlight-guest-bin hyperlight-common - - name: Run Rust tests env: CARGO_TERM_COLOR: always diff --git a/Justfile b/Justfile index 66b06a587..cf32292e5 100644 --- a/Justfile +++ b/Justfile @@ -176,6 +176,19 @@ clippy-apply-fix-unix: clippy-apply-fix-windows: cargo clippy --target x86_64-pc-windows-msvc --fix --all +# Run clippy with feature combinations for all packages +clippy-exhaustive target=default-target: (witguest-wit) + ./hack/clippy-package-features.sh hyperlight-host {{ target }} + ./hack/clippy-package-features.sh hyperlight-guest {{ target }} + ./hack/clippy-package-features.sh hyperlight-guest-bin {{ target }} + ./hack/clippy-package-features.sh hyperlight-common {{ target }} + ./hack/clippy-package-features.sh hyperlight-testing {{ target }} + just clippy-guests {{ target }} + +# Test a specific package with all feature combinations +clippy-package package target=default-target: (witguest-wit) + ./hack/clippy-package-features.sh {{ package }} {{ target }} + # Verify Minimum Supported Rust Version verify-msrv: ./dev/verify-msrv.sh hyperlight-host hyperlight-guest hyperlight-guest-lib hyperlight-common diff --git a/hack/clippy-package-features.sh b/hack/clippy-package-features.sh new file mode 100755 index 000000000..dddf582d8 --- /dev/null +++ b/hack/clippy-package-features.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash + +set -euo pipefail + +# Check for required arguments +if [[ $# -lt 2 ]]; then + echo "Usage: $0 " >&2 + echo "Example: $0 hyperlight-host debug" >&2 + exit 1 +fi + +PACKAGE="$1" +TARGET="$2" + +# Convert target for cargo profile +PROFILE=$([ "$TARGET" = "debug" ] && echo "dev" || echo "$TARGET") + +# Required features needed so the rust packages can compile +if [[ "$PACKAGE" == "hyperlight-host" ]]; then + REQUIRED_FEATURES=("kvm" "mshv3") +elif [[ "$PACKAGE" == "hyperlight-guest-bin" ]]; then + REQUIRED_FEATURES=("printf") +else + REQUIRED_FEATURES=() +fi + +# Get all features for the package (excluding default and required features) +# Always exclude "default", and exclude any required features using jq +features=$(cargo metadata --format-version 1 --no-deps | jq -r --arg pkg "$PACKAGE" '.packages[] | select(.name == $pkg) | .features | keys[] | select(. != "default" and (IN($ARGS.positional[])|not))' --args "${REQUIRED_FEATURES[@]}" || true) + +# Convert required features array to comma-separated string for cargo +if [[ ${#REQUIRED_FEATURES[@]} -gt 0 ]]; then + required_features_str=$(IFS=,; echo "${REQUIRED_FEATURES[*]}") +else + required_features_str="" +fi + +# Test with minimal features +if [[ ${#REQUIRED_FEATURES[@]} -gt 0 ]]; then + echo "Testing $PACKAGE with required features only ($required_features_str)..." + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --features "$required_features_str" --profile="$PROFILE" -- -D warnings) +else + echo "Testing $PACKAGE with no features..." + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --profile="$PROFILE" -- -D warnings) +fi + +echo "Testing $PACKAGE with default features..." +(set -x; cargo clippy -p "$PACKAGE" --all-targets --profile="$PROFILE" -- -D warnings) + +# Test each additional feature individually +for feature in $features; do + if [[ ${#REQUIRED_FEATURES[@]} -gt 0 ]]; then + echo "Testing $PACKAGE with feature: $required_features_str,$feature" + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --features "$required_features_str,$feature" --profile="$PROFILE" -- -D warnings) + else + echo "Testing $PACKAGE with feature: $feature" + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --features "$feature" --profile="$PROFILE" -- -D warnings) + fi +done + +# Test all features together +if [[ -n "$features" ]]; then + all_features=$(echo $features | tr '\n' ',' | sed 's/,$//') + if [[ ${#REQUIRED_FEATURES[@]} -gt 0 ]]; then + echo "Testing $PACKAGE with all features: $required_features_str,$all_features" + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --features "$required_features_str,$all_features" --profile="$PROFILE" -- -D warnings) + else + echo "Testing $PACKAGE with all features: $all_features" + (set -x; cargo clippy -p "$PACKAGE" --all-targets --no-default-features --features "$all_features" --profile="$PROFILE" -- -D warnings) + fi +fi \ No newline at end of file diff --git a/src/hyperlight_guest/src/guest_handle/handle.rs b/src/hyperlight_guest/src/guest_handle/handle.rs index c18ba3540..3a22ee26c 100644 --- a/src/hyperlight_guest/src/guest_handle/handle.rs +++ b/src/hyperlight_guest/src/guest_handle/handle.rs @@ -25,7 +25,7 @@ use hyperlight_common::mem::HyperlightPEB; /// /// Guests are expected to initialize this and store it. For example, you /// could store it in a global variable. -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, Default)] pub struct GuestHandle { peb: Option<*mut HyperlightPEB>, } diff --git a/src/hyperlight_guest/src/guest_handle/host_comm.rs b/src/hyperlight_guest/src/guest_handle/host_comm.rs index 97e90f3a6..972685ae1 100644 --- a/src/hyperlight_guest/src/guest_handle/host_comm.rs +++ b/src/hyperlight_guest/src/guest_handle/host_comm.rs @@ -41,13 +41,13 @@ impl GuestHandle { let user_memory_region_size = unsafe { (*peb_ptr).init_data.size }; if num > user_memory_region_size { - return Err(HyperlightGuestError::new( + Err(HyperlightGuestError::new( ErrorCode::GuestError, format!( "Requested {} bytes from user memory, but only {} bytes are available", num, user_memory_region_size ), - )); + )) } else { let user_memory_region_slice = unsafe { core::slice::from_raw_parts(user_memory_region_ptr, num as usize) }; @@ -142,7 +142,7 @@ impl GuestHandle { /// Write an error to the shared output data buffer. pub fn write_error(&self, error_code: ErrorCode, message: Option<&str>) { let guest_error: GuestError = GuestError::new( - error_code.clone(), + error_code, message.map_or("".to_string(), |m| m.to_string()), ); let guest_error_buffer: Vec = (&guest_error) diff --git a/src/hyperlight_guest_bin/src/exceptions/handler.rs b/src/hyperlight_guest_bin/src/exceptions/handler.rs index e2072bf1f..ab0da4cfe 100644 --- a/src/hyperlight_guest_bin/src/exceptions/handler.rs +++ b/src/hyperlight_guest_bin/src/exceptions/handler.rs @@ -21,8 +21,6 @@ use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode; use hyperlight_common::outb::Exception; use hyperlight_guest::exit::abort_with_code_and_message; -use crate::paging; - /// See AMD64 Architecture Programmer's Manual, Volume 2 /// ยง8.9.3 Interrupt Stack Frame, pp. 283--284 /// Figure 8-14: Long-Mode Stack After Interrupt---Same Privilege, @@ -56,9 +54,9 @@ const _: () = assert!(size_of::() == 152 + 512); // TODO: This will eventually need to end up in a per-thread context, // when there are threads. -pub static handlers: [core::sync::atomic::AtomicU64; 31] = +pub static HANDLERS: [core::sync::atomic::AtomicU64; 31] = [const { core::sync::atomic::AtomicU64::new(0) }; 31]; -type handler_t = fn(n: u64, info: *mut ExceptionInfo, ctx: *mut Context, pf_addr: u64) -> bool; +pub type HandlerT = fn(n: u64, info: *mut ExceptionInfo, ctx: *mut Context, pf_addr: u64) -> bool; /// Exception handler #[unsafe(no_mangle)] @@ -89,15 +87,12 @@ pub extern "C" fn hl_exception_handler( // vectors (0-31) if exception_number < 31 { let handler = - handlers[exception_number as usize].load(core::sync::atomic::Ordering::Acquire); + HANDLERS[exception_number as usize].load(core::sync::atomic::Ordering::Acquire); if handler != 0 && unsafe { - core::mem::transmute::<_, handler_t>(handler)( - exception_number, - exn_info, - ctx, - page_fault_address, - ) + core::mem::transmute:: bool>( + handler, + )(exception_number, exn_info, ctx, page_fault_address) } { return; diff --git a/src/hyperlight_guest_bin/src/exceptions/idtr.rs b/src/hyperlight_guest_bin/src/exceptions/idtr.rs index dc547a199..d1d54830b 100644 --- a/src/hyperlight_guest_bin/src/exceptions/idtr.rs +++ b/src/hyperlight_guest_bin/src/exceptions/idtr.rs @@ -49,6 +49,7 @@ pub(crate) unsafe fn load_idt() { // Use &raw mut to get a mutable raw pointer, then dereference it // this is to avoid the clippy warning "shared reference to mutable static" + #[allow(clippy::deref_addrof)] let idtr = &mut *(&raw mut IDTR); idtr.init(expected_base, idt_size as u16); idtr.load(); diff --git a/src/hyperlight_guest_bin/src/guest_function/call.rs b/src/hyperlight_guest_bin/src/guest_function/call.rs index d829e2a85..0c7c99a0e 100644 --- a/src/hyperlight_guest_bin/src/guest_function/call.rs +++ b/src/hyperlight_guest_bin/src/guest_function/call.rs @@ -42,6 +42,7 @@ pub(crate) fn call_guest_function(function_call: FunctionCall) -> Result // Find the function definition for the function call. // Use &raw const to get an immutable reference to the static HashMap // this is to avoid the clippy warning "shared reference to mutable static" + #[allow(clippy::deref_addrof)] if let Some(registered_function_definition) = unsafe { (*(&raw const REGISTERED_GUEST_FUNCTIONS)).get(&function_call.function_name) } { @@ -90,7 +91,7 @@ fn internal_dispatch_function() -> Result<()> { .expect("Function call deserialization failed"); let result_vec = call_guest_function(function_call).inspect_err(|e| { - handle.write_error(e.kind.clone(), Some(e.message.as_str())); + handle.write_error(e.kind, Some(e.message.as_str())); })?; handle.push_shared_output_data(result_vec) diff --git a/src/hyperlight_guest_bin/src/host_comm.rs b/src/hyperlight_guest_bin/src/host_comm.rs index e21097661..ab0b3d46a 100644 --- a/src/hyperlight_guest_bin/src/host_comm.rs +++ b/src/hyperlight_guest_bin/src/host_comm.rs @@ -101,7 +101,7 @@ pub fn print_output_with_host_print(function_call: &FunctionCall) -> Result> 4) >> 32) as u32; + let srand_seed = (((peb_address << 8) ^ (seed >> 4)) >> 32) as u32; // Set the seed for the random number generator for C code using rand; srand(srand_seed); diff --git a/src/hyperlight_guest_bin/src/paging.rs b/src/hyperlight_guest_bin/src/paging.rs index 3d824e680..749900349 100644 --- a/src/hyperlight_guest_bin/src/paging.rs +++ b/src/hyperlight_guest_bin/src/paging.rs @@ -53,8 +53,16 @@ struct MapResponse { } /// Assumption: all are page-aligned +/// # Safety +/// This function modifies pages backing a virtual memory range which is inherently unsafe w.r.t. +/// the Rust memory model. +/// When using this function note: +/// - No locking is performed before touching page table data structures, +/// as such do not use concurrently with any other page table operations +/// - TLB invalidation is not performed, +/// if previously-unmapped ranges are not being mapped, TLB invalidation may need to be performed afterwards. pub unsafe fn map_region(phys_base: u64, virt_base: *mut u8, len: u64) { - let mut pml4_base: u64 = 0; + let mut pml4_base: u64; unsafe { asm!("mov {}, cr3", out(reg) pml4_base); } @@ -71,12 +79,18 @@ pub unsafe fn map_region(phys_base: u64, virt_base: *mut u8, len: u64) { .map(|r| unsafe { alloc_pte_if_needed(r) }) .flat_map(modify_ptes::<20, 12>) .map(|r| map_normal(phys_base, virt_base, r)) - .collect::<()>(); + .for_each(drop); } #[allow(unused)] /// This function is not presently used for anything, but is useful /// for debugging +/// # Safety +/// This function traverses page table data structures, and should not be called concurrently +/// with any other operations that modify the page table. +/// # Panics +/// This function will panic if: +/// - A page map request resolves to multiple page table entries pub unsafe fn dbg_print_address_pte(address: u64) -> u64 { let mut pml4_base: u64 = 0; unsafe { @@ -105,11 +119,18 @@ pub unsafe fn dbg_print_address_pte(address: u64) -> u64 { if addrs.len() != 1 { panic!("impossible: 1 page map request resolved to multiple PTEs"); } - return addrs[0]; + addrs[0] } /// Allocate n contiguous physical pages and return the physical /// addresses of the pages in question. +/// # Safety +/// This function is not inherently unsafe but will likely become so in the future +/// when a real physical page allocator is implemented. +/// # Panics +/// This function will panic if: +/// - The Layout creation fails +/// - Memory allocation fails pub unsafe fn alloc_phys_pages(n: u64) -> u64 { // Currently, since all of main memory is idmap'd, we can just // allocate any appropriately aligned section of memory. @@ -125,8 +146,11 @@ pub unsafe fn alloc_phys_pages(n: u64) -> u64 { } } -pub unsafe fn require_pte_exist(x: MapResponse) -> MapRequest { - let mut pte: u64 = 0; +/// # Safety +/// This function traverses page table data structures, and should not be called concurrently +/// with any other operations that modify the page table. +unsafe fn require_pte_exist(x: MapResponse) -> MapRequest { + let mut pte: u64; unsafe { asm!("mov {}, qword ptr [{}]", out(reg) pte, in(reg) x.entry_ptr); } @@ -141,9 +165,12 @@ pub unsafe fn require_pte_exist(x: MapResponse) -> MapRequest { } } -/// Page-mapping callback to allocate a next-level page table if necessary -pub unsafe fn alloc_pte_if_needed(x: MapResponse) -> MapRequest { - let mut pte: u64 = 0; +/// Page-mapping callback to allocate a next-level page table if necessary. +/// # Safety +/// This function modifies page table data structures, and should not be called concurrently +/// with any other operations that modify the page table. +unsafe fn alloc_pte_if_needed(x: MapResponse) -> MapRequest { + let mut pte: u64; unsafe { asm!("mov {}, qword ptr [{}]", out(reg) pte, in(reg) x.entry_ptr); } @@ -157,6 +184,9 @@ pub unsafe fn alloc_pte_if_needed(x: MapResponse) -> MapRequest { } let page_addr = unsafe { alloc_phys_pages(1) }; unsafe { ptov(page_addr).write_bytes(0u8, OS_PAGE_SIZE as usize) }; + + #[allow(clippy::identity_op)] + #[allow(clippy::precedence)] let pte = page_addr | 1 << 5 | // A - we don't track accesses at table level 0 << 4 | // PCD - leave caching enabled @@ -178,6 +208,8 @@ pub unsafe fn alloc_pte_if_needed(x: MapResponse) -> MapRequest { /// /// TODO: support permissions; currently mapping is always RWX fn map_normal(phys_base: u64, virt_base: *mut u8, r: MapResponse) { + #[allow(clippy::identity_op)] + #[allow(clippy::precedence)] let pte = (phys_base + (r.vmin as u64 - virt_base as u64)) | 1 << 6 | // D - we don't presently track dirty state for anything 1 << 5 | // A - we don't presently track access for anything @@ -194,27 +226,27 @@ fn map_normal(phys_base: u64, virt_base: *mut u8, r: MapResponse) { #[inline(always)] /// Utility function to extract an (inclusive on both ends) bit range /// from a quadword. -fn bits(x: u64) -> u64 { - (x & ((1 << (high_bit + 1)) - 1)) >> low_bit +fn bits(x: u64) -> u64 { + (x & ((1 << (HIGH_BIT + 1)) - 1)) >> LOW_BIT } -struct ModifyPteIterator { +struct ModifyPteIterator { request: MapRequest, n: u64, } -impl Iterator for ModifyPteIterator { +impl Iterator for ModifyPteIterator { type Item = MapResponse; fn next(&mut self) -> Option { - if (self.n << low_bit) >= self.request.len { + if (self.n << LOW_BIT) >= self.request.len { return None; } // next stage parameters - let next_vmin = self.request.vmin.wrapping_add((self.n << low_bit) as usize); + let next_vmin = self.request.vmin.wrapping_add((self.n << LOW_BIT) as usize); let entry_ptr = ptov(self.request.table_base) - .wrapping_add((bits::(next_vmin as u64) << 3) as usize) + .wrapping_add((bits::(next_vmin as u64) << 3) as usize) as *mut u64; - let len_from_here = self.request.len - (self.n << low_bit); - let next_len = core::cmp::min(len_from_here, 1 << low_bit); + let len_from_here = self.request.len - (self.n << LOW_BIT); + let next_len = core::cmp::min(len_from_here, 1 << LOW_BIT); // update our state self.n += 1; @@ -226,9 +258,9 @@ impl Iterator for ModifyPteIterator( +fn modify_ptes( r: MapRequest, -) -> ModifyPteIterator { +) -> ModifyPteIterator { ModifyPteIterator { request: r, n: 0 } } @@ -236,7 +268,7 @@ pub fn flush_tlb() { // Currently this just always flips CR4.PGE back and forth to // trigger a tlb flush. We should use a faster approach where // available - let mut orig_cr4: u64 = 0; + let mut orig_cr4: u64; unsafe { asm!("mov {}, cr4", out(reg) orig_cr4); }