From b875aa9946b15d4a360b4c24985a827fa557c529 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Sat, 3 Oct 2026 12:36:25 +0200 Subject: [PATCH 1/3] harness: Seed a CS image from a file The QEMU runner filled each FMC backing image with one byte, so a test could hand the device erased flash and nothing else. The integration test needs the device to already hold a firmware image the update agent can offer. cs0_contents and cs1_contents name a file to copy in at offset 0; the remainder stays 0xFF. Without them the fill byte behaves as before, so every existing test is unchanged. A contents file longer than flash_size is an error rather than a truncation. A half-written image still parses far enough to fail verification, which would make a verify-failure scenario pass for the wrong reason. Assisted-by: Claude --- target/ast10x0/defs.bzl | 31 ++++++++++-- target/ast10x0/harness/BUILD.bazel | 16 +++++++ target/ast10x0/harness/qemu_runner.py | 47 ++++++++++++++---- target/ast10x0/harness/qemu_runner_test.py | 55 ++++++++++++++++++++++ 4 files changed, 136 insertions(+), 13 deletions(-) create mode 100644 target/ast10x0/harness/qemu_runner_test.py diff --git a/target/ast10x0/defs.bzl b/target/ast10x0/defs.bzl index 6a2690210..dc2078207 100644 --- a/target/ast10x0/defs.bzl +++ b/target/ast10x0/defs.bzl @@ -9,7 +9,7 @@ TARGET_COMPATIBLE_WITH = select({ "//conditions:default": ["@platforms//:incompatible"], }) -def _system_image_test_impl(ctx): +def _system_image_test_impl(ctx, extra_runfiles = []): image_info = ctx.attr.image[SystemImageInfo] executable_symlink = ctx.actions.declare_file(ctx.label.name) ctx.actions.symlink(output = executable_symlink, target_file = image_info.elf) @@ -31,15 +31,15 @@ def _system_image_test_impl(ctx): runfiles.merge(ctx.attr.slave_image[DefaultInfo].default_runfiles), ) + if extra_runfiles: + runfiles = runfiles.merge(ctx.runfiles(files = extra_runfiles)) + return [DefaultInfo( executable = executable_symlink, runfiles = runfiles, )] def _flash_system_image_test_impl(ctx): - default_info = _system_image_test_impl(ctx)[0] - providers = [default_info] - # fmc_model describes the QEMU FMC device uniformly (JEDEC ID + SFDP # geometry), shared by both chip selects. The qemu_runner seeds fresh # images at $TEST_TMPDIR/ and attaches each present CS image as @@ -54,6 +54,19 @@ def _flash_system_image_test_impl(ctx): if ctx.attr.cs1_image: env["AST10X0_CS1_IMAGE"] = ctx.attr.cs1_image env["AST10X0_CS1_FILL"] = str(ctx.attr.cs1_fill) + + # A contents file is copied into the image instead of filling it, so the + # device already holds something when the guest boots. The runner opens + # the path relative to the runfiles tree the test runs in. + contents = [] + if ctx.file.cs0_contents: + contents.append(ctx.file.cs0_contents) + env["AST10X0_CS0_CONTENTS"] = ctx.file.cs0_contents.short_path + if ctx.file.cs1_contents: + contents.append(ctx.file.cs1_contents) + env["AST10X0_CS1_CONTENTS"] = ctx.file.cs1_contents.short_path + + providers = [_system_image_test_impl(ctx, extra_runfiles = contents)[0]] if ctx.attr.cs0_image or ctx.attr.cs1_image: providers.append(RunEnvironmentInfo(environment = env)) return providers @@ -83,6 +96,11 @@ flash_system_image_test = rule( implementation = _flash_system_image_test_impl, test = True, attrs = { + "cs0_contents": attr.label( + doc = "File copied into cs0_image at offset 0 instead of filling " + + "it. The rest stays erased. Longer than flash_size is an error.", + allow_single_file = True, + ), "cs0_fill": attr.int( doc = "Byte value the qemu_runner seeds cs0_image with (default 0xFF, erased).", default = 0xFF, @@ -92,6 +110,11 @@ flash_system_image_test = rule( "attached as FMC CS0 flash (if=mtd, index=0).", default = "", ), + "cs1_contents": attr.label( + doc = "File copied into cs1_image at offset 0 instead of filling " + + "it. The rest stays erased. Longer than flash_size is an error.", + allow_single_file = True, + ), "cs1_fill": attr.int( doc = "Byte value the qemu_runner seeds cs1_image with (default 0xFF, erased).", default = 0xFF, diff --git a/target/ast10x0/harness/BUILD.bazel b/target/ast10x0/harness/BUILD.bazel index 419fab50e..aa53726f4 100644 --- a/target/ast10x0/harness/BUILD.bazel +++ b/target/ast10x0/harness/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_platform//platform_data:defs.bzl", "platform_data") load("@rules_python//python:py_binary.bzl", "py_binary") +load("@rules_python//python:py_test.bzl", "py_test") py_binary( name = "qemu_runner_bin", @@ -25,6 +26,21 @@ platform_data( visibility = ["//visibility:public"], ) +py_test( + name = "qemu_runner_test", + srcs = [ + "qemu_runner.py", + "qemu_runner_test.py", + ], + imports = ["."], + main = "qemu_runner_test.py", + deps = [ + "@@pigweed++cipd+pigweed.qemu//:qemu-system-arm-runfiles", + "@pigweed//pw_tokenizer/py:detokenize", + "@rules_python//python/runfiles", + ], +) + py_binary( name = "test_runner_bin", srcs = [ diff --git a/target/ast10x0/harness/qemu_runner.py b/target/ast10x0/harness/qemu_runner.py index b6e2e3a28..5fbc5a286 100644 --- a/target/ast10x0/harness/qemu_runner.py +++ b/target/ast10x0/harness/qemu_runner.py @@ -131,14 +131,37 @@ def _sentinel_watcher( print(f"Exception watching sentinel: {e}", file=sys.stderr) -def _seed_flash_image(path: str, size: int, fill: int = 0xFF) -> None: - """Create/overwrite `path` with `size` bytes of `fill` (0xFF = erased).""" +def _seed_flash_image( + path: str, size: int, fill: int = 0xFF, contents: str = "" +) -> None: + """Create/overwrite `path` with `size` bytes. + + Without `contents` the image is `size` bytes of `fill` (0xFF = erased). + With it, the file is copied in at offset 0 and the remainder is left + erased, so a test can hand the device a real image to read. + + A `contents` file longer than `size` is an error rather than a truncation: + a half-written image still parses far enough to fail verification, which + would make a verify-failure scenario pass for the wrong reason. + """ + if not contents: + with open(path, "wb") as f: + f.write(bytes([fill & 0xFF]) * size) + return + + data = Path(contents).read_bytes() + if len(data) > size: + raise ValueError( + f"{contents} is {len(data)} bytes, larger than the {size}-byte " + f"flash it seeds" + ) with open(path, "wb") as f: - f.write(bytes([fill & 0xFF]) * size) + f.write(data) + f.write(b"\xff" * (size - len(data))) def _resolve_flash_drives(args): - """Return a list of (index, path, size, fill) FMC backing images. + """Return a list of (index, path, size, fill, contents) FMC backing images. index 0 -> FMC CS0, index 1 -> FMC CS1. Each image is re-seeded on every run so tests start from a known device state. An explicit --flash-image @@ -146,20 +169,26 @@ def _resolve_flash_drives(args): AST10X0_CS0_IMAGE / AST10X0_CS1_IMAGE (basenames) plus AST10X0_FLASH_SIZE and per-CS AST10X0_CS0_FILL / AST10X0_CS1_FILL, resolved against $TEST_TMPDIR so each run gets private, freshly-seeded images. + + AST10X0_CS0_CONTENTS / AST10X0_CS1_CONTENTS name a file to copy in at + offset 0 instead of filling, for a test that needs the device to already + hold an image. The path is a runfile, resolved by the test rule. """ base = os.environ.get("TEST_TMPDIR", tempfile.gettempdir()) size = int(os.environ.get("AST10X0_FLASH_SIZE", str(args.flash_size))) drives = [] if args.flash_image: - drives.append((1, args.flash_image, size, 0xFF)) + drives.append((1, args.flash_image, size, 0xFF, "")) cs0 = os.environ.get("AST10X0_CS0_IMAGE") if cs0: fill = int(os.environ.get("AST10X0_CS0_FILL", "255")) - drives.append((0, os.path.join(base, cs0), size, fill)) + contents = os.environ.get("AST10X0_CS0_CONTENTS", "") + drives.append((0, os.path.join(base, cs0), size, fill, contents)) cs1 = os.environ.get("AST10X0_CS1_IMAGE") if cs1: fill = int(os.environ.get("AST10X0_CS1_FILL", "255")) - drives.append((1, os.path.join(base, cs1), size, fill)) + contents = os.environ.get("AST10X0_CS1_CONTENTS", "") + drives.append((1, os.path.join(base, cs1), size, fill, contents)) return drives @@ -191,8 +220,8 @@ def _main(args) -> None: args.image, ] - for index, path, size, fill in drives: - _seed_flash_image(path, size, fill) + for index, path, size, fill, contents in drives: + _seed_flash_image(path, size, fill, contents) qemu_args += [ "-drive", f"file={path},format=raw,if=mtd,index={index}", diff --git a/target/ast10x0/harness/qemu_runner_test.py b/target/ast10x0/harness/qemu_runner_test.py new file mode 100644 index 000000000..d2fe10098 --- /dev/null +++ b/target/ast10x0/harness/qemu_runner_test.py @@ -0,0 +1,55 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 +"""Tests for the flash seeding in qemu_runner.""" + +import tempfile +import unittest + +from pathlib import Path + +from qemu_runner import _seed_flash_image + + +class SeedFlashImageTest(unittest.TestCase): + """A seeded image is always exactly flash_size bytes.""" + + def setUp(self) -> None: + self._dir = tempfile.TemporaryDirectory() + self.addCleanup(self._dir.cleanup) + self.image = str(Path(self._dir.name) / "cs1.img") + + def _contents_file(self, data: bytes) -> str: + path = Path(self._dir.name) / "contents.bin" + path.write_bytes(data) + return str(path) + + def test_without_contents_the_image_is_all_fill(self) -> None: + _seed_flash_image(self.image, 32, fill=0xFF) + self.assertEqual(Path(self.image).read_bytes(), b"\xff" * 32) + + def test_fill_is_honoured(self) -> None: + _seed_flash_image(self.image, 8, fill=0x00) + self.assertEqual(Path(self.image).read_bytes(), b"\x00" * 8) + + def test_contents_land_at_offset_zero_and_the_rest_is_erased(self) -> None: + _seed_flash_image(self.image, 16, contents=self._contents_file(b"openprot")) + self.assertEqual(Path(self.image).read_bytes(), b"openprot" + b"\xff" * 8) + + def test_contents_override_fill(self) -> None: + """Fill describes the erased remainder, not the seeded bytes.""" + _seed_flash_image(self.image, 4, fill=0x00, contents=self._contents_file(b"ab")) + self.assertEqual(Path(self.image).read_bytes(), b"ab\xff\xff") + + def test_contents_may_fill_the_whole_image(self) -> None: + _seed_flash_image(self.image, 4, contents=self._contents_file(b"abcd")) + self.assertEqual(Path(self.image).read_bytes(), b"abcd") + + def test_contents_larger_than_the_flash_is_an_error(self) -> None: + """Truncating would hand the guest an image that fails for the + wrong reason.""" + with self.assertRaises(ValueError): + _seed_flash_image(self.image, 4, contents=self._contents_file(b"abcde")) + + +if __name__ == "__main__": + unittest.main() From 2035289090b77a64348d3e4c58a42026018ce4b0 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Sat, 3 Oct 2026 12:46:12 +0200 Subject: [PATCH 2/3] tools: Add the flash image builder A QEMU test that offers a firmware image needs that image on flash before the guest boots. The harness can now seed a chip select from a file; this makes the file. The layout arrives as arguments but is built with the same Region, Slot and ImageLayout constructors the board tables use, so a layout this tool accepts is one the orchestrator would accept: unique slot ids, no overlap, no zero-length region, nothing past the end of the offset space. Re-deriving those rules here would let a test image drift from what the firmware believes about the same flash. Everything no image covers stays 0xFF, and a payload too large for its slot is an error rather than a truncation. The flash_image rule wraps the tool so a BUILD file can hand the result straight to cs0_contents or cs1_contents. Assisted-by: Claude --- tools/flash_image/BUILD.bazel | 40 ++++ tools/flash_image/defs.bzl | 67 +++++++ tools/flash_image/example_image_test.sh | 36 ++++ tools/flash_image/main.rs | 199 ++++++++++++++++++ tools/flash_image/testdata/slot_a.bin | 1 + tools/flash_image/testdata/slot_b.bin | 1 + tools/flash_image/tests.rs | 255 ++++++++++++++++++++++++ 7 files changed, 599 insertions(+) create mode 100644 tools/flash_image/BUILD.bazel create mode 100644 tools/flash_image/defs.bzl create mode 100755 tools/flash_image/example_image_test.sh create mode 100644 tools/flash_image/main.rs create mode 100644 tools/flash_image/testdata/slot_a.bin create mode 100644 tools/flash_image/testdata/slot_b.bin create mode 100644 tools/flash_image/tests.rs diff --git a/tools/flash_image/BUILD.bazel b/tools/flash_image/BUILD.bazel new file mode 100644 index 000000000..d1c933ad7 --- /dev/null +++ b/tools/flash_image/BUILD.bazel @@ -0,0 +1,40 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 + +load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_test") +load(":defs.bzl", "flash_image") + +# Host build tool: makes the flash image a QEMU test hands the device. +rust_binary( + name = "flash_image", + srcs = [ + "main.rs", + "tests.rs", + ], + edition = "2024", + visibility = ["//visibility:public"], + deps = ["//services/orchestrator/config:orchestrator_config"], +) + +rust_test( + name = "flash_image_test", + crate = ":flash_image", +) + +# Proves the rule wiring end to end: the builder runs under Bazel, both +# payloads land at their bases, and the result is usable as cs0_contents or +# cs1_contents on a flash_system_image_test. +flash_image( + name = "example_image", + flash_size = 64, + slots = { + "0:0x0:0x20": "testdata/slot_a.bin", + "1:0x20:0x20": "testdata/slot_b.bin", + }, +) + +sh_test( + name = "example_image_test", + srcs = ["example_image_test.sh"], + data = [":example_image"], +) diff --git a/tools/flash_image/defs.bzl b/tools/flash_image/defs.bzl new file mode 100644 index 000000000..be552711a --- /dev/null +++ b/tools/flash_image/defs.bzl @@ -0,0 +1,67 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 +"""Builds a flash image from a slot layout, for QEMU tests to hand the device.""" + +def _flash_image_impl(ctx): + image = ctx.actions.declare_file(ctx.label.name + ".img") + + args = ctx.actions.args() + args.add("--flash-size", ctx.attr.flash_size) + args.add("--output", image) + + payloads = [] + for spec, target in ctx.attr.slots.items(): + files = target.files.to_list() + if len(files) != 1: + fail("slot {}: {} is not a single file".format(spec, target.label)) + payloads.append(files[0]) + args.add("--slot", "{}={}".format(spec, files[0].path)) + + if ctx.attr.golden: + files = ctx.attr.golden.files.to_list() + if len(files) != 1: + fail("golden: {} is not a single file".format(ctx.attr.golden.label)) + payloads.append(files[0]) + args.add("--golden", "{}={}".format(ctx.attr.golden_region, files[0].path)) + + ctx.actions.run( + mnemonic = "FlashImage", + executable = ctx.executable._builder, + arguments = [args], + inputs = payloads, + outputs = [image], + ) + + return [DefaultInfo(files = depset([image]))] + +flash_image = rule( + implementation = _flash_image_impl, + doc = "A flash image with each payload written at its slot's base, the " + + "rest left erased. The layout goes through the same constructors " + + "the board tables use, so a layout the orchestrator would refuse " + + "fails the build here.", + attrs = { + "flash_size": attr.int( + doc = "Size in bytes of the image, matching the test's flash_size.", + mandatory = True, + ), + "golden": attr.label( + doc = "Payload for the golden image, if the layout has one.", + allow_single_file = True, + ), + "golden_region": attr.string( + doc = "Where the golden image lives, as base:len.", + default = "", + ), + "slots": attr.string_keyed_label_dict( + doc = "Maps id:base:len to the file that fills that slot. Base " + + "and len take decimal or 0x hex.", + allow_files = True, + ), + "_builder": attr.label( + default = "//tools/flash_image:flash_image", + executable = True, + cfg = "exec", + ), + }, +) diff --git a/tools/flash_image/example_image_test.sh b/tools/flash_image/example_image_test.sh new file mode 100755 index 000000000..32884e2d6 --- /dev/null +++ b/tools/flash_image/example_image_test.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 +# +# Checks the image the flash_image rule built: both payloads at their slot +# bases, erased everywhere else, and exactly flash_size bytes. This covers +# the Bazel wiring; the layout rules themselves are covered by +# //tools/flash_image:flash_image_test. + +set -euo pipefail + +image="${TEST_SRCDIR}/_main/tools/flash_image/example_image.img" + +# wc rather than stat: runfiles entries are symlinks, and GNU stat reports +# the link itself unless told otherwise. +actual_size=$(wc -c < "${image}") +if [ "${actual_size}" -ne 64 ]; then + echo "expected a 64-byte image, got ${actual_size}" >&2 + exit 1 +fi + +# od keeps this readable: slot A at 0x00, slot B at 0x20, 0xFF between. +expected=$( + cat <<'EOF' +0000000 S L O T - A - C O N T E N T 377 377 +0000020 377 377 377 377 377 377 377 377 377 377 377 377 377 377 377 377 +0000040 S L O T - B 377 377 377 377 377 377 377 377 377 377 +0000060 377 377 377 377 377 377 377 377 377 377 377 377 377 377 377 377 +0000100 +EOF +) + +if ! diff <(od -c "${image}") <(echo "${expected}"); then + echo "image contents do not match the declared layout" >&2 + exit 1 +fi diff --git a/tools/flash_image/main.rs b/tools/flash_image/main.rs new file mode 100644 index 000000000..6d0f17ff6 --- /dev/null +++ b/tools/flash_image/main.rs @@ -0,0 +1,199 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! Writes a flash image for a QEMU test from a board's slot layout. +//! +//! The layout comes in as arguments but is built with the same +//! `Region`/`Slot`/`ImageLayout` constructors the board tables use, so a +//! layout this tool accepts is one the orchestrator would accept: slot ids +//! unique, no two regions overlapping, no zero-length region, nothing past +//! the end of the offset space. Re-deriving those rules here would let a +//! test image drift from what the firmware believes. +//! +//! Everything the layout does not name stays 0xFF, so the image looks like +//! erased flash with images written into it. +//! +//! ```text +//! flash-image --flash-size 0x4000000 \ +//! --slot 0:0x0:0x100000=slot_a.bin \ +//! --slot 1:0x100000:0x100000=slot_b.bin \ +//! --golden 0x200000:0x100000=golden.bin \ +//! --output cs1.img +//! ``` + +use std::collections::BTreeMap; +use std::fs; +use std::path::PathBuf; +use std::process::ExitCode; + +use orchestrator_config::{Golden, ImageLayout, Region, Slot, SlotId}; + +/// Erased flash. Everything no image covers keeps this value. +const ERASED: u8 = 0xFF; + +/// One `--slot` or `--golden` argument: where the image goes and what +/// fills it. +struct Placement { + region: Region, + payload: PathBuf, +} + +/// `base:len=path`, with base and len in decimal or 0x hex. +fn parse_region(spec: &str) -> Result { + let (region, path) = spec + .split_once('=') + .ok_or_else(|| format!("{spec}: expected base:len=path"))?; + let (base, len) = region + .split_once(':') + .ok_or_else(|| format!("{spec}: expected base:len=path"))?; + Ok(Placement { + region: Region::new(parse_u32(base)?, parse_u32(len)?), + payload: PathBuf::from(path), + }) +} + +/// `id:base:len=path`, the slot id in front of a region spec. +fn parse_slot(spec: &str) -> Result<(SlotId, Placement), String> { + let (id, rest) = spec + .split_once(':') + .ok_or_else(|| format!("{spec}: expected id:base:len=path"))?; + let id: u8 = id + .parse() + .map_err(|_| format!("{id}: slot id must be 0 to 255"))?; + Ok((SlotId(id), parse_region(rest)?)) +} + +fn parse_u32(text: &str) -> Result { + let parsed = match text.strip_prefix("0x") { + Some(hex) => u32::from_str_radix(hex, 16), + None => text.parse(), + }; + parsed.map_err(|_| format!("{text}: not a 32-bit number")) +} + +struct Args { + flash_size: u32, + slots: Vec<(SlotId, Placement)>, + golden: Option, + output: PathBuf, +} + +fn parse_args(argv: impl Iterator) -> Result { + let mut flash_size = None; + let mut slots = Vec::new(); + let mut golden = None; + let mut output = None; + + let mut argv = argv.peekable(); + while let Some(flag) = argv.next() { + let mut value = || { + argv.next() + .ok_or_else(|| format!("{flag}: missing its value")) + }; + match flag.as_str() { + "--flash-size" => flash_size = Some(parse_u32(&value()?)?), + "--slot" => slots.push(parse_slot(&value()?)?), + "--golden" => golden = Some(parse_region(&value()?)?), + "--output" => output = Some(PathBuf::from(value()?)), + other => return Err(format!("{other}: unknown argument")), + } + } + + Ok(Args { + flash_size: flash_size.ok_or("--flash-size is required")?, + slots, + golden, + output: output.ok_or("--output is required")?, + }) +} + +/// Builds the image. The layout is constructed first so a bad one is +/// rejected before any file is read. +fn build(args: &Args) -> Result, String> { + // ImageLayout borrows the slots for 'static, matching board tables + // that declare them as consts. A build tool runs once and exits, so + // leaking the one list it makes costs nothing. + let declared: Vec = args + .slots + .iter() + .map(|(id, placement)| Slot::new(*id, placement.region)) + .collect(); + let layout = ImageLayout::new( + Box::leak(declared.into_boxed_slice()), + args.golden.as_ref().map(|g| Golden::new(g.region)), + ); + + // Slot ids are unique by now, so a payload per id is unambiguous. + let payloads: BTreeMap = args + .slots + .iter() + .map(|(id, placement)| (id.0, &placement.payload)) + .collect(); + + let mut image = vec![ERASED; args.flash_size as usize]; + let mut place = |region: Region, payload: &PathBuf, what: &str| { + let bytes = fs::read(payload).map_err(|e| format!("{}: {e}", payload.display()))?; + if bytes.len() > region.len() as usize { + return Err(format!( + "{}: {} bytes does not fit the {}-byte {what}", + payload.display(), + bytes.len(), + region.len() + )); + } + let base = region.base() as usize; + let end = base + bytes.len(); + if region.end() as usize > image.len() { + return Err(format!( + "{what} ends at {:#x}, past the {:#x}-byte flash", + region.end(), + image.len() + )); + } + image[base..end].copy_from_slice(&bytes); + Ok(()) + }; + + for slot in layout.slots() { + let payload = payloads[&slot.id().0]; + place(slot.region(), payload, "slot")?; + } + if let (Some(golden), Some(placement)) = (layout.golden(), args.golden.as_ref()) { + place(golden.region(), &placement.payload, "golden image")?; + } + + Ok(image) +} + +fn main() -> ExitCode { + // skip(1) drops argv[0], which is the part semgrep's rule is about: + // the caller chooses it and it need not name anything real. Nothing + // here reads it. + // nosemgrep: rust.lang.security.args.args + let args = match parse_args(std::env::args().skip(1)) { + Ok(args) => args, + Err(e) => { + eprintln!("flash-image: {e}"); + return ExitCode::FAILURE; + } + }; + + let image = match build(&args) { + Ok(image) => image, + Err(e) => { + eprintln!("flash-image: {e}"); + return ExitCode::FAILURE; + } + }; + + match fs::write(&args.output, &image) { + Ok(()) => ExitCode::SUCCESS, + Err(e) => { + eprintln!("flash-image: {}: {e}", args.output.display()); + ExitCode::FAILURE + } + } +} + +#[cfg(test)] +mod tests; diff --git a/tools/flash_image/testdata/slot_a.bin b/tools/flash_image/testdata/slot_a.bin new file mode 100644 index 000000000..1840751b6 --- /dev/null +++ b/tools/flash_image/testdata/slot_a.bin @@ -0,0 +1 @@ +SLOT-A-CONTENT \ No newline at end of file diff --git a/tools/flash_image/testdata/slot_b.bin b/tools/flash_image/testdata/slot_b.bin new file mode 100644 index 000000000..1e02e3704 --- /dev/null +++ b/tools/flash_image/testdata/slot_b.bin @@ -0,0 +1 @@ +SLOT-B \ No newline at end of file diff --git a/tools/flash_image/tests.rs b/tools/flash_image/tests.rs new file mode 100644 index 000000000..29304a286 --- /dev/null +++ b/tools/flash_image/tests.rs @@ -0,0 +1,255 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +use super::*; + +/// Writes `data` to a uniquely named file under the test's temp dir and +/// returns its path. `name` only has to be unique within one test. +fn payload(name: &str, data: &[u8]) -> PathBuf { + let dir = std::env::var("TEST_TMPDIR").unwrap_or_else(|_| "/tmp".into()); + let path = PathBuf::from(dir).join(format!("flash-image-{name}.bin")); + fs::write(&path, data).unwrap(); + path +} + +fn args(flash_size: u32, slots: Vec<(SlotId, Placement)>) -> Args { + Args { + flash_size, + slots, + golden: None, + output: PathBuf::from("unused"), + } +} + +#[test] +fn parse_region_takes_decimal_and_hex() { + let p = parse_region("0x10:32=some/path").unwrap(); + assert_eq!(p.region.base(), 0x10); + assert_eq!(p.region.len(), 32); + assert_eq!(p.payload, PathBuf::from("some/path")); +} + +#[test] +fn parse_slot_reads_the_id_first() { + let (id, p) = parse_slot("3:0:16=x").unwrap(); + assert_eq!(id.0, 3); + assert_eq!(p.region.len(), 16); +} + +#[test] +fn a_spec_without_a_payload_is_refused() { + assert!(parse_region("0:16").is_err()); +} + +#[test] +fn a_spec_without_a_length_is_refused() { + assert!(parse_region("0=x").is_err()); +} + +#[test] +fn an_unknown_argument_is_refused() { + let argv = ["--rubbish".to_string(), "1".to_string()]; + assert!(parse_args(argv.into_iter()).is_err()); +} + +#[test] +fn a_flag_without_its_value_is_refused() { + let argv = ["--flash-size".to_string()]; + assert!(parse_args(argv.into_iter()).is_err()); +} + +#[test] +fn payloads_land_at_their_slot_base_and_the_rest_stays_erased() { + let a = payload("land-a", b"AAAA"); + let b = payload("land-b", b"BB"); + let image = build(&args( + 32, + vec![ + ( + SlotId(0), + Placement { + region: Region::new(0, 16), + payload: a, + }, + ), + ( + SlotId(1), + Placement { + region: Region::new(16, 16), + payload: b, + }, + ), + ], + )) + .unwrap(); + + assert_eq!(&image[0..4], b"AAAA"); + assert_eq!(&image[4..16], &[ERASED; 12]); + assert_eq!(&image[16..18], b"BB"); + assert_eq!(&image[18..32], &[ERASED; 14]); +} + +#[test] +fn declaration_order_does_not_have_to_match_address_order() { + let high = payload("order-high", b"H"); + let low = payload("order-low", b"L"); + let image = build(&args( + 32, + vec![ + ( + SlotId(7), + Placement { + region: Region::new(16, 16), + payload: high, + }, + ), + ( + SlotId(2), + Placement { + region: Region::new(0, 16), + payload: low, + }, + ), + ], + )) + .unwrap(); + + assert_eq!(image[0], b'L'); + assert_eq!(image[16], b'H'); +} + +#[test] +fn a_payload_larger_than_its_slot_is_refused() { + let big = payload("too-big", b"AAAAAAAA"); + let err = build(&args( + 32, + vec![( + SlotId(0), + Placement { + region: Region::new(0, 4), + payload: big, + }, + )], + )) + .unwrap_err(); + assert!(err.contains("does not fit"), "{err}"); +} + +#[test] +fn a_slot_past_the_end_of_the_flash_is_refused() { + let p = payload("past-end", b"A"); + let err = build(&args( + 16, + vec![( + SlotId(0), + Placement { + region: Region::new(8, 16), + payload: p, + }, + )], + )) + .unwrap_err(); + assert!(err.contains("past the"), "{err}"); +} + +#[test] +fn a_missing_payload_is_refused() { + let err = build(&args( + 16, + vec![( + SlotId(0), + Placement { + region: Region::new(0, 8), + payload: PathBuf::from("no/such/file"), + }, + )], + )) + .unwrap_err(); + assert!(err.contains("no/such/file"), "{err}"); +} + +#[test] +fn the_golden_image_is_written_too() { + let slot = payload("golden-slot", b"S"); + let golden = payload("golden-image", b"G"); + let image = build(&Args { + flash_size: 32, + slots: vec![( + SlotId(0), + Placement { + region: Region::new(0, 16), + payload: slot, + }, + )], + golden: Some(Placement { + region: Region::new(16, 16), + payload: golden, + }), + output: PathBuf::from("unused"), + }) + .unwrap(); + + assert_eq!(image[0], b'S'); + assert_eq!(image[16], b'G'); +} + +/// `ImageLayout::new` is what rejects these, not any check in this tool. +/// The point of the panic is that a test image cannot describe a layout +/// the orchestrator would refuse to boot. +#[test] +#[should_panic(expected = "slots must not overlap")] +fn overlapping_slots_are_rejected_by_the_layout() { + let a = payload("overlap-a", b"A"); + let b = payload("overlap-b", b"B"); + let _ = build(&args( + 64, + vec![ + ( + SlotId(0), + Placement { + region: Region::new(0, 32), + payload: a, + }, + ), + ( + SlotId(1), + Placement { + region: Region::new(16, 32), + payload: b, + }, + ), + ], + )); +} + +#[test] +#[should_panic(expected = "slot ids must be unique")] +fn duplicate_slot_ids_are_rejected_by_the_layout() { + let a = payload("dup-a", b"A"); + let b = payload("dup-b", b"B"); + let _ = build(&args( + 64, + vec![ + ( + SlotId(0), + Placement { + region: Region::new(0, 16), + payload: a, + }, + ), + ( + SlotId(0), + Placement { + region: Region::new(16, 16), + payload: b, + }, + ), + ], + )); +} + +#[test] +#[should_panic(expected = "region length must not be zero")] +fn a_zero_length_region_is_rejected_by_the_layout() { + let _ = parse_region("0:0=x"); +} From e79d8095fc142afa657af3afa8f165122da2b8ce Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Sat, 3 Oct 2026 21:39:08 +0200 Subject: [PATCH 3/3] services/mctp: Add the loopback transport A QEMU test has no wire and no second chip, but the PLDM firmware device still has to reach an update agent. This is the transport for that: each server queues what its router fragments, and the caller feeds that queue to the other server's inbound. One server's outbox is the other's wire. Two servers, not one looping back to itself. A message addressed to a server's own EID has no route out and back, which the router reports as BadArgument before the sender is ever consulted. The queue belongs to the caller rather than to the sender, because the router takes the sender by value and offers no way back to it. Draining in the caller's loop also keeps the send path non-blocking. The fragment buffer holds the MTU plus the four-byte MCTP header. A buffer of exactly the MTU makes the router refuse the send rather than truncate it. The arrangement comes from services/mctp/echo/tests/echo_host.rs, which does the same thing with test-local helpers. This is the library version, so an app can use it. Assisted-by: Claude --- services/mctp/transport-loopback/BUILD.bazel | 28 +++ services/mctp/transport-loopback/src/lib.rs | 160 ++++++++++++++++++ services/mctp/transport-loopback/src/tests.rs | 137 +++++++++++++++ 3 files changed, 325 insertions(+) create mode 100644 services/mctp/transport-loopback/BUILD.bazel create mode 100644 services/mctp/transport-loopback/src/lib.rs create mode 100644 services/mctp/transport-loopback/src/tests.rs diff --git a/services/mctp/transport-loopback/BUILD.bazel b/services/mctp/transport-loopback/BUILD.bazel new file mode 100644 index 000000000..b630f4896 --- /dev/null +++ b/services/mctp/transport-loopback/BUILD.bazel @@ -0,0 +1,28 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 + +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") + +rust_library( + name = "mctp_transport_loopback", + srcs = [ + "src/lib.rs", + "src/tests.rs", + ], + crate_name = "openprot_mctp_transport_loopback", + edition = "2024", + visibility = ["//visibility:public"], + deps = [ + "@rust_crates//:heapless", + "@rust_crates//:mctp", + "@rust_crates//:mctp-lib", + ], +) + +# Host tests: the round trip goes through a real server, which is why the +# server is a test-only dependency rather than a dependency of the library. +rust_test( + name = "mctp_transport_loopback_test", + crate = ":mctp_transport_loopback", + deps = ["//services/mctp/server:mctp_server_lib"], +) diff --git a/services/mctp/transport-loopback/src/lib.rs b/services/mctp/transport-loopback/src/lib.rs new file mode 100644 index 000000000..43a90ca40 --- /dev/null +++ b/services/mctp/transport-loopback/src/lib.rs @@ -0,0 +1,160 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! An MCTP transport that goes nowhere: packets come straight back in. +//! +//! On a board the server's [`Sender`] hands fragments to a wire. In a QEMU +//! test there is no wire and no second chip, so both endpoints live in one +//! image. This sender queues what its router fragments, and the caller +//! feeds that queue to the *other* endpoint's `Server::inbound`. One +//! server's outbox is the other's wire. +//! +//! The queue is owned by the caller rather than by the sender, because the +//! router takes the sender by value and offers no way back to it. The +//! caller keeps [`LoopbackQueue`] and lends it to [`LoopbackSender`]: +//! +//! ```ignore +//! let out_a = LoopbackQueue::<8>::new(); +//! let out_b = LoopbackQueue::<8>::new(); +//! let mut a = Server::new(Eid(8), now, LoopbackSender::new(&out_a)); +//! let mut b = Server::new(Eid(42), now, LoopbackSender::new(&out_b)); +//! // a sends; what a queued is what b receives. +//! while let Some(pkt) = out_a.take() { +//! b.inbound(&pkt)?; +//! } +//! ``` +//! +//! Two servers, not one looping to itself: a message addressed to a +//! server's own EID has no route out and back. +//! +//! Draining in the caller's loop rather than re-entering the router keeps +//! the send path non-blocking and the borrow straight, since the router +//! owns the sender and offers no way back to it. +//! +//! What this does not model: a wire that drops, reorders, delays or +//! corrupts. Every packet arrives, in order, as soon as the caller drains. +//! A test that wants retry or timeout behaviour needs something else. + +#![cfg_attr(not(test), no_std)] +#![forbid(unsafe_code)] + +use core::cell::RefCell; + +use heapless::Deque; +use mctp::{Error, Result, Tag}; +use mctp_lib::fragment::{Fragmenter, SendOutput}; +use mctp_lib::Sender; + +/// Largest payload this transport carries in one fragment. Nothing here is +/// limited by a bus, so the number only has to be agreed by both ends, +/// which it is by construction. +pub const LOOPBACK_MTU: usize = 255; + +/// MCTP transport header, which rides in front of the payload. The +/// fragment buffer has to hold both: a buffer of exactly the MTU makes the +/// router reject the send rather than truncate it. +const MCTP_HEADER_SIZE: usize = 4; + +/// One fragmented packet waiting to be fed back in, header included. +pub type Packet = heapless::Vec; + +/// Packets on their way from the router back to it. +/// +/// `DEPTH` bounds how many fragments may be in flight between drains. A +/// caller that drains after every send needs one; one that sends a whole +/// message first needs as many fragments as that message takes. +pub struct LoopbackQueue { + inner: RefCell>, +} + +struct Inner { + packets: Deque, + dropped: u32, +} + +impl Default for LoopbackQueue { + fn default() -> Self { + Self::new() + } +} + +impl LoopbackQueue { + /// An empty queue. + #[must_use] + pub const fn new() -> Self { + Self { + inner: RefCell::new(Inner { + packets: Deque::new(), + dropped: 0, + }), + } + } + + /// Takes the oldest queued packet, or `None` when none are waiting. + /// Feed what this returns to `Server::inbound`. + pub fn take(&self) -> Option { + self.inner.borrow_mut().packets.pop_front() + } + + /// How many packets are waiting. + #[must_use] + pub fn pending(&self) -> usize { + self.inner.borrow().packets.len() + } + + /// How many packets were discarded because the queue was full. + /// + /// A wire drops packets silently and MCTP is built to cope, so a full + /// queue is not an error here. It is still worth reporting, because in + /// this transport it means `DEPTH` is too small rather than that the + /// medium is lossy, and the test that follows will fail for a reason + /// that looks unrelated. + #[must_use] + pub fn dropped(&self) -> u32 { + self.inner.borrow().dropped + } +} + +/// The [`Sender`] half. Holds no packets of its own; everything goes to the +/// queue the caller kept. +pub struct LoopbackSender<'q, const DEPTH: usize> { + queue: &'q LoopbackQueue, +} + +impl<'q, const DEPTH: usize> LoopbackSender<'q, DEPTH> { + /// Lends `queue` to the router. + #[must_use] + pub const fn new(queue: &'q LoopbackQueue) -> Self { + Self { queue } + } +} + +impl Sender for LoopbackSender<'_, DEPTH> { + fn send_vectored(&mut self, mut fragmenter: Fragmenter, payload: &[&[u8]]) -> Result { + loop { + let mut buf = [0u8; LOOPBACK_MTU + MCTP_HEADER_SIZE]; + match fragmenter.fragment_vectored(payload, &mut buf) { + SendOutput::Packet(p) => { + let mut packet = Packet::new(); + if packet.extend_from_slice(p).is_err() { + // A fragment longer than the MTU we advertised. + return Err(Error::NoSpace); + } + let mut inner = self.queue.inner.borrow_mut(); + if inner.packets.push_back(packet).is_err() { + inner.dropped = inner.dropped.saturating_add(1); + } + } + SendOutput::Complete { tag, .. } => break Ok(tag), + SendOutput::Error { err, .. } => break Err(err), + } + } + } + + fn get_mtu(&self) -> usize { + LOOPBACK_MTU + } +} + +#[cfg(test)] +mod tests; diff --git a/services/mctp/transport-loopback/src/tests.rs b/services/mctp/transport-loopback/src/tests.rs new file mode 100644 index 000000000..2355141ed --- /dev/null +++ b/services/mctp/transport-loopback/src/tests.rs @@ -0,0 +1,137 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! The queue contract, plus a round trip between two cross-wired servers. +//! +//! A `Fragmenter` is built by the router rather than by callers, so +//! `send_vectored` is exercised through real servers rather than called +//! directly. The two-server arrangement mirrors +//! `services/mctp/echo/tests/echo_host.rs`, which is where it comes from. + +use super::*; + +use mctp::Eid; +use openprot_mctp_server::Server; + +/// The two endpoints. Values are arbitrary; they only have to differ. +const EID_A: u8 = 8; +const EID_B: u8 = 42; + +/// Vendor-defined message type. Nothing here cares what the bytes mean. +const MSG_TYPE: u8 = 0x7E; + +/// Moves everything one side queued into the other side, the way a wire +/// would. Returns how many packets crossed. +fn transfer( + from: &LoopbackQueue, + to: &mut Server, +) -> usize { + let mut crossed = 0; + while let Some(pkt) = from.take() { + to.inbound(&pkt).expect("inbound accepts the packet"); + crossed += 1; + } + crossed +} + +#[test] +fn a_new_queue_has_nothing_in_it() { + let q: LoopbackQueue<4> = LoopbackQueue::new(); + assert_eq!(q.pending(), 0); + assert_eq!(q.dropped(), 0); +} + +#[test] +fn the_advertised_mtu_excludes_the_header() { + let q: LoopbackQueue<4> = LoopbackQueue::new(); + let s = LoopbackSender::new(&q); + assert_eq!(s.get_mtu(), LOOPBACK_MTU); + // The packets it queues carry the header on top of that payload. + assert_eq!(Packet::new().capacity(), LOOPBACK_MTU + MCTP_HEADER_SIZE); +} + +#[test] +fn taking_from_an_empty_queue_gives_nothing() { + let q: LoopbackQueue<4> = LoopbackQueue::new(); + assert!(q.take().is_none()); +} + +#[test] +fn a_message_crosses_from_one_server_to_the_other() { + let out_a: LoopbackQueue<16> = LoopbackQueue::new(); + let out_b: LoopbackQueue<16> = LoopbackQueue::new(); + let mut a: Server<_, 16> = Server::new(Eid(EID_A), 0, LoopbackSender::new(&out_a)); + let mut b: Server<_, 16> = Server::new(Eid(EID_B), 0, LoopbackSender::new(&out_b)); + + let listener = b.listener(MSG_TYPE).expect("B binds a listener"); + + let payload = b"openprot"; + let req = a.req(EID_B).expect("A opens a request to B"); + a.send(Some(req), MSG_TYPE, Some(EID_B), None, false, payload) + .expect("A sends"); + + assert!(transfer(&out_a, &mut b) > 0, "nothing crossed the wire"); + + let mut buf = [0u8; LOOPBACK_MTU]; + let meta = b.try_recv(listener, &mut buf).expect("B receives it"); + assert_eq!(&buf[..payload.len()], payload); + assert_eq!(meta.remote_eid, EID_A); +} + +#[test] +fn a_payload_longer_than_the_mtu_arrives_whole() { + let out_a: LoopbackQueue<16> = LoopbackQueue::new(); + let out_b: LoopbackQueue<16> = LoopbackQueue::new(); + let mut a: Server<_, 16> = Server::new(Eid(EID_A), 0, LoopbackSender::new(&out_a)); + let mut b: Server<_, 16> = Server::new(Eid(EID_B), 0, LoopbackSender::new(&out_b)); + + let listener = b.listener(MSG_TYPE).expect("B binds a listener"); + + // More than one fragment, so reassembly is actually exercised. + let payload: std::vec::Vec = (0..(LOOPBACK_MTU * 2) as u16).map(|i| i as u8).collect(); + let req = a.req(EID_B).expect("A opens a request to B"); + a.send(Some(req), MSG_TYPE, Some(EID_B), None, false, &payload) + .expect("A sends"); + + let fragments = transfer(&out_a, &mut b); + assert!(fragments > 1, "expected several fragments, got {fragments}"); + + let mut buf = [0u8; LOOPBACK_MTU * 3]; + b.try_recv(listener, &mut buf).expect("B receives it"); + assert_eq!(&buf[..payload.len()], &payload[..]); +} + +#[test] +fn the_reply_crosses_back() { + let out_a: LoopbackQueue<16> = LoopbackQueue::new(); + let out_b: LoopbackQueue<16> = LoopbackQueue::new(); + let mut a: Server<_, 16> = Server::new(Eid(EID_A), 0, LoopbackSender::new(&out_a)); + let mut b: Server<_, 16> = Server::new(Eid(EID_B), 0, LoopbackSender::new(&out_b)); + + let listener = b.listener(MSG_TYPE).expect("B binds a listener"); + let req = a.req(EID_B).expect("A opens a request to B"); + a.send(Some(req), MSG_TYPE, Some(EID_B), None, false, b"ping") + .expect("A sends"); + transfer(&out_a, &mut b); + + let mut buf = [0u8; LOOPBACK_MTU]; + let meta = b + .try_recv(listener, &mut buf) + .expect("B receives the request"); + + // A response carries no handle and reuses the request's tag. + b.send( + None, + MSG_TYPE, + Some(EID_A), + Some(meta.msg_tag), + false, + b"pong", + ) + .expect("B replies"); + assert!(transfer(&out_b, &mut a) > 0, "the reply did not cross"); + + let mut reply = [0u8; LOOPBACK_MTU]; + a.try_recv(req, &mut reply).expect("A receives the reply"); + assert_eq!(&reply[..4], b"pong"); +}