DNM: Test: Openhcl noiommu iommufd cdev changes

[Naman Jain (namancse)]

No description provided.

[github-actions]

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

[Copilot]

Pull request overview

This PR refactors OpenHCL’s VFIO noiommu path to support the modern VFIO cdev + iommufd interface (including device keep-alive via VFIO_DEVICE_FEATURE), while retaining the legacy group+container path as a fallback and wiring the selection through DMA-client capabilities and Underhill options.

Changes:

• Remove the legacy VFIO group private keep-alive ioctl and add cdev-based keep-alive via VFIO_DEVICE_FEATURE_KEEP_ALIVE.
• Auto-select VFIO legacy vs cdev+iommufd in VfioDevice based on whether the primary DMA client provides an iommufd noiommu IOAS handle.
• Add an Underhill env option (OPENHCL_VFIO_IOMMUFD_CDEV) to default to the cdev+iommufd interface and plumb iommufd IOAS handling through DMA client creation.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vm/devices/user_driver/vfio_sys/src/lib.rs	Removes the legacy VFIO group keep-alive plumbing.
vm/devices/user_driver/vfio_sys/src/cdev.rs	Adds VFIO_DEVICE_FEATURE ioctl and cdev-based keep-alive setter.
vm/devices/user_driver/src/vfio.rs	Introduces transport selection (legacy vs cdev+iommufd), cdev bind/attach logic, and cdev node discovery.
vm/devices/user_driver/src/lib.rs	Extends DmaClient with an optional iommufd IOAS capability hook.
vm/devices/user_driver/src/iommufd_dma.rs	Adds a shared handle type for an iommufd context + noiommu IOAS to drive cdev selection.
openhcl/underhill_core/src/worker.rs	Plumbs the vfio_iommufd_cdev flag into device DMA-client setup (MANA + NVMe wiring).
openhcl/underhill_core/src/options.rs	Adds OPENHCL_VFIO_IOMMUFD_CDEV option (default-on) and parses it from env.
openhcl/underhill_core/src/nvme_manager/device.rs	Allocates/attaches a noiommu IOAS for NVMe DMA clients when cdev+iommufd is selected.
openhcl/underhill_core/src/lib.rs	Passes the new option into worker environment config.
openhcl/openhcl_dma_manager/src/lib.rs	Adds iommufd IOAS association to DMA clients and exposes constructors that carry the IOAS handle.
nix/openhcl_kernel.nix	Updates the OpenHCL kernel version/url details to a custom cdevtest kernel artifact.
flowey/flowey_lib_hvlite/src/_jobs/cfg_versions.rs	Updates the configured OpenHCL stable kernel version string used by flowey pipelines.

Suppressed comments (1)

openhcl/openhcl_dma_manager/src/lib.rs:534

• The attach_pending_buffers comment assumes buffers are mapped into the IOAS and warns about collisions on re-mapping, but the current IommufdIoas::map_block implementation doesn’t create any IOAS mappings in noiommu mode. This comment should be updated to match the current behavior to avoid future confusion.

        // Pending buffers are persistent allocations being re-attached after a
        // servicing/keepalive restart. When this client maps into an iommufd
        // IOAS, that IOAS (and its buffer mappings) is preserved across the
        // restart along with the iommufd fd, so the buffers are already mapped
        // and their pool-provided PFNs are still the correct physical pages.
        // Re-mapping here would collide with the surviving mappings, so return
        // the backing blocks unchanged regardless of `iommufd_ioas`.

[Naman Jain (namancse)]

Moving to review, just to get these openvmm tests run on this PR. In Draft mode, these were not running.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (7)

vm/devices/user_driver/vfio_sys/src/cdev.rs:93

• VFIO_DEVICE_FEATURE_KEEP_ALIVE is defined as feature index 11, but this crate already uses feature index 11 for VFIO_DEVICE_FEATURE_DMA_BUF in vfio_sys/src/lib.rs. If these indices collide, the kernel will interpret the keep-alive ioctl as the dmabuf feature and likely reject it (e.g., EINVAL due to a too-small argsz), so keep-alive will never succeed.

const VFIO_DEVICE_FEATURE_SET: u32 = 1 << 17;
/// Feature index `VFIO_DEVICE_FEATURE_KEEP_ALIVE` (Microsoft addition).
const VFIO_DEVICE_FEATURE_KEEP_ALIVE: u32 = 11;

