Rework fw_cfg and add bootorder entry#170
Open
scholzp wants to merge 9 commits into
Open
Conversation
Some entries are also 2 and 8 bytes in size. We therefore add the missing content sizes. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
If the guest reads a specific amount of bytes through MMIO, the read should be served with the respective amount of bytes. Because the array is initialized with a slice of length 0, the current implementation returned success without any byte written to the guest buffer, which might be unexpected by the guest. We fix this by setting a default value of the respective byte length. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
We would do it analogous for `NetConfig`. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Qemu has the ability to pass boot order hints via the `fw_cfg` device to firmware. EDK2/OVMF, for instance, respect these hints. [0] To support this, we need to be able to collect open firmware device paths form the device tree. [0] https://github.com/tianocore/edk2/blob/2816ff0ab0d6505bf580aa8eec02cc2b89f04230/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c#L1222 Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
For consumption by the `fw_cfg` device, we make `DeviceManager` collect and expose the firmware device paths of devices for which the ` bootorder` attribute is defined, in an ordered list. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Qemu's implementation does not provide any default binary blobs for initrd, kernel and command line. We therefore also refrain from doing so. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Using `bootorder` in device configs requires a `fw_cfg` device to be present. The current way of instantiating one is by explicitly adding one to the command line. In cases one wasn't created this way, we need to implicitly add one to make the `bootorder` applicable. Adding it in the `device_manager` if we require one works for both cases, VM instantiation via an API call and via the CLI. Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com
Coffeeri
requested changes
Jun 12, 2026
Coffeeri
left a comment
Coffeeri
left a comment
There was a problem hiding this comment.
Amazing work! Thank you for implementing this feature!
| image_type=<raw,qcow2,vhd,vhdx>,lock_granularity=byte-range|full"; | ||
|
|
||
| #[cfg(feature = "fw_cfg")] | ||
| pub const SYNTAX: &'static str = "Disk parameters \ |
There was a problem hiding this comment.
I would prefer a concatenation to the existing DiskConfig::SYNTAX, instead of duplicating and having to maintain both.
|
|
||
| #[cfg(feature = "fw_cfg")] | ||
| pub fn fw_cfg_path_by_id(&self, id: &str) -> String { | ||
| self.get(id).unwrap().openfw_device_path().unwrap() |
There was a problem hiding this comment.
Can we handle the error cases DeviceNodePathError::{NoPciDevice,DeviceNotBootable,NoBusDevice} here instead of unwraping? If not and you'd like to panic early, maybe consider .expect(...).
| name: "bootorder".to_string(), | ||
| content: FwCfgContent::Bytes(content), | ||
| }; | ||
| fw_cfg_item_list.push(bootorder_item); |
There was a problem hiding this comment.
Should we still push the bootorder_item when bootitems is empty?
| known_items[FW_CFG_ID as usize] = FwCfgContent::Slice(&FW_CFG_FEATURE); | ||
| let file_buf = Vec::from(FwCfgFilesHeader { count_be: 0 }.as_mut_bytes()); | ||
| known_items[FW_CFG_FILE_DIR as usize] = FwCfgContent::Bytes(file_buf); | ||
| known_items[FW_CFG_UUID as usize] = FwCfgContent::Bytes(Default::default()); /* TODO? */ |
There was a problem hiding this comment.
What are all these TODO's? Do we need those?
|
|
||
| #[cfg(feature = "fw_cfg")] | ||
| { | ||
| // if we have a boot order but no fw_cfg device, we must create on |
There was a problem hiding this comment.
Suggested change
| // if we have a boot order but no fw_cfg device, we must create on | |
| // if we have a boot order but no fw_cfg device, we must create one |
Coffeeri
reviewed
Jun 15, 2026
| /// | ||
| /// [0] https://github.com/tianocore/edk2/blob/2816ff0ab0d6505bf580aa8eec02cc2b89f04230/OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c#L1222 | ||
| #[cfg(feature = "fw_cfg")] | ||
| pub fn openfw_device_path(&self) -> Result<String, DeviceNodePathError> { |
There was a problem hiding this comment.
nit, feel free to resolve: if we flatten the nested clause, it will lower the cognitive load to read this function.
e.g. use guard clauses in the beginning
#[cfg(feature = "fw_cfg")]
pub fn openfw_device_path(&self) -> Result<String, DeviceNodePathError> {
let Some(handle) = &self.pci_device_handle else {
return Err(DeviceNodePathError::NoBusDevice);
};
let PciDeviceHandle::Virtio(virtio_handle) = handle else {
return Err(DeviceNodePathError::NoPciDevice);
};
let bdf = self.pci_bdf.expect("PCI devices should have a bdf!");
let device_type = virtio_handle
.lock()
.unwrap()
.virtio_device()
.lock()
.unwrap()
.device_type();
match device_type {
0x2 /* VirtioBlk */ => Ok(format!("/pci@i0cf8/scsi@{:x}/disk@0,0", bdf.device())),
_ => Err(DeviceNodePathError::DeviceNotBootable),
}
Member
|
What is your plan regarding upstream?
I think this also depends on how urgent this is. |
Author
My plan is to develop a minimal version that fits our needs for the feature as this has a relative high priority and then begin working on making the The last means bringing all in a state that it works reliably without breaking anything in the firmware or guest. The latter reaching full compatibility with Qemu and implementing all it's defined defaults. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We need the
bootorderentry infw_cfgto provide boot order hints to OVMF. This way we make OpenStack's rescue boot work and solve https://github.com/cobaltcore-dev/cobaltcore/issues/491.Pipeline: https://gitlab.cyberus-technology.de/cyberus/cloud/libvirt/-/merge_requests/234
Maybe we can discuss what to do with the
fw_cfgfeature here. Currently the X-commit serves the purpose to enable the pipeline, so ignore it. We agreed with upstream to first work on makingfw_cfga default feature and then implementing thebootorderfeature. For us, this should make no difference.