Fix port IO in `fw_cfg` device for x86 by scholzp · Pull Request #190 · cyberus-technology/cloud-hypervisor

scholzp · 2026-08-04T12:45:47Z

This PR is the starting ground for the fw_cfg rework in CHV by reworking the port IO transportation path. The current implementation has many issues such as panicking when reading beyond item lengths. Moreover, the current implementation doesn't follow QEMU semantics, which is addressed in this PR. The next followup will introduce compatibility for well-known legacy items. After that, we can safely merge the bootorder feature, as the fw_cfg device will than semantically act similar to the QEMU implementation from the guest's perspective with a reduced set of featues.

The DMA path is broken. Therefore it is deactivated in this patch series. For our fork we decided to use the Port IO interface only. This doesn't has any implication to the bootorder feature.

We reject building for aarch64, because the MMIO mapped register based transportation layer for aarch64 is broken and doesn't follow QEMU semantics either. This, similar to other cleanup, is left for followups. The entire rework of fw_cfg will target upstream and we can sooner or later replace this version with the upstream one. A list of all defects can be found here: https://github.com/cobaltcore-dev/cobaltcore/issues/641

This is also includes the fix for the DATA register read handling, which accepts arbitrary length at the moment. This needs to be fixed in kvm_ioctls crate and then globally in CHV.

I tested locally that the bootorder feature still works. This commit series contains a commit that activates the fw_cfg feature. I'll remove it once the pipeline finished. It's only purpose is to run the pipeline with fw_cfg activated. Find a pipeline here: https://gitlab.cyberus-technology.de/cyberus/cloud/libvirt/-/merge_requests/268

The DMA interface in `fw_cfg` in the CHV implementation is broken and needs some overhaul. We deactivate it to force a guest to use the traditional interface instead and ignore the DMA interface.[0] We make DMA transfers no-ops for now and alter the test to verify this instead until we do a rework of the DMA path. [0] https://www.qemu.org/docs/master/specs/fw_cfg.html#guest-side-dma-interface On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

The selector 0x0000 has to contain the bytes "QEMU" in the traditional interface.[0] Using the DMA signature in a way currently done returns the wrong signature if a guest reads more than 4 bytes from the selector 0x0000. [0] https://www.qemu.org/docs/master/specs/fw_cfg.html#signature-key-0x0000-fw-cfg-signature On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

In the traditional/POI interface the DATA register has a width of one byte on x86.[0] We therefore do not allow reads with higher widths. Currently, all tests assume a read with of 1 byte too. We reject `fw_cfg` in aarch64 builds for now as these changes introduce an incompatibility that adds to the incomplete implementation of it. Aarch64 support is a task to be solved in followups, as this also includes making corrections to the MMIO transport implementation and FDT corrections. [0] https://www.qemu.org/docs/master/specs/fw_cfg.html#data-register On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

QEMU checks if the read has an allowed width.[0] If it detects an invalid read, then QEMU will treat the register reads as an read to unassigned memory.[1] As a result, it will return 0x0 for the whole buffer. We mimic this behavior for CHV's `fw_cfg` design for improved compatibility. [0] https://github.com/qemu/qemu/blob/6e9a825c1d4e7b62d072e99a89ecd1a74c7f0d55/hw/nvram/fw_cfg.c#L533 [1] https://github.com/qemu/qemu/blob/6e9a825c1d4e7b62d072e99a89ecd1a74c7f0d55/system/memory.c#L1480 On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

While the documentation states that the SELECTOR register is write-only, QEMU actually allows reading the SELECTOR register. This is because QEMU uses a contiguous mapping for the SELECTOR and DATA registers to allow the 16-bit width of the SELECTOR register.[0] As a consequence a read from SELECTOR is delegated to the same callback as a read from DATA. We mimic this for maximal compatibility. [0] https://github.com/qemu/qemu/blob/6e9a825c1d4e7b62d072e99a89ecd1a74c7f0d55/hw/nvram/fw_cfg.c#L539 On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

Currently, the read implementation isn't complete and doesn't handle some error cases gracefully. We rework it with the aim of maximal compatibility to QEMU and add tests for it. Problem of the old implementation include: * Reads beyond EOF should yield 0x0 [0]. These currently panic. * Register reads with invalid SELECTOR should also yield 0x0 [1] * If provided with a buffer larger than an item, then the reamining buffer bytes should be set to zero.[1] [0] https://www.qemu.org/docs/master/specs/fw_cfg.html#data-register [1] https://github.com/qemu/qemu/blob/6e9a825c1d4e7b62d072e99a89ecd1a74c7f0d55/hw/nvram/fw_cfg.c#L382 On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

