Skip to content
Draft
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
4 changes: 2 additions & 2 deletions .github/workflows/qemu.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ env:
CARGO_TERM_COLOR: always
# IGVM C wrapper might require a different Rust version than SVSM
RUST_VERSION_IGVM: 1.84.1
QEMU_REPO: https://github.com/coconut-svsm/qemu.git
QEMU_REF: svsm-igvm
QEMU_REPO: https://github.com/luigix25/qemu.git
QEMU_REF: dtc_rebase
IGVM_REPO: https://github.com/microsoft/igvm.git
IGVM_REF: igvm-v0.3.4

Expand Down
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ cocoon-tpm-utils-common = { version = "0.1.0", default-features = false }
concat-kdf = "0.1.0"
gdbstub = { version = "0.7.10", default-features = false }
gdbstub_arch = { version = "0.3.3" }
fdt = { version = "0.1.5" }
igvm = { version = "0.4.0", default-features = false }
igvm_defs = { version = "0.4.0", default-features = false }
intrusive-collections = "0.9.6"
Expand Down
7 changes: 7 additions & 0 deletions boot/defs/src/boot_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ pub struct BootParamBlock {
/// The size, in bytes, of the MADT area.
pub madt_size: u32,

/// The offset, in bytes, from the base of the parameter block to the base
/// of the host-supplied device tree.
pub dt_offset: u32,

/// The size, in bytes, of the DT area.
pub dt_size: u32,

/// The offset, in bytes, from the base of the parameter block to the base
/// of the memory map (which is in IGVM format).
pub memory_map_offset: u32,
Expand Down
1 change: 1 addition & 0 deletions kernel/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ libaproxy = { workspace = true, optional = true }
elf.workspace = true
syscall.workspace = true

fdt.workspace = true
aes-gcm = { workspace = true, features = ["aes", "alloc"] }
aes-kw = { workspace = true, optional = true }
bitfield-struct.workspace = true
Expand Down
19 changes: 19 additions & 0 deletions kernel/src/boot_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub struct BootParams<'a> {
igvm_param_page: &'a IgvmParamPage,
igvm_memory_map: &'a IgvmMemoryMap,
igvm_madt: &'a [u8],
igvm_device_tree: Option<&'a [u8]>,
guest_context: Option<&'a InitialGuestContext>,
}

Expand All @@ -59,6 +60,19 @@ impl BootParams<'_> {
let madt = unsafe {
slice::from_raw_parts(madt_address.as_ptr::<u8>(), param_block.madt_size as usize)
};
let device_tree = if param_block.dt_size != 0 {
let dt_address = addr + param_block.dt_offset as usize;
// SAFETY: the parameter block correctly describes the bounds of the device tree.
unsafe {
Some(slice::from_raw_parts(
dt_address.as_ptr::<u8>(),
param_block.dt_size as usize,
))
}
} else {
None
};

let guest_context = if param_block.guest_context_offset != 0 {
let offset = usize::try_from(param_block.guest_context_offset).unwrap();
Some(Self::try_aligned_ref::<InitialGuestContext>(addr + offset)?)
Expand All @@ -71,6 +85,7 @@ impl BootParams<'_> {
igvm_param_page: param_page,
igvm_memory_map: memory_map,
igvm_madt: madt,
igvm_device_tree: device_tree,
guest_context,
})
}
Expand Down Expand Up @@ -281,6 +296,10 @@ impl BootParams<'_> {
ACPITable::new(self.igvm_madt).and_then(|t| load_acpi_cpu_info(&t))
}

pub fn get_device_tree(&self) -> Option<&[u8]> {
self.igvm_device_tree
}

pub fn should_launch_fw(&self) -> bool {
self.boot_param_block.firmware.size != 0
}
Expand Down
81 changes: 0 additions & 81 deletions kernel/src/fw_cfg.rs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we still need the fw_cfg ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

not really! After this PR fw_cfg can be dropped entirely.
I think there is some leftover code for MADT that is not used anymore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cool, so let's add a commit to remove it completely

