Support registering extra ioeventfds by DemiMarie · Pull Request #7841 · cloud-hypervisor/cloud-hypervisor

DemiMarie · 2026-03-14T03:13:01Z

This is the other prerequisite needed for vhost-guest.

sboeuf · 2026-03-19T17:35:44Z

virtio-devices/src/transport/pci_device.rs

    Isr = 3,
    Device = 4,
    Pci = 5,
+    Doorbell = 6,


Can we safely rely on the spec you pointed out in the commit message?
The latest virtio spec has the following description:

/* Common configuration */ #define VIRTIO_PCI_CAP_COMMON_CFG 1 /* Notifications */ #define VIRTIO_PCI_CAP_NOTIFY_CFG 2 /* ISR Status */ #define VIRTIO_PCI_CAP_ISR_CFG 3 /* Device specific configuration */ #define VIRTIO_PCI_CAP_DEVICE_CFG 4 /* PCI configuration access */ #define VIRTIO_PCI_CAP_PCI_CFG 5 /* Shared memory region */ #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8 /* Vendor-specific data */ #define VIRTIO_PCI_CAP_VENDOR_CFG 9

so I guess it should be safe to use doorbell = 6, but I wanted to double check (/cc @rbradford )

Good callout. Given this is not a part of the official spec, Let's add a comment above to document the source variant spec we are referring to.

vmm/src/device_manager.rs

virtio-devices/src/transport/mod.rs

virtio-devices/src/device.rs

virtio-devices/src/transport/pci_device.rs

virtio-devices/src/device.rs

virtio-devices/src/transport/pci_device.rs

DemiMarie

I updated the commit messages. Hopefully that answers your questions.

virtio-devices/src/device.rs

alyssais · 2026-03-23T10:59:16Z

The first commit message uses the terminology "virtio-device-backend", which has not been previously introduced. It would be less confusing to keep calling it virtio-vhost-user for now, ideally until we have a spec proposal that refers to it as whatever name you want to give it.

virtio-vhost-user requires for PCI doorbells used to notify the frontend of events. Add support for them. The specification for this capability is <https://stefanha.github.io/virtio/vhost-user-slave.html>. No functional change intended as the code is currently not used. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

Virtio-vhost-user devices need one interrupt per queue, plus one interrupt per config change, plus one interrupt per queue of the *device they are implementing*. Allow devices to override the number of interrupts they need instead of assuming it is always one more than the number of queues. No functional change for existing devices, which use a default implementation that returns the number of queues. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

It will always be 0. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

No functional change intended. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

The code will soon need to iterate over ioeventfds provided by the inner virtio device. Accessing that device requires a lock, which works fine with internal iteration but breaks with external iteration. While an Arc could be used to avoid the need for the callback, implementations of this method will need to grab a device-internal lock and invoke the callback with the lock held. Returning a MutexGuard would constrain the device implementation unnecessarily. This also changes VirtioDevice::ioeventfds() to take *two* addresses, not one. The callback also takes two addresses. This is because relocating a BAR must happen atomically with respect to registering and unregistering ioeventfds by the device. If the device manager used one call to unregister ioeventfds and a second call to reregister them, a device implementation could register ioeventfds between the two calls. Such ioeventfds would not be unregistered, potentially causing errors during reregistration. Using a single call for both unregistration and reregistration allows the device implementation to hold a lock across both operations. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

The device manager calls VirtioTransport::ioeventfds whenever it needs to unregister or register ioeventfds. This function invokes a callback with each virtqueue ioeventfd, its old base address, and its new base address. Also invoke the callback with ioeventfds provided by the device itself. No functional change as no current device provides any. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

Devices that register their own ioeventfds need to know the address they should register them at. It is not possible for this to be a callback on PciDevice because PciDevice contains the VirtioDevice. Calls to the callback after activation are ignored. Such calls are always preceeded by a call to VirtioPciDevice::ioeventfds(), which already provides the new address. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

likebreath · 2026-04-03T22:45:25Z

@sboeuf Appears all previous comments are addressed. Can you please take another look?

sboeuf

Yes thanks for addressing the comments, LGTM

likebreath

Some comments below.

likebreath · 2026-04-08T17:15:48Z

virtio-devices/src/transport/pci_device.rs

+/// Maximum number of doorbells.
+pub const MAX_DOORBELLS: u16 = (u8::MAX as u16) * 2 + 1;


Can you add some context as comments to explain why it is defined as 511?

