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

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?

Tl;dr: Yes, its part of the discovery mechanism.

For a little more context: fw_cfg is a device defines by QEMU. There is no other specification to it, except QEMU's documentation and code. The signature is part of the discovery routine. So if reading 4 bytes from 0x511 gives you "QEMU", then you know that there might be a fw_cfg device present. That's what OVMF does, for example.

Just to conclude the picture: If you afterwards select and read FW_CFG_FEATURE and you find that DMA is supported, you might do two 4-byte long reads (or one 8-byte long read from 0x514), one on port 0x514 and one on 0x518 to retrieve the DMA signature, which must be "QEMU CFG".

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.

I think you are right but also this is solved by a later commit.

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?

Fixed. I now differ between reads of unsupported sizes from valid ports and reads from unknown ports.

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?

This seems to be solved. :D

The error variant below where this comment points to is ToLarge, which should be TooLarge.

Fixed. Thanks for pointing out again!

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?

I created a new issue: https://github.com/cobaltcore-dev/cobaltcore/issues/647 and linked it in a comment.

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?

I opened and linked a tracking issue.
See https://github.com/cobaltcore-dev/cobaltcore/issues/650

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?

Should be fixed for all. :D

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?

I added a bit of documentation to all methods I introduced.

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.}

I replaced the full path with a type definition. When I was on it I improved the error messages.

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/Into instead of as.

I replaced the cast.

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?

There actually is one! I'll linked it.
rust-vmm/kvm#371

The DMA interface in `fw_cfg` in the CHV implementation is broken and needs an 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 adjust 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 return the bytes "QEMU" in the traditional interface.[0] Each additional read beyond those four bytes should return 0x0. The previous implementation exposed the complete DMA signature if a guest reads more than four bytes from the selector 0x0000 instead. We fix this by treating each signature separately. [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/PIO interface the DATA register has a width of one byte on x86.[0] We therefore do not allow reads with larger widths. Currently, all tests assume a read with a width of one 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 follow-up work, 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 a read from unassigned I/O.[1] As a result, it will return 0x0 for the whole buffer. QEMU maps two separate memory regions to fw_cfg. One starting at FW_CFG_IO_BASE which is 0x510 and one starting at 0x514.[2] The first is two bytes, the size of the latter is eight bytes. This leaves a hole at addresses 0x512 and 0x513 in the x86 I/O port mapping. We mimic this behavior for CHV's `fw_cfg` design for improved compatibility and respect the hole accordingly. [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 [2] https://github.com/qemu/qemu/blob/6e9a825c1d4e7b62d072e99a89ecd1a74c7f0d55/hw/i386/fw_cfg.c#L130 On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

Using the result of an addition comes with the risk of it overflowing. In terms of the address matching in `read` and `write` it makes no difference if we use the provided offset directly instead of adding the base to it. 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. It does not return the SELECTOR value. 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. Problems 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 remaining 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 I/O exit with the respective port, the read size of the instruction and the count of such reads. `kvm-ioctls` creates a buffer from this with the size of `count * size` bytes.[0] This makes it impossible to decide if the buffer was a single four-byte-width read of the kind `inl` or `rep ins` with RCX 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 On-behalf-of: SAP pascal.scholz@sap.com Signed-off-by: Pascal Scholz <pascal.scholz@cyberus-technology.de>

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 3 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

scholzp added 4 commits August 7, 2026 17:54

scholzp force-pushed the fw_cfg_fix_pio branch from 2a7766b to c72d0ab Compare August 7, 2026 16:01

scholzp added 3 commits August 7, 2026 18:28

scholzp force-pushed the fw_cfg_fix_pio branch from c72d0ab to e719416 Compare August 7, 2026 16:33

scholzp force-pushed the fw_cfg_fix_pio branch from e719416 to 09e43f2 Compare August 7, 2026 16:35

		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!

Choose a reason for hiding this comment

Uh oh!

scholzp Aug 7, 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!

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!

Choose a reason for hiding this comment

Uh oh!

arctic-alpaca Aug 7, 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!

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!

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!

arctic-alpaca 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

scholzp commented Aug 4, 2026 •

edited

Loading

scholzp Aug 7, 2026 •

edited

Loading

amphi Aug 6, 2026 •

edited

Loading

arctic-alpaca Aug 7, 2026 •

edited

Loading

arctic-alpaca Aug 6, 2026 •

edited

Loading