From ff767ca18e9da03ae08d5530a5e2d066fef3eb17 Mon Sep 17 00:00:00 2001 From: Dylan Reid Date: Wed, 30 Apr 2025 00:55:10 -0700 Subject: [PATCH 1/4] imsic: Specific error for location alignment Use a different error for different failure cases. Signed-off-by: Dylan Reid --- drivers/src/imsic/core.rs | 2 +- drivers/src/imsic/error.rs | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/src/imsic/core.rs b/drivers/src/imsic/core.rs index 34a85389..a2db1538 100644 --- a/drivers/src/imsic/core.rs +++ b/drivers/src/imsic/core.rs @@ -357,7 +357,7 @@ impl Imsic { // Each MMIO region must map the start of a group. let location = geometry .addr_to_location(region_base_addr) - .ok_or_else(|| Error::InvalidMmioRegion(region_base_addr.bits()))?; + .ok_or_else(|| Error::InvalidMmioRegionLocation(region_base_addr.bits()))?; if location.hart().bits() != 0 || location.file().bits() != 0 { return Err(Error::InvalidMmioRegion(region_base_addr.bits())); } diff --git a/drivers/src/imsic/error.rs b/drivers/src/imsic/error.rs index 225e0c96..664e0ff8 100644 --- a/drivers/src/imsic/error.rs +++ b/drivers/src/imsic/error.rs @@ -27,6 +27,8 @@ pub enum Error { MisalignedMmioRegion(u64), /// An MMIO region specified in the device tree did not match the expected pattern. InvalidMmioRegion(u64), + /// Regions doesn't map to a hart-stride aligned address. + InvalidMmioRegionLocation(u64), /// Invalid parent interrupt specification in the device tree. InvalidParentInterrupt(u32, u32), /// The given CPU does not have a valid IMSIC location specified in the device tree. From 4059df277cb435a9db1c09b40394c838854a96d2 Mon Sep 17 00:00:00 2001 From: Dylan Reid Date: Wed, 30 Apr 2025 00:57:11 -0700 Subject: [PATCH 2/4] imsic: Support sparse imsic regions If m-mode interrupt files are interleaved with S and guest files, then the device tree may report holes in the reg map for the imsics, with each cpu's files separated by a missing page. Secondary reg regions will have non-zero hart ids, so remove the check that all regions are for hart 0. Similarly the per_hart_size is more of a stride and it's possible to have less than that size in each region. Signed-off-by: Dylan Reid --- drivers/src/imsic/core.rs | 8 ++++---- drivers/src/imsic/error.rs | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/src/imsic/core.rs b/drivers/src/imsic/core.rs index a2db1538..ae414e48 100644 --- a/drivers/src/imsic/core.rs +++ b/drivers/src/imsic/core.rs @@ -358,15 +358,15 @@ impl Imsic { let location = geometry .addr_to_location(region_base_addr) .ok_or_else(|| Error::InvalidMmioRegionLocation(region_base_addr.bits()))?; - if location.hart().bits() != 0 || location.file().bits() != 0 { + if location.file().bits() != 0 { return Err(Error::InvalidMmioRegion(region_base_addr.bits())); } // Unwrap ok, we've guaranteed there are an even number of `reg` cells. let region_size = regs.next().unwrap(); - if region_size % per_hart_size != 0 { - return Err(Error::MisalignedMmioRegion(region_base_addr.bits())); + if region_size < guests_per_hart as u64 * PageSize::Size4k as u64 { + return Err(Error::MmioRegionTooSmall(region_size)); } - let harts_in_region = region_size / per_hart_size; + let harts_in_region = core::cmp::max(region_size / per_hart_size, 1); for i in 0..harts_in_region { // Each 'interrupts-extended' property is of the from " ". diff --git a/drivers/src/imsic/error.rs b/drivers/src/imsic/error.rs index 664e0ff8..d197f3fc 100644 --- a/drivers/src/imsic/error.rs +++ b/drivers/src/imsic/error.rs @@ -39,6 +39,8 @@ pub enum Error { TooManyInterruptFiles, /// There were fewer interrupt files than CPUs found in the device tree. MissingInterruptFiles, + /// An imsic reg regions was specified that is too small to hold the guest files. + MmioRegionTooSmall(u64), /// Failed to add an MMIO region to the system memory map. AddingMmioRegion(page_tracking::MemMapError), /// The requested CPU does not exist. From 411a9d304377346ba41feb89202c88052c4e3069 Mon Sep 17 00:00:00 2001 From: Dylan Reid Date: Fri, 2 May 2025 12:51:14 -0700 Subject: [PATCH 3/4] page-tracking: record size in no space error Having the amount of space needed available makes debugging more convenient. Signed-off-by: Dylan Reid --- page-tracking/src/page_info.rs | 2 +- page-tracking/src/page_tracker.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/page-tracking/src/page_info.rs b/page-tracking/src/page_info.rs index bf58a231..6957ad6b 100644 --- a/page-tracking/src/page_info.rs +++ b/page-tracking/src/page_info.rs @@ -488,7 +488,7 @@ impl PageMap { let page_map_region = mem_map .regions() .find(|r| r.region_type() == HwMemRegionType::Available && r.size() >= page_map_size) - .ok_or(PageTrackingError::PageMapNoSpace)?; + .ok_or(PageTrackingError::PageMapNoSpace(page_map_size))?; let page_map_base = page_map_region.base(); diff --git a/page-tracking/src/page_tracker.rs b/page-tracking/src/page_tracker.rs index 1b385664..ca5e2dd7 100644 --- a/page-tracking/src/page_tracker.rs +++ b/page-tracking/src/page_tracker.rs @@ -66,7 +66,7 @@ pub enum Error { /// Failed to reserve region for page map PageMapReserveRegion(hw_mem_map::Error), /// No memory available for page map - PageMapNoSpace, + PageMapNoSpace(u64), } /// Holds the result of page tracking operations. From 4a78ce6b3bd2f15c01a8932a16868735ddbce7eb Mon Sep 17 00:00:00 2001 From: Dylan Reid Date: Wed, 30 Apr 2025 01:29:01 -0700 Subject: [PATCH 4/4] main: Don't fail on missing uart The UART driver supports only the 16550a that is only available on QEMU. Print an error but continue so salus can run on spike and HW. Signed-off-by: Dylan Reid --- src/main.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index c571c21c..aff1b190 100644 --- a/src/main.rs +++ b/src/main.rs @@ -359,7 +359,6 @@ enum RequiredDeviceProbe { Imsic(drivers::imsic::ImsicError), Pci(drivers::pci::PciError), Reset(drivers::reset::Error), - Uart(drivers::uart::Error), } impl Display for RequiredDeviceProbe { @@ -369,7 +368,6 @@ impl Display for RequiredDeviceProbe { Imsic(e) => write!(f, "IMSIC: {:?}", e), Pci(e) => write!(f, "PCI: {:?}", e), Reset(e) => write!(f, "Reset: {:?}", e), - Uart(e) => write!(f, "UART: {:?}", e), } } } @@ -478,8 +476,9 @@ fn primary_init(hart_id: u64, fdt_addr: u64) -> Result { let hyp_dt = DeviceTree::from(&hyp_fdt).map_err(Error::FdtCreation)?; // Find the UART and switch to it as the system console. - UartDriver::probe_from(&hyp_dt, &mut mem_map) - .map_err(|e| Error::RequiredDeviceProbe(RequiredDeviceProbe::Uart(e)))?; + if let Err(e) = UartDriver::probe_from(&hyp_dt, &mut mem_map) { + println!("Failed to probe UART: {:?}", e); + } // Discover the CPU topology. CpuInfo::parse_from(&hyp_dt).map_err(Error::CpuTopologyGeneration)?;