Re: [PATCH pve-qemu-server-rs 2/9] pci: add machine abstraction and PCI bridge generation

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

On 9/28/26 2:55 PM, Elias Huhsovitz wrote:
> Comments inline.
> 
> On Tue Sep 22, 2026 at 12:55 PM CEST, Dominik Csapak wrote:
>> Besides the address of a single device, callers also need the list of
>> bridge devices a guest has to be started with. Which bridges those are
>> depends on more than one property of the guest config, so pass them in
>> as a `Machine` struct rather than as a growing list of parameters. It
>> can be extended with the layout variant and other guest wide settings
>> later on, without touching every call site again.
>>
>> The bridge list itself follows the Perl implementation: q35 machines
>> already define the bridges up to three in their machine definition,
>> pci.3 is only needed for virtio-scsi-single, and pci.4 only once a
>> SCSI controller above index one exists or the machine version is new
>> enough to always include it.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
> 
> [snip]
> 
>> +/// Renders the bridge devices a guest needs, see
>> +/// [`crate::get_pci_bridges`].
>> +pub(crate) fn get_pci_bridges(machine: &Machine) -> Vec<String> {
> 
> nit: naming: If you remove the prefix from the print_ functions, i would
> also remove the prefix here for consistency. i.e., `pci_bridges`.
> 
>> +    // at most four bridges, each contributing a flag and its value
>> +    let mut res = Vec::with_capacity(8);
>> +
>> +    // some SCSI controllers can only have 7 disks each, so scsi14 and upwards
>> +    // need scsihw2 and above, which live on pci.4
>> +    let include_pci4 = machine.version_at_least(11, 1, 0) || machine.max_scsihw > 1;
>> +
>> +    for i in 1..=4 {
>> +        // q35.cfg already includes the bridges up to three
>> +        if i < 4 && machine.q35 {
>> +            continue;
>> +        }
>> +
>> +        if i == 3 && !machine.virtio_scsi_single() {
>> +            continue;
>> +        }
>> +
>> +        if i == 4 && !include_pci4 {
>> +            continue;
>> +        }
>> +
>> +        let id = if i == 2 && machine.legacy_igd {
>> +            format!("pci.{i}-igd")
>> +        } else {
>> +            format!("pci.{i}")
>> +        };
>> +
>> +        let addr = match print_pci_addr(&id, machine.arch) {
>> +            Ok(addr) => addr,
>> +            Err(err) => {
>> +                log::warn!("could not find address for bridge {id}: {err}");
>> +                continue;
>> +            }
> 
> Consider `log::error!` instead of `log::warn!`.
> 
> IMO a missing bridge is a cause for concern and should be reflected with
> an error.

i think returning an error here would even be better. we already
have to touch the perl side, so an additional eval { }
shouldn't hurt (or we simply die there; it shouldn't be possible
anyway?)

> 
>> +        };
>> +        res.push("-device".to_string());
>> +        res.push(format!("pci-bridge,id=pci.{i},chassis_nr={i}{addr}"));
>> +    }
>> +
>> +    res
>> +}
>> +
> 
> [snip]
> 
>>   }
>> diff --git a/pve-qemu-server-pci/src/lib.rs b/pve-qemu-server-pci/src/lib.rs
>> index 5e8b8a7..7d0bd92 100644
>> --- a/pve-qemu-server-pci/src/lib.rs
>> +++ b/pve-qemu-server-pci/src/lib.rs
>> @@ -10,6 +10,9 @@
>>   
>>   pub mod constants;
>>   
>> +mod machine;
>> +pub use machine::Machine;
>> +
>>   mod types;
>>   use pve_api_types::ClusterResourceHostArch as Arch;
>>   pub use types::*;
>> @@ -42,3 +45,18 @@ pub fn print_pcie_addr(id: &str) -> Result<String, PciConfigError> {
>>   pub fn print_pcie_root_port(index: u8) -> Result<String, PciConfigError> {
>>       layout::legacy::print_pcie_root_port(index)
>>   }
>> +
>> +/// Returns the number of the PCI bridge `id` sits on, or `None` if it is on
>> +/// the root bus or not a known device.
>> +pub fn get_pci_bridge_for_device(id: &str) -> Option<u8> {
>> +    layout::legacy::get_pci_bridge_for_device(id)
>> +}
>> +
>> +/// Returns the QEMU arguments for all PCI bridges `machine` needs, as
>> +/// alternating `-device` flags and their values.
>> +///
>> +/// Bridges whose address cannot be determined are skipped with a warning
>> +/// rather than failing the whole guest.
>> +pub fn get_pci_bridges(machine: &Machine) -> Vec<String> {
>> +    layout::legacy::get_pci_bridges(machine)
>> +}
>> diff --git a/pve-qemu-server-pci/src/machine.rs b/pve-qemu-server-pci/src/machine.rs
>> new file mode 100644
>> index 0000000..10ee9db
>> --- /dev/null
>> +++ b/pve-qemu-server-pci/src/machine.rs
>> @@ -0,0 +1,99 @@
>> +use pve_api_types::{ClusterResourceHostArch as Arch, QemuConfigOstype, QemuConfigScsihw};
>> +
>> +/// The parts of a guest configuration that influence which PCI layout is used
>> +/// and which quirks have to be applied to it.
>> +///
>> +/// This is deliberately not the full guest config: it only carries what the
>> +/// address generation needs, so that callers can build it from whatever
>> +/// configuration representation they have.
>> +#[derive(Clone, Debug)]
>> +pub struct Machine {
> 
> nit naming: Machine seems a bit strange to me here, since we a passing
> the relevant guest config. So why not just call it `LayoutContext`, or
> `GuestConfig | `GuestLayoutConfig`.
> 

yeah, machine was not a very well chosen name :P
I'll think about what to name it, thanks for the suggestions!

>> +    pub arch: Arch,
>> +    pub q35: bool,
> 
> Since q35 is a boolean, it is possible to configure:
> Machine { arch: Arch::Aarch64, q35: true }
> 
> What about making the chipset part of the Arch enum like this:
> 
> pub enum X86Chipset {
>      I440fx,
>      Q35,
> }
> 
> pub enum Arch {
>      X8664(X86Chipset),
>      Aarch64,
> }
> 
> This would allow us to remove the q35 boolean value.
> 
> Or if you want to keep a field here, what about a replacing q35 with an
> enum `machine_type` with 3 staes (Q35, I440fx, Virt)

since i want to match (and at some point use) the pve config fields here
having an enum for the 3 machine types makes most sense IMO

> 
>> +    pub scsihw: Option<QemuConfigScsihw>,
>> +    /// Highest `scsihw` controller index in use. Controllers from index 2 on
>> +    /// live on `pci.4`, so this decides whether that bridge is needed.
>> +    pub max_scsihw: u8,
>> +    pub legacy_igd: bool,
>> +    pub ostype: Option<QemuConfigOstype>,
>> +    /// QEMU machine version as `(major, minor, pve)`, where the PVE specific
>> +    /// revision is optional.
>> +    pub version: (u16, u16, Option<u16>),
> 
> nit: naming: Consequently we should rename `version` -> `machine_version`
> 
>> +}
>> +
>> +impl Machine {
>> +    /// Builds a [`Machine`] from the raw configuration values.
>> +    ///
>> +    /// Unparsable `arch`, `scsihw` and `ostype` values are not an error here:
>> +    /// `arch` falls back to x86_64 and the other two to `None`, which is the
>> +    /// same defaulting the Perl implementation does.
>> +    #[allow(clippy::too_many_arguments)]
>> +    pub fn new(
>> +        arch: &str,
>> +        q35: bool,
>> +        scsihw: Option<String>,
>> +        max_scsihw: u8,
>> +        legacy_igd: bool,
>> +        ostype: Option<String>,
>> +        major: u16,
>> +        minor: u16,
>> +        pve: Option<u16>,
>> +    ) -> Self {
>> +        let arch = arch.parse().unwrap_or(Arch::X8664);
>> +        let scsihw = scsihw.and_then(|hw| hw.parse().ok());
>> +        let ostype = ostype.and_then(|ostype| ostype.parse().ok());
>> +        Self {
>> +            arch,
>> +            q35,
>> +            scsihw,
>> +            max_scsihw,
>> +            legacy_igd,
>> +            ostype,
>> +            version: (major, minor, pve),
>> +        }
>> +    }
>> +
>> +    /// Whether the guest's root bus is PCIe. True for q35 machines and for
>> +    /// aarch64, which only has a PCIe host bridge.
>> +    pub fn is_pcie(&self) -> bool {
>> +        self.q35 || self.arch == Arch::Aarch64
>> +    }
>> +
>> +    /// Whether each SCSI disk gets its own controller, which needs the extra
>> +    /// `pci.3` bridge.
>> +    pub fn virtio_scsi_single(&self) -> bool {
>> +        self.scsihw == Some(QemuConfigScsihw::VirtioScsiSingle)
>> +    }
>> +
>> +    /// Whether the machine version is at least `major.minor.pve`.
>> +    ///
>> +    /// A machine without a PVE revision counts as being at least any requested
>> +    /// one, since the plain QEMU version already implies all PVE changes made
>> +    /// for it.
>> +    pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool {
>> +        if self.version.0 > major {
>> +            return true;
>> +        }
>> +        if self.version.0 < major {
>> +            return false;
>> +        }
>> +
>> +        if self.version.1 > minor {
>> +            return true;
>> +        }
>> +        if self.version.1 < minor {
>> +            return false;
>> +        }
>> +
>> +        if let Some(our_pve) = self.version.2 {
>> +            if our_pve > pve {
>> +                return true;
>> +            }
>> +            if our_pve < pve {
>> +                return false;
>> +            }
>> +        }
>> +
>> +        true
>> +    }
> 
> IMO we can just let rusts tuple compare logic handle this:
> 
> pub fn version_at_least(&self, major: u16, minor: u16, pve: u16) -> bool {
>      let (our_major, our_minor, our_pve) = self.version;
> 	(our_major, our_minor, our_pve.unwrap_or(u16::MAX)) >= (major, minor, pve)
> }

true. This is a thing that should probably live in a helper crate
somewhere. I guess well reuse this kind of version comparison more than 
once.

> 
>> +}
>