Re: [PATCH qemu-server-rs v3 1/3] cpu: add pve-qemu-server-cpu crate for vCPU binding

[Dominik Csapak <d.csapak@proxmox.com>]

high level:

I'd like to see more comments for methods to explain why they do what 
they do, or if they are complex a short summary of what they do.

some comments inline

On 10/22/26 11:23 AM, Elias Huhsovitz wrote:
> Add a new library crate that implements CPU binding on top of libhwloc.
> Two public entry points are exposed:
> 
> * validate_binding(mode, sockets, cores, cfg) - pure pre-start
>    validation. Checks cfg syntax, host CPU existence, vCPU range, and
>    NUMA node existence against the local topology. No side effects.
> 
> * bind_vcpus(mode, cfg, cpuinfo_json) - applies the binding to a running
>    QEMU process. Consumes the query-cpus-fast QMP response and binds each
>    vCPU thread by kernel TID.
> 
> Five modes are implemented:
> * none: no binding
> * manual: explicit vCPU:hostCPU mapping
> * one-to-one: one vCPU per physical core, SMT stripped
> * numa: per-vNUMA-node set-to-set binding
> * numa-manual: explicit vNUMA->host NUMA mapping
> 
> hwloc is accessed via a small FFI subset (src/bindings.rs).
> 
> Thread binding uses hwloc_linux_set_tid_cpubind, which accepts a kernel
> TID directly. The TID used is reported by qemu via query-cpus-fast.
> 
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
>   Cargo.toml                                |   2 +-
>   build.sh                                  |   2 +-
>   pve-qemu-server-cpu/Cargo.toml            |  16 +
>   pve-qemu-server-cpu/debian/changelog      |   5 +
>   pve-qemu-server-cpu/debian/control        |  41 ++
>   pve-qemu-server-cpu/debian/copyright      |  18 +
>   pve-qemu-server-cpu/debian/debcargo.toml  |   9 +
>   pve-qemu-server-cpu/src/bind.rs           | 478 ++++++++++++++++++++++
>   pve-qemu-server-cpu/src/error.rs          |  38 ++
>   pve-qemu-server-cpu/src/hwloc/bindings.rs | 171 ++++++++
>   pve-qemu-server-cpu/src/hwloc/mod.rs      | 222 ++++++++++
>   pve-qemu-server-cpu/src/lib.rs            |  92 +++++
>   pve-qemu-server-cpu/src/qmp_cpu.rs        |  99 +++++
>   13 files changed, 1191 insertions(+), 2 deletions(-)
>   create mode 100644 pve-qemu-server-cpu/Cargo.toml
>   create mode 100644 pve-qemu-server-cpu/debian/changelog
>   create mode 100644 pve-qemu-server-cpu/debian/control
>   create mode 100644 pve-qemu-server-cpu/debian/copyright
>   create mode 100644 pve-qemu-server-cpu/debian/debcargo.toml
>   create mode 100644 pve-qemu-server-cpu/src/bind.rs
>   create mode 100644 pve-qemu-server-cpu/src/error.rs
>   create mode 100644 pve-qemu-server-cpu/src/hwloc/bindings.rs
>   create mode 100644 pve-qemu-server-cpu/src/hwloc/mod.rs
>   create mode 100644 pve-qemu-server-cpu/src/lib.rs
>   create mode 100644 pve-qemu-server-cpu/src/qmp_cpu.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 71bc8e5..c100e5a 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -1,5 +1,6 @@
>   [workspace]
>   members = [
> +    "pve-qemu-server-cpu",
>       "pve-qemu-server-pci"
>   ]
>   exclude = [
> @@ -26,4 +27,3 @@ strum = { version = "0.26", features = ["derive"] }
>   
>   # proxmox dependencies
>   pve-api-types = "8.1"
> -
> diff --git a/build.sh b/build.sh
> index 3c2293b..c3ee7b6 100755
> --- a/build.sh
> +++ b/build.sh
> @@ -28,7 +28,7 @@ debcargo package \
>       --no-overlay-write-back \
>       --directory "$PWD/"${BUILDDIR}"/${CRATE}" \
>       "${CRATE}" \
> -    "$(dpkg-parsechangelog -l "${CRATE}/debian/changelog" -SVersion | sed -e 's/-.*//')"
> +    "$(dpkg-parsechangelog -l "${CRATE}/debian/changelog" -SVersion | sed -e 's/-.*//' -e 's/~/-/')"
>   
>   cd "${BUILDDIR}/${CRATE}"
>   rm -f debian/source/format.debcargo.hint
> diff --git a/pve-qemu-server-cpu/Cargo.toml b/pve-qemu-server-cpu/Cargo.toml
> new file mode 100644
> index 0000000..b236796
> --- /dev/null
> +++ b/pve-qemu-server-cpu/Cargo.toml
> @@ -0,0 +1,16 @@
> +[package]
> +name = "pve-qemu-server-cpu"
> +version = "0.1.0"
> +authors.workspace = true
> +edition.workspace = true
> +license.workspace = true
> +repository.workspace = true
> +homepage.workspace = true
> +exclude.workspace = true
> +rust-version.workspace = true
> +
> +[dependencies]
> +serde = { version = "1", features = ["derive"] }
> +serde_json = "1"
> +thiserror = "2"
> +libc = "0.2"
> diff --git a/pve-qemu-server-cpu/debian/changelog b/pve-qemu-server-cpu/debian/changelog
> new file mode 100644
> index 0000000..3731cfd
> --- /dev/null
> +++ b/pve-qemu-server-cpu/debian/changelog
> @@ -0,0 +1,5 @@
> +rust-pve-qemu-server-cpu (0.1.0) trixie; urgency=medium
> +
> +  * initial release
> +
> + -- Proxmox Support Team <support@proxmox.com>  Tue, 01 Sep 2026 14:02:25 +0200
> diff --git a/pve-qemu-server-cpu/debian/control b/pve-qemu-server-cpu/debian/control
> new file mode 100644
> index 0000000..1298d61
> --- /dev/null
> +++ b/pve-qemu-server-cpu/debian/control
> @@ -0,0 +1,41 @@
> +Source: rust-pve-qemu-server-cpu
> +Section: rust
> +Priority: optional
> +Build-Depends: debhelper-compat (= 13),
> + dh-sequence-cargo,
> + libhwloc-dev (>= 2.12)
> +Build-Depends-Arch: cargo:native <!nocheck>,
> + rustc:native (>= 1.94) <!nocheck>,
> + libstd-rust-dev <!nocheck>,
> + librust-libc-0.2+default-dev <!nocheck>,
> + librust-serde-1+default-dev <!nocheck>,
> + librust-serde-1+derive-dev <!nocheck>,
> + librust-serde-json-1+default-dev <!nocheck>,
> + librust-thiserror-2+default-dev <!nocheck>
> +Maintainer: Proxmox Support Team <support@proxmox.com>
> +Standards-Version: 4.7.2
> +Vcs-Git: git://git.proxmox.com/git/pve-qemu-server-rs.git
> +Vcs-Browser: https://git.proxmox.com/?p=pve-qemu-server-rs.git
> +Homepage: https://proxmox.com
> +X-Cargo-Crate: pve-qemu-server-cpu
> +
> +Package: librust-pve-qemu-server-cpu-dev
> +Architecture: any
> +Multi-Arch: same
> +Depends:
> + ${misc:Depends},
> + librust-libc-0.2+default-dev,
> + librust-serde-1+default-dev,
> + librust-serde-1+derive-dev,
> + librust-serde-json-1+default-dev,
> + librust-thiserror-2+default-dev
> +Provides:
> + librust-pve-qemu-server-cpu+default-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0+default-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0.1-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0.1+default-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0.1.0-dev (= ${binary:Version}),
> + librust-pve-qemu-server-cpu-0.1.0+default-dev (= ${binary:Version})
> +Description: Rust crate "pve-qemu-server-cpu" - Rust source code
> + Source code for Debianized Rust crate "pve-qemu-server-cpu"
> diff --git a/pve-qemu-server-cpu/debian/copyright b/pve-qemu-server-cpu/debian/copyright
> new file mode 100644
> index 0000000..01138fa
> --- /dev/null
> +++ b/pve-qemu-server-cpu/debian/copyright
> @@ -0,0 +1,18 @@
> +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
> +
> +Files:
> + *
> +Copyright: 2026 Proxmox Server Solutions GmbH <support@proxmox.com>
> +License: AGPL-3.0-or-later
> + This program is free software: you can redistribute it and/or modify it under
> + the terms of the GNU Affero General Public License as published by the Free
> + Software Foundation, either version 3 of the License, or (at your option) any
> + later version.
> + .
> + This program is distributed in the hope that it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> + FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
> + details.
> + .
> + You should have received a copy of the GNU Affero General Public License along
> + with this program. If not, see <https://www.gnu.org/licenses/>.
> diff --git a/pve-qemu-server-cpu/debian/debcargo.toml b/pve-qemu-server-cpu/debian/debcargo.toml
> new file mode 100644
> index 0000000..23feea4
> --- /dev/null
> +++ b/pve-qemu-server-cpu/debian/debcargo.toml
> @@ -0,0 +1,9 @@
> +overlay = "."
> +crate_src_path = ".."
> +maintainer = "Proxmox Support Team <support@proxmox.com>"
> +allow_prerelease_deps = true
> +
> +[source]
> +vcs_git = "git://git.proxmox.com/git/pve-qemu-server-rs.git"
> +vcs_browser = "https://git.proxmox.com/?p=pve-qemu-server-rs.git"
> +build_depends = ["libhwloc-dev (>= 2.12)"]
> diff --git a/pve-qemu-server-cpu/src/bind.rs b/pve-qemu-server-cpu/src/bind.rs
> new file mode 100644
> index 0000000..4f2927d
> --- /dev/null
> +++ b/pve-qemu-server-cpu/src/bind.rs
> @@ -0,0 +1,478 @@
> +use std::collections::HashSet;
> +
> +use crate::hwloc::{self, CpuSet, Topology};
> +use crate::qmp_cpu::QmpCpu;
> +use crate::BindError;
> +
> +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
> +struct Cpu(u32);
> +
> +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
> +struct NumaNodeId(u32);
> +
> +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
> +struct Vcpu(u32);

for these i think it would make sense to implement
deref too, so we can simply use it as their inner values?

> +
> +#[derive(Debug, Clone, PartialEq, Eq, Hash)]
> +struct VnumaNode {
> +    id: NumaNodeId,
> +    cpus: Vec<Vcpu>,


wouldn't it make more sense for this to be a hashset? otherwise
i could have duplicates in the list?
AFAICS the only way we use it is by iterating, so a hashset
makes more sense imo


> +}
> +
> +#[derive(Debug, Clone)]
> +struct BindRule {
> +    vcpus: Vec<Vcpu>,

same here

> +    cpuset: CpuSet,
> +}
> +
> +#[derive(Debug, Clone)]
> +struct NumaBindRule {
> +    vnode: VnumaNode,
> +    host_node: NumaNodeId,
> +}
> +
> +impl From<u32> for Cpu {
> +    fn from(v: u32) -> Self {
> +        Self(v)
> +    }
> +}
> +impl From<u32> for Vcpu {
> +    fn from(v: u32) -> Self {
> +        Self(v)
> +    }
> +}
> +impl From<u32> for NumaNodeId {
> +    fn from(v: u32) -> Self {
> +        Self(v)
> +    }
> +}
> +
> +trait BindableRule {
> +    fn vcpus(&self) -> &[Vcpu];
> +    fn resolve_cpuset(&self, topo: &Topology) -> Result<CpuSet, BindError>;
> +}

i'd really like to see some documentation for these here. What does a
'bindablerule' do? and what should it resolve to?

> +
> +impl BindableRule for BindRule {
> +    fn vcpus(&self) -> &[Vcpu] {
> +        &self.vcpus
> +    }
> +    fn resolve_cpuset(&self, _topo: &Topology) -> Result<CpuSet, BindError> {
> +        Ok(self.cpuset.clone())
> +    }
> +}
> +
> +impl BindableRule for NumaBindRule {
> +    fn vcpus(&self) -> &[Vcpu] {
> +        &self.vnode.cpus
> +    }
> +    fn resolve_cpuset(&self, topo: &Topology) -> Result<CpuSet, BindError> {
> +        hwloc::numa_node_cpuset(topo, self.host_node.0).ok_or(BindError::UnknownHostCpu(self.host_node.0))
> +    }
> +}

instead of having a trait here, having an

enum BindRule {
     Regular { .. },
     Numa { .. }
}

would make the code more readable i think and forgo the
generic functions.

I don't forsee many more rule types and since the trait is not
public, others can't implment it anyway.

> +
> +fn bind(topo: &Topology, vcpu: &Vcpu, tid: u32, set: &CpuSet) -> Result<(), BindError> {
> +    // Note: DO NOT singlify here — we want the scheduler to be free to move
> +    // the vCPU within the specified set.
> +    hwloc::bind_thread(topo, tid as i32, set).map_err(|message| BindError::Bind {
> +        vcpu: vcpu.0,
> +        message,
> +    })
> +}
> +
> +fn bind_rules<R: BindableRule>(
> +    topo: &Topology,
> +    rules: &[R],
> +    qmp_cpuinfo: &[QmpCpu],
> +) -> Result<(), BindError> {
> +    for rule in rules {
> +        let set = rule.resolve_cpuset(topo)?;
> +        for vcpu in rule.vcpus() {
> +            let tid = qmp_cpuinfo
> +                .get(vcpu.0 as usize)
> +                .ok_or(BindError::ThreadNotFound(vcpu.0))?
> +                .thread_id;
> +            bind(topo, vcpu, tid, &set)?;
> +        }
> +    }
> +    Ok(())
> +}
> +
> +pub enum Mode {
> +    None,
> +    Manual,
> +    OneToOne,
> +    Numa,
> +    NumaManual,
> +}
> +
> +impl std::str::FromStr for Mode {
> +    type Err = BindError;
> +    fn from_str(s: &str) -> Result<Self, Self::Err> {
> +        match s {
> +            "none" => Ok(Mode::None),
> +            "manual" => Ok(Mode::Manual),
> +            "one-to-one" => Ok(Mode::OneToOne),
> +            "numa" => Ok(Mode::Numa),
> +            "numa-manual" => Ok(Mode::NumaManual),
> +            other => Err(BindError::BadSyntax(format!("unknown cpu-bind-mode '{other}'"))),
> +        }
> +    }
> +}
> +
> +fn to_cpuset(cpus: &[u32]) -> CpuSet {
> +    let mut set = CpuSet::new();
> +    for &c in cpus {
> +        set.set(c);
> +    }
> +    set
> +}
> +

this could be a `From<&[u32]>` implementation on CpuSet
(or simply have it inline in the one place it's needed)

> +fn parse_cpu_binding_map(map: &str) -> Result<Vec<BindRule>, BindError> {
> +    if map.is_empty() {
> +        return Err(BindError::BadSyntax("binding map is empty".into()));
> +    }
> +
> +    let mut rules = Vec::new();
> +    for entry in map.split(';') {
> +        let (vcpu_side, host_side) = entry
> +            .split_once(':')
> +            .ok_or_else(|| BindError::MissingColon(entry.to_string()))?;
> +
> +        let vcpus: Vec<Vcpu> = parse_side(vcpu_side)?.into_iter().map(Vcpu::from).collect();
> +        let cpuset = to_cpuset(&parse_side(host_side)?);
> +
> +        rules.push(BindRule { vcpus, cpuset });
> +    }
> +    Ok(rules)
> +}
> +
> +fn parse_side(s: &str) -> Result<Vec<u32>, BindError> {
> +    if let Some(inner) = s.strip_prefix('{').and_then(|r| r.strip_suffix('}')) {
> +        let mut out = Vec::new();
> +        for part in inner.split(',') {
> +            out.extend(parse_element(part)?);
> +        }
> +        Ok(out)
> +    } else {
> +        if s.contains(',') {
> +            return Err(BindError::BadSyntax(format!(
> +                "unions require braces: write '{{0,1}}', not '{s}'"
> +            )));
> +        }
> +        parse_element(s)
> +    }
> +}
> +
> +fn parse_element(s: &str) -> Result<Vec<u32>, BindError> {
> +    if let Some((a, b)) = s.split_once('-') {
> +        let a: u32 = a
> +            .parse()
> +            .map_err(|_| BindError::BadSyntax(format!("bad index in '{s}'")))?;
> +        let b: u32 = b
> +            .parse()
> +            .map_err(|_| BindError::BadSyntax(format!("bad index in '{s}'")))?;
> +        if a > b {
> +            return Err(BindError::BadSyntax(format!("reversed range '{s}'")));
> +        }
> +        Ok((a..=b).collect())

this seems excessive? a config like 0-40000000 allocates a bunch of 
memory for saving a list of vcpus?

we should either limit these upfront somewhere, or just saving a sparse
list/range/etc.

> +    } else {
> +        Ok(vec![s
> +            .parse()
> +            .map_err(|_| BindError::BadSyntax(format!("bad index '{s}'")))?])
> +    }
> +}
> +
> +fn parse_index_list<T: From<u32>>(s: &str) -> Result<Vec<T>, BindError> {
> +    if s.is_empty() {
> +        return Ok(Vec::new());
> +    }
> +    let mut out = Vec::new();
> +    for part in s.split(';') {
> +        out.extend(parse_element(part)?.into_iter().map(T::from));
> +    }
> +    Ok(out)
> +}
> +
> +fn parse_numa_binding_map(map: &str) -> Result<Vec<(NumaNodeId, NumaNodeId)>, BindError> {
> +    if map.is_empty() {
> +        return Err(BindError::BadSyntax("numa binding map is empty".into()));
> +    }
> +    let mut pairs = Vec::new();
> +    for entry in map.split(';') {
> +        let entry = entry.trim();
> +        let (v, h) = entry
> +            .split_once(':')
> +            .ok_or_else(|| BindError::MissingColon(entry.to_string()))?;
> +        let vnode: u32 = v
> +            .trim()
> +            .parse()
> +            .map_err(|_| BindError::BadSyntax(format!("bad vNUMA node in '{entry}'")))?;
> +        let hnode: u32 = h
> +            .trim()
> +            .parse()
> +            .map_err(|_| BindError::BadSyntax(format!("bad host NUMA node in '{entry}'")))?;
> +        pairs.push((NumaNodeId(vnode), NumaNodeId(hnode)));
> +    }
> +    Ok(pairs)
> +}
> +
> +fn collect_vnuma_nodes(qmp_cpuinfo: &[QmpCpu]) -> Vec<VnumaNode> {
> +    let max_node = qmp_cpuinfo
> +        .iter()
> +        .map(|c| c.props.node_id)
> +        .max()
> +        .unwrap_or(0);
> +    let mut buckets: Vec<Vec<Vcpu>> = vec![Vec::new(); (max_node + 1) as usize];
> +    for cpu in qmp_cpuinfo {
> +        buckets[cpu.props.node_id as usize].push(Vcpu::from(cpu.cpu_index));
> +    }
> +    buckets
> +        .into_iter()
> +        .enumerate()
> +        .map(|(id, cpus)| VnumaNode {
> +            id: NumaNodeId(id as u32),
> +            cpus,
> +        })
> +        .collect()
> +}

couldn't this be also inferred from the config instead from the qmp
output? I guess it does not make much difference, but I'd like
to use the config as 'authority' since the actual vm should (must?)
follow the config anyway, otherwise we'd have a bug somewhere....

[snip]

> diff --git a/pve-qemu-server-cpu/src/error.rs b/pve-qemu-server-cpu/src/error.rs
> new file mode 100644
> index 0000000..73a569e
> --- /dev/null
> +++ b/pve-qemu-server-cpu/src/error.rs
> @@ -0,0 +1,38 @@
> +use thiserror::Error;
> +
> +#[derive(Debug, Error)]
> +pub enum BindError {
> +    // Parse errors
> +    #[error("map entry '{0}' is missing a ':' separator")]
> +    MissingColon(String),
> +
> +    #[error("invalid syntax in binding map: '{0}'")]
> +    BadSyntax(String),
> +
> +    // Validation errors (pre-start)
> +    #[error("host CPU {0} does not exist in this topology")]
> +    UnknownHostCpu(u32),
> +
> +    #[error("vCPU {0} appears in more than one map entry")]
> +    VcpuReused(u32),
> +
> +    #[error("vCPU {vcpu} is out of range (VM has vCPUs 0..{max})")]
> +    VcpuOutOfRange { vcpu: u32, max: u32 },
> +
> +    // Runtime errors (post-start)
> +    #[error("no running thread found for vCPU {0}")]
> +    ThreadNotFound(u32),
> +
> +    #[error("invalid kernel TID {tid} for vCPU {vcpu}")]
> +    BadThreadId { vcpu: u32, tid: u32 },
> +
> +    #[error("failed to init host topology: '{0}'")]
> +    TopologyInit(String),
> +
> +    #[error("binding vCPU {vcpu} failed: {message}")]
> +    Bind {
> +        vcpu: u32,
> +        /// Human-readable error from libhwloc (errno-based).
> +        message: String,
> +    },
> +}

i we'd reuse the numaX binding modes, most of these would not be
necessary (as well as the parsing etc.)

in general if we'd restrict ourselves (for the moment) on the basic
modes + using affinity to limit the set further.

the amount of information we'd need from sysfs would
be a numa node -> vcpu list sysfs read

everything else (no smt sibling, only 'big' cores, etc.) we could
do via the affinity and the vcpu -> hostnuma node manual pinning
we could do via numaX

the manual mode is not really necessary since a user can simply do
it themselves if they want to.