This is a fix to allow string reads that map to `rep ins`. KVM returns an IO exit with the respective port, the width of `x` bytes and the number of `n` bytes in `count`. `kvm_ioctls` creates a buffer from this with the size of `x * n` bytes.[0] This makes it impossible to decide if the buffer was a single n-byte-width read of the kind `insd` or `rep ins` with EDX set to 4, for example. While the first would be invalid according to QEMU semantics, the second is a valid repeated access with one byte width. We therefore accept reads of any size until we can solve this issue. [0] https://github.com/rust-vmm/kvm/blob/b4c9ed8df95a9e10a68f50f5ef5e7d04108759ba/kvm-ioctls/src/ioctls/vcpu.rs#L1549 Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de> On-behalf-of: SAP pascal.scholz@sap.com

arctic-alpaca · 2026-08-05T14:16:19Z

The entire rework of fw_cfg will target upstream and we can sooner or later replace this version with the upstream one.

I'm unsure what that means, could you elaborate on what the current state of upstream is and how we get to using upstream?

scholzp · 2026-08-06T07:15:57Z

The entire rework of fw_cfg will target upstream and we can sooner or later replace this version with the upstream one.

I'm unsure what that means, could you elaborate on what the current state of upstream is and how we get to using upstream?

Good call!
I found a list of defects in the current implementation that would all need to be gradually fixed. You can find them here: https://github.com/cobaltcore-dev/cobaltcore/issues/641

amphi

This is a really nice commit history, thanks!

amphi · 2026-08-06T07:52:37Z

+pub const FW_CFG_DMA_SIGNATURE_CONTENT: [u8; 8] = *b"QEMU CFG";
+pub const FW_CFG_SIGNATURE_CONTENT: [u8; 4] = *b"QEMU";


Even in Cloud Hypervisor the signature has to be QEMU? Do you know what happens if the signature is something else?

amphi · 2026-08-06T07:55:48Z

                error!("fw_cfg: selector register is write-only.");
            }
-            (PORT_FW_CFG_DATA, _) => _ = self.read_data(data, size as u32),
+            (PORT_FW_CFG_DATA, 1) => _ = self.read_data(data, size as u32),


Maybe there should be a case for (PORT_FW_CFG_DATA, _)? Otherwise we will see read from unknown port, which is not really true as the port is known.

amphi · 2026-08-06T07:57:03Z

    pub content: FwCfgContent,
 }

+#[cfg(all(feature = "fw_cfg", target_arch = "aarch64"))]


Commit devices: Restrict x86 fw_cfg PIO reads to one byte contains a misspelling:

In the traditional/POI interface the DATA register has a width of one
byte on x86.[0]

should be

In the traditional/PIO interface the DATA register has a width of one
byte on x86.[0]

amphi · 2026-08-06T07:59:03Z

            _ => {
                debug!(
-                    "fw_cfg: read from unknown port {port:#x}: {size:#x} bytes and offset {offset:#x}."
+                    "fw_cfg: Unsupported {:#x}-byte read from port {port:#x}.",
+                    data.len()
                );
+                data.fill(0x0);


You are not really checking whether this is a read to a valid port with an incorrect width. This is the catch-all for "something is wrong". Is this really the intended behavior?

amphi · 2026-08-06T08:01:05Z

 #[cfg(target_arch = "aarch64")]
 use linux_loader::loader::pe::arm64_image_header as boot_params;
 use log::{debug, error};
+use thiserror::Error;


Misspelled commit message:

devices: Rework fw_cfg traditional/POI interface reads

should be

devices: Rework fw_cfg traditional/PIO interface reads

I think.

amphi · 2026-08-06T08:02:59Z


+#[derive(Error, Debug)]
+pub enum FwCfgError {
+    #[error("Reading the source (mostly a host file) failed.")]


What does mostly a host file mean here? Does it mean that the source is most likely a host file? If yes, I don't think this should be part of the error message.

amphi · 2026-08-06T08:23:35Z

+    IllegalSelector,
+    #[error("The cursor already points to the item'e end")]
+    CursorBehindContent,
+    #[error("The accessed item is too large")]


The accessed item is too large

should be

The accessed item is to large

too large

is correct.

I always mix this up, I hate it. But I guess then the error should be TooLarge?

arctic-alpaca

Very nice commit history indeed 👍

arctic-alpaca · 2026-08-06T09:48:15Z

-// DMA Toggle Bit (enabled by default)
 const FW_CFG_F_DMA: u8 = 1 << 1;
-pub const FW_CFG_FEATURE: [u8; 4] = [FW_CFG_F_RESERVED | FW_CFG_F_DMA, 0, 0, 0];
+pub const FW_CFG_FEATURE: [u8; 4] = [FW_CFG_F_RESERVED, 0, 0, 0];


As far as I understood, this change is temporary. Could you add a comment explaining that, ideally with a tracking issue?

arctic-alpaca · 2026-08-06T09:50:28Z

+#[cfg(all(feature = "fw_cfg", target_arch = "aarch64"))]
+compile_error!(
+    "fw_cfg is not supported on aarch64: the MMIO transport is incomplete and defective."
+);


Could you add a link to an issue to track the status?

arctic-alpaca · 2026-08-06T09:54:00Z

+        // Reads with unsupported size zero the whole buffer in QEMU. We mimic this behavior.
+        let mut fw_cfg = FwCfg::new(GuestMemoryAtomic::new(GuestMemoryMmap::new()));
+        fw_cfg.write(0, SELECTOR_OFFSET, &[FW_CFG_SIGNATURE as u8, 0]);
+        // Two byte read forbidden


Hyper-nit: Could you end all sentences with a period?

arctic-alpaca · 2026-08-06T09:56:44Z

+    fn get_selected_content(&self) -> std::result::Result<&FwCfgContent, FwCfgError> {
+        if let Some(known_item) = self.known_items.get(self.selector as usize) {
+            Ok(known_item)
+        } else if let Some(item) = self.items.get((self.selector - FW_CFG_FILE_FIRST) as usize) {
+            Ok(&item.content)
+        } else {
+            Err(FwCfgError::IllegalSelector)
+        }
+    }


Could you add a line of documentation to the method?

arctic-alpaca · 2026-08-06T09:57:47Z

        Ok(())
    }

+    fn get_selected_content(&self) -> std::result::Result<&FwCfgContent, FwCfgError> {


Do we need the full path for Result?
_{Same below.}

arctic-alpaca · 2026-08-06T09:59:18Z

    }

+    fn get_selected_content(&self) -> std::result::Result<&FwCfgContent, FwCfgError> {
+        if let Some(known_item) = self.known_items.get(self.selector as usize) {


Since selector is u16, we can use From instead of as.

arctic-alpaca · 2026-08-06T10:02:18Z

+            // TODO: For now we need to allow arbitrary length reads from DATA because we cannot
+            // distinguish between on one multi byte long read and multiple single-byte reads.
+            (PORT_FW_CFG_DATA, _) => self.read_data(data),


Is there already an issue for this in kvm_ioctls that this could link to?

scholzp added 3 commits August 4, 2026 14:47

X: use fw_cfg

b5ee6b4

scholzp force-pushed the fw_cfg_fix_pio branch from a1c4b04 to 825a362 Compare August 4, 2026 12:47

scholzp requested review from amphi, arctic-alpaca and hertrste August 4, 2026 12:48

scholzp force-pushed the fw_cfg_fix_pio branch from 825a362 to 0763e1c Compare August 4, 2026 14:26

scholzp added 5 commits August 4, 2026 16:37

scholzp force-pushed the fw_cfg_fix_pio branch 2 times, most recently from f2df444 to 2a7766b Compare August 5, 2026 13:39

amphi reviewed Aug 6, 2026

View reviewed changes

arctic-alpaca reviewed Aug 6, 2026

View reviewed changes

		pub const FW_CFG_DMA_SIGNATURE_CONTENT: [u8; 8] = *b"QEMU CFG";
		pub const FW_CFG_SIGNATURE_CONTENT: [u8; 4] = *b"QEMU";

Uh oh!

Conversation

scholzp commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arctic-alpaca commented Aug 5, 2026

Uh oh!

scholzp commented Aug 6, 2026

Uh oh!

amphi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amphi Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arctic-alpaca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scholzp commented Aug 4, 2026 •

edited

Loading

amphi Aug 6, 2026 •

edited

Loading