@luigix25 luigix25 May 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah i'll open a new PR for that, as fw_cfg MADT support was already removed some time ago.
Then this PR will remove the remaining parts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yep, even better!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See here #1090!

Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@
extern crate alloc;

use crate::error::SvsmError;
use zerocopy::FromBytes;

use super::io::IOPort;
use alloc::string::String;
use alloc::vec::Vec;
use core::mem::size_of;

const FW_CFG_CTL: u16 = 0x510;
Expand All @@ -22,27 +20,6 @@ const FW_CFG_FILE_DIR: u16 = 0x19;

const MAX_FW_CFG_FILES: u32 = 0x1000;

mod hardware_info {
use zerocopy::{FromBytes, Immutable, KnownLayout};

pub const HW_INFO_FILE: &str = "etc/hardware-info";

pub const TYPE_SVSM_VIRTIO_MMIO: u64 = 0x1000;

#[derive(FromBytes, Debug, Immutable, KnownLayout)]
#[repr(C)]
pub struct Header {
pub hw_type: u64,
pub size: u64,
}

#[derive(FromBytes, Debug, Immutable, KnownLayout)]
#[repr(C)]
pub struct SimpleDevice {
pub mmio_address: u64,
}
}

#[non_exhaustive]
#[derive(Debug)]
pub struct FwCfg<'a> {
Expand Down Expand Up @@ -84,39 +61,6 @@ impl FwCfgFile {
}
}

#[derive(Debug)]
struct FwCfgFileIterator<'a> {
fw_cfg: &'a FwCfg<'a>,
file_size: usize,
}

impl<'a> FwCfgFileIterator<'a> {
pub fn new(fw_cfg: &'a FwCfg<'a>, file_name: &str) -> Result<Self, SvsmError> {
let file = fw_cfg.file_selector(file_name)?;
fw_cfg.select(file.selector);
Ok(Self {
fw_cfg,
file_size: file.size as usize,
})
}
}

impl Iterator for FwCfgFileIterator<'_> {
type Item = u8;

fn next(&mut self) -> Option<Self::Item> {
if self.file_size == 0 {
return None;
}
self.file_size -= 1;
Some(self.fw_cfg.read_u8())
}

fn size_hint(&self) -> (usize, Option<usize>) {
(self.file_size, Some(self.file_size))
}
}

impl<'a> FwCfg<'a> {
pub fn new(driver: &'a dyn IOPort) -> Self {
FwCfg { driver }
Expand Down Expand Up @@ -179,29 +123,4 @@ impl<'a> FwCfg<'a> {

Err(SvsmError::FwCfg(FwCfgError::FileNotFound))
}

/// Try reading an instance of T from the iterator.
fn read_from_it<T: FromBytes>(i: &mut impl Iterator<Item = u8>) -> Result<T, FwCfgError> {
let buffer: Vec<u8> = i.take(size_of::<T>()).collect();
T::read_from_bytes(buffer.as_slice()).map_err(|_| FwCfgError::InvalidFormat)
}

pub fn get_virtio_mmio_addresses(&self) -> Result<Vec<u64>, SvsmError> {
use hardware_info::*;

let mut it = FwCfgFileIterator::new(self, HW_INFO_FILE)?;

let mut addresses: Vec<u64> = Vec::<u64>::new();

while let Ok(header) = Self::read_from_it::<Header>(&mut it) {
if header.hw_type == TYPE_SVSM_VIRTIO_MMIO {
addresses.push(Self::read_from_it::<SimpleDevice>(&mut it)?.mmio_address);
} else {
for _ in 0..header.size as usize {
it.next();
}
}
}
Ok(addresses)
}
}
3 changes: 3 additions & 0 deletions kernel/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,14 @@ pub const SVSM_CS_ATTRIBUTES: u16 = 0xa09b;
pub const SVSM_DS_ATTRIBUTES: u16 = 0xc093;
pub const SVSM_TR_ATTRIBUTES: u16 = 0x89;

/// VMPL level SVSM will be executed at.
pub const SVSM_VMPL: usize = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mmm, is this related ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

if you look in the get_virtio_mmio_addresses function, I need to check if a device is meant for SVSM or the guest. I didn't want to hardcode a magic number, so I added a new const

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, this is not mentioned at all in the commit description. IMO better to split in another commit.

Also, are you sure SVSM is running in VMPL1? Isn't VMPL0?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that said, I think we can just use the already defined VMPL0, since SVSM can only run on it. while for the guest was a choice to select 2, but it can be 1 or 3, so for that reason we added a const to keep track of it.

/// VMPL level the guest OS will be executed at.
/// Keep VMPL 1 for the SVSM and execute the OS at VMPL-2. This leaves VMPL-3
/// free for the OS to use in the future.
pub const GUEST_VMPL: usize = 2;

const _: () = assert!(SVSM_VMPL > 0 && SVSM_VMPL < GUEST_VMPL);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

const _: () = assert!(GUEST_VMPL > 0 && GUEST_VMPL < VMPL_MAX);

pub const MAX_CPUS: usize = 512;
Expand Down
40 changes: 30 additions & 10 deletions kernel/src/virtio/mmio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@
extern crate alloc;
use alloc::vec::Vec;
use core::ptr::NonNull;
use fdt::Fdt;

use virtio_drivers::transport::{DeviceType, Transport, mmio::MmioTransport};

use crate::address::PhysAddr;
use crate::boot_params::BootParams;
use crate::fw_cfg::FwCfg;
use crate::mm::{GlobalRangeGuard, map_global_range_4k_shared, pagetable::PTEntryFlags};
use crate::platform::SVSM_PLATFORM;
use crate::types::PAGE_SIZE;
use crate::types::{PAGE_SIZE, SVSM_VMPL};
use crate::virtio::hal::{SvsmHal, virtio_init};

#[derive(Debug)]
Expand All @@ -29,9 +28,28 @@ pub struct MmioSlots {
slots: Vec<MmioSlot>,
}

/// Parses the device tree to extract base addresses of virtio-MMIO devices
/// assigned to the SVSM plane.
fn get_virtio_mmio_addresses(device_tree: Fdt<'_>) -> Vec<u64> {
device_tree
.find_all_nodes("/virtio_mmio")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be documented somewhere ?

I mean this is a contract with the VMM that should use exactly virtio_mmio and not virtio-mmio or virtio mmio for example.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

mmh good point. What about removing the filter on /virtio_mmio and using just the compatible part?
I'm using the linux compatible convention: https://www.kernel.org/doc/Documentation/devicetree/bindings/virtio/mmio.txt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know TBH. My point is more related to the documentation.

@luigix25 luigix25 May 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok, i'll write something in the doc. I'll also drop the filtering on /virtio_mmio, it was not necessary and may cause problems.

.filter(|dev| {
dev.compatible()
.is_some_and(|c| c.all().any(|c| c == "virtio,mmio"))
})
.filter(|dev| {
dev.property("plane")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I think all of this must be documented somewhere, and also mention if are optional or not. E.g. why we need plane here? Can the VMM just expose the device for SVSM plane or there is an use case where we need to expose also devices for other planes ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The idea is that the HV exposes the full DT, SVSM gets what it's on its plane, removes those entries and forwards the DT to the firmware.

(see coconut-svsm/qemu#31 (comment))

Note that edk2 is not able to consume a DT from igvm yet. So this is a long term plan

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OKay, now it makes sense. Sorry, I was missing this point, I thought we needed DT just for SVSM.

.and_then(|p| p.as_usize())
.is_some_and(|plane| plane == SVSM_VMPL)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this expected also with native platform where we don't have VMPLs ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

currently the plane property is fake. It's just a value that is passed via the CLI (see launch_guest) so even on native platform we can have "planes".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

okay, but in the future how we plan to handle it? Should we mark plane as optional? Or should we ask to have it in any case set to 0 to assign a device to svsm?

We can fix later, but we need to add a big TODO here. (I prefer to define this now TBH)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is a very good question, I think it depends on how the planes will be integrated in QEMU. IIRC (maybe @joergroedel can correct me) in the qemu branch you can still override the planes property, even in native mode.

})
.filter_map(|dev| dev.reg()?.next())
.map(|reg| reg.starting_address as u64)
.collect()
}

/// Probes and enumerates all virtio-MMIO devices available in the system.
///
/// This function queries the fw_cfg interface to discover virtio-MMIO device
/// This function parses the device tree to discover virtio-MMIO device
/// addresses and maps their MMIO regions.
///
/// # Usage
Expand All @@ -44,22 +62,24 @@ pub struct MmioSlots {
/// # Returns
///
/// Returns an [`MmioSlots`] collection containing all discovered virtio-MMIO devices.
/// Returns an empty collection if no devices are found or if the fw_cfg interface
/// Returns an empty collection if no devices are found or if the device tree
/// is unavailable.
pub fn probe_mmio_slots(boot_params: &BootParams<'_>) -> MmioSlots {
// Virtio MMIO addresses are discovered via fw_cfg, so skip probing
// Virtio MMIO addresses are discovered via device tree, so skip probing
// if it is not present.
if !boot_params.has_fw_cfg_port() {
let Some(device_tree) = boot_params.get_device_tree() else {
log::warn!("MmioSlots: No device tree found.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be really a warning ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

what do you suggest?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the previous code, if fw_cfg didn't provide any slot we didn't print anything, so I'm asking why now this need to be a warning or not an info or debug ?
Why we are worried about this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

right, I can drop the log, like it was before.

return MmioSlots::default();
}
};

virtio_init();

let cfg = FwCfg::new(SVSM_PLATFORM.get_io_port());
let Ok(dev) = cfg.get_virtio_mmio_addresses() else {
let Ok(parsed_dt) = Fdt::new(device_tree) else {
log::warn!("MmioSlots: Failed to parse device tree");
return MmioSlots::default();
};

let dev = get_virtio_mmio_addresses(parsed_dt);
let mut slots = Vec::with_capacity(dev.len());

for addr in dev {
Expand Down
10 changes: 7 additions & 3 deletions scripts/launch_guest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ STATE_DEVICE=""
VSOCK_DEVICE=""
VIRTIO=0

# Must match SVSM_VMPL and GUEST_VMPL in kernel/src/types.rs
SVSM_VMPL=1
GUEST_VMPL=2

while [[ $# -gt 0 ]]; do
case $1 in
-q|--qemu)
Expand All @@ -54,7 +58,7 @@ while [[ $# -gt 0 ]]; do
--state)
VIRTIO=1
STATE_DEVICE+="-drive file=$2,format=raw,if=none,id=svsm_storage,cache=none "
STATE_DEVICE+="-device virtio-blk-device,drive=svsm_storage "
STATE_DEVICE+="-device virtio-blk-device,drive=svsm_storage,plane=$SVSM_VMPL "
shift
shift
;;
Expand Down Expand Up @@ -89,7 +93,7 @@ while [[ $# -gt 0 ]]; do
;;
--vsock)
VIRTIO=1
VSOCK_DEVICE="-device vhost-vsock-device,guest-cid=$2 "
VSOCK_DEVICE="-device vhost-vsock-device,guest-cid=$2,plane=$SVSM_VMPL "
shift
shift
;;
Expand Down Expand Up @@ -147,7 +151,7 @@ case "$CGS" in
echo "Error: Unexpected CGS value '$CGS'"
exit 1
esac
MACHINE=q35,memory-backend=mem0,igvm-cfg=igvm0,accel=$ACCEL
MACHINE=q35,memory-backend=mem0,igvm-cfg=igvm0,accel=$ACCEL,device-plane=$GUEST_VMPL
[ -n "$SNP_GUEST" ] && MACHINE+=",confidential-guest-support=cgs0"
[ -n "$VIRTIO_ENABLE" ] && MACHINE+=",${VIRTIO_ENABLE}"

Expand Down
Loading