openhcl/openhcl_dma_manager/src/lib.rs:512

• This doc comment says buffers are “mapped into” a cdev device’s IOAS and that PFNs reflect “physical addresses recovered from the kernel”, but IommufdIoas::map_block currently returns the MemoryBlock unchanged and the iommufd IOAS is used primarily to drive VFIO onto the cdev path / keep the context alive in noiommu mode. The current wording is misleading and suggests behavior that doesn’t happen.

    /// When present, buffers allocated by this client are mapped into a
    /// cdev-based VFIO device's noiommu IOAS, and their PFNs reflect the
    /// physical addresses recovered from the kernel.
    #[inspect(skip)]
    iommufd_ioas: Option<Arc<IommufdIoas>>,

openhcl/openhcl_dma_manager/src/lib.rs:532

• The attach_pending_buffers comment still describes IOAS “buffer mappings” surviving across restart and warns about remapping collisions, but in this noiommu design blocks are not mapped into the IOAS (see IommufdIoas::map_block). This comment should be updated to match the actual behavior so future changes don’t rely on an incorrect assumption.

        // Pending buffers are persistent allocations being re-attached after a
        // servicing/keepalive restart. When this client maps into an iommufd
        // IOAS, that IOAS (and its buffer mappings) is preserved across the
        // restart along with the iommufd fd, so the buffers are already mapped
        // and their pool-provided PFNs are still the correct physical pages.

openhcl/openhcl_dma_manager/src/lib.rs:439

• Same as above: this comment claims buffers are “mapped into” an iommufd noiommu IOAS, but in noiommu mode blocks are not mapped into the IOAS here. Please reword to avoid implying IOAS mappings that don’t exist.

    /// Creates a new DMA client whose buffers are additionally mapped into the
    /// given iommufd noiommu IOAS, for use with a cdev-based VFIO device.

vm/devices/user_driver/src/vfio.rs:245

• The keep-alive failure path says the cdev+iommufd path is “selected automatically whenever keep-alive is requested”, but selection is actually driven solely by whether the DMA client exposes an iommufd IOAS (DmaClient::iommufd_ioas). This comment/error message is misleading and doesn’t tell callers what they must provide to enable keep-alive.

                    // exclusively by the cdev + iommufd path (which is selected
                    // automatically whenever keep-alive is requested).
                    anyhow::bail!(
                        "keep-alive is only supported on the VFIO cdev + iommufd path; \
                         the legacy group keep-alive interface has been removed"

vm/devices/user_driver/src/vfio.rs:633

• find_cdev_node uses read_dir(...).next() without filtering for the actual vfioN child. Sysfs directories commonly include other entries (e.g., uevent), and read_dir ordering is unspecified, so this can nondeterministically return a non-vfioN entry and build an invalid /dev/vfio/devices/<name> path.

    let entry = std::fs::read_dir(vfio_dev_sysfs)
        .with_context(|| format!("failed to read {}", vfio_dev_sysfs.display()))?
        .next()
        .context("no vfio-dev entry present for device")?
        .context("failed to read vfio-dev directory entry")?;

openhcl/openhcl_dma_manager/src/lib.rs:390

• These doc comments say buffers are “mapped into” an iommufd noiommu IOAS, but the noiommu design here does not map individual buffers into the IOAS (see IommufdIoas::map_block, which returns the block unchanged). This wording is misleading; the IOAS handle is primarily used to bind/attach the VFIO cdev and to drive interface selection.

This issue also appears in the following locations of the same file:

• line 438
• line 508
• line 528

    /// Creates a new DMA client whose buffers are additionally mapped into the
    /// given iommufd noiommu IOAS, for use with a cdev-based VFIO device.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

vm/devices/user_driver/src/vfio.rs:241

• The comment says the cdev+iommufd path is "selected automatically whenever keep-alive is requested", but selection currently depends only on dma_clients.primary().iommufd_ioas(). This is misleading for callers and makes the error harder to diagnose. Clarify that keep-alive requires an iommufd-backed DMA client (so the cdev path is chosen).

                    // The legacy VFIO group keep-alive ioctl has been removed.
                    // Device keep-alive across servicing is now provided
                    // exclusively by the cdev + iommufd path (which is selected
                    // automatically whenever keep-alive is requested).
                    anyhow::bail!(

openhcl/openhcl_dma_manager/src/lib.rs:439

• Same as above: the IOAS handle is used to attach the VFIO cdev, not to map individual buffers in noiommu mode. This docstring currently implies additional per-buffer mappings.

    /// Creates a new DMA client whose buffers are additionally mapped into the
    /// given iommufd noiommu IOAS, for use with a cdev-based VFIO device.

openhcl/openhcl_dma_manager/src/lib.rs:512

• This field comment states buffers are mapped into the IOAS and PFNs reflect addresses recovered from the kernel, but IommufdIoas::map_block() currently returns the block unchanged (no per-buffer IOAS map). Update the comment to describe the actual purpose: carrying the IOAS handle so the VFIO cdev can be attached to it.

    /// When present, buffers allocated by this client are mapped into a
    /// cdev-based VFIO device's noiommu IOAS, and their PFNs reflect the
    /// physical addresses recovered from the kernel.
    #[inspect(skip)]
    iommufd_ioas: Option<Arc<IommufdIoas>>,

openhcl/openhcl_dma_manager/src/lib.rs:532

• The attach_pending_buffers() comment assumes per-buffer IOAS mappings survive a servicing restart, but noiommu mode intentionally performs no per-buffer IOAS mapping (IommufdIoas::map_block() is a no-op). Reword this to reflect that pending buffers are simply re-attached from the pool and no IOAS work is performed here.

        // Pending buffers are persistent allocations being re-attached after a
        // servicing/keepalive restart. When this client maps into an iommufd
        // IOAS, that IOAS (and its buffer mappings) is preserved across the
        // restart along with the iommufd fd, so the buffers are already mapped
        // and their pool-provided PFNs are still the correct physical pages.

vm/devices/user_driver/src/vfio.rs:633

• read_dir(...).next() picks an arbitrary directory entry; if vfio-dev/ ever contains non-device entries (e.g. symlinks) or multiple matches, this can resolve to the wrong /dev/vfio/devices/* node. Filter for vfioN-style entries and enforce exactly one match to avoid nondeterministic device selection.

    let entry = std::fs::read_dir(vfio_dev_sysfs)
        .with_context(|| format!("failed to read {}", vfio_dev_sysfs.display()))?
        .next()
        .context("no vfio-dev entry present for device")?
        .context("failed to read vfio-dev directory entry")?;

openhcl/openhcl_dma_manager/src/lib.rs:390

• This docstring says buffers are "mapped" into the IOAS, but IommufdIoas::map_block() is explicitly a no-op in noiommu mode (the IOAS exists to satisfy bind/attach). Rewording avoids implying per-buffer IOAS mappings that don't exist and helps future maintainers reason about DMA behavior.

This issue also appears in the following locations of the same file:

• line 438
• line 528

    /// Creates a new DMA client whose buffers are additionally mapped into the
    /// given iommufd noiommu IOAS, for use with a cdev-based VFIO device.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (7)

openhcl/openhcl_dma_manager/src/lib.rs:512

• This field-level doc claims allocations are mapped into the IOAS and that PFNs are recovered from the kernel, but the current IommufdIoas implementation does not perform per-buffer IOAS mapping in noiommu mode. The comment should reflect that this is an association/handle used for VFIO cdev attachment.

    /// When present, buffers allocated by this client are mapped into a
    /// cdev-based VFIO device's noiommu IOAS, and their PFNs reflect the
    /// physical addresses recovered from the kernel.
    #[inspect(skip)]
    iommufd_ioas: Option<Arc<IommufdIoas>>,

openhcl/openhcl_dma_manager/src/lib.rs:532

• This comment describes buffers being "already mapped" into the IOAS and remapping collisions, but there is currently no per-buffer IOAS mapping step for noiommu (see IommufdIoas::map_block). The rationale should be updated to avoid implying mappings exist.

        // servicing/keepalive restart. When this client maps into an iommufd
        // IOAS, that IOAS (and its buffer mappings) is preserved across the
        // restart along with the iommufd fd, so the buffers are already mapped
        // and their pool-provided PFNs are still the correct physical pages.

vm/devices/user_driver/vfio_sys/src/cdev.rs:163

• size_of::<...>() is used unqualified here (and elsewhere in this module) but there is no use std::mem::size_of; import in scope, so this will not compile. Either add a module-level import or qualify all call sites as std::mem::size_of::<...>().

    pub fn set_keep_alive(&self) -> anyhow::Result<()> {
        let mut feature = VfioDeviceFeature {
            argsz: size_of::<VfioDeviceFeature>() as u32,
            flags: VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_KEEP_ALIVE,
        };

vm/devices/user_driver/src/vfio.rs:244

• The anyhow::bail! string uses \ line-continuation plus indentation, which will embed a large run of spaces in the emitted error message. This makes the message harder to read and copy/paste.

                    anyhow::bail!(
                        "keep-alive is only supported on the VFIO cdev + iommufd path; \
                         the legacy group keep-alive interface has been removed"
                    );

openhcl/openhcl_dma_manager/src/lib.rs:390

• The doc comment says buffers are "additionally mapped into" the IOAS, but IommufdIoas::map_block is currently a no-op for noiommu (it returns the block unchanged). This is misleading for callers/readers.

This issue also appears in the following locations of the same file:

• line 508
• line 529

    /// Creates a new DMA client whose buffers are additionally mapped into the
    /// given iommufd noiommu IOAS, for use with a cdev-based VFIO device.
    pub fn new_client_with_iommufd_ioas(
        &self,
        params: DmaClientParameters,
        iommufd_ioas: Arc<IommufdIoas>,
    ) -> anyhow::Result<Arc<OpenhclDmaClient>> {
        self.inner.new_dma_client(params, Some(iommufd_ioas))

flowey/flowey_lib_hvlite/src/_jobs/cfg_versions.rs:32

• Overwriting OPENHCL_KERNEL_STABLE_VERSION with a custom test build will change the kernel used across non-dev OpenHCL pipelines. If this kernel is meant only for experimental/dev validation, prefer setting OPENHCL_KERNEL_DEV_VERSION = Some("...") and leaving the stable version on the normal rolling-lts release.

pub const OPENHCL_KERNEL_DEV_VERSION: Option<&str> = None;
pub const OPENHCL_KERNEL_STABLE_VERSION: &str = "6.18.namjain.cdevtest";

nix/openhcl_kernel.nix:4

• version uses an if is_dev conditional where both branches are identical. This makes the expression harder to read and suggests a distinction that no longer exists.

  version = if is_dev then "6.18.namjain.cdevtest" else "6.18.namjain.cdevtest";

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (3)

vm/devices/user_driver/src/vfio.rs:244

• The comment says the cdev+iommufd path is “selected automatically whenever keep-alive is requested”, but device_type selection is based solely on dma_clients.primary().iommufd_ioas(). In configurations where keepalive == true but the DMA client doesn’t provide an iommufd IOAS, this branch will always error and the comment/message is misleading. Make the error actionable by explicitly stating that keep-alive requires DMA clients backed by an iommufd noiommu IOAS (or adjust selection logic if you intend to auto-select cdev on keepalive).

                    // automatically whenever keep-alive is requested).
                    anyhow::bail!(
                        "keep-alive is only supported on the VFIO cdev + iommufd path; \
                         the legacy group keep-alive interface has been removed"
                    );

vm/devices/user_driver/src/vfio.rs:632

• find_cdev_node documents that the kernel creates exactly one vfioN child under vfio-dev, but the implementation just takes the first directory entry. If multiple entries ever appear (or ordering changes), this can silently pick the wrong node. Validate the “exactly one” assumption and return an error if more than one entry exists.

    let entry = std::fs::read_dir(vfio_dev_sysfs)
        .with_context(|| format!("failed to read {}", vfio_dev_sysfs.display()))?
        .next()
        .context("no vfio-dev entry present for device")?
        .context("failed to read vfio-dev directory entry")?;

openhcl/openhcl_dma_manager/src/lib.rs:534

• The comment on attach_pending_buffers says buffers are “already mapped” into the iommufd IOAS and re-mapping would collide, but in this PR IommufdIoas::map_block() is a no-op (by design for noiommu). This comment should be updated to match the actual behavior so future readers don’t assume there are persistent IOAS mappings involved.

        // servicing/keepalive restart. When this client maps into an iommufd
        // IOAS, that IOAS (and its buffer mappings) is preserved across the
        // restart along with the iommufd fd, so the buffers are already mapped
        // and their pool-provided PFNs are still the correct physical pages.
        // Re-mapping here would collide with the surviving mappings, so return

[github-actions]

1 Petri tests failed (0 unstable)