From 9f773bbf89f76dfb0f90f958aec7cc6e17c3d5aa Mon Sep 17 00:00:00 2001 From: Artie Poole Date: Tue, 19 May 2026 12:32:25 +0100 Subject: [PATCH 1/4] dbus: platforms: softeners: move all universal specific logic out of FPGA trait implements two new dbus methods: - StatusInterface::universal - ControlInterface::universal functional changes: - set/get flags now universal only - read/write_property[_bytes] now universal only Signed-off-by: Artie Poole --- daemon/src/comm/dbus.rs | 222 +----- daemon/src/comm/dbus/control_interface.rs | 133 +--- daemon/src/comm/dbus/status_interface.rs | 62 +- daemon/src/platforms/platform.rs | 19 - daemon/src/platforms/universal.rs | 695 +++++++++++++++++- .../universal_components/universal_fpga.rs | 86 +-- .../xilinx_dfx_mgr/xilinx_dfx_mgr_fpga.rs | 65 -- 7 files changed, 721 insertions(+), 561 deletions(-) diff --git a/daemon/src/comm/dbus.rs b/daemon/src/comm/dbus.rs index e857648a..a09cebfb 100644 --- a/daemon/src/comm/dbus.rs +++ b/daemon/src/comm/dbus.rs @@ -39,130 +39,23 @@ //! | Interface | Method | Parameters | Returns / Notes | //! |-----------|--------------------------|----------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------| //! | [status](status_interface) | `get_fpga_state` | `platform_string: &str`, `device_handle: &str` | `String` – Current FPGA state (`unknown`, `operating`, etc.) | -//! | [status](status_interface) | `get_fpga_flags` | `platform_string: &str`, `device_handle: &str` | `String` – Current FPGA flags from hexadecimal integer to string | //! | [status](status_interface) | `get_overlay_status` | `platform_string: &str`, `overlay_handle: &str` | `String` – Overlay status; errors if handle empty or invalid | //! | [status](status_interface) | `get_overlays` | None | `String` – List of available overlay handles (`\n` separated) | //! | [status](status_interface) | `get_platform_type` | `device_handle: &str` | `String` – Compatibility string for device | //! | [status](status_interface) | `get_platform_types` | None | `String` – List of all fpga devices and their compatibility strings (`:\n`) | -//! | [status](status_interface) | `read_property` | `property_path_str: &str` | `String` – Contents of arbitrary FPGA attribute value | -//! | [control](control_interface) | `set_fpga_flags` | `platform_string: &str`, `device_handle: &str`, `flags: u32` | `String` – Confirmation with new flags in hex | +//! | [status](status_interface) | `universal` | `sub_cmd: &str`, `path_str: &str` | `String` – For universal reads; use `read_property` for sysfs property paths or `read_flags` for device handles | //! | [control](control_interface) | `write_bitstream_direct` | `platform_string: &str`, `device_handle: &str`, `bitstream_path_str: &str`, `firmware_lookup_path: &str` | `String` – Confirmation of bitstream load; acquires write lock | //! | [control](control_interface) | `apply_overlay` | `platform_string: &str`, `overlay_handle: &str`, `overlay_source_path: &str`, `firmware_lookup_path: &str` | `String` – Overlay applied; confirmation including firmware prefix; write lock used to protect against firmware search path race conditions | //! | [control](control_interface) | `remove_overlay` | `platform_string: &str`, `overlay_handle: &str` | `String` – Overlay removed; confirmation including overlay filesystem path | -//! | [control](control_interface) | `write_property` | `property_path_str: &str`, `data: &str` | `String` – Confirmation of data written; path must be under `/sys/class/fpga_manager/` | -//! | [control](control_interface) | `write_property_bytes` | `property_path_str: &str`, `data: &[u8]` | `String` – Confirmation of bytes written; path must be under `/sys/class/fpga_manager/` | +//! | [control](control_interface) | `universal` | `sub_cmd: &str`, `path_str: &str`, `value_str: &str` | `String` – For universal writes; use `write_flags` for device handles, `write_property` for string data, or `write_property_bytes` for raw bytes | pub mod control_interface; pub mod status_interface; use crate::config; use crate::error::FpgadError; -use crate::system_io::fs_read; -use std::path; -use std::path::{Component, Path, PathBuf}; -/// Validates that a property path is constrained under the fpga manager directory and does not contain explicit parent traversal segments. -/// This is used to validate paths for all read/write property access methods in the control and status interfaces. -/// -/// # Arguments -/// * `property_path` - The property path to validate as a Path. -/// -/// # Returns: `Result` -/// A `PathBuf` representing the validated property path if it is valid, or a `FpgadError` if the path is invalid. -/// -/// # Examples -/// ```rust,no_run -/// let valid_path = validate_property_path("/sys/class/fpga_manager/fpga0/name")?; -/// assert_eq!(valid_path.to_string_lossy(), "/sys/class/fpga_manager/fpga0/name"); -/// let invalid_path = validate_property_path("/sys/class/fpga_manager/../etc/passwd"); -/// assert!(invalid_path.is_err()); -/// ``` -pub(crate) fn validate_property_path(property_path: &Path) -> Result { - validate_property_path_with_base(property_path, Path::new(config::FPGA_MANAGERS_DIR)) -} - -/// Validates that a property path is constrained under a specified base path and does not contain -/// explicit parent traversal segments. This is a more general version of `validate_property_path` which -/// can be used to validate paths under different base directories, such as the firmware lookup control path. -/// -/// # Arguments -/// * `property_path` - The property path to validate as a Path. -/// * `base_path` - The base path under which the property path must be constrained. -/// -/// # Returns: `Result` -/// A `PathBuf` representing the validated property path if it is valid, or a `FpgadError` if the path is invalid. -/// -/// # Examples -/// ```rust,no_run -/// let valid_path = validate_property_path_with_base("/sys/class/fpga_manager/fpga0/name", Path::new("/sys/class/fpga_manager/"))?; -/// assert_eq!(valid_path.to_string_lossy(), "/sys/class/fpga_manager/fpga0/name"); -/// let invalid_path = validate_property_path_with_base("/sys/class/fpga_manager/../etc/passwd", Path::new("/sys/class/fpga_manager/")); -/// assert!(invalid_path.is_err()); -/// ``` -pub(crate) fn validate_property_path_with_base( - property_path: &Path, - base_path: &Path, -) -> Result { - let property_path = PathBuf::from(property_path); - if property_path - .components() - .any(|component| matches!(component, Component::ParentDir)) - { - return Err(FpgadError::Argument(format!( - "Cannot access property {}: path traversal ('..') is not allowed", - property_path.display() - ))); - } - - let canonical_base = path::absolute(base_path).map_err(|e| { - FpgadError::Argument(format!( - "Cannot access property {}: failed to resolve base path {}: {}", - property_path.display(), - base_path.display(), - e - )) - })?; - let canonical_property = path::absolute(&property_path).map_err(|e| { - FpgadError::Argument(format!( - "Cannot access property {}: failed to resolve property path: {}", - property_path.display(), - e - )) - })?; - - if !canonical_property.starts_with(&canonical_base) { - return Err(FpgadError::Argument(format!( - "Cannot access property {}: resolved path {} is outside {}", - property_path.display(), - canonical_property.display(), - canonical_base.display() - ))); - } - - Ok(canonical_property) -} - -/// Read the current contents of an FPGA device property, e.g. "name". The property path must be a subdirectory of the fpga manager directory (typically, /sys/class/fpga_manager/) -/// -/// # Arguments -/// -/// * `property_path_str`: path to the variable to read e.g. /sys/class/fpga_manager/fpga0/name -/// -/// # Returns: `Result` -/// * `String` - the contents of the property path -/// -/// * `FpgadError::Argument` if the path is not found within the compile time [config::FPGA_MANAGERS_DIR] -/// -/// # Examples -/// -/// ```rust,no_run -/// let device_name = fs_read_property("/sys/class/fpga_manager/fpga0/name")?; -/// assert_eq!(device_name, "Xilinx ZynqMP FPGA Manager\n") -/// ``` -pub fn fs_read_property(property_path_str: &str) -> Result { - let property_path = validate_property_path(Path::new(property_path_str))?; - fs_read(&property_path) -} +use std::path::PathBuf; /// Helper function to check that a device with given handle does exist. /// @@ -199,112 +92,3 @@ pub(crate) fn validate_device_handle(device_handle: &str) -> Result<(), FpgadErr }; Ok(()) } - -#[cfg(test)] -mod test_validate_property_path { - use crate::comm::dbus::validate_property_path_with_base; - use googletest::prelude::*; - use std::fs; - use std::path::PathBuf; - use std::time::{SystemTime, UNIX_EPOCH}; - - fn unique_test_dir(test_name: &str) -> PathBuf { - let nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system clock before unix epoch") - .as_nanos(); - std::env::temp_dir().join(format!("fpgad_validate_property_path_{test_name}_{nanos}")) - } - - #[gtest] - fn should_pass_valid_path() { - let root = unique_test_dir("valid_path"); - let base = root.join("fpga_manager"); - let property = base.join("fpga0").join("name"); - - fs::create_dir_all(property.parent().expect("property should have parent")) - .expect("create parent dirs"); - fs::write(&property, "name\n").expect("create property file"); - - let expected = fs::canonicalize(&property).expect("canonicalize property"); - let result = validate_property_path_with_base(&property, &base); - - fs::remove_dir_all(root).expect("cleanup temp dirs"); - assert_that!(&result, ok(eq(&expected))); - } - - #[gtest] - fn should_fail_for_path_outside_fpga_dir() { - let root = unique_test_dir("outside_base"); - let base = root.join("fpga_manager"); - let outside = root.join("outside").join("evil_file.sh"); - - fs::create_dir_all(&base).expect("create base dir"); - fs::create_dir_all(outside.parent().expect("outside should have parent")) - .expect("create outside dir"); - fs::write(&outside, "evil\n").expect("create outside file"); - - let result = validate_property_path_with_base(&outside, &base); - - fs::remove_dir_all(root).expect("cleanup temp dirs"); - assert_that!(&result, err(displays_as(contains_substring("is outside")))); - } - - #[gtest] - fn should_fail_for_root_path_traversal() { - let root = unique_test_dir("root_traversal"); - let base = root.join("fpga_manager"); - fs::create_dir_all(&base).expect("create base dir"); - - let traversal = base.join("..").join("outside").join("evil_file.sh"); - let result = validate_property_path_with_base(&traversal, &base); - - fs::remove_dir_all(root).expect("cleanup temp dirs"); - assert_that!( - &result, - err(displays_as(contains_substring("path traversal"))) - ); - } - - #[gtest] - fn should_fail_for_device_path_traversal() { - let root = unique_test_dir("device_traversal"); - let base = root.join("fpga_manager"); - fs::create_dir_all(base.join("fpga0")).expect("create fpga0 dir"); - - let traversal = base.join("fpga0").join("..").join("name"); - let result = validate_property_path_with_base(&traversal, &base); - - fs::remove_dir_all(root).expect("cleanup temp dirs"); - assert_that!( - &result, - err(displays_as(contains_substring("path traversal"))) - ); - } - - #[cfg(unix)] - #[gtest] - fn should_allow_symlink_path_without_resolution() { - use std::os::unix::fs::symlink; - use std::path::absolute; - - let root = unique_test_dir("symlink_escape"); - let base = root.join("fpga_manager"); - let outside = root.join("outside"); - let link_target_file = outside.join("escaped_name"); - let fpga0_dir = base.join("fpga0"); - let link_in_base = fpga0_dir.join("link_outside"); - - fs::create_dir_all(&fpga0_dir).expect("create fpga0 dir"); - fs::create_dir_all(&outside).expect("create outside dir"); - fs::write(&link_target_file, "evil\n").expect("create outside target file"); - symlink(&outside, &link_in_base).expect("create symlink escaping base"); - - let escaped_path = link_in_base.join("escaped_name"); - let expected = absolute(&escaped_path).expect("resolve absolute escaped path"); - let result = validate_property_path_with_base(&escaped_path, &base); - - fs::remove_dir_all(root).expect("cleanup temp dirs"); - assert_that!(&result, ok(eq(&expected))); - } -} diff --git a/daemon/src/comm/dbus/control_interface.rs b/daemon/src/comm/dbus/control_interface.rs index 5b33b719..eed14d55 100644 --- a/daemon/src/comm/dbus/control_interface.rs +++ b/daemon/src/comm/dbus/control_interface.rs @@ -16,11 +16,11 @@ //! If FPGAd raises the error, then the fdo::Error strings are prepended with the relevant FPGAd error type e.g. `FpgadError::Argument: `. See [crate::comm::dbus] for a summary of this interface's methods. //! -use crate::comm::dbus::{validate_device_handle, validate_property_path}; +use crate::comm::dbus::validate_device_handle; use crate::error::map_error_io_to_fdo; use crate::platforms::platform::{platform_for_known_platform, platform_from_compat_or_device}; +use crate::platforms::universal::universal_write_handler; use crate::softeners::error::FpgadSoftenerError; -use crate::system_io::{fs_write, fs_write_bytes}; use log::{info, trace}; use std::env; use std::path::Path; @@ -75,48 +75,6 @@ pub struct ControlInterface {} /// [crate::comm::dbus::control_interface] for a summary of this interface in general. #[interface(name = "com.canonical.fpgad.control")] impl ControlInterface { - /// Set the flags for a specific FPGA device. - /// - /// # Arguments - /// - /// * `platform_string`: Platform compatibility string. - /// * `device_handle`: FPGA device handle (e.g., `fpga0`). - /// * `flags`: Bitmask flags to apply to the device. - /// - /// # Returns: `Result` - /// * `Ok(String)` – Confirmation message, including the new flags in hex. - /// * `Err(fdo::Error)` if device validation or flag setting fails. - /// - /// # Examples - /// - /// Specify device - /// ``` - /// let result = control_interface - /// .set_fpga_flags("xlnx,zynqmp-pcap-fpga", "fpga0", 0x20) - /// .await?; - /// assert_eq!(result, "Flags set to 0x20 for fpga0"); - /// ``` - /// - /// Don't specify compat string (fetches compat string based on `device_handle`) - /// ```rust - /// let result = control_interface - /// .set_fpga_flags("", "fpga0", 0b100000) - /// .await?; - /// assert_eq!(result, "Flags set to 0x20 for fpga0"); - /// ``` - async fn set_fpga_flags( - &self, - platform_string: &str, - device_handle: &str, - flags: u32, - ) -> Result { - // TODO(Artie): https://github.com/canonical/fpgad/issues/187 - info!("set_fpga_flags called with name: {device_handle} and flags: {flags}"); - validate_device_handle(device_handle)?; - let platform = platform_from_compat_or_device(platform_string, device_handle)?; - Ok(platform.fpga(device_handle)?.set_flags(flags)?) - } - /// Trigger a bitstream-only load of a bitstream to a given FPGA device (i.e. no device-tree changes or driver installation). /// /// # Arguments @@ -335,87 +293,22 @@ impl ControlInterface { Ok(fpga.remove_firmware(handle)?) } - /// Write a string value to an arbitrary FPGA device property. - /// - /// # Arguments - /// - /// * `property_path_str`: Full path under [crate::config::FPGA_MANAGERS_DIR]. - /// * `data`: String data to write. - /// - /// # Returns: `Result` - /// - /// * `Ok(String)` – Confirmation of written data. - /// * `Err(fdo::Error)` if path is outside FPGA managers, or if the writing failed for any - /// other reason - /// **Notes:** - /// - /// * Path must be under [crate::config::FPGA_MANAGERS_DIR] - determined at compile time. - /// - /// # Examples - /// - /// ``` - /// let result = control_interface - /// .write_property( - /// "xlnx,zynqmp-pcap-fpga", - /// "/sys/class/fpga_manager/fpga0/key", - /// "BADBADBADBAD") - /// .await?; - /// assert_eq!(result, "BADBADBADBAD written to /sys/class/fpga_manager/fpga0/key"); - /// ``` - async fn write_property( - &self, - property_path_str: &str, - data: &str, - ) -> Result { - // TODO(Artie): https://github.com/canonical/fpgad/issues/187 - info!("write_property called with property_path_str: {property_path_str} and data: {data}"); - let property_path = validate_property_path(Path::new(property_path_str))?; - fs_write(&property_path, false, data)?; - Ok(format!("{data} written to {property_path_str}")) - } - - /// Write raw bytes to an arbitrary FPGA device property. + /// Entrypoint for universal platform specific operations. /// /// # Arguments /// - /// * `property_path_str`: Full path under [crate::config::FPGA_MANAGERS_DIR]. - /// * `data`: Byte array to write. - /// - /// # Returns: `Result` - /// - /// * `Ok(String)` – Confirmation of written data. - /// * `Err(fdo::Error)` if path is outside FPGA managers, or if the writing failed for any - /// other reason - /// - /// **Notes:** - /// - /// * Path must be under [crate::config::FPGA_MANAGERS_DIR] - determined at compile time. - /// - /// # Examples - /// - /// ``` - /// let result = control_interface - /// .write_property_bytes( - /// "xlnx,zynqmp-pcap-fpga", - /// "/sys/class/fpga_manager/fpga0/key", - /// &[0xBA, 0xDB, 0xAD, 0xBA, 0xDB, 0xAD]) - /// .await?; - /// assert_eq!(result, "Byte string successfully written to /sys/class/fpga_manager/fpga0/key"); - /// ``` - async fn write_property_bytes( + /// * `sub_cmd` - One of `write_flags`, `write_property`, `write_property_bytes` + /// * `path_str` - Device handle for `write_flags`, or sysfs property path for property writes + /// * `value_str` - Value to write (flags value, string payload for `write_property`, + /// or raw byte string for `write_property_bytes`) + async fn universal( &self, - property_path_str: &str, - data: &[u8], + sub_cmd: &str, + path_str: &str, + value_str: &str, ) -> Result { - // TODO(Artie): https://github.com/canonical/fpgad/issues/187 - info!( - "write_property called with property_path_str: {property_path_str} and data: {data:?}" - ); - let property_path = validate_property_path(Path::new(property_path_str))?; - fs_write_bytes(&property_path, false, data)?; - Ok(format!( - "Byte string successfully written to {property_path_str}" - )) + info!("universal (write) called with sub_cmd: {sub_cmd}, path_str: {path_str}"); + universal_write_handler(sub_cmd, path_str, value_str) } /// Entrypoint for dfx-mgr specific operations diff --git a/daemon/src/comm/dbus/status_interface.rs b/daemon/src/comm/dbus/status_interface.rs index 269dcd94..85a6e8ba 100644 --- a/daemon/src/comm/dbus/status_interface.rs +++ b/daemon/src/comm/dbus/status_interface.rs @@ -15,11 +15,12 @@ //! All methods return a `Result` and are designed for DBus usage. //! If FPGAd raises the error, then the `fdo::Error` strings are prepended with the relevant FPGAd error type e.g. `FpgadError::Argument: `. See [crate::comm::dbus] for a summary of this interface's methods. //! -use crate::comm::dbus::{fs_read_property, validate_device_handle}; +use crate::comm::dbus::validate_device_handle; use crate::config; use crate::error::FpgadError; use crate::platforms::platform::{list_fpga_managers, read_compatible_string}; use crate::platforms::platform::{platform_for_known_platform, platform_from_compat_or_device}; +use crate::platforms::universal::universal_read_handler; use crate::system_io::fs_read_dir; use log::{error, info}; use zbus::{fdo, interface}; @@ -68,39 +69,6 @@ impl StatusInterface { Ok(platform.fpga(device_handle)?.state()?) } - /// Retrieve the current flags set for a specified FPGA device as a hexadecimal ascii string - /// (missing `0x` prefix). - /// - /// # Arguments - /// - /// * `platform_string`: Platform compatibility string. - /// * `device_handle`: The device handle (e.g., `fpga0`) of the FPGA. - /// - /// # Returns: `Result` - /// * `Ok(String)` – hexadecimal representation of flags without `0x` hex prefix (e.g. "20" - /// for decimal value of 32) - /// * `Err(fdo::Error)` on invalid handle or platform error. - /// - /// # Examples - /// - /// ```rust,no_run - /// let flags = status_interface.get_fpga_flags("xlnx,zynqmp-pcap-fpga", "fpga0").await?; - /// assert_eq!(flags, "20"); - /// ``` - async fn get_fpga_flags( - &self, - platform_string: &str, - device_handle: &str, - ) -> Result { - info!("get_fpga_flags called with name: {device_handle}"); - validate_device_handle(device_handle)?; - let platform = platform_from_compat_or_device(platform_string, device_handle)?; - Ok(platform - .fpga(device_handle)? - .flags() - .map(|flags| flags.to_string())?) - } - /// Retrieve the status of a specific device-tree overlay. /// /// # Arguments @@ -233,28 +201,14 @@ impl StatusInterface { } Ok(ret_string) } - - /// Read an arbitrary FPGA device property from `/sys/class/fpga_manager//`. + /// Unified read entrypoint for universal platform operations. /// /// # Arguments /// - /// * `property_path_str`: Full path to the property file. - /// - /// # Returns: `Result` - /// * `Ok(String)` – Contents of the property file. - /// * `Err(fdo::Error)` If the property cannot be read. - /// - /// # Examples - /// - /// ```rust,no_run - /// let name = status_interface. - /// read_property("/sys/class/fpga_manager/fpga0/name") - /// .await?; - /// assert_eq!(name, "Xilinx ZynqMP FPGA Manager\n"); - /// ``` - /// - async fn read_property(&self, property_path_str: &str) -> Result { - info!("read_property called with property_path_str: {property_path_str}"); - Ok(fs_read_property(property_path_str)?) + /// * `sub_cmd` - One of `read_property` or `read_flags` + /// * `path_str` - Sysfs property path for `read_property`, or device handle for `read_flags` + async fn universal(&self, sub_cmd: &str, path_str: &str) -> Result { + info!("universal (read) called with sub_cmd: {sub_cmd}, path_str: {path_str}"); + universal_read_handler(sub_cmd, path_str) } } diff --git a/daemon/src/platforms/platform.rs b/daemon/src/platforms/platform.rs index ba74b207..401a7480 100644 --- a/daemon/src/platforms/platform.rs +++ b/daemon/src/platforms/platform.rs @@ -111,25 +111,6 @@ pub trait Fpga { /// * `Err(FpgadError::IORead)` - Failed to read state file fn state(&self) -> Result; - /// Get the current programming flags for the FPGA device. - /// - /// # Returns: `Result` - /// * `Ok(u32)` - Current flags value - /// * `Err(FpgadError::IORead)` - Failed to read flags file - /// * `Err(FpgadError::Flag)` - Failed to parse flags value - fn flags(&self) -> Result; - - /// Set the programming flags for the FPGA device. - /// - /// # Arguments - /// - /// * `flags` - The flags value to set - /// - /// # Returns: `Result` - /// * `Ok(String)` - Confirmation message including flags value and device handle - /// * `Err(FpgadError::IOWrite)` - Failed to write flags file - fn set_flags(&self, flags: u32) -> Result; - /// Load a bitstream firmware file to the FPGA device. /// /// # Arguments diff --git a/daemon/src/platforms/universal.rs b/daemon/src/platforms/universal.rs index 7a6a0c80..96e23a3f 100644 --- a/daemon/src/platforms/universal.rs +++ b/daemon/src/platforms/universal.rs @@ -45,15 +45,19 @@ //! let state = fpga.state()?; //! ``` +use crate::comm::dbus::validate_device_handle; use crate::config; use crate::error::FpgadError; use crate::platforms::platform::{Fpga, OverlayHandler, Platform, list_fpga_managers}; use crate::platforms::universal_components::universal_fpga::UniversalFPGA; use crate::platforms::universal_components::universal_overlay_handler::UniversalOverlayHandler; -use crate::system_io::fs_read_dir; +use crate::system_io::{fs_read, fs_read_dir, fs_write, fs_write_bytes}; use fpgad_macros::platform; -use log::trace; +use log::{error, info, trace, warn}; +use std::path; +use std::path::{Component, Path, PathBuf}; use std::sync::OnceLock; +use zbus::fdo; /// Universal platform implementation for generic FPGA management. /// @@ -217,3 +221,690 @@ impl Platform for UniversalPlatform { "universal".into() } } + +/// Validates that a property path is constrained under the fpga manager directory and does not contain explicit parent traversal segments. +/// This is used to validate paths for all read/write property access methods in the control and status interfaces. +/// +/// # Arguments +/// * `property_path` - The property path to validate as a Path. +/// +/// # Returns: `Result` +/// A `PathBuf` representing the validated property path if it is valid, or a `FpgadError` if the path is invalid. +/// +/// # Examples +/// ```rust,no_run +/// let valid_path = validate_property_path("/sys/class/fpga_manager/fpga0/name")?; +/// assert_eq!(valid_path.to_string_lossy(), "/sys/class/fpga_manager/fpga0/name"); +/// let invalid_path = validate_property_path("/sys/class/fpga_manager/../etc/passwd"); +/// assert!(invalid_path.is_err()); +/// ``` +pub(crate) fn validate_property_path(property_path: &Path) -> Result { + validate_property_path_with_base(property_path, Path::new(config::FPGA_MANAGERS_DIR)) +} + +/// Validates that a property path is constrained under a specified base path and does not contain +/// explicit parent traversal segments. This is a more general version of `validate_property_path` which +/// can be used to validate paths under different base directories, such as the firmware lookup control path. +/// +/// # Arguments +/// * `property_path` - The property path to validate as a Path. +/// * `base_path` - The base path under which the property path must be constrained. +/// +/// # Returns: `Result` +/// A `PathBuf` representing the validated property path if it is valid, or a `FpgadError` if the path is invalid. +/// +/// # Examples +/// ```rust,no_run +/// let valid_path = validate_property_path_with_base("/sys/class/fpga_manager/fpga0/name", Path::new("/sys/class/fpga_manager/"))?; +/// assert_eq!(valid_path.to_string_lossy(), "/sys/class/fpga_manager/fpga0/name"); +/// let invalid_path = validate_property_path_with_base("/sys/class/fpga_manager/../etc/passwd", Path::new("/sys/class/fpga_manager/")); +/// assert!(invalid_path.is_err()); +/// ``` +fn validate_property_path_with_base( + property_path: &Path, + base_path: &Path, +) -> Result { + let property_path = PathBuf::from(property_path); + if property_path + .components() + .any(|component| matches!(component, Component::ParentDir)) + { + return Err(FpgadError::Argument(format!( + "Cannot access property {}: path traversal ('..') is not allowed", + property_path.display() + ))); + } + + let canonical_base = path::absolute(base_path).map_err(|e| { + FpgadError::Argument(format!( + "Cannot access property {}: failed to resolve base path {}: {}", + property_path.display(), + base_path.display(), + e + )) + })?; + let canonical_property = path::absolute(&property_path).map_err(|e| { + FpgadError::Argument(format!( + "Cannot access property {}: failed to resolve property path: {}", + property_path.display(), + e + )) + })?; + + if !canonical_property.starts_with(&canonical_base) { + return Err(FpgadError::Argument(format!( + "Cannot access property {}: resolved path {} is outside {}", + property_path.display(), + canonical_property.display(), + canonical_base.display() + ))); + } + + Ok(canonical_property) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum ReadSubCommand { + ReadProp, + ReadFlags, +} + +impl ReadSubCommand { + pub fn as_str(self) -> &'static str { + match self { + ReadSubCommand::ReadFlags => "read_flags", + ReadSubCommand::ReadProp => "read_property", + } + } +} + +impl std::str::FromStr for ReadSubCommand { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "read_flags" => Ok(ReadSubCommand::ReadFlags), + "read_property" => Ok(ReadSubCommand::ReadProp), + _ => Err(()), + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum WriteSubCommand { + WriteFlags, + WriteProp, + WriteByte, +} + +impl WriteSubCommand { + pub fn as_str(self) -> &'static str { + match self { + WriteSubCommand::WriteFlags => "write_flags", + WriteSubCommand::WriteProp => "write_property", + WriteSubCommand::WriteByte => "write_property_bytes", + } + } +} + +impl std::str::FromStr for WriteSubCommand { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "write_flags" => Ok(WriteSubCommand::WriteFlags), + "write_property" => Ok(WriteSubCommand::WriteProp), + "write_property_bytes" => Ok(WriteSubCommand::WriteByte), + _ => Err(()), + } + } +} + +/// Read the current contents of an FPGA device property, e.g. "name". The property path must be a subdirectory of the fpga manager directory (typically, /sys/class/fpga_manager/) +/// +/// # Arguments +/// +/// * `property_path_str`: path to the variable to read e.g. /sys/class/fpga_manager/fpga0/name +/// +/// # Returns: `Result` +/// * `String` - the contents of the property path +/// +/// * `FpgadError::Argument` if the path is not found within the compile time [config::FPGA_MANAGERS_DIR] +/// +/// # Examples +/// +/// ```rust,no_run +/// let device_name = fs_read_property("/sys/class/fpga_manager/fpga0/name")?; +/// assert_eq!(device_name, "Xilinx ZynqMP FPGA Manager\n") +/// ``` +pub fn fs_read_property(property_path_str: &str) -> Result { + let property_path = validate_property_path(Path::new(property_path_str))?; + fs_read(&property_path) +} + +/// Read an arbitrary FPGA device property from `/sys/class/fpga_manager//`. +/// +/// # Arguments +/// +/// * `property_path_str`: Full path to the property file. +/// +/// # Returns: `Result` +/// * `Ok(String)` – Contents of the property file. +/// * `Err(fdo::Error)` If the property cannot be read. +/// +/// # Examples +/// +/// ```rust,no_run +/// let name = status_interface +/// .universal("read_property", "/sys/class/fpga_manager/fpga0/name") +/// .await?; +/// assert_eq!(name, "Xilinx ZynqMP FPGA Manager\n"); +/// ``` +/// +pub fn read_property(property_path_str: &str) -> Result { + info!("read_property called with property_path_str: {property_path_str}"); + Ok(fs_read_property(property_path_str)?) +} + +/// Write a string value to an arbitrary FPGA device property. +/// +/// # Arguments +/// +/// * `property_path_str`: Full path under [crate::config::FPGA_MANAGERS_DIR]. +/// * `data`: String data to write. +/// +/// # Returns: `Result` +/// +/// * `Ok(String)` – Confirmation of written data. +/// * `Err(fdo::Error)` if path is outside FPGA managers, or if the writing failed for any +/// other reason +/// +/// **Notes:** +/// +/// * Path must be under [crate::config::FPGA_MANAGERS_DIR] - determined at compile time. +/// +/// # Examples +/// +/// ``` +/// let result = control_interface +/// .universal( +/// "write_property", +/// "/sys/class/fpga_manager/fpga0/key", +/// "BADBADBADBAD") +/// .await?; +/// assert_eq!(result, "BADBADBADBAD written to /sys/class/fpga_manager/fpga0/key"); +/// ``` +pub fn write_property(property_path_str: &str, data: &str) -> Result { + info!("write_property called with property_path_str: {property_path_str} and data: {data}"); + let property_path = validate_property_path(Path::new(property_path_str))?; + fs_write(&property_path, false, data)?; + Ok(format!("{data} written to {property_path_str}")) +} + +/// Write raw bytes to an arbitrary FPGA device property. +/// +/// # Arguments +/// +/// * `property_path_str`: Full path under [crate::config::FPGA_MANAGERS_DIR]. +/// * `data`: Byte array to write. +/// +/// # Returns: `Result` +/// +/// * `Ok(String)` – Confirmation of written data. +/// * `Err(fdo::Error)` if path is outside FPGA managers, or if the writing failed for any +/// other reason +/// +/// **Notes:** +/// +/// * Path must be under [crate::config::FPGA_MANAGERS_DIR] - determined at compile time. +/// +/// # Examples +/// +/// ``` +/// let result = control_interface +/// .universal( +/// "write_property_bytes", +/// "/sys/class/fpga_manager/fpga0/key", +/// "BADBAD") +/// .await?; +/// assert_eq!(result, "Byte string successfully written to /sys/class/fpga_manager/fpga0/key"); +/// ``` +fn write_property_bytes(property_path_str: &str, data: &[u8]) -> Result { + info!("write_property called with property_path_str: {property_path_str} and data: {data:?}"); + let property_path = validate_property_path(Path::new(property_path_str))?; + fs_write_bytes(&property_path, false, data)?; + Ok(format!( + "Byte string successfully written to {property_path_str}" + )) +} + +/// Read the current programming flags from sysfs. +/// +/// Reads `/sys/class/fpga_manager//flags`, parses the hexadecimal string +/// (format: "0x...", or undecorated), and returns the flags as u32. +/// +/// # Returns: `Result` +/// * `Ok(u32)` - Current flags value +/// * `Err(FpgadError::IORead)` - Failed to read flags file +/// * `Err(FpgadError::Flag)` - Failed to parse hexadecimal value +fn flags(fpga: &UniversalFPGA) -> Result { + let flag_path = Path::new(config::FPGA_MANAGERS_DIR) + .join(fpga.device_handle()) + .join("flags"); + let contents = fs_read(&flag_path)?; + let trimmed = contents.trim().trim_start_matches("0x"); + u32::from_str_radix(trimmed, 16).map_err(|_| FpgadError::Flag("Parsing flags failed".into())) +} + +/// Set the programming flags in sysfs. +/// +/// Writes the flags to `/sys/class/fpga_manager//flags` in undecorated +/// hexadecimal (decimal `32` -> undecorated hex `20`) and verifies that the write +/// succeeded by reading the value back. +/// Also checks and logs the FPGA state after setting flags. +/// +/// # Arguments +/// +/// * `flags` - The flags value to set +/// +/// # Returns: `Result<(), FpgadError>` +/// * `Ok(())` - Flags set and verified successfully +/// * `Err(FpgadError::IOWrite)` - Failed to write flags file +/// * `Err(FpgadError::IORead)` - Failed to read back flags or state +/// * `Err(FpgadError::Flag)` - Read-back value doesn't match written value +fn set_flags(fpga: &UniversalFPGA, new_flags: u32) -> Result { + let device_handle = fpga.device_handle(); + let flag_path = Path::new(config::FPGA_MANAGERS_DIR) + .join(device_handle) + .join("flags"); + + trace!("Writing '0x{new_flags:X}' to '{flag_path:?}'"); + if let Err(e) = fs_write(&flag_path, false, format!("0x{new_flags:X}")) { + error!("Failed to read state."); + return Err(e); + } + + match fpga.state() { + Ok(state) => match state.as_str() { + "operating" => { + info!( + "{}'s state is 'operating' after writing flags.", + device_handle + ) + } + _ => { + warn!( + "{}'s state is '{}' after writing flags.", + device_handle, state + ); + } + }, + Err(e) => return Err(e), + }; + + let returned_flags = flags(fpga)?; + if returned_flags == new_flags { + Ok(format!( + "Flags set to '0x{:X}' for '{}'", + new_flags, device_handle + )) + } else { + Err(FpgadError::Flag(format!( + "Setting flags of '{}' to '0x{:X}' failed. Resulting flag was '0x{:X}'", + device_handle, new_flags, returned_flags + ))) + } +} + +/// Parse a hexadecimal string into raw bytes. +/// +/// Accepts an ASCII string representing a hexadecimal byte stream. The input may +/// contain optional whitespace separators and may optionally include `0x` prefixes. +/// +/// All whitespace is removed and all occurrences of 0x are removed from the input prior to parsing. +/// The remaining string is treated as a continuous hexadecimal stream. +/// +/// If the resulting stream has an odd number of hexadecimal digits, a leading `0` +/// is implicitly prepended to allow nibble-aligned parsing. +/// +/// Each pair of hexadecimal digits is then converted into a single `u8`. +/// +/// Supported input formats: +/// - `"00 04 02 20 20"` → `[0x00, 0x04, 0x02, 0x20, 0x20]` +/// - `"0x00 0x04 0x02 0x20 0x20"` → `[0x00, 0x04, 0x02, 0x20, 0x20]` +/// - `"0004022020"` → `[0x00, 0x04, 0x02, 0x20, 0x20]` +/// - `"AA"` → `[0xAA]` +/// - `"aa"` → `[0xAA]` +/// - `"1FF"` → `[0x01, 0xFF]` +/// - `"0x1FF"` → `[0x01, 0xFF]` +/// - `"0x0x10"` → `[0x10]` +/// +/// Note: +/// - Whitespace is ignored and only acts as a separator. +/// - Optional `0x` prefixes are stripped before parsing. +/// - Input is treated as a continuous hex stream after normalization. +/// - Odd-length hex streams are left-padded with a single `0` before decoding. +/// - Each resulting byte must be within `0x00..=0xFF`. +/// +/// # Arguments +/// +/// * `value_str` - A hexadecimal-formatted ASCII string representing a byte stream +/// +/// # Returns: `Result, FpgadError>` +/// +/// * `Ok(Vec)` - Successfully parsed byte vector from hex stream +/// * `Err(FpgadError::Argument)` - Input contains invalid hexadecimal characters +/// or malformed byte values that cannot be parsed into `u8` +fn hex_from_string(value_str: &str) -> Result, FpgadError> { + let clean: String = value_str + .split_whitespace() + .collect::() + .to_lowercase() + .replace("0x", ""); + + let mut chars = clean.chars().collect::>(); + + // If odd number of nibbles, left-pad with '0' + if chars.len() % 2 != 0 { + chars.insert(0, '0'); + } + + chars + .chunks(2) + .map(|chunk| { + let s: String = chunk.iter().collect(); + + u8::from_str_radix(&s, 16) + .map_err(|e| FpgadError::Argument(format!("Invalid hex byte '{s}': {e}"))) + }) + .collect() +} + +/// Dispatches write subcommands for the universal DBus control API. +/// +/// Supported `sub_cmd_str` values: +/// - `write_flags` to set FPGA flags for a given device handle or `property_path`. +/// Uses the validated device handle to create an FPGA object +/// - `write_property` to write string data to a validated sysfs property path +/// - `write_property_bytes` to write the raw bytes of `value_str` to a validated sysfs property path +pub fn universal_write_handler( + sub_cmd_str: &str, + property_path: &str, + value_str: &str, +) -> Result { + match sub_cmd_str.parse::() { + Ok(WriteSubCommand::WriteFlags) => { + // extracts device handle e.g. "fpga0" from "/sys/class/fpga_manager/fpga0/flags" or + // just treats input as a device handle + let device_handle = if let Some(rest) = + property_path.strip_prefix(config::FPGA_MANAGERS_DIR) + { + let handle = rest + .split('/') + .next() + .filter(|s| !s.is_empty()) + .ok_or_else(|| { + FpgadError::Argument(format!( + "Invalid FPGA manager path '{property_path}', could not extract device handle" + )) + })?; + // todo(artie): decide whether to check that "flags" is the final part or just allow + validate_device_handle(handle)?; + handle + } else { + validate_device_handle(property_path)?; + property_path + }; + + let fpga = UniversalFPGA::new(device_handle); + + let trimmed = value_str.trim(); + let parsed_flags = if let Some(hex) = trimmed + .strip_prefix("0x") + .or_else(|| trimmed.strip_prefix("0X")) + { + u32::from_str_radix(hex, 16) + } else { + trimmed + .parse::() + .or_else(|_| u32::from_str_radix(trimmed, 16)) + } + .map_err(|e| { + FpgadError::Argument(format!( + "Invalid flags value '{value_str}': expected decimal or hexadecimal u32 ({e})" + )) + })?; + + set_flags(&fpga, parsed_flags).map_err(Into::into) + } + Ok(WriteSubCommand::WriteProp) => { + validate_property_path(Path::new(property_path))?; + write_property(property_path, value_str) + } + Ok(WriteSubCommand::WriteByte) => { + validate_property_path(Path::new(property_path))?; + // todo: consider using "hex" which is available from cargo as v0.4.0 + let hex_data = hex_from_string(value_str)?; + write_property_bytes(property_path, &hex_data) + } + Err(()) => { + Err(FpgadError::Argument(format!("Unknown write subcommand '{sub_cmd_str}'")).into()) + } + } +} + +/// Dispatches read subcommands for the universal DBus status API. +/// +/// Supported `sub_cmd_str` values: +/// - `read_property` to read a validated sysfs property path +/// - `read_flags` to read FPGA flags for a validated device handle +pub fn universal_read_handler( + sub_cmd_str: &str, + property_path: &str, +) -> Result { + match sub_cmd_str.parse::() { + Ok(ReadSubCommand::ReadFlags) => { + validate_device_handle(property_path)?; + let fpga = UniversalFPGA::new(property_path); + Ok(flags(&fpga)?.to_string()) + } + Ok(ReadSubCommand::ReadProp) => { + validate_property_path(Path::new(property_path))?; + read_property(property_path) + } + Err(()) => { + Err(FpgadError::Argument(format!("Unknown read subcommand '{sub_cmd_str}'")).into()) + } + } +} + +#[cfg(test)] +mod test_validate_property_path { + use crate::platforms::universal::validate_property_path_with_base; + use googletest::prelude::*; + use std::fs; + use std::path::PathBuf; + use std::time::{SystemTime, UNIX_EPOCH}; + + fn unique_test_dir(test_name: &str) -> PathBuf { + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock before unix epoch") + .as_nanos(); + std::env::temp_dir().join(format!("fpgad_validate_property_path_{test_name}_{nanos}")) + } + + #[gtest] + fn should_pass_valid_path() { + let root = unique_test_dir("valid_path"); + let base = root.join("fpga_manager"); + let property = base.join("fpga0").join("name"); + + fs::create_dir_all(property.parent().expect("property should have parent")) + .expect("create parent dirs"); + fs::write(&property, "name\n").expect("create property file"); + + let expected = fs::canonicalize(&property).expect("canonicalize property"); + let result = validate_property_path_with_base(&property, &base); + + fs::remove_dir_all(root).expect("cleanup temp dirs"); + assert_that!(&result, ok(eq(&expected))); + } + + #[gtest] + fn should_fail_for_path_outside_fpga_dir() { + let root = unique_test_dir("outside_base"); + let base = root.join("fpga_manager"); + let outside = root.join("outside").join("evil_file.sh"); + + fs::create_dir_all(&base).expect("create base dir"); + fs::create_dir_all(outside.parent().expect("outside should have parent")) + .expect("create outside dir"); + fs::write(&outside, "evil\n").expect("create outside file"); + + let result = validate_property_path_with_base(&outside, &base); + + fs::remove_dir_all(root).expect("cleanup temp dirs"); + assert_that!(&result, err(displays_as(contains_substring("is outside")))); + } + + #[gtest] + fn should_fail_for_root_path_traversal() { + let root = unique_test_dir("root_traversal"); + let base = root.join("fpga_manager"); + fs::create_dir_all(&base).expect("create base dir"); + + let traversal = base.join("..").join("outside").join("evil_file.sh"); + let result = validate_property_path_with_base(&traversal, &base); + + fs::remove_dir_all(root).expect("cleanup temp dirs"); + assert_that!( + &result, + err(displays_as(contains_substring("path traversal"))) + ); + } + + #[gtest] + fn should_fail_for_device_path_traversal() { + let root = unique_test_dir("device_traversal"); + let base = root.join("fpga_manager"); + fs::create_dir_all(base.join("fpga0")).expect("create fpga0 dir"); + + let traversal = base.join("fpga0").join("..").join("name"); + let result = validate_property_path_with_base(&traversal, &base); + + fs::remove_dir_all(root).expect("cleanup temp dirs"); + assert_that!( + &result, + err(displays_as(contains_substring("path traversal"))) + ); + } + + #[cfg(unix)] + #[gtest] + fn should_allow_symlink_path_without_resolution() { + use std::os::unix::fs::symlink; + use std::path::absolute; + + let root = unique_test_dir("symlink_escape"); + let base = root.join("fpga_manager"); + let outside = root.join("outside"); + let link_target_file = outside.join("escaped_name"); + let fpga0_dir = base.join("fpga0"); + let link_in_base = fpga0_dir.join("link_outside"); + + fs::create_dir_all(&fpga0_dir).expect("create fpga0 dir"); + fs::create_dir_all(&outside).expect("create outside dir"); + fs::write(&link_target_file, "evil\n").expect("create outside target file"); + symlink(&outside, &link_in_base).expect("create symlink escaping base"); + + let escaped_path = link_in_base.join("escaped_name"); + let expected = absolute(&escaped_path).expect("resolve absolute escaped path"); + let result = validate_property_path_with_base(&escaped_path, &base); + + fs::remove_dir_all(root).expect("cleanup temp dirs"); + assert_that!(&result, ok(eq(&expected))); + } +} + +#[cfg(test)] +mod test_hex_from_string { + use super::*; + use googletest::prelude::*; + + #[test] + fn parses_plain_hex_bytes() { + let result = hex_from_string("00 04 02 20 20"); + let expected: Vec = vec![0, 4, 2, 32, 32]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn parses_continuous_single_byte() { + let result = hex_from_string("AA"); + let expected: Vec = vec![0xAA]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn parses_lowercase_hex() { + let result = hex_from_string("aa bb cc"); + let expected: Vec = vec![0xAA, 0xBB, 0xCC]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn parses_with_0x_prefix() { + let result = hex_from_string("0x00 0x04 0x02 0x20 0x20"); + let expected: Vec = vec![0, 4, 2, 32, 32]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn parses_mixed_prefix_and_plain_tokens() { + let result = hex_from_string("0x00 04 0x02 20"); + let expected: Vec = vec![0, 4, 2, 32]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn ignores_extra_whitespace() { + let result = hex_from_string(" 00 04 02 20 "); + let expected: Vec = vec![0, 4, 2, 32]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn rejects_invalid_hex_characters() { + let result = hex_from_string("00 GG 02"); + assert_that!( + result, + err(pat!(FpgadError::Argument(contains_substring( + "Invalid hex" + )))) + ); + } + + #[test] + fn parses_odd_length_as_stream() { + let result = hex_from_string("1FF"); + let expected: Vec = vec![0x01, 0xFF]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn parses_no_spaces() { + let result = hex_from_string("DEADBEEF"); + let expected: Vec = vec![0xDE, 0xAD, 0xBE, 0xEF]; + assert_that!(result, ok(eq(&expected))); + } + + #[test] + fn empty_input_returns_empty_vec() { + let result = hex_from_string(""); + let expected: Vec = vec![]; + assert_that!(result, ok(eq(&expected))); + } +} diff --git a/daemon/src/platforms/universal_components/universal_fpga.rs b/daemon/src/platforms/universal_components/universal_fpga.rs index 39308e0b..f074f112 100644 --- a/daemon/src/platforms/universal_components/universal_fpga.rs +++ b/daemon/src/platforms/universal_components/universal_fpga.rs @@ -48,8 +48,9 @@ //! - `flags` - Programming flags (hexadecimal format: "0x...") //! - `firmware` - Trigger bitstream loading by writing filename //! -//! with any other files being controllable using the `write_property_bytes` and -//! `write_property` DBus methods. +//! with any other files being controllable using the `universal` DBus control method +//! (`write_property` / `write_property_bytes` subcommands), and readable using +//! the `universal` DBus status method (`read_property` subcommand). //! See the [`control_interface`](crate::comm::dbus::control_interface) documentation for more details. //! //! # Examples @@ -73,7 +74,7 @@ use crate::platforms::platform::Fpga; use crate::platforms::universal_components::universal_helpers; use crate::system_io::{fs_read, fs_write}; use crate::{config, system_io}; -use log::{error, info, trace, warn}; +use log::{info, trace}; use std::path::Path; /// Universal FPGA device implementation using standard Linux FPGA subsystem. @@ -180,85 +181,6 @@ impl Fpga for UniversalFPGA { fs_read(&state_path).map(|s| s.trim_end_matches('\n').to_string()) } - /// Read the current programming flags from sysfs. - /// - /// Reads `/sys/class/fpga_manager//flags`, parses the hexadecimal string - /// (format: "0x...", or undecorated), and returns the flags as u32. - /// - /// # Returns: `Result` - /// * `Ok(u32)` - Current flags value - /// * `Err(FpgadError::IORead)` - Failed to read flags file - /// * `Err(FpgadError::Flag)` - Failed to parse hexadecimal value - fn flags(&self) -> Result { - let flag_path = Path::new(config::FPGA_MANAGERS_DIR) - .join(self.device_handle.clone()) - .join("flags"); - let contents = fs_read(&flag_path)?; - let trimmed = contents.trim().trim_start_matches("0x"); - u32::from_str_radix(trimmed, 16) - .map_err(|_| FpgadError::Flag("Parsing flags failed".into())) - } - - /// Set the programming flags in sysfs. - /// - /// Writes the flags to `/sys/class/fpga_manager//flags` in undecorated - /// hexadecimal (decimal `32` -> undecorated hex `20`) and verifies that the write - /// succeeded by reading the value back. - /// Also checks and logs the FPGA state after setting flags. - /// - /// # Arguments - /// - /// * `flags` - The flags value to set - /// - /// # Returns: `Result<(), FpgadError>` - /// * `Ok(())` - Flags set and verified successfully - /// * `Err(FpgadError::IOWrite)` - Failed to write flags file - /// * `Err(FpgadError::IORead)` - Failed to read back flags or state - /// * `Err(FpgadError::Flag)` - Read-back value doesn't match written value - fn set_flags(&self, flags: u32) -> Result { - let flag_path = Path::new(config::FPGA_MANAGERS_DIR) - .join(self.device_handle.clone()) - .join("flags"); - trace!("Writing '0x{flags:X}' to '{flag_path:?}'"); - if let Err(e) = fs_write(&flag_path, false, format!("0x{flags:X}")) { - error!("Failed to read state."); - return Err(e); - } - - match self.state() { - Ok(state) => match state.as_str() { - "operating" => { - info!( - "{}'s state is 'operating' after writing flags.", - self.device_handle - ) - } - _ => { - warn!( - "{}'s state is '{}' after writing flags.", - self.device_handle, state - ); - } - }, - Err(e) => return Err(e), - }; - - match self.flags() { - Ok(returned_flags) if returned_flags == flags => Ok(format!( - "Flags set to '0x{:X}' for '{}'", - flags, self.device_handle - )), - Ok(returned_flags) => Err(FpgadError::Flag(format!( - "Setting flags of '{}' to '{}' failed. Resulting flag was '{}'", - self.device_handle, flags, returned_flags - ))), - Err(e) => Err(FpgadError::Flag(format!( - "Failed to read device '{}' flags after setting to '{}': {}", - self.device_handle, flags, e - ))), - } - } - /// Load a bitstream firmware file directly to the FPGA. /// /// Writes the firmware filename (relative to the kernel firmware search path) to diff --git a/daemon/src/softeners/xilinx_dfx_mgr/xilinx_dfx_mgr_fpga.rs b/daemon/src/softeners/xilinx_dfx_mgr/xilinx_dfx_mgr_fpga.rs index 4ae90e85..b16300b6 100644 --- a/daemon/src/softeners/xilinx_dfx_mgr/xilinx_dfx_mgr_fpga.rs +++ b/daemon/src/softeners/xilinx_dfx_mgr/xilinx_dfx_mgr_fpga.rs @@ -37,12 +37,9 @@ //! # } //! ``` -use crate::config; use crate::error::FpgadError; use crate::platforms::platform::Fpga; use crate::softeners::xilinx_dfx_mgr; -use crate::system_io::{fs_read, fs_write}; -use log::{error, trace}; use std::path::Path; /// Xilinx DFX Manager FPGA device implementation. @@ -116,68 +113,6 @@ impl Fpga for XilinxDfxMgrFPGA { Ok(xilinx_dfx_mgr::list_package()?) } - /// Read the current programming flags from sysfs. - /// - /// Reads `/sys/class/fpga_manager//flags`, parses the hexadecimal string - /// (format: "0x...", or undecorated), and returns the flags as u32. - /// - /// # Returns: `Result` - /// * `Ok(u32)` - Current flags value - /// * `Err(FpgadError::IORead)` - Failed to read flags file - /// * `Err(FpgadError::Flag)` - Failed to parse hexadecimal value - fn flags(&self) -> Result { - let flag_path = Path::new(config::FPGA_MANAGERS_DIR) - .join(self.device_handle.clone()) - .join("flags"); - let contents = fs_read(&flag_path)?; - let trimmed = contents.trim().trim_start_matches("0x"); - u32::from_str_radix(trimmed, 16) - .map_err(|_| FpgadError::Flag("Parsing flags failed".into())) - } - - /// Set the programming flags in sysfs. - /// - /// Writes the flags to `/sys/class/fpga_manager//flags` in undecorated - /// hexadecimal (decimal `32` -> undecorated hex `20`) and verifies that the write - /// succeeded by reading the value back. - /// Also checks and logs the FPGA state after setting flags. - /// - /// # Arguments - /// - /// * `flags` - The flags value to set - /// - /// # Returns: `Result<(), FpgadError>` - /// * `Ok(())` - Flags set and verified successfully - /// * `Err(FpgadError::IOWrite)` - Failed to write flags file - /// * `Err(FpgadError::IORead)` - Failed to read back flags or state - /// * `Err(FpgadError::Flag)` - Read-back value doesn't match written value - fn set_flags(&self, flags: u32) -> Result { - let flag_path = Path::new(config::FPGA_MANAGERS_DIR) - .join(self.device_handle.clone()) - .join("flags"); - trace!("Writing '0x{flags:X}' to '{flag_path:#?}'"); - if let Err(e) = fs_write(&flag_path, false, format!("0x{flags:X}")) { - error!("Failed to read state."); - return Err(e); - } - // TODO(Artie): https://github.com/canonical/fpgad/issues/187 - - match self.flags() { - Ok(returned_flags) if returned_flags == flags => Ok(format!( - "Flags set to '0x{:X}' for '{}'", - flags, self.device_handle - )), - Ok(returned_flags) => Err(FpgadError::Flag(format!( - "Setting '{}'s flags to '{}' failed. Resulting flag was '{}'", - self.device_handle, flags, returned_flags - ))), - Err(e) => Err(FpgadError::Flag(format!( - "Failed to read '{}'s flags after setting to '{}': {}", - self.device_handle, flags, e - ))), - } - } - fn load_firmware(&self, firmware_path: &Path, _: &Path) -> Result { Ok(xilinx_dfx_mgr::load_bitstream(firmware_path)?) } From 2dd597070e163baabec955cc317416d1d7becd23 Mon Sep 17 00:00:00 2001 From: Artie Poole Date: Mon, 25 May 2026 11:08:02 +0100 Subject: [PATCH 2/4] move back to universal-subcommand - add docs todo Signed-off-by: Artie Poole --- cli/src/lib.rs | 3 +++ daemon/src/comm/dbus/control_interface.rs | 1 + daemon/src/comm/dbus/status_interface.rs | 2 ++ daemon/src/platforms/universal.rs | 3 +++ 4 files changed, 9 insertions(+) diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 89ca6363..548355df 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -172,6 +172,9 @@ //! ./target/debug/cli --device=fpga0 status //! ``` +// TODO(Artie): add universal and dfx-mgr subcommand documentation help strings +// dfx-mgr can link to their repo +// universal should properly contain all valid types mod proxies; pub mod load; diff --git a/daemon/src/comm/dbus/control_interface.rs b/daemon/src/comm/dbus/control_interface.rs index eed14d55..7fcfba56 100644 --- a/daemon/src/comm/dbus/control_interface.rs +++ b/daemon/src/comm/dbus/control_interface.rs @@ -293,6 +293,7 @@ impl ControlInterface { Ok(fpga.remove_firmware(handle)?) } + // TODO: make sure this documentation links to the possible options properly /// Entrypoint for universal platform specific operations. /// /// # Arguments diff --git a/daemon/src/comm/dbus/status_interface.rs b/daemon/src/comm/dbus/status_interface.rs index 85a6e8ba..e3d5016d 100644 --- a/daemon/src/comm/dbus/status_interface.rs +++ b/daemon/src/comm/dbus/status_interface.rs @@ -201,6 +201,8 @@ impl StatusInterface { } Ok(ret_string) } + + // TODO: make sure this documentation links to the possible options properly /// Unified read entrypoint for universal platform operations. /// /// # Arguments diff --git a/daemon/src/platforms/universal.rs b/daemon/src/platforms/universal.rs index 96e23a3f..5ca57b71 100644 --- a/daemon/src/platforms/universal.rs +++ b/daemon/src/platforms/universal.rs @@ -309,8 +309,10 @@ pub enum ReadSubCommand { ReadProp, ReadFlags, } +// TODO: make sure this is documented upstream impl ReadSubCommand { + // TODO: make sure this is documented upstream pub fn as_str(self) -> &'static str { match self { ReadSubCommand::ReadFlags => "read_flags", @@ -340,6 +342,7 @@ pub enum WriteSubCommand { } impl WriteSubCommand { + // TODO: make sure this is documented upstream pub fn as_str(self) -> &'static str { match self { WriteSubCommand::WriteFlags => "write_flags", From 4cf50445b378722bdbe1108e10dc5918a978165e Mon Sep 17 00:00:00 2001 From: Artie Poole Date: Tue, 19 May 2026 12:44:14 +0100 Subject: [PATCH 3/4] cli: add universal and dfx-mgr top level subcommands Signed-off-by: Artie Poole --- README.md | 2 +- cli/Cargo.toml | 2 +- cli/README.md | 131 +++++++++++++++++++++++-------- cli/src/dfx_mgr.rs | 65 +++++++++++++++ cli/src/lib.rs | 59 ++++++++++++++ cli/src/main.rs | 8 +- cli/src/proxies/control_proxy.rs | 78 ++++++++---------- cli/src/proxies/status_proxy.rs | 13 +-- cli/src/set.rs | 2 +- cli/src/universal.rs | 100 +++++++++++++++++++++++ 10 files changed, 370 insertions(+), 90 deletions(-) create mode 100644 cli/src/dfx_mgr.rs create mode 100644 cli/src/universal.rs diff --git a/README.md b/README.md index c5d7031b..0c897655 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ cargo build --release [opts] ### run Using default build settings, the resulting binaries will be in `./target/debug/` -such as `./target/debug/fpgad` for the daemon and `./target/debug/cli` for the +such as `./target/debug/fpgad` for the daemon and `./target/debug/fpgad_cli` for the command line application. If using release, then the directory is `./target/release/`. If running as a standalone binary (not snap) then dbus needs to be configured to know about diff --git a/cli/Cargo.toml b/cli/Cargo.toml index c5537d67..4a11d155 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -20,4 +20,4 @@ clap = { version = "4.5.41", default-features = false, features = ["std", "deriv env_logger = { version = "0.11.8", default-features = false } log = "0.4.27" tokio = { version = "1.47.0", default-features = false, features = ["rt-multi-thread", "macros", "sync"] } -zbus = { version = "5.9.0", default-features = false, features = ["tokio"] } \ No newline at end of file +zbus = { version = "5.9.0", default-features = false, features = ["tokio"] } diff --git a/cli/README.md b/cli/README.md index d4431d84..6a899640 100644 --- a/cli/README.md +++ b/cli/README.md @@ -5,58 +5,88 @@ Usage: [snap run] fpgad [OPTIONS] OPTIONs: - -h, --help Print help - --handle fpga device `HANDLE` to be used for the operations. - Default value for this option is calculated in runtime - and the application picks the first available fpga device - in the system (under `/sys/class/fpga_manager/`) + -h, --help Print help + -d, --device FPGA device handle to be used for operations. + Default value is calculated at runtime and picks + the first available FPGA device in the system + (under `/sys/class/fpga_manager/`) + -p, --platform Override the platform detection. Format: + "universal" or "vendor,device" + Examples: --platform=universal + --platform=xlnx,zynqmp-pcap-fpga COMMANDs: ├── load Load a bitstream or overlay -│ ├── overlay [--handle ] -│ │ Load overlay (.dtbo) into the system using the default OVERLAY_HANDLE +│ ├── overlay [--name ] +│ │ Load overlay (.dtbo) into the system using the default OVERLAY_NAME │ │ (either the provided DEVICE_HANDLE or "overlay0") or provide -│ │ --handle: to name the overlay directory +│ │ --name (-n): to name the overlay directory │ └── bitstream │ Load bitstream (e.g. `.bit.bin` file) into the FPGA │ ├── set │ Set an attribute/flag under `/sys/class/fpga_manager//` │ -├── status [--handle ] -│ Show FPGA status (all devices and overlays) or provide -│ --handle: for a specific device status +├── status +│ Show FPGA status (all devices and overlays) │ -└── remove Remove an overlay or bitstream - ├── overlay [--handle ] - │ Removes the first overlay found (call repeatedly to remove all) or provide - │ --handle: to remove overlay previously loaded with given OVERLAY_HANDLE - └── bitstream - Remove active bitstream from FPGA (bitstream removal is vendor specific) +├── remove Remove an overlay or bitstream +│ ├── overlay [--name ] +│ │ Removes the first overlay found (call repeatedly to remove all) or provide +│ │ --name (-n): to remove overlay previously loaded with given OVERLAY_NAME +│ └── bitstream [--handle ] +│ Remove active bitstream from FPGA (bitstream removal is vendor specific) +│ Optionally specify --handle to name the removal handle +│ +├── universal Low-level read/write access to FPGA manager properties +│ ├── read +│ │ Read from sysfs path where SUB_CMD is: +│ │ read_property: read property from sysfs path +│ │ read_flags: read flags from device handle +│ └── write +│ Write to sysfs path where SUB_CMD is: +│ write_flags: write flags to device handle +│ write_property: write string property to sysfs path +│ write_property_bytes: write bytes to sysfs path +│ +└── dfx-mgr Pass commands to dfx-mgr-client (Xilinx DFX manager) + Examples: listPackage, listSlot, load, remove ``` ### Loading ```shell -fpgad [--handle=] load ( (overlay [--handle=]) | (bitstream ) ) +fpgad [--device=] load ( (overlay [--name=]) | (bitstream ) ) ``` ### Removing ```shell -fpgad [--handle=] remove ( ( overlay ) | ( bitstream ) ) +fpgad [--device=] remove ( ( overlay [--name=] ) | ( bitstream [--handle=] ) ) ``` ### Set ```shell -fpgad [--handle=] set ATTRIBUTE VALUE +fpgad [--device=] set ATTRIBUTE VALUE ``` ### Status ```shell -fpgad [--handle=] status +fpgad [--device=] status +``` + +### Universal + +```shell +fpgad [--device=] universal ( (read ) | (write ) ) +``` + +### DFX Manager + +```shell +fpgad dfx-mgr ``` ## examples (for testing) @@ -64,31 +94,68 @@ fpgad [--handle=] status ### Load ```shell -sudo ./target/debug/cli load bitstream /lib/firmware/k26-starter-kits.bit.bin -sudo ./target/debug/cli --handle=fpga0 load bitstream /lib/firmware/k26-starter-kits.bit.bin +sudo ./target/debug/fpgad_cli load bitstream /lib/firmware/k26-starter-kits.bit.bin +sudo ./target/debug/fpgad_cli --device=fpga0 load bitstream /lib/firmware/k26-starter-kits.bit.bin -sudo ./target/debug/cli load overlay /lib/firmware/k26-starter-kits.dtbo -sudo ./target/debug/cli load overlay /lib/firmware/k26-starter-kits.dtbo --handle=overlay_handle -sudo ./target/debug/cli --handle=fpga0 load overlay /lib/firmware/k26-starter-kits.dtbo --handle=overlay_handle +sudo ./target/debug/fpgad_cli load overlay /lib/firmware/k26-starter-kits.dtbo +sudo ./target/debug/fpgad_cli load overlay /lib/firmware/k26-starter-kits.dtbo --name=overlay_handle +sudo ./target/debug/fpgad_cli --device=fpga0 load overlay /lib/firmware/k26-starter-kits.dtbo --name=overlay_handle ``` ### Remove ```shell -sudo ./target/debug/cli --handle=fpga0 remove overlay -sudo ./target/debug/cli --handle=fpga0 remove overlay --handle=overlay_handle +sudo ./target/debug/fpgad_cli --device=fpga0 remove overlay +sudo ./target/debug/fpgad_cli --device=fpga0 remove overlay --name=overlay_handle +sudo ./target/debug/fpgad_cli remove bitstream +sudo ./target/debug/fpgad_cli remove bitstream --handle=my_handle ``` ### Set ```shell -sudo ./target/debug/cli set flags 0 -sudo ./target/debug/cli --handle=fpga0 set flags 0 +sudo ./target/debug/fpgad_cli set flags 0 +sudo ./target/debug/fpgad_cli --device=fpga0 set flags 0 ``` ### Status ```shell -./target/debug/cli status -./target/debug/cli --handle=fpga0 status +./target/debug/fpgad_cli status +./target/debug/fpgad_cli --device=fpga0 status +``` + +### Universal + +```shell +# Read operations +sudo ./target/debug/fpgad_cli universal read read_property /sys/class/fpga_manager/fpga0/name +sudo ./target/debug/fpgad_cli universal read read_flags fpga0 + +# Write operations +sudo ./target/debug/fpgad_cli universal write write_flags fpga0 0 +sudo ./target/debug/fpgad_cli universal write write_property /sys/class/fpga_manager/fpga0/flags 0 +sudo ./target/debug/fpgad_cli universal write write_property_bytes /sys/class/fpga_manager/fpga0/firmware BYTES +``` + +### DFX Manager + +```shell +# List packages and slots +sudo ./target/debug/fpgad_cli dfx-mgr listPackage +sudo ./target/debug/fpgad_cli dfx-mgr listSlot + +# Load and remove with dfx-mgr +sudo ./target/debug/fpgad_cli dfx-mgr "load slot:0 package:my-package" +sudo ./target/debug/fpgad_cli dfx-mgr "remove slot:0" +``` + +### Platform Override + +```shell +# Force universal platform +sudo ./target/debug/fpgad_cli --platform=universal load bitstream /lib/firmware/bitstream.bin + +# Force specific Xilinx platform +sudo ./target/debug/fpgad_cli --platform=xlnx,zynqmp-pcap-fpga load bitstream /lib/firmware/bitstream.bin ``` diff --git a/cli/src/dfx_mgr.rs b/cli/src/dfx_mgr.rs new file mode 100644 index 00000000..a34f4b9f --- /dev/null +++ b/cli/src/dfx_mgr.rs @@ -0,0 +1,65 @@ +// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. +// +// Copyright 2025 Canonical Ltd. +// +// SPDX-License-Identifier: GPL-3.0-only +// +// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. +// +// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. + +//! DFX Manager command implementation for the FPGA CLI. +//! +//! This module passes commands directly to `dfx-mgr-client` via the fpgad daemon's +//! DBus interface. +//! +//! If the connected daemon does not expose `dfx_mgr` (for example because it was built +//! without `xilinx-dfx-mgr`), this handler returns a clear compatibility error. +//! +//! +//! # Usage +//! +//! ```shell +//! fpgad dfx-mgr "-listPackage" +//! fpgad dfx-mgr "-load 0 my_design" +//! fpgad dfx-mgr "-remove 0" +//! ``` +//! +//! The entire command string is passed as a single argument and split by the daemon +//! before forwarding to `dfx-mgr-client`. + +use crate::proxies::control_proxy; +use zbus::Connection; + +/// Main handler for the dfx-mgr command. +/// +/// Forwards the provided command string to the daemon's `dfx_mgr` DBus method, +/// which in turn passes it to `dfx-mgr-client`. +/// +/// # Arguments +/// +/// * `cmd` - Space-separated arguments to pass to `dfx-mgr-client` +/// (e.g. `"-listPackage"` or `"-load 0 my_design"`) +/// +/// # Returns: `Result` +/// * `Ok(String)` - Exit status, stdout, and stderr from `dfx-mgr-client` +/// * `Err(zbus::Error)` - DBus communication error, missing component, or FpgadError. +/// See [Error Handling](../index.html#error-handling) for details. +pub async fn dfx_mgr_handler(cmd: &str) -> Result { + let connection = Connection::system().await?; + let proxy = control_proxy::ControlProxy::new(&connection).await?; + match proxy.dfx_mgr(cmd).await { + Ok(out) => Ok(out), + Err(zbus::Error::MethodError(name, _, _)) + if name.as_str() == "org.freedesktop.DBus.Error.UnknownMethod" => + { + Err(zbus::Error::Failure( + "feature not enabled in daemon, or CLI/daemon out of date (missing dfx_mgr method)" + .to_string(), + )) + } + Err(e) => Err(e), + } +} diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 548355df..f6f61b65 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -185,6 +185,10 @@ pub mod status; pub mod set; +pub mod universal; + +pub mod dfx_mgr; + use clap::{Parser, Subcommand}; /// Command-line interface structure for FPGA management operations. @@ -327,6 +331,40 @@ pub enum RemoveSubcommand { }, } +/// Subcommands for the universal platform interface. +/// +/// Provides direct access to the daemon's `universal` read/write DBus methods, +/// allowing low-level control of FPGA manager sysfs properties and flags. +/// +/// # Examples +/// +/// ```shell +/// fpgad universal read read_flags fpga0 +/// fpgad universal read read_property /sys/class/fpga_manager/fpga0/name +/// fpgad universal write write_flags fpga0 0x20 +/// fpgad universal write write_property /sys/class/fpga_manager/fpga0/key VALUE +/// fpgad universal write write_property_bytes /sys/class/fpga_manager/fpga0/key BYTES +/// ``` +#[derive(Subcommand, Debug)] +pub enum UniversalSubcommand { + /// Read an FPGA property using the universal interface + Read { + /// Subcommand: `read_property` (sysfs path) or `read_flags` (device handle) + sub_cmd: String, + /// Sysfs property path for `read_property`, or device handle for `read_flags` + path: String, + }, + /// Write an FPGA property using the universal interface + Write { + /// Subcommand: `write_flags` (device handle), `write_property`, or `write_property_bytes` + sub_cmd: String, + /// Device handle for `write_flags`, or sysfs property path for property writes + path: String, + /// Value to write + value: String, + }, +} + /// Top-level commands supported by the CLI. /// /// This enum represents all the primary operations available through the fpgad CLI: @@ -334,6 +372,8 @@ pub enum RemoveSubcommand { /// - **Set**: Configure FPGA attributes and flags (e.g., programming flags) /// - **Status**: Query the current state of FPGA devices and loaded overlays /// - **Remove**: Unload bitstreams or device tree overlays from the FPGA +/// - **Universal**: Low-level read/write access to FPGA manager properties via the universal interface +/// - **DfxMgr**: Pass commands directly to `dfx-mgr-client` (requires dfx-mgr component) /// /// Each command communicates with the fpgad daemon via DBus to perform privileged /// operations on FPGA devices managed through the Linux kernel's FPGA subsystem. @@ -355,6 +395,15 @@ pub enum RemoveSubcommand { /// /// # Remove an overlay by name /// fpgad remove overlay --name=my_overlay +/// +/// # Read FPGA flags via universal interface +/// fpgad universal read read_flags fpga0 +/// +/// # Write flags via universal interface +/// fpgad universal write write_flags fpga0 0x20 +/// +/// # Invoke dfx-mgr-client +/// fpgad dfx-mgr "-listPackage" /// ``` #[derive(Subcommand, Debug)] pub enum Commands { @@ -372,4 +421,14 @@ pub enum Commands { #[command(subcommand)] command: RemoveSubcommand, }, + /// Low-level read/write access to FPGA manager properties (universal platform interface) + Universal { + #[command(subcommand)] + command: UniversalSubcommand, + }, + /// Pass a command directly to dfx-mgr-client (requires dfx-mgr snap component) + DfxMgr { + /// Space-separated arguments to pass to dfx-mgr-client (e.g. "-listPackage") + cmd: String, + }, } diff --git a/cli/src/main.rs b/cli/src/main.rs index 72dceaf9..1f870b80 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -12,8 +12,8 @@ use clap::Parser; use fpgad_cli::{ - Cli, Commands, load::load_handler, remove::remove_handler, set::set_handler, - status::status_handler, + Cli, Commands, dfx_mgr::dfx_mgr_handler, load::load_handler, remove::remove_handler, + set::set_handler, status::status_handler, universal::universal_handler, }; use log::{debug, error}; use std::error::Error; @@ -33,6 +33,8 @@ use std::error::Error; /// - `remove_handler` for removing bitstreams/overlays /// - `set_handler` for setting FPGA attributes /// - `status_handler` for querying device status +/// - `universal_handler` for low-level property read/write via the universal interface +/// - `dfx_mgr_handler` for passing commands to dfx-mgr-client /// 4. Prints success messages or logs errors /// /// # Returns @@ -63,6 +65,8 @@ async fn main() -> Result<(), Box> { set_handler(cli.platform(), cli.device(), attribute, value).await } Commands::Status => status_handler(cli.platform(), cli.device()).await, + Commands::Universal { command } => universal_handler(command).await, + Commands::DfxMgr { cmd } => dfx_mgr_handler(cmd).await, }; match result { Ok(msg) => { diff --git a/cli/src/proxies/control_proxy.rs b/cli/src/proxies/control_proxy.rs index 4f5d33d5..a7169edd 100644 --- a/cli/src/proxies/control_proxy.rs +++ b/cli/src/proxies/control_proxy.rs @@ -17,8 +17,8 @@ //! - Loading FPGA bitstreams //! - Applying device tree overlays //! - Removing overlays -//! - Writing to FPGA manager properties -//! - Setting FPGA flags +//! - Writing to FPGA manager properties via the `universal` method +//! - Invoking dfx-mgr-client via the `dfx_mgr` method //! //! The proxy is generated using the `zbus` crate's `#[proxy]` macro and provides //! type-safe, asynchronous access to the daemon's control interface. @@ -66,25 +66,6 @@ use zbus::{Result, proxy}; default_path = "/com/canonical/fpgad/control" )] pub trait Control { - /// Set FPGA programming flags for a device. - /// - /// # Arguments - /// - /// * `platform_string` - Platform identifier (can be empty for auto-detection) - /// * `device_handle` - [Device handle](../../index.html#device-handles) (e.g., "fpga0") - /// * `flags` - Programming flags as a 32-bit unsigned integer - /// - /// # Returns: `Result` - /// * `Ok(String)` - Success message with confirmation of flags set - /// * `Err(zbus::Error)` - DBus error or FpgadError. - /// See [Error Handling](../../index.html#error-handling) - async fn set_fpga_flags( - &self, - platform_string: &str, - device_handle: &str, - flags: u32, - ) -> Result; - /// Write a bitstream directly to an FPGA device. /// /// # Arguments @@ -140,32 +121,6 @@ pub trait Control { /// See [Error Handling](../../index.html#error-handling) async fn remove_overlay(&self, platform_str: &str, overlay_handle: &str) -> Result; - /// Write a string value to an FPGA manager property. - /// - /// # Arguments - /// - /// * `property_path_str` - Full sysfs path to the property (must be under `/sys/class/fpga_manager/`) - /// * `data` - String data to write - /// - /// # Returns: `Result` - /// * `Ok(String)` - Success message confirming write - /// * `Err(zbus::Error)` - DBus error or FpgadError. - /// See [Error Handling](../../index.html#error-handling) - async fn write_property(&self, property_path_str: &str, data: &str) -> Result; - - /// Write binary data to an FPGA manager property. - /// - /// # Arguments - /// - /// * `property_path_str` - Full sysfs path to the property (must be under `/sys/class/fpga_manager/`) - /// * `data` - Binary data to write as a byte slice - /// - /// # Returns: `Result` - /// * `Ok(String)` - Success message confirming write - /// * `Err(zbus::Error)` - DBus error or FpgadError. - /// See [Error Handling](../../index.html#error-handling) - async fn write_property_bytes(&self, property_path_str: &str, data: &[u8]) -> Result; - /// Remove a previously loaded bitstream, identifiable by its `bitstream_handle` or `slot`. /// /// # Arguments @@ -188,4 +143,33 @@ pub trait Control { device_handle: &str, bitstream_handle: &str, ) -> Result; + + /// Write to an FPGA property using the universal write interface. + /// + /// # Arguments + /// + /// * `sub_cmd` - One of `write_flags`, `write_property`, `write_property_bytes` + /// * `path_str` - Device handle for `write_flags`, or sysfs property path for property writes + /// * `value_str` - Value to write (flags value, string payload, or raw byte string) + /// + /// # Returns: `Result` + /// * `Ok(String)` - Success message from the daemon + /// * `Err(zbus::Error)` - DBus error or FpgadError. + /// See [Error Handling](../../index.html#error-handling) + async fn universal(&self, sub_cmd: &str, path_str: &str, value_str: &str) -> Result; + + /// Pass a command string directly to the `dfx-mgr-client` binary. + /// + /// Requires the `dfx-mgr` snap component to be installed. + /// + /// # Arguments + /// + /// * `cmd_string` - Space-separated arguments to pass to `dfx-mgr-client` + /// (e.g. `"-listPackage"` or `"-load 0 my_design"`) + /// + /// # Returns: `Result` + /// * `Ok(String)` - Exit status, stdout, and stderr from `dfx-mgr-client` + /// * `Err(zbus::Error)` - DBus error, missing component, daemon method mismatch, or FpgadError. + /// See [Error Handling](../../index.html#error-handling) + async fn dfx_mgr(&self, cmd_string: &str) -> Result; } diff --git a/cli/src/proxies/status_proxy.rs b/cli/src/proxies/status_proxy.rs index 61e791e7..2022802d 100644 --- a/cli/src/proxies/status_proxy.rs +++ b/cli/src/proxies/status_proxy.rs @@ -106,19 +106,20 @@ pub trait Status { /// * `Err(zbus::Error)` - DBus error or FpgadError. See [Error Handling](../../index.html#error-handling) async fn get_fpga_state(&self, platform_string: &str, device_handle: &str) -> Result; - /// Get the current programming flags for an FPGA device. + /// Get the current programming flags for an FPGA device via the universal interface. /// - /// Returns the flags as a hexadecimal string with no prefix (decimal value of 32 -> "20"). + /// Use `universal("read_flags", device_handle)` to read flags, or + /// `universal("read_property", property_path)` to read an arbitrary sysfs property. /// /// # Arguments /// - /// * `platform_string` - Platform identifier (can be empty for auto-detection) - /// * `device_handle` - [Device handle](../../index.html#device-handles) (e.g., "fpga0") + /// * `sub_cmd` - One of `read_property` or `read_flags` + /// * `path_str` - Sysfs property path for `read_property`, or device handle for `read_flags` /// /// # Returns: `Result` - /// * `Ok(String)` - Current flags in hexadecimal format + /// * `Ok(String)` - Property value or flags in hexadecimal format /// * `Err(zbus::Error)` - DBus error or FpgadError. See [Error Handling](../../index.html#error-handling) - async fn get_fpga_flags(&self, platform_string: &str, device_handle: &str) -> Result; + async fn universal(&self, sub_cmd: &str, path_str: &str) -> Result; /// Get the status of a specific device tree overlay. /// diff --git a/cli/src/set.rs b/cli/src/set.rs index 3b2b5533..6fd19464 100644 --- a/cli/src/set.rs +++ b/cli/src/set.rs @@ -137,7 +137,7 @@ fn build_property_path(device_handle: &str, attribute: &str) -> Result Result { let connection = Connection::system().await?; let proxy = control_proxy::ControlProxy::new(&connection).await?; - proxy.write_property(property, value).await + proxy.universal("write_property", property, value).await } /// Main handler for the set command. diff --git a/cli/src/universal.rs b/cli/src/universal.rs new file mode 100644 index 00000000..dbe3f006 --- /dev/null +++ b/cli/src/universal.rs @@ -0,0 +1,100 @@ +// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. +// +// Copyright 2025 Canonical Ltd. +// +// SPDX-License-Identifier: GPL-3.0-only +// +// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. +// +// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. + +//! Universal command implementation for the FPGA CLI. +//! +//! This module provides direct access to the daemon's `universal` read/write interface, +//! allowing low-level control of FPGA manager sysfs properties and flags without needing +//! a platform-specific subcommand. +//! +//! # Subcommands +//! +//! ## Read +//! +//! ```shell +//! fpgad universal read read_property /sys/class/fpga_manager/fpga0/name +//! fpgad universal read read_flags fpga0 +//! ``` +//! +//! ## Write +//! +//! ```shell +//! fpgad universal write write_flags fpga0 0x20 +//! fpgad universal write write_property /sys/class/fpga_manager/fpga0/key VALUE +//! fpgad universal write write_property_bytes /sys/class/fpga_manager/fpga0/key BYTES +//! ``` + +use crate::UniversalSubcommand; +use crate::proxies::{control_proxy, status_proxy}; +use zbus::Connection; + +/// Sends the `universal` read command to the daemon's status interface. +/// +/// # Arguments +/// +/// * `sub_cmd` - One of `read_property` or `read_flags` +/// * `path` - Sysfs property path for `read_property`, or device handle for `read_flags` +/// +/// # Returns: `Result` +/// * `Ok(String)` - Result from the daemon +/// * `Err(zbus::Error)` - DBus communication error or FpgadError. +/// See [Error Handling](../index.html#error-handling) for details. +async fn call_universal_read(sub_cmd: &str, path: &str) -> Result { + let connection = Connection::system().await?; + let proxy = status_proxy::StatusProxy::new(&connection).await?; + proxy.universal(sub_cmd, path).await +} + +/// Sends the `universal` write command to the daemon's control interface. +/// +/// # Arguments +/// +/// * `sub_cmd` - One of `write_flags`, `write_property`, `write_property_bytes` +/// * `path` - Device handle for `write_flags`, or sysfs property path for property writes +/// * `value` - Value to write +/// +/// # Returns: `Result` +/// * `Ok(String)` - Success message from the daemon +/// * `Err(zbus::Error)` - DBus communication error or FpgadError. +/// See [Error Handling](../index.html#error-handling) for details. +async fn call_universal_write( + sub_cmd: &str, + path: &str, + value: &str, +) -> Result { + let connection = Connection::system().await?; + let proxy = control_proxy::ControlProxy::new(&connection).await?; + proxy.universal(sub_cmd, path, value).await +} + +/// Main handler for the universal command. +/// +/// Dispatches to the appropriate read or write call based on the subcommand. +/// +/// # Arguments +/// +/// * `sub_command` - The universal subcommand (read or write) with its arguments +/// +/// # Returns: `Result` +/// * `Ok(String)` - Result from the daemon +/// * `Err(zbus::Error)` - DBus communication error or FpgadError. +/// See [Error Handling](../index.html#error-handling) for details. +pub async fn universal_handler(sub_command: &UniversalSubcommand) -> Result { + match sub_command { + UniversalSubcommand::Read { sub_cmd, path } => call_universal_read(sub_cmd, path).await, + UniversalSubcommand::Write { + sub_cmd, + path, + value, + } => call_universal_write(sub_cmd, path, value).await, + } +} From f930890c515e600f56826994c874ee10b24bdd75 Mon Sep 17 00:00:00 2001 From: Artie Poole Date: Tue, 19 May 2026 15:52:57 +0100 Subject: [PATCH 4/4] tests: fix daemon and snap tests after universal/dfx-mgr subcommands Updates tests to work with the new universal and dfx-mgr top-level CLI subcommands introduced in the last 3 commits. Changes: - daemon/tests: Update DBus proxy interfaces to use unified 'universal()' method instead of separate set_fpga_flags, write_property, read_property, etc. methods - daemon/tests: Remove duplicate xilinx_dfx_mgr tests that are now covered by universal tests (set_fpga_flags, write_property, read_property, get_fpga_flags) - tests/snap_testing: Add test_universal_command.py with tests for the new 'universal' command (read/write operations) - tests/snap_testing: Add test_dfx_mgr_command.py with tests for the new 'dfx-mgr' command - they require running like `fpgad dfx-mgr -- ` - tests/snap_testing: Add help tests for universal and dfx-mgr commands - cli/README.md: Update universal command documentation to reflect actual subcommand names (read_property, write_flags, etc.) Signed-off-by: Artie Poole --- daemon/tests/common/proxies/control_proxy.rs | 10 +-- daemon/tests/common/proxies/status_proxy.rs | 3 +- daemon/tests/universal/control/set_bytes.rs | 13 +++- .../tests/universal/control/set_fpga_flags.rs | 46 ++++++------- .../control/write_bitstream_direct.rs | 4 +- .../tests/universal/control/write_property.rs | 2 +- .../sequences/bitstream_management.rs | 16 ++--- .../universal/sequences/overlay_management.rs | 24 +++---- .../tests/universal/status/get_fpga_flags.rs | 4 +- .../tests/universal/status/read_property.rs | 2 +- daemon/tests/xilinx_dfx_mgr/control.rs | 2 - .../xilinx_dfx_mgr/control/set_fpga_flags.rs | 65 ------------------- .../control/write_bitstream_direct.rs | 4 +- .../xilinx_dfx_mgr/control/write_property.rs | 58 ----------------- .../sequences/bitstream_management.rs | 4 +- .../sequences/overlay_management.rs | 4 +- daemon/tests/xilinx_dfx_mgr/status.rs | 2 - .../xilinx_dfx_mgr/status/get_fpga_flags.rs | 38 ----------- .../xilinx_dfx_mgr/status/read_property.rs | 53 --------------- .../test_default/test_dfx_mgr_command.py | 37 +++++++++++ tests/snap_testing/test_default/test_help.py | 20 ++++++ .../test_default/test_universal_command.py | 64 ++++++++++++++++++ 22 files changed, 185 insertions(+), 290 deletions(-) delete mode 100644 daemon/tests/xilinx_dfx_mgr/control/set_fpga_flags.rs delete mode 100644 daemon/tests/xilinx_dfx_mgr/control/write_property.rs delete mode 100644 daemon/tests/xilinx_dfx_mgr/status/get_fpga_flags.rs delete mode 100644 daemon/tests/xilinx_dfx_mgr/status/read_property.rs create mode 100644 tests/snap_testing/test_default/test_dfx_mgr_command.py create mode 100644 tests/snap_testing/test_default/test_universal_command.py diff --git a/daemon/tests/common/proxies/control_proxy.rs b/daemon/tests/common/proxies/control_proxy.rs index 8b08e749..412ab615 100644 --- a/daemon/tests/common/proxies/control_proxy.rs +++ b/daemon/tests/common/proxies/control_proxy.rs @@ -17,13 +17,6 @@ use zbus::{Result, proxy}; default_path = "/com/canonical/fpgad/control" )] pub trait Control { - async fn set_fpga_flags( - &self, - platform_string: &str, - device_handle: &str, - flags: u32, - ) -> Result; - async fn write_bitstream_direct( &self, platform_string: &str, @@ -47,7 +40,6 @@ pub trait Control { device_handle: &str, bitstream_handle: &str, ) -> Result; - async fn write_property(&self, property_path_str: &str, data: &str) -> Result; - async fn write_property_bytes(&self, property_path_str: &str, data: &[u8]) -> Result; + async fn universal(&self, sub_cmd: &str, path_str: &str, value_str: &str) -> Result; async fn dfx_mgr(&self, cmd_string: &str) -> Result; } diff --git a/daemon/tests/common/proxies/status_proxy.rs b/daemon/tests/common/proxies/status_proxy.rs index 06392cd5..ca74f0cb 100644 --- a/daemon/tests/common/proxies/status_proxy.rs +++ b/daemon/tests/common/proxies/status_proxy.rs @@ -19,7 +19,6 @@ use zbus::{Result, proxy}; pub trait Status { async fn get_status_message(&self, platform_string: &str) -> Result; async fn get_fpga_state(&self, platform_string: &str, device_handle: &str) -> Result; - async fn get_fpga_flags(&self, platform_string: &str, device_handle: &str) -> Result; async fn get_overlay_status( &self, platform_string: &str, @@ -28,5 +27,5 @@ pub trait Status { async fn get_overlays(&self) -> Result; async fn get_platform_type(&self, device_handle: &str) -> Result; async fn get_platform_types(&self) -> Result; - async fn read_property(&self, property_path_str: &str) -> Result; + async fn universal(&self, sub_cmd: &str, path_str: &str) -> Result; } diff --git a/daemon/tests/universal/control/set_bytes.rs b/daemon/tests/universal/control/set_bytes.rs index 89479bc5..fbb5082f 100644 --- a/daemon/tests/universal/control/set_bytes.rs +++ b/daemon/tests/universal/control/set_bytes.rs @@ -55,7 +55,7 @@ fn trim_trailing_vals(mut data: Vec, val: u8) -> Vec { ok(contains_substring("")) )] async fn set_key_cases Matcher<&'a Result>>( - #[case] platform_string: &str, + #[case] path_str: &str, #[case] data: Vec, #[case] condition: M, _setup: (), @@ -66,12 +66,19 @@ async fn set_key_cases Matcher<&'a Result>>( let proxy = ControlProxy::new(&connection) .await .expect("failed to create control proxy"); - let res = proxy.write_property_bytes(platform_string, &data).await; + // Convert bytes to hex string for universal method + let hex_str = data + .iter() + .map(|b| format!("{:02x}", b)) + .collect::(); + let res = proxy + .universal("write_property_bytes", path_str, &hex_str) + .await; expect_that!(&res, condition); if res.is_ok() { println!("{res:?}"); - let file_data = fs::read(platform_string).expect("failed to read back file"); + let file_data = fs::read(path_str).expect("failed to read back file"); // trim newlines from file read and trailing nulls from input because of // how of the xilinx kernel fpga_mgr.c driver formats key data assert_eq!( diff --git a/daemon/tests/universal/control/set_fpga_flags.rs b/daemon/tests/universal/control/set_fpga_flags.rs index ae3d2752..b39001da 100644 --- a/daemon/tests/universal/control/set_fpga_flags.rs +++ b/daemon/tests/universal/control/set_fpga_flags.rs @@ -11,7 +11,7 @@ // You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. use crate::common::proxies::control_proxy::ControlProxy; -use crate::universal::{PLATFORM_STRING, setup}; +use crate::universal::setup; use googletest::prelude::*; use rstest::*; use tokio; @@ -21,44 +21,40 @@ use zbus::Result; #[gtest] #[tokio::test] #[rstest] -#[case::no_device_handle( - PLATFORM_STRING, - "", - 0, - err(displays_as(contains_substring("FpgadError::Argument:"))) -)] +#[case::no_device_handle("", 0, err(displays_as(contains_substring("FpgadError::Argument:"))))] #[case::bad_device_handle( - PLATFORM_STRING, "dev0", 0, err(displays_as(contains_substring("FpgadError::Argument:"))) )] -#[case::no_platform_str( - "", - "fpga0", +#[case::max_u32_val("fpga0", u32::MAX, ok(contains_substring("Flags set to")))] +#[case::bad_typo_path( + "/sy/class/fpga_manager/", 0, - ok(contains_substring("Flags set to '0x0' for 'fpga0'")) + err(displays_as(contains_substring("FpgadError::Argument:"))) )] -#[case::max_u32_val( - PLATFORM_STRING, - "fpga0", - u32::MAX, - ok(contains_substring("Flags set to")) +#[case::bad_short_path( + "/sys/class/fpga_manager/", + 0, + err(displays_as(contains_substring("FpgadError::Argument:"))) )] -#[case::bad_platform_string( - "xln", - "fpga0", +#[case::bad_handle_in_path( + "/sys/class/fpga_manager/no-dev/flags", 0, err(displays_as(contains_substring("FpgadError::Argument:"))) )] -#[case::all_good( - PLATFORM_STRING, - "fpga0", +#[case::bad_not_full_path_no_flags( + "/sys/class/fpga_manager/fpga0/", + 0, + ok(contains_substring("Flags set to '0x0' for 'fpga0'")) +)] +#[case::all_good_full_path( + "/sys/class/fpga_manager/fpga0/flags", 0, ok(contains_substring("Flags set to '0x0' for 'fpga0'")) )] +#[case::all_good("fpga0", 0, ok(contains_substring("Flags set to '0x0' for 'fpga0'")))] async fn cases Matcher<&'a Result>>( - #[case] platform_string: &str, #[case] device_hande: &str, #[case] flags: u32, #[case] condition: M, @@ -71,7 +67,7 @@ async fn cases Matcher<&'a Result>>( .await .expect("failed to create control proxy"); let res = proxy - .set_fpga_flags(platform_string, device_hande, flags) + .universal("write_flags", device_hande, &flags.to_string()) .await; expect_that!(&res, condition) } diff --git a/daemon/tests/universal/control/write_bitstream_direct.rs b/daemon/tests/universal/control/write_bitstream_direct.rs index 1389954b..36a65cab 100644 --- a/daemon/tests/universal/control/write_bitstream_direct.rs +++ b/daemon/tests/universal/control/write_bitstream_direct.rs @@ -102,7 +102,7 @@ async fn should_pass Matcher<&'a zbus::Result>>( .expect("failed to create control proxy"); proxy - .set_fpga_flags(platform_str, device_hande, 0) + .universal("write_flags", device_hande, "0") .await .expect("Failed to reset flags during test"); @@ -144,7 +144,7 @@ async fn should_timeout( .expect("failed to create control proxy"); proxy - .set_fpga_flags(platform_str, device_hande, 0) + .universal("write_flags", device_hande, "0") .await .expect("Failed to reset flags during test"); diff --git a/daemon/tests/universal/control/write_property.rs b/daemon/tests/universal/control/write_property.rs index 9062dc60..61b5e277 100644 --- a/daemon/tests/universal/control/write_property.rs +++ b/daemon/tests/universal/control/write_property.rs @@ -48,6 +48,6 @@ pub async fn cases Matcher<&'a Result>>( let proxy = ControlProxy::new(&connection) .await .expect("failed to create control proxy"); - let res = proxy.write_property(path, data).await; + let res = proxy.universal("write_property", path, data).await; expect_that!(&res, condition); } diff --git a/daemon/tests/universal/sequences/bitstream_management.rs b/daemon/tests/universal/sequences/bitstream_management.rs index d7882130..15c40486 100644 --- a/daemon/tests/universal/sequences/bitstream_management.rs +++ b/daemon/tests/universal/sequences/bitstream_management.rs @@ -38,19 +38,19 @@ async fn load_from_lib_firmware( .expect("failed to create status proxy"); control_proxy - .set_fpga_flags(platform_handle, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(platform_handle, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0") ); control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); control_proxy @@ -86,12 +86,12 @@ async fn bad_flags( .expect("failed to create status proxy"); control_proxy - .set_fpga_flags(platform_handle, device_handle, 127) + .universal("write_flags", device_handle, "127") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(platform_handle, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("127"), @@ -99,7 +99,7 @@ async fn bad_flags( ); control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); let r = control_proxy @@ -117,12 +117,12 @@ async fn bad_flags( // reset flags control_proxy - .set_fpga_flags(platform_handle, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(platform_handle, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0") diff --git a/daemon/tests/universal/sequences/overlay_management.rs b/daemon/tests/universal/sequences/overlay_management.rs index 7e8ade42..8d7b87e0 100644 --- a/daemon/tests/universal/sequences/overlay_management.rs +++ b/daemon/tests/universal/sequences/overlay_management.rs @@ -53,12 +53,12 @@ async fn expected_good_overlay_process(_setup: ()) { // reset flags control_proxy - .set_fpga_flags(platform_handle, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(platform_handle, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0"), @@ -67,7 +67,7 @@ async fn expected_good_overlay_process(_setup: ()) { // reset encryption key control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); @@ -136,12 +136,12 @@ async fn overlay_already_applied(_setup: ()) { // reset flags control_proxy - .set_fpga_flags(platform_handle, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(platform_handle, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0"), @@ -150,7 +150,7 @@ async fn overlay_already_applied(_setup: ()) { // reset encryption key control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); @@ -248,12 +248,12 @@ async fn argument_errors( // reset flags control_proxy - .set_fpga_flags(good_platform_handle, good_device_handle, 0) + .universal("write_flags", good_device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(good_platform_handle, good_device_handle) + .universal("read_flags", good_device_handle) .await .expect("failed to get fpga flags"), eq("0"), @@ -262,7 +262,7 @@ async fn argument_errors( // reset encryption key control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); @@ -336,12 +336,12 @@ async fn io_errors( // reset flags control_proxy - .set_fpga_flags(good_platform_handle, good_device_handle, 0) + .universal("write_flags", good_device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(good_platform_handle, good_device_handle) + .universal("read_flags", good_device_handle) .await .expect("failed to get fpga flags"), eq("0"), @@ -350,7 +350,7 @@ async fn io_errors( // reset encryption key control_proxy - .write_property("/sys/class/fpga_manager/fpga0/key", "") + .universal("write_property", "/sys/class/fpga_manager/fpga0/key", "") .await .expect("failed to reset the encryption key"); diff --git a/daemon/tests/universal/status/get_fpga_flags.rs b/daemon/tests/universal/status/get_fpga_flags.rs index f8827d2d..9083c856 100644 --- a/daemon/tests/universal/status/get_fpga_flags.rs +++ b/daemon/tests/universal/status/get_fpga_flags.rs @@ -33,7 +33,7 @@ use zbus::Connection; )] #[case::all_good("universal", "fpga0", ok(eq("0")))] async fn cases Matcher<&'a zbus::Result>>( - #[case] platform_string: &str, + #[case] _platform_string: &str, #[case] device_handle: &str, #[case] condition: M, _setup: (), @@ -44,6 +44,6 @@ async fn cases Matcher<&'a zbus::Result>>( let proxy = status_proxy::StatusProxy::new(&connection) .await .expect("failed to create control proxy"); - let res = proxy.get_fpga_flags(platform_string, device_handle).await; + let res = proxy.universal("read_flags", device_handle).await; expect_that!(&res, condition); } diff --git a/daemon/tests/universal/status/read_property.rs b/daemon/tests/universal/status/read_property.rs index f05386d8..0fa730d4 100644 --- a/daemon/tests/universal/status/read_property.rs +++ b/daemon/tests/universal/status/read_property.rs @@ -42,6 +42,6 @@ async fn cases Matcher<&'a zbus::Result>>( let proxy = status_proxy::StatusProxy::new(&connection) .await .expect("failed to create control proxy"); - let res = proxy.read_property(property_path).await; + let res = proxy.universal("read_property", property_path).await; expect_that!(&res, condition); } diff --git a/daemon/tests/xilinx_dfx_mgr/control.rs b/daemon/tests/xilinx_dfx_mgr/control.rs index d1bf1222..a4a03718 100644 --- a/daemon/tests/xilinx_dfx_mgr/control.rs +++ b/daemon/tests/xilinx_dfx_mgr/control.rs @@ -12,6 +12,4 @@ mod apply_overlay; pub mod dfx_mgr; -pub mod set_fpga_flags; pub mod write_bitstream_direct; -pub mod write_property; diff --git a/daemon/tests/xilinx_dfx_mgr/control/set_fpga_flags.rs b/daemon/tests/xilinx_dfx_mgr/control/set_fpga_flags.rs deleted file mode 100644 index a74ec4fb..00000000 --- a/daemon/tests/xilinx_dfx_mgr/control/set_fpga_flags.rs +++ /dev/null @@ -1,65 +0,0 @@ -// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. -// -// Copyright 2026 Canonical Ltd. -// -// SPDX-License-Identifier: GPL-3.0-only -// -// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. -// -// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. - -use crate::common::proxies::control_proxy::ControlProxy; -use crate::xilinx_dfx_mgr::{PLATFORM_STRING, setup}; -use googletest::prelude::*; -use rstest::*; -use tokio; -use zbus::Connection; -use zbus::Result; - -#[gtest] -#[tokio::test] -#[rstest] -#[case::bad_device( - "dev0", - 0u32, - err(displays_as(contains_substring("FpgadError::Argument:"))) -)] -#[case::no_device( - "", - 0u32, - err(displays_as(contains_substring("FpgadError::Argument:"))) -)] -#[case::all_good( - "fpga0", - 0u32, - ok(contains_substring("Flags set to '0x0' for 'fpga0'")) -)] -#[case::all_good_0x1( - "fpga0", - 1u32, - ok(contains_substring("Flags set to '0x1' for 'fpga0'")) -)] -#[case::all_good_0x10( - "fpga0", - 16u32, - ok(contains_substring("Flags set to '0x10' for 'fpga0'")) -)] -async fn set_flags_cases Matcher<&'a Result>>( - #[case] device_handle: &str, - #[case] flags: u32, - #[case] condition: M, - _setup: (), -) { - let connection = Connection::system() - .await - .expect("failed to create connection"); - let proxy = ControlProxy::new(&connection) - .await - .expect("failed to create control proxy"); - let res = proxy - .set_fpga_flags(PLATFORM_STRING, device_handle, flags) - .await; - expect_that!(&res, condition); -} diff --git a/daemon/tests/xilinx_dfx_mgr/control/write_bitstream_direct.rs b/daemon/tests/xilinx_dfx_mgr/control/write_bitstream_direct.rs index 577d9f97..8bb024de 100644 --- a/daemon/tests/xilinx_dfx_mgr/control/write_bitstream_direct.rs +++ b/daemon/tests/xilinx_dfx_mgr/control/write_bitstream_direct.rs @@ -53,9 +53,7 @@ async fn load_bitstream_cases Matcher<&'a Result>>( // Reset flags before test if !device_handle.is_empty() { - let _ = proxy - .set_fpga_flags(PLATFORM_STRING, device_handle, 0) - .await; + let _ = proxy.universal("write_flags", device_handle, "0").await; } let res = proxy diff --git a/daemon/tests/xilinx_dfx_mgr/control/write_property.rs b/daemon/tests/xilinx_dfx_mgr/control/write_property.rs deleted file mode 100644 index c6a1ce5d..00000000 --- a/daemon/tests/xilinx_dfx_mgr/control/write_property.rs +++ /dev/null @@ -1,58 +0,0 @@ -// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. -// -// Copyright 2026 Canonical Ltd. -// -// SPDX-License-Identifier: GPL-3.0-only -// -// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. -// -// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. - -use crate::common::proxies::control_proxy::ControlProxy; -use crate::xilinx_dfx_mgr::setup; -use googletest::prelude::*; -use rstest::*; -use std::fs; -use tokio; -use zbus::Connection; -use zbus::Result; -#[gtest] -#[tokio::test] -#[rstest] -#[case::no_path("", "", err(displays_as(contains_substring("FpgadError::Argument:"))))] -#[case::bad_path( - "key", - "", - err(displays_as(contains_substring("FpgadError::Argument:"))) -)] -#[case::path_traversal( - "/sys/class/fpga_manager/../../../usr/bin/evil_file.sh", - "", - err(displays_as(contains_substring("path traversal"))) -)] -async fn write_property_cases Matcher<&'a Result>>( - #[case] platform_string: &str, - #[case] data: &str, - #[case] condition: M, - _setup: (), -) { - let connection = Connection::system() - .await - .expect("failed to create connection"); - let proxy = ControlProxy::new(&connection) - .await - .expect("failed to create control proxy"); - let res = proxy.write_property(platform_string, data).await; - expect_that!(&res, condition); - if res.is_ok() && platform_string.starts_with("/sys/") { - println!("{res:?}"); - let file_data = fs::read_to_string(platform_string).expect("failed to read back file"); - let trimmed_data = file_data.trim(); - assert_eq!( - trimmed_data, data, - "file contents do not match expected data" - ); - } -} diff --git a/daemon/tests/xilinx_dfx_mgr/sequences/bitstream_management.rs b/daemon/tests/xilinx_dfx_mgr/sequences/bitstream_management.rs index b03aa26b..8b5656e9 100644 --- a/daemon/tests/xilinx_dfx_mgr/sequences/bitstream_management.rs +++ b/daemon/tests/xilinx_dfx_mgr/sequences/bitstream_management.rs @@ -38,12 +38,12 @@ async fn load_bitstream_via_dfx_mgr( // Reset flags control_proxy - .set_fpga_flags(PLATFORM_STRING, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(PLATFORM_STRING, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0") diff --git a/daemon/tests/xilinx_dfx_mgr/sequences/overlay_management.rs b/daemon/tests/xilinx_dfx_mgr/sequences/overlay_management.rs index 618896ba..c8608bb5 100644 --- a/daemon/tests/xilinx_dfx_mgr/sequences/overlay_management.rs +++ b/daemon/tests/xilinx_dfx_mgr/sequences/overlay_management.rs @@ -39,12 +39,12 @@ async fn apply_overlay_via_dfx_mgr(_setup: ()) { // Reset flags control_proxy - .set_fpga_flags(PLATFORM_STRING, device_handle, 0) + .universal("write_flags", device_handle, "0") .await .expect("failed to set fpga flags"); expect_that!( status_proxy - .get_fpga_flags(PLATFORM_STRING, device_handle) + .universal("read_flags", device_handle) .await .expect("failed to get fpga flags"), eq("0"), diff --git a/daemon/tests/xilinx_dfx_mgr/status.rs b/daemon/tests/xilinx_dfx_mgr/status.rs index 9842bf0e..9fed6c36 100644 --- a/daemon/tests/xilinx_dfx_mgr/status.rs +++ b/daemon/tests/xilinx_dfx_mgr/status.rs @@ -10,11 +10,9 @@ // // You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. -mod get_fpga_flags; mod get_fpga_state; mod get_overlay_status; mod get_overlays; mod get_platform_type; mod get_platform_types; mod get_status_message; -mod read_property; diff --git a/daemon/tests/xilinx_dfx_mgr/status/get_fpga_flags.rs b/daemon/tests/xilinx_dfx_mgr/status/get_fpga_flags.rs deleted file mode 100644 index 9191f59c..00000000 --- a/daemon/tests/xilinx_dfx_mgr/status/get_fpga_flags.rs +++ /dev/null @@ -1,38 +0,0 @@ -// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. -// -// Copyright 2026 Canonical Ltd. -// -// SPDX-License-Identifier: GPL-3.0-only -// -// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. -// -// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. - -use crate::common::proxies::status_proxy; -use crate::xilinx_dfx_mgr::{PLATFORM_STRING, setup}; -use googletest::prelude::*; -use rstest::*; -use zbus::Connection; - -#[gtest] -#[tokio::test] -#[rstest] -#[case::no_device("", err(displays_as(contains_substring("FpgadError::Argument:"))))] -#[case::bad_device("dev0", err(displays_as(contains_substring("FpgadError::Argument:"))))] -#[case::all_good("fpga0", ok(anything()))] -async fn cases Matcher<&'a zbus::Result>>( - #[case] device_handle: &str, - #[case] condition: M, - _setup: (), -) { - let connection = Connection::system() - .await - .expect("failed to get fpga flags"); - let proxy = status_proxy::StatusProxy::new(&connection) - .await - .expect("failed to create status proxy"); - let res = proxy.get_fpga_flags(PLATFORM_STRING, device_handle).await; - expect_that!(&res, condition); -} diff --git a/daemon/tests/xilinx_dfx_mgr/status/read_property.rs b/daemon/tests/xilinx_dfx_mgr/status/read_property.rs deleted file mode 100644 index 669dc014..00000000 --- a/daemon/tests/xilinx_dfx_mgr/status/read_property.rs +++ /dev/null @@ -1,53 +0,0 @@ -// This file is part of fpgad, an application to manage FPGA subsystem together with device-tree and kernel modules. -// -// Copyright 2026 Canonical Ltd. -// -// SPDX-License-Identifier: GPL-3.0-only -// -// fpgad is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License version 3, as published by the Free Software Foundation. -// -// fpgad is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License along with this program. If not, see http://www.gnu.org/licenses/. - -use crate::common::proxies::status_proxy; -use crate::xilinx_dfx_mgr::setup; -use googletest::prelude::*; -use rstest::*; -use std::fs; -use zbus::Connection; -use zbus::Result; - -#[gtest] -#[tokio::test] -#[rstest] -#[case::no_path("", err(displays_as(contains_substring("FpgadError::Argument:"))))] -#[case::bad_path("key", err(displays_as(contains_substring("FpgadError::Argument:"))))] -#[case::path_traversal( - "/sys/class/fpga_manager/../../../usr/bin/evil_file.sh", - err(displays_as(contains_substring("path traversal"))) -)] -#[case::all_good("/sys/class/fpga_manager/fpga0/name", ok(anything()))] -async fn read_property_cases Matcher<&'a Result>>( - #[case] property_path: &str, - #[case] condition: M, - _setup: (), -) { - let connection = Connection::system() - .await - .expect("failed to create connection"); - let proxy = status_proxy::StatusProxy::new(&connection) - .await - .expect("failed to create status proxy"); - let res = proxy.read_property(property_path).await; - expect_that!(&res, condition); - - // Verify result matches direct file read - if res.is_ok() && property_path.starts_with("/sys/") { - let direct_read = fs::read_to_string(property_path) - .expect("failed to read file directly") - .to_string(); - let result = res.unwrap(); - expect_that!(result.as_str(), eq(direct_read.as_str())); - } -} diff --git a/tests/snap_testing/test_default/test_dfx_mgr_command.py b/tests/snap_testing/test_default/test_dfx_mgr_command.py new file mode 100644 index 00000000..17d3a093 --- /dev/null +++ b/tests/snap_testing/test_default/test_dfx_mgr_command.py @@ -0,0 +1,37 @@ +#!/usr/bin/env python3 +"""Tests for the dfx-mgr command (passes commands to dfx-mgr-client).""" + +import unittest +from common.base_test import FPGATestBase +from common.helpers import is_dfx_mgr_available + + +class TestDfxMgrCommand(FPGATestBase): + """Test the dfx-mgr command for Xilinx DFX manager operations.""" + + @unittest.skipUnless( + is_dfx_mgr_available(), + "dfx-mgr component not installed. Install with: snap install fpgad+dfx-mgr.comp", + ) + def test_dfx_mgr_list_package(self): + """Test listing packages via dfx-mgr command.""" + proc = self.run_fpgad(["dfx-mgr", "--", "-listPackage"]) + self.assert_proc_succeeds(proc) + + def test_dfx_mgr_without_component(self): + """Test dfx-mgr command fails gracefully when component is not installed.""" + if is_dfx_mgr_available(): + self.skipTest("Test only runs when dfx-mgr is NOT available") + + proc = self.run_fpgad(["dfx-mgr", "--", "-listPackage"]) + self.assert_proc_fails(proc) + self.assertIn("feature not enabled", proc.stderr.lower()) + + @unittest.skipUnless( + is_dfx_mgr_available(), + "dfx-mgr component not installed. Install with: snap install fpgad+dfx-mgr.comp", + ) + def test_dfx_mgr_invalid_command(self): + """Test dfx-mgr with invalid command.""" + proc = self.run_fpgad(["dfx-mgr", "invalidCommand"]) + self.assert_proc_fails(proc) diff --git a/tests/snap_testing/test_default/test_help.py b/tests/snap_testing/test_default/test_help.py index e7fae9ee..5c681b6f 100644 --- a/tests/snap_testing/test_default/test_help.py +++ b/tests/snap_testing/test_default/test_help.py @@ -51,3 +51,23 @@ def test_help_load_overlay(self): """Test help for load overlay subcommand.""" proc = self.run_fpgad(["help", "load", "overlay"]) self.assert_proc_succeeds(proc) + + def test_help_universal(self): + """Test help for universal command.""" + proc = self.run_fpgad(["help", "universal"]) + self.assert_proc_succeeds(proc) + + def test_help_universal_read(self): + """Test help for universal read subcommand.""" + proc = self.run_fpgad(["help", "universal", "read"]) + self.assert_proc_succeeds(proc) + + def test_help_universal_write(self): + """Test help for universal write subcommand.""" + proc = self.run_fpgad(["help", "universal", "write"]) + self.assert_proc_succeeds(proc) + + def test_help_dfx_mgr(self): + """Test help for dfx-mgr command.""" + proc = self.run_fpgad(["help", "dfx-mgr"]) + self.assert_proc_succeeds(proc) diff --git a/tests/snap_testing/test_default/test_universal_command.py b/tests/snap_testing/test_default/test_universal_command.py new file mode 100644 index 00000000..d0de4bdb --- /dev/null +++ b/tests/snap_testing/test_default/test_universal_command.py @@ -0,0 +1,64 @@ +#!/usr/bin/env python3 +"""Tests for the universal command (low-level read/write interface).""" + +from common.base_test import FPGATestBase + + +class TestUniversalCommand(FPGATestBase): + """Test the universal command for low-level property access.""" + + def test_universal_read_property_name(self): + """Test reading FPGA device name via universal read.""" + proc = self.run_fpgad( + ["universal", "read", "read_property", "/sys/class/fpga_manager/fpga0/name"] + ) + self.assert_proc_succeeds(proc) + self.assertIn("zynqmp", proc.stdout.lower()) + + def test_universal_read_flags(self): + """Test reading FPGA flags via universal read.""" + proc = self.run_fpgad(["universal", "read", "read_flags", "fpga0"]) + self.assert_proc_succeeds(proc) + + def test_universal_write_flags(self): + """Test writing FPGA flags via universal write.""" + proc = self.run_fpgad(["universal", "write", "write_flags", "fpga0", "0"]) + self.assert_proc_succeeds(proc) + + def test_universal_write_property(self): + """Test writing FPGA property via universal write.""" + proc = self.run_fpgad( + [ + "universal", + "write", + "write_property", + "/sys/class/fpga_manager/fpga0/flags", + "0", + ] + ) + self.assert_proc_succeeds(proc) + + def test_universal_read_nonexistent_property(self): + """Test reading non-existent property via universal read.""" + proc = self.run_fpgad( + [ + "universal", + "read", + "read_property", + "/sys/class/fpga_manager/fpga0/nonexistent", + ] + ) + self.assert_proc_fails(proc) + + def test_universal_write_nonexistent_property(self): + """Test writing to non-existent property via universal write.""" + proc = self.run_fpgad( + [ + "universal", + "write", + "write_property", + "/sys/class/fpga_manager/fpga0/nonexistent", + "value", + ] + ) + self.assert_proc_fails(proc)