From 6c546734cf1ed2385216ab524245d407546b4721 Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Sat, 21 Dec 2024 06:01:03 -0800 Subject: [PATCH 1/6] irr-proxy filter code complete for hardware CVM (SNP/TDX), compiling, testt pendiing --- openhcl/hcl/src/ioctl.rs | 21 ++++ openhcl/hcl/src/protocol.rs | 1 + openhcl/virt_mshv_vtl/src/lib.rs | 110 ++++++++++++++++++ .../src/processor/hardware_cvm/mod.rs | 10 ++ openhcl/virt_mshv_vtl/src/processor/mod.rs | 54 ++++++++- .../virt_mshv_vtl/src/processor/mshv/arm64.rs | 6 +- .../virt_mshv_vtl/src/processor/mshv/x64.rs | 6 +- .../virt_mshv_vtl/src/processor/snp/mod.rs | 3 +- .../virt_mshv_vtl/src/processor/tdx/mod.rs | 3 +- openvmm/hvlite_core/src/partition.rs | 5 +- vmm_core/virt_hvf/src/lib.rs | 6 +- vmm_core/virt_kvm/src/arch/aarch64/mod.rs | 5 +- vmm_core/virt_kvm/src/arch/x86_64/mod.rs | 5 +- vmm_core/virt_mshv/src/lib.rs | 10 +- vmm_core/virt_whp/src/lib.rs | 5 +- 15 files changed, 237 insertions(+), 13 deletions(-) diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index 967619e89..5ebba0a1d 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -1854,10 +1854,31 @@ impl<'a, T: Backing> ProcessorRunner<'a, T> { *r = irr.swap(0, Ordering::Relaxed); } } + + // `proxy_irr`received from host is untrusted, only allow vectors that L2 expects + for (f, v) in self.run.as_ref().proxy_irr_filter.iter().zip(r.iter_mut()) { + *v &= *f; + } Some(r) } } + /// Update the `proxy_irr_filter` in run page + pub fn update_proxy_irr_filter(&mut self, irr_filter: [u32; 8]) { + // N.B filter is only updated by user mode and by this processor only + unsafe { + for (dst_irr, src_irr) in self + .run + .as_mut() + .proxy_irr_filter + .iter_mut() + .zip(irr_filter.iter()) + { + *dst_irr = *src_irr; + } + } + } + /// Runs the VP via the sidecar kernel. pub fn run_sidecar(&mut self) -> Result, Error> { self.sidecar.as_mut().unwrap().run().map_err(Error::Sidecar) diff --git a/openhcl/hcl/src/protocol.rs b/openhcl/hcl/src/protocol.rs index 5d9ae0f93..c66211d16 100644 --- a/openhcl/hcl/src/protocol.rs +++ b/openhcl/hcl/src/protocol.rs @@ -126,6 +126,7 @@ pub struct hcl_run { pub vtl_ret_actions: [u8; VTL_RETURN_ACTION_SIZE], pub proxy_irr: [u32; 8], pub target_vtl: HvInputVtl, + pub proxy_irr_filter: [u32; 8], } // The size of hcl_run must be less than or equal to a single 4K page. diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs index f6a2a4840..31704f381 100644 --- a/openhcl/virt_mshv_vtl/src/lib.rs +++ b/openhcl/virt_mshv_vtl/src/lib.rs @@ -37,6 +37,8 @@ pub use processor::UhProcessor; use anyhow::Context as AnyhowContext; use bitfield_struct::bitfield; use bitvec::boxed::BitBox; +#[cfg(guest_arch = "x86_64")] +use bitvec::prelude::*; use bitvec::vec::BitVec; use guestmem::GuestMemory; use hcl::ioctl::Hcl; @@ -166,6 +168,59 @@ pub enum RevokeGuestVsmError { Vtl1AlreadyEnabled, } +/// Device IRR filter global for a partition +#[cfg(guest_arch = "x86_64")] +struct DeviceIrrFilter { + device_irr_filter: BitBox, + proxy_irr_filter_update_vps: BitBox, +} + +#[cfg(guest_arch = "x86_64")] +impl DeviceIrrFilter { + /// New instance for requested VP count + fn new(vp_count: u32) -> Self { + DeviceIrrFilter { + device_irr_filter: BitVec::repeat(false, 255).into_boxed_bitslice(), + proxy_irr_filter_update_vps: BitVec::repeat(false, vp_count as usize) + .into_boxed_bitslice(), + } + } + + /// Check if the `proxy_irr_filter` update is pending/requested + fn is_vp_proxy_irr_filter_update_set(&self, vp_index: u32) -> bool { + self.proxy_irr_filter_update_vps[vp_index as usize] + } + + /// Mark the completion for `proxy_irr_filter` update for VP + fn clr_vp_proxy_irr_filter_update(&self, vp_index: u32) { + self.proxy_irr_filter_update_vps + .set_aliased(vp_index as usize, false); + } + + /// add vector to `device_irr_filter` + fn add_device_vector(&self, vector: u8) { + self.device_irr_filter.set_aliased(vector as usize, true); + } + + /// Set `proxy_irr_filter_update_vps` for all vps, exclusing requester + fn set_vps_proxy_irr_filter_update(&self, req_vp_index: u32) { + for vp_index in self.proxy_irr_filter_update_vps.iter_zeros() { + if vp_index == req_vp_index as usize { + continue; + } + self.proxy_irr_filter_update_vps.set_aliased(vp_index, true); + } + } + + /// Get (OR) all device vector from `device_irr_filter` + fn get_device_irr_vectors(&self, irr_vectors: &mut [u32; 8]) { + let irr_bits = irr_vectors.view_bits_mut::(); + for irr in self.device_irr_filter.iter_ones() { + irr_bits.set(irr, true); + } + } +} + /// Underhill partition. #[derive(Inspect)] pub struct UhPartition { @@ -223,6 +278,9 @@ struct UhPartitionInner { no_sidecar_hotplug: AtomicBool, use_mmio_hypercalls: bool, backing_shared: BackingShared, + #[inspect(skip)] + #[cfg(guest_arch = "x86_64")] + device_irr_filter: RwLock>, } #[derive(Inspect)] @@ -749,6 +807,54 @@ impl UhPartitionInner { self.vps.get(index.index() as usize) } + /// For requester VP to issue `proxy_irr_filter` update to other VPs + #[cfg(guest_arch = "x86_64")] + fn request_proxy_irr_filter_update(&self, vtl: GuestVtl, device_vector: u8, req_vp_index: u32) { + // At a time only one requester VP can issue `proxy_irr_filter` update to other VPs + let device_irr = self.device_irr_filter.write(); + + // Add device vector to `device_irr_filter` + device_irr[vtl].add_device_vector(device_vector); + + // Update `proxy_irr_filter_update_vps` bitmap for all VPs + // excluding the requester VP (requester itself take care of updating its filter) + device_irr[vtl].set_vps_proxy_irr_filter_update(req_vp_index); + + // Wake all the VPs, once the VP wakeup, it will query if `proxy_irr_filter` + // update is pending, and then will update filter (with SINT + device_irr) + for vp in self.vps.iter() { + if vp.cpu_index != req_vp_index { + vp.wake_vtl2(); + } + } + } + + /// Invoke provided callback tp perfom update and + /// Clear `proxy_irr_filter` udpate for given VP[vtl] + #[cfg(guest_arch = "x86_64")] + fn complete_vp_proxy_filter_update(&self, vtl: GuestVtl, vp_index: u32, mut filter_update: F) + where + F: FnMut(), + { + // `device_irr_filter` might be under update from other VP + let device_irr = self.device_irr_filter.read(); + + // Perform filter update action (if update is pending) + if device_irr[vtl].is_vp_proxy_irr_filter_update_set(vp_index) { + filter_update(); + } + + device_irr[vtl].clr_vp_proxy_irr_filter_update(vp_index); + } + + /// Add the current `device_irr_filter` vector to given irr vector + #[cfg(guest_arch = "x86_64")] + fn get_device_irr_filter_vectors(&self, vtl: GuestVtl, irr_vectors: &mut [u32; 8]) { + // `device_irr_filter` might be under update from other VP + let device_irr = self.device_irr_filter.read(); + device_irr[vtl].get_device_irr_vectors(irr_vectors); + } + fn inspect_extra(&self, resp: &mut inspect::Response<'_>) { let mut wake_vps = false; resp.field_mut( @@ -1582,6 +1688,8 @@ impl<'a> UhProtoPartition<'a> { let cvm_state = None; let enter_modes = EnterModes::default(); + #[cfg(guest_arch = "x86_64")] + let vps_count = vps.len() as u32; let partition = Arc::new(UhPartitionInner { hcl, @@ -1609,6 +1717,8 @@ impl<'a> UhProtoPartition<'a> { no_sidecar_hotplug: params.no_sidecar_hotplug.into(), use_mmio_hypercalls: params.use_mmio_hypercalls, backing_shared: BackingShared::new(isolation, BackingSharedParams { cvm_state })?, + #[cfg(guest_arch = "x86_64")] + device_irr_filter: RwLock::new(VtlArray::from_fn(|_| DeviceIrrFilter::new(vps_count))), }); if cfg!(guest_arch = "x86_64") { diff --git a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs index 1999fbe3a..a685d6e89 100644 --- a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs @@ -734,6 +734,16 @@ impl UhHypercallHandler<'_, '_, T, B> { multicast: bool, target_processors: &[u32], ) -> HvResult<()> { + // Before dispatching retarget_device_interrupt, update `proxy_irr_filter` on all other VPs + self.vp.partition.request_proxy_irr_filter_update( + self.intercepted_vtl, + vector as u8, + self.vp.vp_index().index(), + ); + + // Update `proxy_irr_filter` for this VP itself + self.vp.update_proxy_irr_filter(self.intercepted_vtl); + self.vp.partition.hcl.retarget_device_interrupt( device_id, hvdef::hypercall::InterruptEntry { diff --git a/openhcl/virt_mshv_vtl/src/processor/mod.rs b/openhcl/virt_mshv_vtl/src/processor/mod.rs index ae1468828..b4e480471 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mod.rs @@ -35,6 +35,8 @@ use super::UhVpInner; use crate::GuestVsmState; use crate::GuestVtl; use crate::WakeReason; +#[cfg(guest_arch = "x86_64")] +use bitvec::prelude::*; use hcl::ioctl; use hcl::ioctl::ProcessorRunner; use hv1_emulator::message_queues::MessageQueues; @@ -560,7 +562,10 @@ impl<'a, T: Backing> UhProcessor<'a, T> { impl<'p, T: Backing> Processor for UhProcessor<'p, T> { type Error = ProcessorError; type RunVpError = UhRunVpError; - type StateAccess<'a> = T::StateAccess<'p, 'a> where Self: 'a; + type StateAccess<'a> + = T::StateAccess<'p, 'a> + where + Self: 'a; #[cfg(guest_arch = "aarch64")] fn set_debug_state( @@ -689,6 +694,17 @@ impl<'p, T: Backing> Processor for UhProcessor<'p, T> { } for vtl in [GuestVtl::Vtl1, GuestVtl::Vtl0] { + #[cfg(guest_arch = "x86_64")] + if self.partition.isolation.is_hardware_isolated() { + // Complete any proxy filter update if required + self.partition.complete_vp_proxy_filter_update( + vtl, + self.vp_index().index(), + || { + self.update_proxy_irr_filter(vtl); + }, + ); + } // Process interrupts. if self.backing.hv(vtl).is_some() { self.update_synic(vtl, false); @@ -965,6 +981,18 @@ impl<'a, T: Backing> UhProcessor<'a, T> { #[cfg(guest_arch = "x86_64")] fn write_msr(&mut self, msr: u32, value: u64, vtl: GuestVtl) -> Result<(), MsrError> { if msr & 0xf0000000 == 0x40000000 { + // If udpate is Synic MSR, then check if its proxy or previous was proxy + // in eaither case, we need to update the `roxy_irr_filter` + if msr >= hvdef::HV_X64_MSR_SINT0 && msr <= hvdef::HV_X64_MSR_SINT15 { + if let Some(hv) = self.backing.hv(vtl).as_ref() { + let sint_curr = + HvSynicSint::from(hv.synic.sint((msr - hvdef::HV_X64_MSR_SINT0) as u8)); + let sint_new = HvSynicSint::from(value); + if sint_curr.proxy() || sint_new.proxy() { + self.update_proxy_irr_filter(vtl); + } + } + } if let Some(hv) = self.backing.hv_mut(vtl).as_mut() { let r = hv.msr_write(msr, value); if !matches!(r, Err(MsrError::Unknown)) { @@ -1125,6 +1153,30 @@ impl<'a, T: Backing> UhProcessor<'a, T> { self.request_sint_notifications(vtl, pending_sints); } + + #[cfg(guest_arch = "x86_64")] + fn update_proxy_irr_filter(&mut self, vtl: GuestVtl) { + let mut proxy_irr_filter: [u32; 8] = Default::default(); + let irr_bits = proxy_irr_filter.view_bits_mut::(); + + // Get all not maksed && proxy SINT vectors + for sint in 0..NUM_SINTS as u8 { + if let Some(hv) = self.backing.hv(vtl).as_ref() { + let sint_msr = hv.synic.sint(sint); + let hv_sint = HvSynicSint::from(sint_msr); + if hv_sint.proxy() && !hv_sint.masked() { + irr_bits.set(hv_sint.vector() as usize, true); + } + } + } + + // Get all device vectors + self.partition + .get_device_irr_filter_vectors(vtl, &mut proxy_irr_filter); + + // Update final filter in run page + self.runner.update_proxy_irr_filter(proxy_irr_filter); + } } fn signal_mnf(dev: &impl CpuIo, connection_id: u32) { diff --git a/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs b/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs index 30f670b5f..15130ad39 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mshv/arm64.rs @@ -128,7 +128,11 @@ impl BackingPrivate for HypervisorBackedArm64 { fn init(_this: &mut UhProcessor<'_, Self>) {} - type StateAccess<'p, 'a> = UhVpStateAccess<'a, 'p, Self> where Self: 'a + 'p, 'p: 'a; + type StateAccess<'p, 'a> + = UhVpStateAccess<'a, 'p, Self> + where + Self: 'a + 'p, + 'p: 'a; fn access_vp_state<'a, 'p>( this: &'a mut UhProcessor<'p, Self>, diff --git a/openhcl/virt_mshv_vtl/src/processor/mshv/x64.rs b/openhcl/virt_mshv_vtl/src/processor/mshv/x64.rs index 3994bc202..5b9057e78 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mshv/x64.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mshv/x64.rs @@ -176,7 +176,11 @@ impl BackingPrivate for HypervisorBackedX86 { fn init(_this: &mut UhProcessor<'_, Self>) {} - type StateAccess<'p, 'a> = UhVpStateAccess<'a, 'p, Self> where Self: 'a + 'p, 'p: 'a; + type StateAccess<'p, 'a> + = UhVpStateAccess<'a, 'p, Self> + where + Self: 'a + 'p, + 'p: 'a; fn access_vp_state<'a, 'p>( this: &'a mut UhProcessor<'p, Self>, diff --git a/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs b/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs index 1fe850150..960547f4e 100644 --- a/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs @@ -369,7 +369,8 @@ impl BackingPrivate for SnpBacked { .expect("set_vp_registers hypercall for direct overlays should succeed"); } - type StateAccess<'p, 'a> = UhVpStateAccess<'a, 'p, Self> + type StateAccess<'p, 'a> + = UhVpStateAccess<'a, 'p, Self> where Self: 'a + 'p, 'p: 'a; diff --git a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs index 55b1968c4..770c01478 100644 --- a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs @@ -716,7 +716,8 @@ impl BackingPrivate for TdxBacked { }) } - type StateAccess<'p, 'a> = UhVpStateAccess<'a, 'p, Self> + type StateAccess<'p, 'a> + = UhVpStateAccess<'a, 'p, Self> where Self: 'a + 'p, 'p: 'a; diff --git a/openvmm/hvlite_core/src/partition.rs b/openvmm/hvlite_core/src/partition.rs index cfb28a475..e648ac6f6 100644 --- a/openvmm/hvlite_core/src/partition.rs +++ b/openvmm/hvlite_core/src/partition.rs @@ -301,7 +301,10 @@ impl InspectMut for WrappedVp<'_, T> { impl Processor for WrappedVp<'_, T> { type Error = T::Error; type RunVpError = T::RunVpError; - type StateAccess<'a> = T::StateAccess<'a> where Self: 'a; + type StateAccess<'a> + = T::StateAccess<'a> + where + Self: 'a; fn set_debug_state( &mut self, diff --git a/vmm_core/virt_hvf/src/lib.rs b/vmm_core/virt_hvf/src/lib.rs index 1e25ba3b5..7f26a05a5 100644 --- a/vmm_core/virt_hvf/src/lib.rs +++ b/vmm_core/virt_hvf/src/lib.rs @@ -381,7 +381,8 @@ impl virt::PartitionMemoryMap for HvfPartitionInner { } impl virt::PartitionAccessState for HvfPartition { - type StateAccess<'a> = HvfPartitionStateAccess<'a> + type StateAccess<'a> + = HvfPartitionStateAccess<'a> where Self: 'a; @@ -742,7 +743,8 @@ impl<'p> Processor for HvfProcessor<'p> { type Error = Error; type RunVpError = Error; - type StateAccess<'a> = vp_state::HvfVpStateAccess<'a, 'p> + type StateAccess<'a> + = vp_state::HvfVpStateAccess<'a, 'p> where Self: 'a; diff --git a/vmm_core/virt_kvm/src/arch/aarch64/mod.rs b/vmm_core/virt_kvm/src/arch/aarch64/mod.rs index 383dcf162..23ba54f2d 100644 --- a/vmm_core/virt_kvm/src/arch/aarch64/mod.rs +++ b/vmm_core/virt_kvm/src/arch/aarch64/mod.rs @@ -378,7 +378,10 @@ impl<'a> virt::vm::AccessVmState for &'a KvmPartition { impl virt::Processor for KvmProcessor<'_> { type Error = KvmError; type RunVpError = KvmRunVpError; - type StateAccess<'a> = &'a mut Self where Self: 'a; + type StateAccess<'a> + = &'a mut Self + where + Self: 'a; fn set_debug_state( &mut self, diff --git a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs index a5b1d0dc9..8689ce44a 100644 --- a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs +++ b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs @@ -968,7 +968,10 @@ impl hv1_hypercall::SignalEvent for KvmHypercallExit<'_, T> { impl Processor for KvmProcessor<'_> { type Error = KvmError; type RunVpError = KvmRunVpError; - type StateAccess<'a> = KvmVpStateAccess<'a> where Self: 'a; + type StateAccess<'a> + = KvmVpStateAccess<'a> + where + Self: 'a; fn set_debug_state( &mut self, diff --git a/vmm_core/virt_mshv/src/lib.rs b/vmm_core/virt_mshv/src/lib.rs index ebb491181..d63aff818 100644 --- a/vmm_core/virt_mshv/src/lib.rs +++ b/vmm_core/virt_mshv/src/lib.rs @@ -386,7 +386,10 @@ pub struct MshvProcessorBinder { } impl virt::BindProcessor for MshvProcessorBinder { - type Processor<'a> = MshvProcessor<'a> where Self: 'a; + type Processor<'a> + = MshvProcessor<'a> + where + Self: 'a; type Error = Error; fn bind(&mut self) -> Result, Self::Error> { @@ -1265,7 +1268,10 @@ impl InspectMut for MshvProcessor<'_> { impl virt::Processor for MshvProcessor<'_> { type Error = Error; type RunVpError = MshvRunVpError; - type StateAccess<'a> = &'a mut Self where Self: 'a; + type StateAccess<'a> + = &'a mut Self + where + Self: 'a; fn set_debug_state( &mut self, diff --git a/vmm_core/virt_whp/src/lib.rs b/vmm_core/virt_whp/src/lib.rs index 0b4af4f24..6db7904e7 100644 --- a/vmm_core/virt_whp/src/lib.rs +++ b/vmm_core/virt_whp/src/lib.rs @@ -1468,7 +1468,10 @@ impl Drop for WhpProcessor<'_> { impl<'p> virt::Processor for WhpProcessor<'p> { type Error = Error; type RunVpError = WhpRunVpError; - type StateAccess<'a> = WhpVpStateAccess<'a, 'p> where Self: 'a; + type StateAccess<'a> + = WhpVpStateAccess<'a, 'p> + where + Self: 'a; fn set_debug_state( &mut self, From ff5ca4a927b4280399873d05cd783ae16675720a Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Sat, 21 Dec 2024 06:20:52 -0800 Subject: [PATCH 2/6] FIX: Moved filter update after SINT is updated --- openhcl/virt_mshv_vtl/src/processor/mod.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/openhcl/virt_mshv_vtl/src/processor/mod.rs b/openhcl/virt_mshv_vtl/src/processor/mod.rs index b4e480471..5a5919689 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mod.rs @@ -981,21 +981,26 @@ impl<'a, T: Backing> UhProcessor<'a, T> { #[cfg(guest_arch = "x86_64")] fn write_msr(&mut self, msr: u32, value: u64, vtl: GuestVtl) -> Result<(), MsrError> { if msr & 0xf0000000 == 0x40000000 { - // If udpate is Synic MSR, then check if its proxy or previous was proxy - // in eaither case, we need to update the `roxy_irr_filter` + // If updated is Synic MSR, then check if its proxy or previous was proxy + // in either case, we need to update the `proxy_irr_filter` + let mut irr_filter_update = false; if msr >= hvdef::HV_X64_MSR_SINT0 && msr <= hvdef::HV_X64_MSR_SINT15 { if let Some(hv) = self.backing.hv(vtl).as_ref() { let sint_curr = HvSynicSint::from(hv.synic.sint((msr - hvdef::HV_X64_MSR_SINT0) as u8)); let sint_new = HvSynicSint::from(value); if sint_curr.proxy() || sint_new.proxy() { - self.update_proxy_irr_filter(vtl); + irr_filter_update = true; } } } if let Some(hv) = self.backing.hv_mut(vtl).as_mut() { let r = hv.msr_write(msr, value); if !matches!(r, Err(MsrError::Unknown)) { + // SINT updated, check if proxy filter update was required + if irr_filter_update { + self.update_proxy_irr_filter(vtl); + } return r; } } From 2b3bfcb69fa6dd83511d43b0b82b993d1fe65c5c Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Thu, 2 Jan 2025 15:34:06 -0800 Subject: [PATCH 3/6] proxy_irr_filtering working and some refactoring --- Cargo.lock | 1 + openhcl/hcl/Cargo.toml | 1 + openhcl/hcl/src/ioctl.rs | 19 ++++++--------- openhcl/hcl/src/protocol.rs | 2 +- openhcl/virt_mshv_vtl/src/lib.rs | 24 +++++++++++-------- .../src/processor/hardware_cvm/mod.rs | 3 ++- openhcl/virt_mshv_vtl/src/processor/mod.rs | 13 +++++----- 7 files changed, 32 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ce0c623f3..f0773634a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2593,6 +2593,7 @@ name = "hcl" version = "0.0.0" dependencies = [ "bitfield-struct", + "bitvec", "build_rs_guest_arch", "fs-err", "getrandom", diff --git a/openhcl/hcl/Cargo.toml b/openhcl/hcl/Cargo.toml index fd15017a7..980630029 100644 --- a/openhcl/hcl/Cargo.toml +++ b/openhcl/hcl/Cargo.toml @@ -27,6 +27,7 @@ fs-err.workspace = true libc.workspace = true nix = { workspace = true, features = ["ioctl"] } bitfield-struct.workspace = true +bitvec = { workspace = true, features = ["std", "atomic"] } safe_intrinsics.workspace = true open_enum.workspace = true getrandom.workspace = true diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index 5ebba0a1d..e3b727a14 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -25,6 +25,7 @@ use crate::protocol::HCL_VMSA_GUEST_VSM_PAGE_OFFSET; use crate::protocol::HCL_VMSA_PAGE_OFFSET; use crate::protocol::MSHV_APIC_PAGE_OFFSET; use crate::GuestVtl; +use bitvec::prelude::*; use hvdef::hypercall::AssertVirtualInterrupt; use hvdef::hypercall::HostVisibilityType; use hvdef::hypercall::HvGpaRange; @@ -1864,18 +1865,12 @@ impl<'a, T: Backing> ProcessorRunner<'a, T> { } /// Update the `proxy_irr_filter` in run page - pub fn update_proxy_irr_filter(&mut self, irr_filter: [u32; 8]) { - // N.B filter is only updated by user mode and by this processor only - unsafe { - for (dst_irr, src_irr) in self - .run - .as_mut() - .proxy_irr_filter - .iter_mut() - .zip(irr_filter.iter()) - { - *dst_irr = *src_irr; - } + pub fn update_proxy_irr_filter(&mut self, irr_filter: &BitBox) { + // SAFETY: `proxy_irr_filter` is only updated by current processor (and only in user mode for now) + let proxy_irr_filter = unsafe { &mut *addr_of_mut!((*self.run.as_ptr()).proxy_irr_filter) }; + for irr_bit in irr_filter.iter_ones() { + tracing::info!(irr_bit, "update_proxy_irr_filter"); + proxy_irr_filter[irr_bit >> 5] = 1 << (irr_bit & 0x1F); } } diff --git a/openhcl/hcl/src/protocol.rs b/openhcl/hcl/src/protocol.rs index c66211d16..be7199560 100644 --- a/openhcl/hcl/src/protocol.rs +++ b/openhcl/hcl/src/protocol.rs @@ -125,8 +125,8 @@ pub struct hcl_run { pub context: [u8; 1024], pub vtl_ret_actions: [u8; VTL_RETURN_ACTION_SIZE], pub proxy_irr: [u32; 8], - pub target_vtl: HvInputVtl, pub proxy_irr_filter: [u32; 8], + pub target_vtl: HvInputVtl, } // The size of hcl_run must be less than or equal to a single 4K page. diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs index 85b35a566..13cb93e86 100644 --- a/openhcl/virt_mshv_vtl/src/lib.rs +++ b/openhcl/virt_mshv_vtl/src/lib.rs @@ -37,8 +37,6 @@ pub use processor::UhProcessor; use anyhow::Context as AnyhowContext; use bitfield_struct::bitfield; use bitvec::boxed::BitBox; -#[cfg(guest_arch = "x86_64")] -use bitvec::prelude::*; use bitvec::vec::BitVec; use guestmem::GuestMemory; use hcl::ioctl::Hcl; @@ -171,7 +169,7 @@ pub enum RevokeGuestVsmError { /// Device IRR filter global for a partition #[cfg(guest_arch = "x86_64")] struct DeviceIrrFilter { - device_irr_filter: BitBox, + device_irr_filter: BitBox, proxy_irr_filter_update_vps: BitBox, } @@ -180,7 +178,7 @@ impl DeviceIrrFilter { /// New instance for requested VP count fn new(vp_count: u32) -> Self { DeviceIrrFilter { - device_irr_filter: BitVec::repeat(false, 255).into_boxed_bitslice(), + device_irr_filter: BitVec::repeat(false, 256).into_boxed_bitslice(), proxy_irr_filter_update_vps: BitVec::repeat(false, vp_count as usize) .into_boxed_bitslice(), } @@ -213,10 +211,9 @@ impl DeviceIrrFilter { } /// Get (OR) all device vector from `device_irr_filter` - fn get_device_irr_vectors(&self, irr_vectors: &mut [u32; 8]) { - let irr_bits = irr_vectors.view_bits_mut::(); + fn get_device_irr_vectors(&self, irr_vectors: &mut BitBox) { for irr in self.device_irr_filter.iter_ones() { - irr_bits.set(irr, true); + irr_vectors.set(irr, true); } } } @@ -810,6 +807,12 @@ impl UhPartitionInner { /// For requester VP to issue `proxy_irr_filter` update to other VPs #[cfg(guest_arch = "x86_64")] fn request_proxy_irr_filter_update(&self, vtl: GuestVtl, device_vector: u8, req_vp_index: u32) { + tracing::info!( + ?vtl, + device_vector, + req_vp_index, + "request_proxy_irr_filter_update" + ); // At a time only one requester VP can issue `proxy_irr_filter` update to other VPs let device_irr = self.device_irr_filter.write(); @@ -841,15 +844,16 @@ impl UhPartitionInner { // Perform filter update action (if update is pending) if device_irr[vtl].is_vp_proxy_irr_filter_update_set(vp_index) { + tracing::info!(?vtl, vp_index, "complete_vp_proxy_filter_update"); filter_update(); + // clear pending IRR update for VP + device_irr[vtl].clr_vp_proxy_irr_filter_update(vp_index); } - - device_irr[vtl].clr_vp_proxy_irr_filter_update(vp_index); } /// Add the current `device_irr_filter` vector to given irr vector #[cfg(guest_arch = "x86_64")] - fn get_device_irr_filter_vectors(&self, vtl: GuestVtl, irr_vectors: &mut [u32; 8]) { + fn get_device_irr_filter_vectors(&self, vtl: GuestVtl, irr_vectors: &mut BitBox) { // `device_irr_filter` might be under update from other VP let device_irr = self.device_irr_filter.read(); device_irr[vtl].get_device_irr_vectors(irr_vectors); diff --git a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs index ce1e324c7..72ba502f6 100644 --- a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs @@ -723,7 +723,8 @@ impl UhHypercallHandler<'_, '_, T, B> { multicast: bool, target_processors: &[u32], ) -> HvResult<()> { - // Before dispatching retarget_device_interrupt, update `proxy_irr_filter` on all other VPs + // Before dispatching retarget_device_interrupt, mark `proxy_irr_filter` + // update pending on all other VPs and add device vector to `device_irr_filter` self.vp.partition.request_proxy_irr_filter_update( self.intercepted_vtl, vector as u8, diff --git a/openhcl/virt_mshv_vtl/src/processor/mod.rs b/openhcl/virt_mshv_vtl/src/processor/mod.rs index 7b987dc06..0735a6156 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mod.rs @@ -985,7 +985,7 @@ impl<'a, T: Backing> UhProcessor<'a, T> { // If updated is Synic MSR, then check if its proxy or previous was proxy // in either case, we need to update the `proxy_irr_filter` let mut irr_filter_update = false; - if msr >= hvdef::HV_X64_MSR_SINT0 && msr <= hvdef::HV_X64_MSR_SINT15 { + if matches!(msr, hvdef::HV_X64_MSR_SINT0..=hvdef::HV_X64_MSR_SINT15) { if let Some(hv) = self.backing.hv(vtl).as_ref() { let sint_curr = HvSynicSint::from(hv.synic.sint((msr - hvdef::HV_X64_MSR_SINT0) as u8)); @@ -1164,12 +1164,11 @@ impl<'a, T: Backing> UhProcessor<'a, T> { #[cfg(guest_arch = "x86_64")] fn update_proxy_irr_filter(&mut self, vtl: GuestVtl) { - let mut proxy_irr_filter: [u32; 8] = Default::default(); - let irr_bits = proxy_irr_filter.view_bits_mut::(); + let mut irr_bits: BitBox = BitVec::repeat(false, 256).into_boxed_bitslice(); // Get all not maksed && proxy SINT vectors - for sint in 0..NUM_SINTS as u8 { - if let Some(hv) = self.backing.hv(vtl).as_ref() { + if let Some(hv) = self.backing.hv(vtl).as_ref() { + for sint in 0..NUM_SINTS as u8 { let sint_msr = hv.synic.sint(sint); let hv_sint = HvSynicSint::from(sint_msr); if hv_sint.proxy() && !hv_sint.masked() { @@ -1180,10 +1179,10 @@ impl<'a, T: Backing> UhProcessor<'a, T> { // Get all device vectors self.partition - .get_device_irr_filter_vectors(vtl, &mut proxy_irr_filter); + .get_device_irr_filter_vectors(vtl, &mut irr_bits); // Update final filter in run page - self.runner.update_proxy_irr_filter(proxy_irr_filter); + self.runner.update_proxy_irr_filter(&irr_bits); } } From 5a0e6ca046371e6a7ba6edca684a364def7a4c51 Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Thu, 2 Jan 2025 16:14:39 -0800 Subject: [PATCH 4/6] Removed IRR filter todo comment --- openhcl/virt_mshv_vtl/src/processor/snp/mod.rs | 2 +- openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs b/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs index f664079a3..39751d903 100644 --- a/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/snp/mod.rs @@ -396,7 +396,7 @@ impl BackingPrivate for SnpBacked { // TODO SNP GUEST VSM supporting VTL 1 proxy irrs requires kernel changes if vtl == GuestVtl::Vtl0 { if let Some(irr) = this.runner.proxy_irr() { - // TODO SNP: filter proxy IRRs. + // N.B proxy IRRs received are filtered this.backing.cvm.lapics[vtl] .lapic .request_fixed_interrupts(irr); diff --git a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs index af78b5970..b95b1db59 100644 --- a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs @@ -856,7 +856,7 @@ impl UhProcessor<'_, TdxBacked> { // Check for interrupt requests from the host. let mut update_rvi = false; if let Some(irr) = self.runner.proxy_irr() { - // TODO TDX: filter proxy IRRs. + // N.B proxy IRRs received are filtered if self.backing.cvm.lapics[GuestVtl::Vtl0] .lapic .can_offload_irr() From 1bae5f60b5629ef44239f9a4e875d3d2b40fa8f1 Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Thu, 9 Jan 2025 21:42:41 -0800 Subject: [PATCH 5/6] Accessing as atomic during updates, implemented inspect for filter and review comments --- Cargo.lock | 1 - openhcl/hcl/Cargo.toml | 1 - openhcl/hcl/src/ioctl.rs | 24 +++++++++++----------- openhcl/virt_mshv_vtl/src/lib.rs | 2 +- openhcl/virt_mshv_vtl/src/processor/mod.rs | 21 ++++++++----------- support/inspect/src/lib.rs | 1 + 6 files changed, 23 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2b086ced5..ba21e2ee7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2593,7 +2593,6 @@ name = "hcl" version = "0.0.0" dependencies = [ "bitfield-struct", - "bitvec", "build_rs_guest_arch", "fs-err", "getrandom", diff --git a/openhcl/hcl/Cargo.toml b/openhcl/hcl/Cargo.toml index 980630029..fd15017a7 100644 --- a/openhcl/hcl/Cargo.toml +++ b/openhcl/hcl/Cargo.toml @@ -27,7 +27,6 @@ fs-err.workspace = true libc.workspace = true nix = { workspace = true, features = ["ioctl"] } bitfield-struct.workspace = true -bitvec = { workspace = true, features = ["std", "atomic"] } safe_intrinsics.workspace = true open_enum.workspace = true getrandom.workspace = true diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index 3168261e6..0a5d339a6 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -26,8 +26,6 @@ use crate::protocol::HCL_VMSA_GUEST_VSM_PAGE_OFFSET; use crate::protocol::HCL_VMSA_PAGE_OFFSET; use crate::protocol::MSHV_APIC_PAGE_OFFSET; use crate::GuestVtl; -use bitvec::prelude::BitArray; -use bitvec::prelude::Lsb0; use hvdef::hypercall::AssertVirtualInterrupt; use hvdef::hypercall::HostVisibilityType; use hvdef::hypercall::HvGpaRange; @@ -1594,7 +1592,8 @@ impl HclVp { let fd = &hcl.mshv_vtl.file; let run: MappedPage = MappedPage::new(fd, vp as i64).map_err(|e| Error::MmapVp(e, None))?; - // SAFETY: Initializing `proxy_irr_blocked` to block all initially + // SAFETY: `proxy_irr_blocked` is not accessed by any other VPs/kernel at this point (`HclVp` creation) + // so we know we have exclusive access. Initializing to block all vectors by default let proxy_irr_blocked = unsafe { &mut *addr_of_mut!((*run.0.as_ptr()).proxy_irr_blocked) }; proxy_irr_blocked.fill(0xFFFFFFFF); @@ -1864,16 +1863,17 @@ impl<'a, T: Backing> ProcessorRunner<'a, T> { } /// Update the `proxy_irr_blocked` in run page - pub fn update_proxy_irr_filter(&mut self, irr_filter: &BitArray<[u32; 8], Lsb0>) { - // SAFETY: `proxy_irr_blocked` is accessed in both user and kernel, but from current VP - // By default block all (i.e. set all), and only allow (unset) given vectors - let proxy_irr_blocked = - unsafe { &mut *addr_of_mut!((*self.run.as_ptr()).proxy_irr_blocked) }; - proxy_irr_blocked.fill(0xFFFFFFFF); + pub fn update_proxy_irr_filter(&mut self, irr_filter: &[u32; 8]) { + // SAFETY: `proxy_irr_blocked` is accessed by current VP only, but could + // be concurrently accessed by kernel too, hence accessing as Atomic + let proxy_irr_blocked = unsafe { + &mut *(addr_of_mut!((*self.run.as_ptr()).proxy_irr_blocked).cast::<[AtomicU32; 8]>()) + }; - for irr_bit in irr_filter.iter_ones() { - tracing::debug!(irr_bit, "update_proxy_irr_filter"); - proxy_irr_blocked[irr_bit >> 5] &= !(1 << (irr_bit & 0x1F)); + // By default block all (i.e. set all), and only allow (unset) given vectors + for (filter, irr) in proxy_irr_blocked.iter_mut().zip(irr_filter.iter()) { + filter.store(0xFFFFFFFF & (!irr), Ordering::Relaxed); + tracing::debug!(irr, "update_proxy_irr_filter"); } } diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs index 77e5c18eb..b0cd2e1ac 100644 --- a/openhcl/virt_mshv_vtl/src/lib.rs +++ b/openhcl/virt_mshv_vtl/src/lib.rs @@ -226,9 +226,9 @@ struct UhPartitionInner { no_sidecar_hotplug: AtomicBool, use_mmio_hypercalls: bool, backing_shared: BackingShared, - #[inspect(skip)] #[cfg(guest_arch = "x86_64")] // N.B For now, only one device vector table i.e. for VTL0 only + #[inspect(with = "|x| inspect::iter_by_index(x.read().into_inner().map(inspect::AsHex))")] device_vector_table: RwLock, } diff --git a/openhcl/virt_mshv_vtl/src/processor/mod.rs b/openhcl/virt_mshv_vtl/src/processor/mod.rs index 89d3abebf..3de0399d2 100644 --- a/openhcl/virt_mshv_vtl/src/processor/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/mod.rs @@ -938,10 +938,9 @@ impl<'a, T: Backing> UhProcessor<'a, T> { } #[cfg(guest_arch = "x86_64")] - if wake_reasons.update_proxy_irr_filter() - && self.partition.isolation.is_hardware_isolated() - { + if wake_reasons.update_proxy_irr_filter() { // update `proxy_irr_blocked` filter + debug_assert!(self.partition.isolation.is_hardware_isolated()); self.update_proxy_irr_filter(vtl); } } @@ -976,11 +975,11 @@ impl<'a, T: Backing> UhProcessor<'a, T> { #[cfg(guest_arch = "x86_64")] fn write_msr(&mut self, msr: u32, value: u64, vtl: GuestVtl) -> Result<(), MsrError> { if msr & 0xf0000000 == 0x40000000 { - // If updated is Synic MSR, then check if its proxy or previous was proxy - // in either case, we need to update the `proxy_irr_blocked` - let mut irr_filter_update = false; - if matches!(msr, hvdef::HV_X64_MSR_SINT0..=hvdef::HV_X64_MSR_SINT15) { - if let Some(hv) = self.backing.hv(vtl).as_ref() { + if let Some(hv) = self.backing.hv_mut(vtl).as_mut() { + // If updated is Synic MSR, then check if its proxy or previous was proxy + // in either case, we need to update the `proxy_irr_blocked` + let mut irr_filter_update = false; + if matches!(msr, hvdef::HV_X64_MSR_SINT0..=hvdef::HV_X64_MSR_SINT15) { let sint_curr = HvSynicSint::from(hv.synic.sint((msr - hvdef::HV_X64_MSR_SINT0) as u8)); let sint_new = HvSynicSint::from(value); @@ -988,11 +987,9 @@ impl<'a, T: Backing> UhProcessor<'a, T> { irr_filter_update = true; } } - } - if let Some(hv) = self.backing.hv_mut(vtl).as_mut() { let r = hv.msr_write(msr, value); if !matches!(r, Err(MsrError::Unknown)) { - // SINT updated, check if proxy filter update was required + // Check if proxy filter update was required (in case of SINT writes) if irr_filter_update { self.update_proxy_irr_filter(vtl); } @@ -1175,7 +1172,7 @@ impl<'a, T: Backing> UhProcessor<'a, T> { self.partition.get_device_vectors(vtl, &mut irr_bits); // Update `proxy_irr_blocked` filter in run page - self.runner.update_proxy_irr_filter(&irr_bits); + self.runner.update_proxy_irr_filter(&irr_bits.into_inner()); } } diff --git a/support/inspect/src/lib.rs b/support/inspect/src/lib.rs index 88e5cd542..11af812e8 100644 --- a/support/inspect/src/lib.rs +++ b/support/inspect/src/lib.rs @@ -1605,6 +1605,7 @@ macro_rules! hexbincount { /// Wrapper around `T` that implements [`Inspect`] by writing a value with /// [`ValueFlags::hex`]. +#[derive(Clone, Copy)] pub struct AsHex(pub T); hexbincount!(AsHex, into_hex, u8, u16, u32, u64, usize); From 34bfcc43f0a03d99db9edf0348e42f85d41617c1 Mon Sep 17 00:00:00 2001 From: "Mehta, Kunal" Date: Thu, 9 Jan 2025 22:40:11 -0800 Subject: [PATCH 6/6] clippy warning fix: removed unnecessary mask while updating proxy_irr_blocked filter --- openhcl/hcl/src/ioctl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index 0a5d339a6..3baaf8df4 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -1872,7 +1872,7 @@ impl<'a, T: Backing> ProcessorRunner<'a, T> { // By default block all (i.e. set all), and only allow (unset) given vectors for (filter, irr) in proxy_irr_blocked.iter_mut().zip(irr_filter.iter()) { - filter.store(0xFFFFFFFF & (!irr), Ordering::Relaxed); + filter.store(!irr, Ordering::Relaxed); tracing::debug!(irr, "update_proxy_irr_filter"); } }