-
Notifications
You must be signed in to change notification settings - Fork 97
virtio: add device tree support #1006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8b771c3
6438bc9
924bca0
eaa0e57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmm, is this related ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you look in the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that said, I think we can just use the already defined |
||
| /// 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)] | ||
|
|
@@ -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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| .filter(|dev| { | ||
| dev.compatible() | ||
| .is_some_and(|c| c.all().any(|c| c == "virtio,mmio")) | ||
| }) | ||
| .filter(|dev| { | ||
| dev.property("plane") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this expected also with
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 We can fix later, but we need to add a big TODO here. (I prefer to define this now TBH)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be really a warning ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you suggest?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
|
||
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, even better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here #1090!