Re: [PATCH v2 3/6] automation/qtb: add Python QTB framework with the console-test type

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 27, 2026 at 11:42:49AM +0200, Baptiste Le Duc wrote:
> diff --git a/automation/scripts/qemu_smoke_riscv64.py 
> b/automation/scripts/qemu_smoke_riscv64.py
> new file mode 100755
> index 0000000000..f338fafcc2
> --- /dev/null
> +++ b/automation/scripts/qemu_smoke_riscv64.py
> @@ -0,0 +1,122 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""CLI launcher for the qtb riscv64 dom0less tests.
> +
> +The test type comes first (e.g. `console-test`), then a command. Each type
> +reads its own config file, which ships with the type.
> +
> +Commands:
> +    list  Print every test the type defines in the config, then exit.
> +    run   Boot one test's machine under QEMU and drive it to a pass/fail
> +          verdict.
> +
> +Usage:
> +    ./qemu_smoke_riscv64.py console-test list
> +    ./qemu_smoke_riscv64.py console-test run 
> dom0less-1smp-0domu-1vcpu-aplic-imsic-null

Can we change the usage a bit?
I would put `list` and `run` (the command) as first possitionnal
argument.

`list` would list all types of tests, if alone on the command line.
And `list console-test` would list all tests associated with this kind.

As for the comment of this file, it doesn't feel useful as it is. It's
currently a very verbose way to try to explain how to use the command
line, it doesn't explain everything, "usage" is actually just few
"examples", and I don't think one would look here to find out how to use
it, when running the script with `-h` can print something more standard.

I guess here you could explain that tests from a config.yaml will be run
under qemu, with different parameter for the machine and different tests
to run.

> +"""
> +
> +from __future__ import annotations
> +
> +import argparse
> +import logging
> +import sys
> +from collections.abc import Sequence
> +from traceback import extract_tb, format_exc
> +
> +from qtb.riscv import RiscvQtbTest, TEST_TYPES, RiscvTestMachine, 
> cleanup_temp_dir
> +
> +logger = logging.getLogger(__name__)
> +
> +
> +def _run_test(test: RiscvQtbTest, log_dir: str | None) -> int:
> +    """Compile the machine's device trees, boot it, and run the test."""
> +    vm = RiscvTestMachine(test.machine, timeout=test.timeout, 
> log_dir=log_dir)

Naming that `vm` might be a be counter intuitive. Especially if you
intend to have nested VM later. My guess, `host` or `machine` would be a
bit better.


> +    try:
> +        with vm:
> +            vm.launch()
> +            test.run(vm)
> +    except Exception as exc:
> +        print(
> +            f"FAIL: {test.name}: {type(exc).__name__}: {exc}",
> +            file=sys.stderr,
> +            flush=True,
> +        )
> +        return 1
> +
> +    print(f"PASS: {test.name}", flush=True)
> +    return 0
> +
> +
> +def main(argv: Sequence[str] | None = None) -> int:
> +    ns = setup_parser().parse_args(argv)
> +    logging.basicConfig(
> +        level=logging.DEBUG if ns.verbose else logging.WARNING, 
> format="%(message)s"
> +    )
> +    try:
> +        return ns.func(ns)
> +    except Exception as exc:
> +        logger.debug(format_exc())
> +        frame = extract_tb(exc.__traceback__)[-1]
> +        print(f"{frame.filename}:{frame.lineno}: {exc}")
> +        return 2
> +    finally:
> +        cleanup_temp_dir()

This cleanup_temp_dir() is out of place, when `main()` doesn't create
this temporary dir. Instead you could have the main prepare an
environment, create temporary directories, and give that to the test
classes, and that in turn would justify to do the cleanup here.

> +
> +
> +if __name__ == "__main__":
> +    sys.exit(main())
> diff --git a/automation/scripts/qtb/riscv/config.py 
> b/automation/scripts/qtb/riscv/config.py
> new file mode 100644
> index 0000000000..b5068d6bfb
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/config.py
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""YAML config parser for qtb-based Xen riscv64 tests.
> +
> +Parsing steps:
> +    - validate the YAML
> +    - build the dataclasses: the host device tree is compiled on first use
> +      of MachineConfig.dt
> +"""
> +
> +from __future__ import annotations
> +
> +import functools
> +import inspect
> +from dataclasses import dataclass
> +from functools import cached_property
> +from pathlib import Path
> +
> +import yaml
> +
> +from .paths import resolve_binary, resolve_path
> +from .xen_dt import DeviceTree, build_xen_device_tree
> +
> +XEN_MMU_TYPE_DEFAULT: str = "sv48"
> +XEN_BOOTARGS_DEFAULT: str = ""
> +
> +MACHINE_MEMORY: int = 2048

That feels like a lot, how about just 512MB? Or do you need 2GB for the
tests? And why isn't it called *_DEFAULT like other parameters?

> +MACHINE_INTERRUPT_CONTROLLER: str = "aplic-imsic"

This isn't used here, in `config.py`. Either
MACHINE_INTERRUPT_CONTROLLER doesn't need to exist, or it should be
added to MachineConfig.

> +
> +
> +@dataclass(frozen=True)
> +class MachineConfig:
> +    """One named machine: the test-agnostic description of what to boot.
> +
> +    A machine is reusable across test types; a test (see RiscvQtbTest
> +    subclasses) picks a machine by name and layers its own parameters on top.
> +    """
> +
> +    name: str
> +    pcpu: int
> +    binaries: BinariesConfig
> +    mmu_type: str  # Xen (host) MMU type, injected into the host dts cpus.
> +    xen_bootargs: str
> +
> +    @classmethod
> +    def from_config(cls, file_name: str, machine: str) -> MachineConfig:
> +        """Build only the single named machine from the catalog at `path`.
> +
> +        A test run boots one machine, so there is no need to construct the
> +        whole catalog: parse the YAML, validate the shared binaries, and
> +        build just the requested entry.
> +        """
> +        fpath: Path = resolve_path(file_name)
> +        raw: dict = yaml.safe_load(fpath.read_text())
> +
> +        required = ("binaries", "machines")
> +        missing = [k for k in required if k not in raw]
> +        if missing:
> +            raise ValueError(f"Global config {file_name} missing keys: 
> {missing}")
> +
> +        binaries: BinariesConfig = _parse_binaries(raw["binaries"])
> +
> +        machines = raw["machines"]
> +        if machine not in machines:
> +            known = ", ".join(sorted(machines)) or "(none)"
> +            raise ValueError(f"unknown machine {machine!r}; known machines: 
> {known}")
> +
> +        return _parse_machine(machines[machine], binaries, machine)
> +
> +    @cached_property
> +    def dt(self) -> DeviceTree:
> +        """Host device tree, compiled on first use."""
> +        return build_xen_device_tree(self)
> diff --git a/automation/scripts/qtb/riscv/config.yaml 
> b/automation/scripts/qtb/riscv/config.yaml
> new file mode 100644
> index 0000000000..94897827b7
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/config.yaml
> @@ -0,0 +1,17 @@
> +# Shared config for the qtb riscv64 tests
> +#
> +# A machine is the test-agnostic description of what to boot (cpus, Xen 
> command
> +# line). Test YAMLs (e.g. console-test.yaml) pick a machine by name and layer
> +# their own parameters on top.
> +#
> +# Path resolution (see paths.py): `binaries:` entries resolve against
> +# $QTB_BINARIES_DIR env var if defined else `binaries`. Absolute paths used
> +# as-is.
> +
> +binaries:
> +  xen:      xen
> +
> +machines:
> +  dom0less-1smp-0domu-1vcpu-aplic-imsic-null:
> +    xen_bootargs: "sched=null"

There's a document describing the Xen command line in the xen tree. It
is composed of parameters and options. Also `xl info` calls it
`xen_commandline`. While I understand "bootargs", can we rename that to
something more comment in this context? That is "xen_cmdline" or
"xen_commandline".


> diff --git a/automation/scripts/qtb/riscv/console_test/console-test.yaml 
> b/automation/scripts/qtb/riscv/console_test/console-test.yaml
> new file mode 100644
> index 0000000000..cec0edc510
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/console_test/console-test.yaml
> @@ -0,0 +1,18 @@

Would it be possible to merge this config file into the main one? That
is "config.yaml". I think having a single config file for the whole test
suite would be easier to manage going forward. It would be easier to
look at a single files instead of looking for files in multiple
locations. Then we can just tell the main script which config file to
use, and the different class of tests won't have to guess where their
config file is.


> +# Console string expectation test (run with: qemu_smoke_riscv64.py 
> console-test run <test>).
> +#
> +# Each test uses a machine from config.yaml and maps a console index to the 
> list
> +# of string(s) expected on that console: 0 is Xen's own console. The runner 
> boots
> +# the machine and asserts each string is printed within the timeout. Nothing 
> is
> +# injected.
> +#
> +# Test options:
> +#   timeout: int  # timeout between each string match (in seconds)
> +#   attempts: int # number of tries for a wait before failing. Default 3 
> (min = 1)
> +
> +machine_catalog: config.yaml
> +
> +tests:
> +  dom0less-1smp-0domu-1vcpu-aplic-imsic-null:
> +    machine: dom0less-1smp-0domu-1vcpu-aplic-imsic-null
> +    expect:
> +      0: ["All set up"]

I wonder if configuring the machine here instead of in a separated array
wouldn't make more sense. The string
"dom0less-1smp-0domu-1vcpu-aplic-imsic-null" is duplicated three time.
If instead we had:
    
    tests:
      dom0less-1smp-0domu-1vcpu-aplic-imsic-null:
        machine:
            pcpu: 1
            xen_bootargs: "sched=null"
        expect:
          0: ["All set up"]

> diff --git a/automation/scripts/qtb/riscv/console_test/console_test.py 
> b/automation/scripts/qtb/riscv/console_test/console_test.py
> new file mode 100644
> index 0000000000..95540fd388
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/console_test/console_test.py
> @@ -0,0 +1,145 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""Console string expectation test (`type: console-test`).
> +
> +Per console: assert each expected string is printed within the timeout.
> +Nothing is injected; this only watches console output.
> +
> +YAML (console-test.yaml): each test names the machine it boots (tests may
> +share one) and maps a console index to the string(s) expected on it: 0 is
> +Xen's own console.
> +
> +    machine_catalog: config.yaml
> +    tests:
> +      dom0less-1smp-0domu-1vcpu-aplic-imsic-null:  # test name (run 
> positional)
> +        machine: dom0less-1smp-0domu-1vcpu-aplic-imsic-null  # from 
> config.yaml
> +        expect:                                    # console index -> 
> string(s)
> +          0: [All set up]
> +"""
> +
> +from __future__ import annotations
> +
> +import logging
> +from typing import ClassVar
> +
> +import pexpect
> +import yaml
> +
> +from ..paths import resolve_path
> +from ..config import MachineConfig, required_keys
> +from ..qtb_test import TIMEOUT_DEFAULT, RiscvQtbTest
> +from ..machine import RiscvTestMachine
> +
> +logger = logging.getLogger(__name__)
> +
> +TYPE_ID: str = "console-test"
> +
> +CONFIG_FILE_DEFAULT: str = "console_test/console-test.yaml"
> +DESCRIPTION_DEFAULT: str = "Assert expected string(s) are printed on the Xen 
> console"

"DESCRIPTION_DEFAULT", do you mean it can be changed?

Just curious, is there a reason to have TYPE_ID and DESCRIPTION_DEFAULT
instead of adding the string directly in the class? (That is not using
TYPE_ID at all).


> +ATTEMPTS_DEFAULT: int = 3
> +
> +XEN_CONS_IDX: int = 0

It could be useful to have a description for XEN_CONS_IDX.

> +
> +
> +class ConsoleTest(RiscvQtbTest):
> +    type_id: ClassVar[str] = TYPE_ID
> +    description: ClassVar[str] = DESCRIPTION_DEFAULT
> +    config_file: ClassVar[str] = CONFIG_FILE_DEFAULT
> +
> +    def __init__(self, raw: dict, test_name: str) -> None:
> +        self.name, self.data, self.machine = self._parse_test_cfg(raw, 
> test_name)
> +
> +        self.expect = self.data["expect"]  # expected console string
> +
> +        self.timeout = int(self.data.get("timeout", TIMEOUT_DEFAULT))
> +        if self.timeout < 1:
> +            raise ValueError("timeout < 1, must be at least 1")
> +
> +        self.attempts = int(self.data.get("attempts", ATTEMPTS_DEFAULT))
> +        if self.attempts < 1:
> +            raise ValueError("attempts < 1, must be at least 1")
> +
> +    @staticmethod
> +    def _load_yaml(path) -> dict:
> +        return yaml.safe_load(resolve_path(path).read_text())

This method looks out-of place, it doesn't interact with ConsoleTest at
all. It could be a generic util/tool. Or not exist a all and let the
main create a `dict` from a yaml and git that `dict` to ConsoleTest to
initialise itself, which is already how ConsoleTest is initialised
curently anyway.

> +
> +    @classmethod
> +    def from_config(cls, config_file: str, test_name: str) -> ConsoleTest:
> +        return cls(cls._load_yaml(config_file), test_name)
> +
> +    @staticmethod
> +    @required_keys("test_data", {"machine", "expect"})
> +    def _parse_test_data(
> +        machine_catalog: str, test_data: dict, test_name: str
> +    ) -> tuple[str, dict, MachineConfig]:
> +        """Parse test data dictionary"""
> +        test_machine = MachineConfig.from_config(machine_catalog, 
> test_data["machine"])
> +
> +        def invalid(why: str) -> ValueError:
> +            return ValueError(
> +                f"test {test_name!r}: {why}; expected "
> +                f"{{{XEN_CONS_IDX}: ['str1', 'str2', ...]}}"
> +            )
> +
> +        expect = test_data["expect"]
> +        if not isinstance(expect, dict) or expect.keys() != {XEN_CONS_IDX}:
> +            raise invalid(
> +                f"expect must map console index {XEN_CONS_IDX} (Xen's own "
> +                f"console, the only one) and nothing else, got {expect!r}"
> +            )
> +        strings = expect[XEN_CONS_IDX]
> +        if not isinstance(strings, list):
> +            raise invalid(
> +                f"{ConsoleTest.config_file} expects a list of string(s), "
> +                f"got {type(strings).__name__}"
> +            )
> +        if not strings:
> +            raise invalid("Xen has no expected string")
> +        if not all(isinstance(s, str) and s for s in strings):
> +            raise invalid(f"Xen expects non-empty strings, got {strings!r}")
> +        return (test_name, test_data, test_machine)
> +
> +    @staticmethod
> +    @required_keys("raw", {"machine_catalog", "tests"})
> +    def _parse_test_cfg(raw: dict, test_name: str) -> tuple[str, dict, 
> MachineConfig]:
> +        """Read the config: return the named test dict and its machine."""
> +
> +        tests = raw["tests"]
> +        if test_name not in tests:
> +            known = ", ".join(sorted(tests)) or "(none)"
> +            raise ValueError(f"unknown test {test_name!r}; known tests: 
> {known}")
> +
> +        test_data = tests[test_name]
> +        return ConsoleTest._parse_test_data(
> +            raw["machine_catalog"], test_data, test_name
> +        )
> +
> +    @staticmethod
> +    def list_tests(config_file: str) -> list[str]:
> +        tests = ConsoleTest._load_yaml(config_file).get("tests")
> +        if not tests:
> +            logger.warning("no 'tests' key found in %s", config_file)
> +            return []
> +        return list(tests)
> +
> +    @staticmethod
> +    def _console(vm: RiscvTestMachine):
> +        """Xen's own console (con0)."""
> +        if vm.console is None:
> +            raise RuntimeError("Xen console not wired up, machine not 
> launched?")
> +        return vm.console
> +
> +    def run(self, vm: RiscvTestMachine) -> None:
> +        cons = self._console(vm)
> +        for strings in self.expect.values():
> +            for s in strings:
> +                self._expect_string(cons, s)
> +
> +    def _expect_string(self, cons, expected: str) -> None:

Shouldn't `cons` have a type?

> +        """Wait for `expected` on the console, retrying on timeout."""
> +        for attempt in range(self.attempts):
> +            try:
> +                cons.expect_exact(expected, timeout=self.timeout)
> +                return
> +            except pexpect.TIMEOUT:
> +                if attempt == self.attempts - 1:
> +                    raise

I don't understand the point of this function.
Why to we retry the `grep` on a timeout? Why is the original asked
timeout multiplied by "attempts" ? (which is effectively what is
happening here, more or less)

Also, raising `pexpect.TIMEOUT` might not be the exception to throw. A
new set of exception dedicated for the Test* classes might be better,
and could easily been catch in the main function that run all the tests
(without having to import unrelated module like `pexpect`).

> diff --git a/automation/scripts/qtb/riscv/dt.py 
> b/automation/scripts/qtb/riscv/dt.py
> new file mode 100644
> index 0000000000..f0376979e5
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/dt.py
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""Device tree (DT) handling: compile a .dts source into a .dtb"""
> +
> +from __future__ import annotations
> +
> +import logging
> +import subprocess
> +from pathlib import Path
> +
> +logger = logging.getLogger(__name__)
> +
> +
> +def _compile_dts(src: Path, out: Path):
> +    """Run dtc to compile .dts `src` into the .dtb file at `out`"""
> +    try:
> +        p = subprocess.run(
> +            ["dtc", "-I", "dts", "-O", "dtb", "-o", str(out), str(src)],
> +            check=True,
> +            capture_output=True,
> +            text=True,
> +        )
> +        logger.debug("dtc %s: stdout: %s stderr: %s", src, p.stdout, 
> p.stderr)
> +
> +    except FileNotFoundError as e:
> +        raise RuntimeError("dtc not found in PATH; install 
> device-tree-compiler") from e
> +    except subprocess.CalledProcessError as e:
> +        raise RuntimeError(
> +            f"dtc failed on {str(src)!r} (exit {e.returncode}):\n"
> +            f" stdout: {e.stdout}\n stderr: {e.stderr}"
> +        ) from e
> +
> +
> +def compile_to_dtb(src: Path, out: Path) -> Path:
> +    """
> +    Compile a .dts source to a .dtb under out dir and return the .dtb path.
> +
> +    Raises FileNotFoundError if `src` or `out` don't exist.
> +    """
> +    if not src.exists():
> +        raise FileNotFoundError(
> +            f"Device tree source {str(src)!r} not found"
> +        )
> +
> +    if not out.exists():
> +        raise FileNotFoundError(f"Device Tree output dir: {out} doesn't 
> exist")
> +
> +    dtb = out / (src.stem + ".dtb")
> +    _compile_dts(src, dtb)
> +    return dtb
> +
> +
> +def write_dts(text: str, name: str, out_dir: Path) -> Path:
> +    """Write generated .dts text to <out_dir>/<name>.dts; return its path."""
> +    src = out_dir / f"{name}.dts"
> +    src.parent.mkdir(parents=True, exist_ok=True)
> +    src.write_text(text)
> +    return src

I don't like this function, it doesn't do anything related to device
tree. It's just a generic that allow to write any text to a file in any
directory, even non existing ones, as long as the filename ends with ."dts".
I think whatever is calling this function should deal with the filename
and the directory. And that caller should probably write to an existing
directory.


> diff --git a/automation/scripts/qtb/riscv/machine.py 
> b/automation/scripts/qtb/riscv/machine.py
> new file mode 100644
> index 0000000000..74a05dfc82
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/machine.py
> @@ -0,0 +1,57 @@
> +
> +    def _resolve_binary(self) -> str:
> +        return str(resolve_from_path(self.qemu_bin))

Nothing is using that function. (In that patch at least)

> diff --git a/automation/scripts/qtb/riscv/paths.py 
> b/automation/scripts/qtb/riscv/paths.py
> new file mode 100644
> index 0000000000..237f1bf502
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/paths.py

I don't like this module. Maybe some of those functions here could be
move to the qtb package instead, and rename the module "utils" or
"tools" or something else generic. Because nothing here is riscv
specific.

> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""Path helpers: resolve pkg-relative paths."""
> +
> +from __future__ import annotations
> +
> +import os
> +import shutil
> +import tempfile
> +from functools import lru_cache
> +from pathlib import Path
> +
> +# Every relative path in this module is resolved against this base.
> +_BASE = Path(__file__).resolve().parent
> +
> +# Build artifacts, overridable for CI.
> +_BINARIES_BASE = Path(os.environ.get("QTB_BINARIES_DIR") or _BASE / 
> "binaries")

_BINARIES_BASE is misplaced I think, as it's buried deep into the
modules. I would rather have the location of the "binaries" been set
from the start by the main script, then give the path to the classes
that needs it.

Also, having the "binaries" directory location in a envvar is a nice
idea, but can it be renamed to be more generic for the CI ?
Something like "XEN_CI_BINARIES_DIR". Because this is a variable that
contained shared information between the jobs that build xen and other,
and jobs that run tests.

> +
> +
> +@lru_cache(maxsize=1)
> +def _temp_dir_handle() -> tempfile.TemporaryDirectory:
> +    return tempfile.TemporaryDirectory(prefix="qtb-")
> +
> +
> +def temp_dir() -> Path:
> +    """Process-wide scratch dir for generated/compiled artifacts 
> (singleton)."""
> +    return Path(_temp_dir_handle().name)
> +
> +
> +def cleanup_temp_dir() -> None:
> +    """Remove the scratch dir, if one was created, and clear the cache."""
> +    if _temp_dir_handle.cache_info().currsize:
> +        _temp_dir_handle().cleanup()
> +        _temp_dir_handle.cache_clear()
> +
> +
> +def resolve_path(file_name: str) -> Path:
> +    """Resolve `file_name` against _BASE, absolute paths pass through."""
> +    return _resolve_under(file_name, _BASE)
> +
> +
> +def resolve_binary(file_name: str) -> Path:
> +    """Resolve a build artifact against _BINARIES_BASE, absolute paths pass 
> through."""
> +    return _resolve_under(file_name, _BINARIES_BASE)
> +
> +
> +def resolve_from_path(file_name: str) -> Path:
> +    """Resolve an executable on $PATH, absolute paths pass through."""
> +    path = shutil.which(file_name)
> +    if not path:
> +        raise FileNotFoundError(f"cannot resolve {file_name!r}: not an 
> executable on $PATH")
> +    return Path(path)
> +
> +
> +def _resolve_under(file_name: str, base: Path) -> Path:
> +    """Resolve `file_name` against `base`, absolute paths pass through."""
> +    p = Path(file_name)
> +    path = p if p.is_absolute() else base / p
> +    if not path.exists():
> +        raise FileNotFoundError(f"cannot resolve {str(path)!r}: does not 
> exist")
> +    return path
> diff --git a/automation/scripts/qtb/riscv/qtb_test.py 
> b/automation/scripts/qtb/riscv/qtb_test.py
> new file mode 100644
> index 0000000000..872fbc2fa9
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/qtb_test.py
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""Abstract base for the qtb riscv64 test types.
> +
> +A test type is a RiscvQtbTest subclass owning a config file that describes 
> its
> +tests, each bound to a machine from the shared catalog.
> +"""
> +
> +from __future__ import annotations
> +
> +from abc import ABC, abstractmethod
> +from typing import ClassVar
> +
> +from .config import MachineConfig
> +from .machine import RiscvTestMachine
> +
> +# Default per-test timeout (seconds)
> +TIMEOUT_DEFAULT: int = 120
> +
> +
> +class RiscvQtbTest(ABC):
> +    """One runnable test bound to the machine it boots.
> +
> +    A subclass sets `type_id`, `description`, and `config_file`, and 
> implements
> +    `from_config` to parse its config file, `list_tests` to enumerate the 
> tests
> +    it declares, and `run` to drive the test logic.
> +    """
> +
> +    # Set by each concrete subclass.
> +    type_id: ClassVar[str] = ""
> +    # One-line summary of what the type does, shown in the CLI help.
> +    description: ClassVar[str] = ""
> +    # Config file the type reads its tests from, resolved pkg-relative.
> +    config_file: ClassVar[str] = ""
> +
> +    # Set by the subclass parser.
> +    name: str
> +    data: dict
> +    machine: MachineConfig
> +    timeout: int = TIMEOUT_DEFAULT
> +
> +    @classmethod
> +    @abstractmethod
> +    def from_config(cls, config_file: str, test_name: str) -> RiscvQtbTest:
> +        """Build the test named `test_name` from `config_file`."""
> +
> +    @staticmethod
> +    @abstractmethod
> +    def list_tests(config_file: str) -> list[str]:
> +        """Return the names of every test declared in the config file."""

It's nice to have an abstract class, but can't we implement some
methods like `list_tests()` here? Otherwise, it's likely that the same
exact function is going to be duplicated in every kind of tests.

> +    @abstractmethod
> +    def run(self, vm: RiscvTestMachine) -> None:
> +        """Drive the running machine and assert the expected result."""
> diff --git a/automation/scripts/qtb/riscv/xen_dt.py 
> b/automation/scripts/qtb/riscv/xen_dt.py
> new file mode 100644
> index 0000000000..8881b01b36
> --- /dev/null
> +++ b/automation/scripts/qtb/riscv/xen_dt.py
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +"""Build the Xen host device tree for a MachineConfig.

It feels like both module `dt.py` and `xen_dt.py` could be in the same
module, with something that takes a templates and parameters, and
generate a DTB.

But, seen that xen_dt access directly MachineConfig, maybe all the
functions dealing with the device tree could be in the same file as
the one declaring MachineConfig, or .

> +
> +The tree is rendered from its Jinja2 template (dts/qemu-host.dts.j2), which
> +takes the hart count, the Xen MMU type and the Xen command line, then 
> compiled
> +to a DTB with dtc.
> +"""
> +
> +from __future__ import annotations
> +
> +from dataclasses import dataclass
> +from functools import lru_cache
> +from pathlib import Path
> +from typing import TYPE_CHECKING
> +from jinja2 import Environment, FileSystemLoader
> +
> +from .paths import resolve_path, temp_dir
> +from .dt import compile_to_dtb, write_dts
> +
> +if TYPE_CHECKING:  # config imports this module, so only import it for 
> typing.
> +    from .config import MachineConfig
> +
> +# Directory holding the Jinja2 platform device tree templates.
> +_DTS_DIR = "dts"

Instead of having this hidden variable, how about making it part of the
config file? With full path, that is probably relative to the config
file location.

> +
> +
> +@dataclass(frozen=True)
> +class DeviceTree:
> +    """Compiled device trees for one machine launch."""
> +
> +    dts: Path
> +    dtb: Path

Is there any reason to keep the path to the dts? It doesn't seems to be
ever used.

> +
> +
> +@lru_cache(maxsize=1)
> +def _env() -> Environment:
> +    return Environment(
> +        loader=FileSystemLoader(resolve_path(_DTS_DIR)),
> +        keep_trailing_newline=True,
> +    )
> +
> +
> +def _render_xen_dts(machine: MachineConfig) -> str:
> +    """Render the Xen host device tree source text for `machine`."""
> +    tmpl = _env().get_template("qemu-host.dts.j2")

Same remark as for _DTS_DIR, "qemu-host.dts.j2" could just be part of
the path to the dts on a config file.

> +    return tmpl.render(
> +        ncpus=machine.pcpu,
> +        mmu_type=machine.mmu_type,
> +        xen_bootargs=machine.xen_bootargs,
> +    )
> +
> +
> +def build_xen_device_tree(machine: MachineConfig) -> DeviceTree:
> +    """Compile `machine` device tree into the shared scratch dir."""
> +    out = temp_dir()
> +    dts: Path = write_dts(_render_xen_dts(machine), machine.name, out)
> +    dtb: Path = compile_to_dtb(dts, out)
> +    return DeviceTree(dts=dts, dtb=dtb)
> 


There's something I haven't really talked about yet, and maybe it's too
much changes. I've had in mind that the main script
"qemu_smoke_riscv64.py" could run all the available tests (or even just
all the tests of a single kind/type) and report the result. This would
change a bit how the different Test classes are instantiated.

It might actually be better for several reason to run all tests at once.
- It's a single job in the pipeline.
    - which mean, all the time spent to start a new container, cloning
      the xen tree, downloading the artefacts, cleanup; would be done a
      single time, and all those step likely takes a lot more time that
      the execution of the test.
    - adding a test is easier, there's a single file to edit, and it's
      automatically picked up. No need to also edit the pipeline as well.
    - well, it could be a single job per kind of test, or a single job
      overall. But one job per kind of test is probably better.
- It's easier to run the all the tests locally, just a single command.

Having a single job in the pipeline doesn't prevent from having the
result of a single test easily available, for example all the
"*-tools-test-*" jobs generate a `tests-junit.xml` and you can see the
result in the gitlab WebUI, for example:
https://gitlab.com/xen-project/hardware/xen/-/pipelines/2994120991/test_report


I hope all my comments make sense, and would make the framework better.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech