vng, run: support specifying / autodetecting disk emulation options

This implements support for various disk topology and I/O driver options
for both `--disk` and `--blk-disk` arguments.

Options are accepted as a comma-separated list after the image file path
e.g., `--disk /path/to/file,format=qcow2,...`.

In addition to general QEMU options (format=), I/O driver options
(cache=, aio=, discard=, detect-zeroes=, queues=) and topology options
(log-sec=, phy-sec=, min-io=, opt-io=, disc-gran=) a "topology="
metaoption is accepted to pass through host device queue limits.

The names for these options were chosen to match `lsblk` columns
rather than QEMU's own -drive/-device options, because the latter are
underdocumented and non-uniform.

Signed-off-by: Ivan Shapovalov <intelfx@intelfx.name>

Author: intelfx

virtme/commands/run.py

@@ -20,6 +20,7 @@
 import tempfile
 import termios
 from base64 import b64encode
+from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
 from shutil import which
@@ -36,6 +37,7 @@
     VIRTME_SSH_HOSTNAME_CID_SEPARATORS,
     get_conf,
     scsi_device_id,
+    strtobool,
 )
 
 from .. import architectures, mkinitramfs, modfinder, qemu_helpers, resources, virtmods
@@ -885,21 +887,148 @@ def quote_karg(arg: str) -> str:
 class DiskArg:
     name: str
     path: str
+    opts: dict[str, str]
+
+    _OPTS_HELP = {
+        # meta parameters
+        "topology": ("bool", "Forward host device topology (sector and I/O sizes)"),
+        "iothread": ("bool", "Create a dedicated I/O thread for the disk"),
+        # general format parameters
+        "format": ("str", "Disk image format (raw|qcow2)"),
+        # I/O driver parameters
+        "cache": ("str", "Cache mode (none|writeback|writethrough|directsync|unsafe)"),
+        "aio": ("str", "Asynchronous I/O mode (native|threads|io_uring)"),
+        "discard": (
+            "bool",
+            "Pass through TRIM/UNMAP requests (true=unmap, false=ignore)",
+        ),
+        "detect-zeroes": ("bool", "Detect all-zero writes (true=on/unmap, false=off)"),
+        "queues": ("int", "Number of I/O queues"),
+        # topology parameters
+        # "alignment": ("bytes", "Block alignment offset"),
+        "log-sec": ("bytes", "Logical (LBA) sector size (typically 512 or 4096)"),
+        "phy-sec": ("bytes", "Physical (underlying) sector size (>=log-sec)"),
+        "min-io": ("bytes", "Minimum I/O request size"),
+        "opt-io": ("bytes", "Optimal I/O request size"),
+        "rota": ("bool", "Device is rotational"),
+        # "wzeroes": ("bytes", "Maximum WRITE ZEROES request size"),
+        # "disc-aln": ("bytes", "TRIM/UNMAP alignment offset"),
+        "disc-gran": ("bytes", "TRIM/UNMAP request granularity"),
+        # "disc-max": ("bytes", "Maximum TRIM/UNMAP request size"),
+        # "disc-zero": ("bool", "TRIM/UNMAP zeroes data"),
+    }
+
+    def __post_init__(self):
+        if self.pop_opt("topology", strtobool, False):
+            self.opts = self.topology() | self.opts
+
+    def get_opt(
+        self, name: str, parser: Callable[[str], Any] = str, default: Any = None
+    ) -> Any:
+        opt = self.opts.get(name, None)
+        return parser(opt) if opt is not None else default
+
+    def pop_opt(
+        self, name: str, parser: Callable[[str], Any] = str, default: Any = None
+    ) -> Any:
+        opt = self.opts.pop(name, None)
+        return parser(opt) if opt is not None else default
+
+    def pop_opt_qemu(
+        self,
+        name: str,
+        default: Any = None,
+        *,
+        parser: Callable[[str], Any] = str,
+        dest: str | None = None,
+    ) -> str | None:
+        opt = self.pop_opt(name, parser, default)
+        # return DiskArg.qemu_opt(name=qemu if qemu is not None else name, value=opt)
+        if opt is None:
+            return None
+        if isinstance(opt, bool):
+            opt = "on" if opt else "off"
+        return f"{dest if dest is not None else name}={opt}"
+
+    def topology(self) -> dict[str, str]:
+        # Get the real device name (handles symlinks like /dev/mapper -> /dev/dm-X)
+        real_path = os.path.realpath(self.path, strict=True)
+        dev_name = os.path.basename(real_path)
+        sys_base = Path(f"/sys/block/{dev_name}")
+
+        attributes = {
+            # 'alignment': ('alignment_offset', int),
+            "log-sec": ("queue/logical_block_size", int),
+            "phy-sec": ("queue/physical_block_size", int),
+            "min-io": ("queue/minimum_io_size", int),
+            "opt-io": ("queue/optimal_io_size", int),
+            "rota": ("queue/rotational", bool),
+            # 'wzeroes': ('queue/write_zeroes_max_bytes', int),
+            # 'disc-aln': ('discard_alignment', int),
+            "disc-gran": ("queue/discard_granularity", int),
+            # 'disc-max': ('queue/discard_max_bytes', int),
+            # 'disc-zero': ('queue/discard_zeroes_data', bool),
+        }
+
+        result = {}
+        for key, (path, parser) in attributes.items():
+            try:
+                value = sys_base.joinpath(path).read_text().strip()
+                if parser is int:
+                    parsed = parser(value)
+                    if parsed <= 0:
+                        continue
+                result[key] = value
+            except FileNotFoundError:
+                pass
+            except ValueError:
+                pass
+        return result
 
     # Validate name=path arguments from --disk and --blk-disk
     @classmethod
     def parse(cls, func: str, arg: str) -> "DiskArg":
-        name, sep, fn = arg.partition("=")
+        items = arg.split(",")
+
+        namefile = items[0]
+        extra = items[1:]
+
+        name, sep, fn = namefile.partition("=")
         if not (name and sep and fn):
             arg_fail(f"invalid argument to {func}: {arg}")
         if "=" in fn or "," in fn:
             arg_fail(f"{func} filenames cannot contain '=' or ',': {fn}")
         if "=" in name or "," in name:
             arg_fail(f"{func} device names cannot contain '=' or ',': {name}")
 
+        opts = dict()
+        for i in extra:
+            key, sep, value = i.partition("=")
+            if not key:
+                arg_fail(f"invalid argument to {func}: {arg}")
+            if sep:
+                opts[key] = value
+            else:
+                opts[key] = "1"
+
+        if "help" in opts:
+            print(
+                "\n".join(
+                    [
+                        f"Possible {func} options:",
+                    ]
+                    + [
+                        "{:<20} {}".format(f"{key}=({typ})", value)
+                        for key, (typ, value) in DiskArg._OPTS_HELP.items()
+                    ]
+                )
+            )
+            sys.exit(0)
+
         return cls(
             name=name,
             path=fn,
+            opts=opts,
         )
 
 
@@ -1576,6 +1705,8 @@ def do_it() -> int:
     if args.cpus:
         qemuargs.extend(["-smp", args.cpus])
 
+    iothread_index = 0
+
     if args.blk_disk:
         for i, d in enumerate(args.blk_disk):
             driveid = f"blk-disk{i}"
@@ -1585,14 +1716,64 @@ def do_it() -> int:
                 "if=none",
                 f"id={driveid}",
                 f"file={disk.path}",
-                "format=raw",
             ]
             device_opts = [
                 arch.virtio_dev_type("blk"),
                 f"drive={driveid}",
                 f"serial={disk.name}",
             ]
 
+            # we need those parameters multiple times
+            discard = disk.pop_opt("discard", parser=strtobool, default=None)
+            detect_zeroes = disk.pop_opt(
+                "detect-zeroes", parser=strtobool, default=None
+            )
+            # we need this parameter both to transform other parameters and as itself later
+            # log_sec = disk.get_opt("log-sec", parser=int, default=512)
+
+            drive_opts.extend(
+                [
+                    disk.pop_opt_qemu("format", "raw"),
+                    disk.pop_opt_qemu("cache", None),
+                    disk.pop_opt_qemu("aio", None),
+                    f"discard={'unmap' if discard else 'ignore'}"
+                    if discard is not None
+                    else None,
+                    f"detect-zeroes={('unmap' if discard else 'on') if detect_zeroes else 'off'}"
+                    if detect_zeroes is not None
+                    else None,
+                ]
+            )
+
+            device_opts.extend(
+                [
+                    f"discard={'on' if discard else 'off'}"
+                    if discard is not None
+                    else None,
+                    disk.pop_opt_qemu("disc-gran", dest="discard_granularity"),
+                    disk.pop_opt_qemu("log-sec", dest="logical_block_size"),
+                    disk.pop_opt_qemu("phy-sec", dest="physical_block_size"),
+                    # disk.pop_qemu("disc-max", dest="max-discard-sectors", parser=lambda arg: int(arg) / log_sec),
+                    # disk.pop_qemu("wzeroes", dest="max-write-zeroes-sectors", parser=lambda arg: int(arg) / log_sec),
+                    disk.pop_opt_qemu("min-io", dest="min_io_size"),
+                    disk.pop_opt_qemu("opt-io", dest="opt_io_size"),
+                    disk.pop_opt_qemu("queues", dest="num-queues"),
+                ]
+            )
+            # unused
+            disk.opts.pop("rota", None)
+
+            if disk.pop_opt("iothread", bool, False):
+                iothreadid = f"iothread{iothread_index}"
+                iothread_index += 1
+                qemuargs.extend(
+                    [
+                        "-object",
+                        f"iothread,id={iothreadid}",
+                    ]
+                )
+                device_opts.append(f"iothread={iothreadid}")
+
             qemuargs.extend(
                 [
                     "-drive",
@@ -1602,41 +1783,121 @@ def do_it() -> int:
                 ]
             )
 
-    if args.disk:
-        qemuargs.extend(["-device", "{},id=scsi".format(arch.virtio_dev_type("scsi"))])
+            # any options that were not consumed are errors
+            if disk.opts:
+                raise ValueError(
+                    f"invalid --disk parameter: {d!r}\n(keys were not consumed: {disk.opts.keys()})"
+                )
 
+    if args.disk:
         for i, d in enumerate(args.disk):
+            scsiid = f"scsi{i}"
             driveid = f"disk{i}"
             disk = DiskArg.parse("--disk", d)
 
             # scsi-hd.device_id= is normally defaulted to scsi-hd.serial=,
             # but it must not be longer than 20 characters
             device_id = scsi_device_id(disk.name, 20)
 
+            scsi_opts = [
+                arch.virtio_dev_type("scsi"),
+                f"id={scsiid}",
+            ]
             drive_opts = [
                 "if=none",
                 f"id={driveid}",
                 f"file={disk.path}",
-                "format=raw",
             ]
             device_opts = [
                 "scsi-hd",
                 f"drive={driveid}",
+                f"bus={scsiid}.0",
                 "vendor=virtme",
                 "product=disk",
                 f"serial={disk.name}",
                 f"device_id={device_id}" if device_id != disk.name else None,
             ]
 
+            # we need those parameters multiple times
+            discard = disk.pop_opt("discard", parser=strtobool, default=None)
+            detect_zeroes = disk.pop_opt(
+                "detect-zeroes", parser=strtobool, default=None
+            )
+            # we need this parameter both to transform other parameters and as itself later
+            log_sec = disk.get_opt("log-sec")
+
+            drive_opts.extend(
+                [
+                    disk.pop_opt_qemu("format", "raw"),
+                    disk.pop_opt_qemu("cache", None),
+                    disk.pop_opt_qemu("aio", None),
+                    f"discard={'unmap' if discard else 'ignore'}"
+                    if discard is not None
+                    else None,
+                    f"detect-zeroes={('unmap' if discard else 'on') if detect_zeroes else 'off'}"
+                    if detect_zeroes is not None
+                    else None,
+                ]
+            )
+
+            scsi_opts.extend(
+                [
+                    disk.pop_opt_qemu("queues", dest="num-queues"),
+                ]
+            )
+
+            device_opts.extend(
+                [
+                    disk.pop_opt_qemu("disc-gran", dest="discard_granularity"),
+                    disk.pop_opt_qemu("log-sec", dest="logical_block_size"),
+                    # convenience: QEMU does not automatically adjust physical_block_size
+                    # to be not less than logical_block_size (it errors out instead), so we do it here
+                    disk.pop_opt_qemu(
+                        "phy-sec", dest="physical_block_size", default=log_sec
+                    ),
+                    # disk.pop_qemu("disc-max", dest="max_unmap_size"),
+                    # disk.pop_qemu("wzeroes", dest="???"),
+                    disk.pop_opt_qemu("min-io", dest="min_io_size"),
+                    disk.pop_opt_qemu("opt-io", dest="opt_io_size"),
+                    # sic: set rotation_rate to "1" for non-rotating disks ("1" is a special value
+                    # that means "non-rotating medium"), but set to "0" for rotating disks
+                    # ("0" means "rotation rate not reported").
+                    disk.pop_opt_qemu(
+                        "rota",
+                        dest="rotation_rate",
+                        parser=lambda arg: "0" if strtobool(arg) else "1",
+                    ),
+                ]
+            )
+
+            if disk.pop_opt("iothread", bool, False):
+                iothreadid = f"iothread{iothread_index}"
+                iothread_index += 1
+                qemuargs.extend(
+                    [
+                        "-object",
+                        f"iothread,id={iothreadid}",
+                    ]
+                )
+                scsi_opts.append(f"iothread={iothreadid}")
+
             qemuargs.extend(
                 [
                     "-drive",
                     ",".join(o for o in drive_opts if o is not None),
                     "-device",
+                    ",".join(o for o in scsi_opts if o is not None),
+                    "-device",
                     ",".join(o for o in device_opts if o is not None),
                 ]
             )
 
+            # any options that were not consumed are errors
+            if disk.opts:
+                raise ValueError(
+                    f"invalid --disk parameter: {d!r}\n(keys were not consumed: {disk.opts.keys()})"
+                )
+
     ret_path = None
 
     def cleanup_script_retcode():

virtme_ng/run.py

@@ -1171,8 +1171,17 @@ def _get_virtme_disk(self, args):
         disk_str = ""
 
         def ensure_name(dsk: str) -> str:
-            if "=" not in dsk:
-                return f"{dsk}={dsk}"
+            """
+            `dsk` is a comma-separated list of disk options (KEY=VAL), with the first
+            option specifying the disk name and path (NAME=PATH). As an exception,
+            NAME can be omitted (but the underlying implementation does not know that).
+            This function ensures that the first option has a NAME, and adds one
+            equal to the PATH if it is missing.
+            """
+            items = dsk.split(",")
+            first = items[0]
+            if "=" not in first:
+                return f"{first}={dsk}"
             return dsk
 
         if args.disk is not None: