From 646448f512d6c207df87523d2404b993ec4746ee Mon Sep 17 00:00:00 2001 From: iximeow Date: Thu, 29 Jan 2026 20:07:54 +0000 Subject: [PATCH] Be more specific about version errors, raise min required viona API The recent multiqueue work includes an unconditional ioctl that was introduced in V6 of the viona API. Propolis' expectation should have been raised then, but it was missed in review; instead, trying to create a vNIC on a too-old OS results in propolis panicking about `Inappropriate ioctl for device`. So, raise the minimum viona API as required. propolis-server will exit at start if the OS is too old. The message wasn't super clear though, complaining that: > viona API version mismatch 4 != 6 so this comes with some adjustments to the version error reporting that now produce this error on a too-old host: > 0: checking version of viona > 1: API version 4 is not at or above 6. OS is too old? propolis-standalone allows you to try running something on a too-old host on the expectation you're actively hacking on either the host or Propolis, so it just offers > ERRO viona: API version 4 is not at or above 6. OS is too old? before continuing on (and in this case, panicking about the an inappropriate ioctl). --- bin/propolis-server/src/lib/initializer.rs | 35 ++++++++++------------ bin/propolis-standalone/src/main.rs | 22 ++++---------- lib/propolis/src/api_version.rs | 10 +++++-- lib/propolis/src/hw/virtio/viona.rs | 17 +++++++---- lib/propolis/src/vmm/mod.rs | 6 ++-- 5 files changed, 42 insertions(+), 48 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 8e5d4961b..c53ddb34d 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -771,32 +771,27 @@ impl MachineInitializer<'_> { info!(self.log, "Creating vNIC {}", device_name); let bdf: pci::Bdf = nic.device_spec.pci_path.into(); - // Set viona device parameters if possible. + // Set viona device parameters. The parameters here (copy_data and + // header_pad) require `viona::ApiVersion::V3`, below Propolis' + // minimum of V6, so we can always set them. // // The values chosen here are tuned to maximize performance when // Propolis is used with OPTE in a full Oxide rack deployment, // although they should not negatively impact use outside those // conditions. These parameters and their effects (save for // performance delta) are not guest-visible. - let params = if virtio::viona::api_version() - .expect("can query viona version") - >= virtio::viona::ApiVersion::V3 - { - Some(virtio::viona::DeviceParams { - // Loan guest packet data, rather than allocating fresh - // buffers and copying it. - copy_data: false, - // Leave room for underlay encapsulation: - // - ethernet: 14 - // - IPv6: 40 - // - UDP: 8 - // - Geneve: 8–16 (due to options) - // - (and then round up to nearest 8) - header_pad: 80, - }) - } else { - None - }; + let params = Some(virtio::viona::DeviceParams { + // Loan guest packet data, rather than allocating fresh + // buffers and copying it. + copy_data: false, + // Leave room for underlay encapsulation: + // - ethernet: 14 + // - IPv6: 40 + // - UDP: 8 + // - Geneve: 8–16 (due to options) + // - (and then round up to nearest 8) + header_pad: 80, + }); let viona = virtio::PciVirtioViona::new( &nic.backend_spec.vnic_name, diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index cc5a1d31c..42a97319a 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/bin/propolis-standalone/src/main.rs @@ -1251,15 +1251,9 @@ fn setup_instance( config::VionaDeviceParams::from_opts(&dev.options) .expect("viona params are valid"); - if viona_params.is_some() - && hw::virtio::viona::api_version() - .expect("can query viona version") - < hw::virtio::viona::ApiVersion::V3 - { - // lazy cop-out for now - panic!("can't set viona params on too-old version"); - } - + // The viona_params here (currently just copy_data and + // header_pad) require `viona::ApiVersion::V3`, below + // Propolis' minimum of V6, so we can always set them. let viona = hw::virtio::PciVirtioViona::new( vnic_name, &hdl, @@ -1452,18 +1446,12 @@ fn api_version_checks(log: &slog::Logger) -> std::io::Result<()> { } Err(VersionCheckError { component, + err: source @ Error::TooLow { .. }, path: _, - err: Error::Mismatch(act, exp), }) => { // Make noise about version mismatch, but soldier on and let the // user decide if they want to quit - slog::error!( - log, - "{} API version mismatch {} != {}", - component, - act, - exp - ); + slog::error!(log, "{component}: {source}"); Ok(()) } Ok(_) => Ok(()), diff --git a/lib/propolis/src/api_version.rs b/lib/propolis/src/api_version.rs index a2370fe98..00ba0ecba 100644 --- a/lib/propolis/src/api_version.rs +++ b/lib/propolis/src/api_version.rs @@ -7,8 +7,14 @@ pub enum Error { #[error("IO Error")] Io(#[from] std::io::Error), - #[error("API version {0} did not match expectation {1}")] - Mismatch(u32, u32), + // Newer APIs are backwards compatible with older (for now?), so don't + // bother trying to express "we need a version before .." or any of that. + // + // Also assume that the component being versioned is either part of the OS + // or that the OS version is a decent proxy for it. Not necessarily true in + // general, but true for viona and bhyve. + #[error("API version {have} is not at or above {want}. OS is too old?")] + TooLow { have: u32, want: u32 }, } #[derive(Debug, thiserror::Error)] diff --git a/lib/propolis/src/hw/virtio/viona.rs b/lib/propolis/src/hw/virtio/viona.rs index 1ae5d8000..c9ec21347 100644 --- a/lib/propolis/src/hw/virtio/viona.rs +++ b/lib/propolis/src/hw/virtio/viona.rs @@ -226,8 +226,6 @@ impl Inner { } /// Configuration parmaeters for the underlying viona device -/// -/// These parameters assume an [viona_api::ApiVersion::V3] device #[derive(Copy, Clone)] pub struct DeviceParams { /// When transmitting packets, should viona (allocate and) copy the entire @@ -237,12 +235,18 @@ pub struct DeviceParams { /// There is a performance cost to copying the full packet, but it avoids /// certain issues pertaining to looped-back viona packets being delivered /// to native zones on the machine. + /// + /// This parameter requires [viona_api::ApiVersion::V3] or greater. This is + /// before Propolis' minimum viona API version and can always be set. pub copy_data: bool, /// Byte count for padding added to the head of transmitted packets. This /// padding can be used by subsequent operations in the transmission chain, /// such as encapsulation, which would otherwise need to re-allocate for the /// larger header. + /// + /// This parameter requires [viona_api::ApiVersion::V3] or greater. This is + /// before Propolis' minimum viona API version and can always be set. pub header_pad: u16, } impl DeviceParams { @@ -1393,11 +1397,12 @@ use bits::*; pub(crate) fn check_api_version() -> Result<(), crate::api_version::Error> { let vers = viona_api::api_version()?; - // viona only requires the V2 bits for now - let compare = viona_api::ApiVersion::V2; + // when setting up a vNIC, Propolis will unconditionally do the SET_PAIRS + // ioctl, which requires V6. + let want = viona_api::ApiVersion::V6 as u32; - if vers < compare { - Err(crate::api_version::Error::Mismatch(vers, compare as u32)) + if vers < want { + Err(crate::api_version::Error::TooLow { have: vers, want }) } else { Ok(()) } diff --git a/lib/propolis/src/vmm/mod.rs b/lib/propolis/src/vmm/mod.rs index 033bc8991..80a25cfcb 100644 --- a/lib/propolis/src/vmm/mod.rs +++ b/lib/propolis/src/vmm/mod.rs @@ -19,10 +19,10 @@ pub(crate) fn check_api_version() -> Result<(), crate::api_version::Error> { let vers = ctl.api_version()?; // propolis only requires the bits provided by V8, currently - let compare = bhyve_api::ApiVersion::V8; + let want = bhyve_api::ApiVersion::V8 as u32; - if vers < compare { - return Err(crate::api_version::Error::Mismatch(vers, compare as u32)); + if vers < want { + return Err(crate::api_version::Error::TooLow { have: vers, want }); } Ok(())