likebreath · 2026-04-08T17:17:35Z

virtio-devices/src/device.rs


+    /// The maximum number of doorbells the device supports.
+    /// Most devices don't support any.
+    /// Limited to 511 doorbells.


Nit: better refer to MAX_DOORBELLS rather 511

likebreath · 2026-04-08T17:18:00Z

vmm/src/device_manager.rs

+        // This includes 1 per virtqueue, 1 per doorbell, and 1 for virtio config
+        // space change.


Nit: this change appears to be included in the wrong commit.

likebreath · 2026-04-08T17:33:11Z

virtio-devices/src/transport/pci_device.rs

    Isr = 3,
    Device = 4,
    Pci = 5,
+    Doorbell = 6,


Good callout. Given this is not a part of the official spec, Let's add a comment above to document the source variant spec we are referring to.

likebreath · 2026-04-08T17:47:55Z

virtio-devices/src/transport/pci_device.rs

+        let num_queues =
+            locked_device.queue_max_sizes().len() + usize::from(locked_device.doorbells_max());


This is a behavior change and needs comment to explain why now "num_queues" is now the sum of "queue_num" and "doorbells".

Also, likely this change is should be included to commit "virtio-devices: Add support for PCI doorbells".

likebreath · 2026-04-08T17:52:37Z

virtio-devices/src/transport/pci_device.rs

+const DOORBELL_BAR_OFFSET: u64 =
+    next_bar_addr_align(NOTIFICATION_BAR_OFFSET, NOTIFICATION_SIZE, 1u64 << 16);
+const DOORBELL_OFF_MULTIPLIER: u32 = NOTIFY_OFF_MULTIPLIER;
+const DOORBELL_BAR_SIZE: u64 = (MAX_QUEUES * 2 + 1) * DOORBELL_OFF_MULTIPLIER as u64;


Shouldn't it be:

const DOORBELL_BAR_SIZE: u64 = MAX_DOORBELLS * DOORBELL_OFF_MULTIPLIER as u64;

likebreath · 2026-04-08T21:57:00Z

virtio-devices/src/transport/pci_device.rs

        if num_queues > (NOTIFICATION_SIZE / u64::from(NOTIFY_OFF_MULTIPLIER)) as usize {
            return Err(VirtioPciDeviceError::CreateVirtioPciDevice(anyhow!(
                "Got {} queues, but limit is {}",
                num_queues,
                NOTIFICATION_SIZE / u64::from(NOTIFY_OFF_MULTIPLIER)
            )));
        }
        let num_doorbells = locked_device.doorbells_max();
        if num_doorbells > MAX_DOORBELLS {
            return Err(VirtioPciDeviceError::CreateVirtioPciDevice(anyhow!(
                "Got {num_doorbells} doorbells, but limit is {MAX_DOORBELLS}"
            )));
        }


Following on the comment about num_queues, this check should be done earlier, and num_doorbells should be reused for calculating num_queues.

likebreath · 2026-04-08T22:18:27Z

virtio-devices/src/transport/mod.rs

+    fn ioeventfds<T>(
+        &self,
+        old_base_addr: u64,
+        new_base_addr: u64,
+        cb: &mut dyn FnMut(&EventFd, u64, u64) -> core::result::Result<(), T>,
+    ) -> core::result::Result<(), T>;


Since this is a public trait and its semantics is getting more complex, can you please add a comment above to document its usage, particularly the cb argument?

Also a nit about the commit message for d863181, I believe you meant to say:

This also changes VirtioTransport::ioeventfds() to take ...

likebreath · 2026-04-08T23:01:36Z

virtio-devices/src/transport/pci_device.rs

+        let mut err = Ok(());
+        match self.device.lock().unwrap().ioeventfds(
+            old_base_addr,
+            new_base_addr,
+            &mut |eventfd, old_addr, new_addr| {
+                assert!(err.is_ok());
+                cb(eventfd, old_addr, new_addr).map_err(|e| {
+                    err = Err(e);
+                    PrivatelyConstructableError(PhantomData)
+                })
+            },
+        ) {
+            Ok(()) => {
+                err.unwrap();
+                Ok(())
+            }
+            Err(PrivatelyConstructableError(PhantomData)) => Err(err.unwrap_err()),
+        }


I understand this works to resolve the error type mismatch problem.

Though, I'd suggest we make fn ioeventfds from both VirtioTransport and VirtioDevice return a concrete error type, say Anyhow::Result<>. Since both traits would share the same concrete error type, the transport impl can just forward cb directly to the device with ? — no side-channel, no PhantomData, no error smuggling.

All three call sites already discard type information (formatting into strings or wrapping into a single variant), so the generic buys nothing in practice.

likebreath · 2026-04-08T23:25:08Z

virtio-devices/src/device.rs

+    /// Sets the device's config address base. This is only necessary for
+    /// devices that need to know the guest physical address of the PCI BARs
+    /// they use. Therefore, it is only meaningful for PCIe devices.
+    /// Most devices won't implement this method.
+    ///
+    /// One valid use of this method is to set the address at which the
+    /// device should register custom ioeventfds.
+    ///
+    /// This method will be called *before* [`VirtioDevice::activate`] and
+    /// implementations must be prepared for this.
+    ///
+    /// Calls after [`VirtioDevice::activate`] must not affect ioeventfd
+    /// registration.  This is the job of [`VirtioDevice::ioeventfds`].
+    fn set_config_address_base(&mut self, _base: u64) {}


I'd suggest we drop this (the last) commit from the series. set_config_address_base has no current consumer, while the default impl is empty and no device overrides it. Besides the comment, I can't meaningfully review if the call-site is correct. The remaining 6 commits are self-contained without it.

DemiMarie requested a review from a team as a code owner March 14, 2026 03:13

DemiMarie force-pushed the extra-ioeventfds branch from 5f205ce to e12294f Compare March 14, 2026 04:06

rbradford requested a review from sboeuf March 16, 2026 10:15

sboeuf reviewed Mar 19, 2026

View reviewed changes

sboeuf reviewed Mar 20, 2026

View reviewed changes

vmm/src/device_manager.rs Outdated Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/transport/mod.rs Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/device.rs Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/device.rs Outdated Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/transport/pci_device.rs Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/transport/pci_device.rs Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/transport/pci_device.rs Outdated Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/device.rs Outdated Show resolved Hide resolved

sboeuf reviewed Mar 20, 2026

View reviewed changes

virtio-devices/src/transport/pci_device.rs Show resolved Hide resolved

sboeuf requested changes Mar 20, 2026

View reviewed changes

DemiMarie force-pushed the extra-ioeventfds branch 4 times, most recently from 35d09bd to eb906fa Compare March 21, 2026 08:20

DemiMarie commented Mar 21, 2026

View reviewed changes

virtio-devices/src/device.rs Outdated Show resolved Hide resolved

virtio-devices/src/device.rs Outdated Show resolved Hide resolved

DemiMarie force-pushed the extra-ioeventfds branch 2 times, most recently from 5f7e04f to f4e55a3 Compare March 22, 2026 10:12

DemiMarie force-pushed the extra-ioeventfds branch 2 times, most recently from 6926747 to 63d4b53 Compare March 25, 2026 11:58

DemiMarie mentioned this pull request Apr 1, 2026

Vhost-guest (was virtio-vhost-user) device #7695

Draft

DemiMarie force-pushed the extra-ioeventfds branch from 63d4b53 to 0579999 Compare April 1, 2026 16:13

DemiMarie requested a review from sboeuf April 1, 2026 16:16

DemiMarie force-pushed the extra-ioeventfds branch from 0579999 to 46d4a34 Compare April 2, 2026 21:48

DemiMarie added 2 commits April 3, 2026 02:04

DemiMarie added 5 commits April 3, 2026 02:04

virtio-devices: PCI: Hard-code the settings BAR number

b8bb30f

It will always be 0. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

pci: Reduce use of magic number 4

36ed22c

No functional change intended. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

DemiMarie force-pushed the extra-ioeventfds branch from 46d4a34 to 58899a8 Compare April 3, 2026 06:04

sboeuf approved these changes Apr 7, 2026

View reviewed changes

likebreath reviewed Apr 8, 2026

View reviewed changes

		/// Maximum number of doorbells.
		pub const MAX_DOORBELLS: u16 = (u8::MAX as u16) * 2 + 1;

		// This includes 1 per virtqueue, 1 per doorbell, and 1 for virtio config
		// space change.

		let num_queues =
		locked_device.queue_max_sizes().len() + usize::from(locked_device.doorbells_max());

Conversation

DemiMarie commented Mar 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DemiMarie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssais commented Mar 23, 2026

Uh oh!

likebreath commented Apr 3, 2026

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

likebreath 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants