[RFC proxmox-perl-rs/qemu-server/qemu-server-rs v3 0/9] pci-handling rewrite (part 1/2/3)

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

# Motivation

We want to extend the PCI layout mechanism to be easily extendable, especially
for NUMA awareness and replicating host-topology (e.g. see bug #7283 [0]).

This is necessary for some performance gains in guests, especially with
high-end hardware where users pay extra for higher performance.

# The Problem

PCI addresses (and hardware layout in general) must be fixed for our qemu
guests, since this is part of the machine state and must be stable for
live-migration suspend/resume, etc.

Currently, these addresses are hardcoded in perl hashes, with a long history of
devs (me included) piling new addresses on top, using whatever address was
free.

This led to a very convoluted and scattered assignment list, without any
underlying structure, which makes it harder the more new devices are added.
(Using a wrong address only surfaces when running some tests or starting a
guest with it)

# My Proposed Solution

So to fix the current situation I propose a plan in multiple phases:

## Phase 1 - untangling perl code to rust (this series)

To extend/switch between multiple PCI layouts, we first must untangle the
current situation and make it easier to see what is actually used.

I did this in rust, by using the type system to represent the PCI layout, and
inferring the addresses from the layout, not the other way around.

That makes it easier to see where there is actually free space and where we can
add new things.

It also highlights how messy the current layout is. (See the legacy layout in
the `pve-qemu-server-pci` crate)

I also introduced here a generic `HardwareConfig` struct that's intended to
hold information useful to more call sites in contrast to passing a lot of
arguments everywhere, similar to the Cfg2Cmd class, but usable in the rust
codebase.

## Phase 2 - extending pci layouts (this series)

This introduces a new layout ("grouped") that can be used in an opt-in manner,
and is vastly more logically constructed, which makes it easy to extend and
reason about.

This part entails restructuring the addr calls in qemu-server's perl code, so
that there is only a single entry point ("address" for example) instead of
splitting between 'print_pci_addr' and 'print_pcie_addr'.

This is folded into the commit to using the rust bindings, since it does not
make sense to recreate the old interface in rust, just to replace it with
the next patches anyway.

This now also creates the commandline for the q35 devices instead
of using 'readconfig' (guarded behind a new machine version).

## Phase 3 - more perl to rust code port (this series)

The HardwareConfig also contains a version comparison, so we can widely reuse
it in many methods, since we need it for the 'addr' interface anyway.

These changes are also folded for the same reason as above.

## Phase 4 - Host and NUMA layouts (the next part)

With all the pieces in place, we can then easily extend the new layout by PCI
switches that are needed for assigning to NUMA nodes and replicating the host
topology for passed through devices.

To answer the question from v2 how this relates to the 'pci-layout' option:
I plan to make this a separate setting on top of the grouped layout instead of
adding more layouts, e.g.:

  machine: q35,pci-layout=grouped,hostpci-topology=<flat|numa|host>

* flat (default): every hostpcie device gets its own root port on the root
  bus, the layout in this series
* numa: adds a pxb-pcie expander bridge per guest NUMA node and puts the root
  ports of that node's devices behind it
* host: additionally replicates the host PCIe switches that more than one
  passed through device is behind

The expander bridges go into slots of the grouped layout that are left unused
in this series, so enabling a topology does not move any of the existing
devices, and 'pci-layout' does not need more values for it. qemu-server
provides the NUMA node and the host switches of each device, so nothing has to
be configured per device.

The name and place of the option are not fixed yet, so suggestions are
welcome.

# Notes

* pve-qemu-server-rs is a new git repo, but the crate in it could
  easily be integrated somewhere else. Having this as a separate repo
  could replace 'qemu-server' at one point though.
* This is an RFC, so comments about the general design/plan are desired.
* Names and crate placement are not fixed, I know that some names I chose are
  not optimal, if you do have better names/places, please suggest them.
* Opted to do this in rust for now, since I think that is the best way forward.
  If the consensus is to do the layouting changes in perl, or keep some parts in
  perl, that's fine with me too and I'll change my efforts accordingly
* @Elias: i did not implement every change:
  - the layout type stayed in the types folder (IMO makes more sense)
  - chassis_nr stayed the same (number of bridges can't change
    dynamically)
  - no const fn (i don't think it's possible with a single const fn and
    the same short syntax, but if it is, please do show me that)
  - didn't rename the pci_ methods (IMO this is fine)
  - no thiserror used, IMO not necessary here

changes from v2:
- pve-qemu-server-rs:
* moved some code into later commits where it's necessary
* `HardwareConfig::new` now takes a `HardwareConfigParams` struct, which can
  be deserialized directly and rejects unknown keys
* the layout is deserialized directly into `PciLayout`, so an unknown layout
  is now an error instead of silently falling back to the legacy one
* hostpcie devices are passed as a list of objects (`{ index => N }`)
  instead of plain indices, so they can be extended later without changing
  the interface
* use an internal `Arch` enum, so the config can't hold an unknown
  architecture
* grouped layout: only set 'multifunction=on' on bridges that share their
  slot with other bridges (currently only the net bridges)
* the `Display` implementation of `Device` now prints the configuration ids
  (e.g. 'qga0', 'balloon0', 'virtioN') and is exhaustive
* the xhci controller has no index anymore (we can add more ports with hubs)
* document that `version_at_least` records the required pve version, and use
  relaxed ordering for it
* check the bridge count with `assert!` instead of `debug_assert!`
* drop the unused anyhow and log dependencies
* code style and doc fixes (`'static` layout lifetime, `match` in
  `map_legacy_pci_id`, comments)

- proxmox-perl-rs:
* `new` takes a single hash with the parameters instead of a list of
  positional arguments
* `addr` returns undef for devices that are not part of the layout instead
  of dying
* use 'this' for the object parameter and add the 'Class method'/'Method'
  doc prefixes
* doc fixes

- qemu-server:
* rebased on current master
* construct the HardwareConfig with a parameter hash, and pass the hostpcie
  devices as objects
* drop the now unused `PVE::QemuServer::Machine` import from USB.pm
* grouped layout tests: adapt to the bridges without 'multifunction=on'

0: https://bugzilla.proxmox.com/show_bug.cgi?id=7283


pve-qemu-server-rs:

Dominik Csapak (4):
  add pve-qemu-server-pci crate for guest PCI address generation
  pci: legacy: generate pci bridges
  pci: legacy: generate q35 chipset devices and hostpcie root ports
  pci: layout: add grouped PCI and PCIe layouts


proxmox-perl-rs:

Dominik Csapak (1):
  pve: add bindings for `pve-qemu-server-pci` crate

 pve-rs/Cargo.toml                          |   1 +
 pve-rs/Makefile                            |   1 +
 pve-rs/src/bindings/mod.rs                 |   3 +
 pve-rs/src/bindings/pci/hardware_config.rs | 120 +++++++++++++++++++++
 pve-rs/src/bindings/pci/mod.rs             |   2 +
 5 files changed, 127 insertions(+)
 create mode 100644 pve-rs/src/bindings/pci/hardware_config.rs
 create mode 100644 pve-rs/src/bindings/pci/mod.rs


qemu-server:

Dominik Csapak (4):
  pci: use PVE::RS::PCI bindings for addresses and bus devices
  tree-wide: use HardwareConfig for the machine version checks
  cfg2cmd: q35: generate chipset devices with machine version 11.1+pve1
  machine: add 'pci-layout' option

 src/PVE/API2/Qemu.pm                          |   9 +-
 src/PVE/QemuServer.pm                         | 266 +++++++-------
 src/PVE/QemuServer/Drive.pm                   |   7 +-
 src/PVE/QemuServer/DriveDevice.pm             |  12 +-
 src/PVE/QemuServer/Machine.pm                 |  16 +
 src/PVE/QemuServer/PCI.pm                     | 340 ++----------------
 src/PVE/QemuServer/RNG.pm                     |   6 +-
 src/PVE/QemuServer/USB.pm                     |  59 +--
 src/PVE/QemuServer/Virtiofs.pm                |   7 +-
 src/test/Makefile                             |   5 +-
 src/test/QemuServerTest/CommandLineMocks.pm   |  27 ++
 .../cfg2cmd/efi-secboot-and-tpm-q35.conf.cmd  |  21 +-
 src/test/cfg2cmd/q35-grouped-layout.conf      |  17 +
 src/test/cfg2cmd/q35-grouped-layout.conf.cmd  |  47 +++
 .../cfg2cmd/q35-grouped-virtiofs-viommu.conf  |  14 +
 .../q35-grouped-virtiofs-viommu.conf.cmd      |  46 +++
 src/test/cfg2cmd/q35-ide.conf.cmd             |  21 +-
 .../cfg2cmd/q35-linux-hostpci-11.1+pve1.conf  |  21 ++
 .../q35-linux-hostpci-11.1+pve1.conf.cmd      |  61 ++++
 .../q35-linux-hostpci-driver-keep.conf.cmd    |  25 +-
 .../q35-linux-hostpci-mapping.conf.cmd        |  21 +-
 .../q35-linux-hostpci-multifunction.conf.cmd  |  21 +-
 src/test/cfg2cmd/q35-linux-hostpci-wrong.conf |  16 +
 .../cfg2cmd/q35-linux-hostpci-wrong.conf.cmd  |  41 +++
 ...q35-linux-hostpci-x-pci-overrides.conf.cmd |  21 +-
 src/test/cfg2cmd/q35-linux-hostpci.conf.cmd   |  25 +-
 src/test/cfg2cmd/q35-linux-ivshmem.conf.cmd   |  21 +-
 src/test/cfg2cmd/q35-scsihw2.conf.cmd         |  21 +-
 src/test/cfg2cmd/q35-simple.conf.cmd          |  21 +-
 .../cfg2cmd/q35-viommu-intel-aw-bits.conf.cmd |  21 +-
 .../q35-viommu-intel-guest-phys-bits.conf.cmd |  21 +-
 src/test/cfg2cmd/q35-viommu-intel.conf.cmd    |  21 +-
 .../q35-viommu-virtio-aw-bits.conf.cmd        |  21 +-
 src/test/cfg2cmd/q35-viommu-virtio.conf.cmd   |  21 +-
 src/test/cfg2cmd/q35-win10-hostpci.conf.cmd   |   2 +-
 .../simple-grouped-virtiofs-viommu.conf       |  16 +
 .../simple-grouped-virtiofs-viommu.conf.cmd   |  45 +++
 src/test/cfg2cmd/simple-grouped.conf          |  15 +
 src/test/cfg2cmd/simple-grouped.conf.cmd      |  41 +++
 src/test/cfg2cmd/spice-win-grouped.conf       |  14 +
 src/test/cfg2cmd/spice-win-grouped.conf.cmd   |  48 +++
 src/test/cfg2cmd/virtiofs-viommu.conf         |  16 +
 src/test/cfg2cmd/virtiofs-viommu.conf.cmd     |  40 +++
 src/test/run_pci_addr_checks.pl               | 141 --------
 44 files changed, 1039 insertions(+), 679 deletions(-)
 create mode 100644 src/test/cfg2cmd/q35-grouped-layout.conf
 create mode 100644 src/test/cfg2cmd/q35-grouped-layout.conf.cmd
 create mode 100644 src/test/cfg2cmd/q35-grouped-virtiofs-viommu.conf
 create mode 100644 src/test/cfg2cmd/q35-grouped-virtiofs-viommu.conf.cmd
 create mode 100644 src/test/cfg2cmd/q35-linux-hostpci-11.1+pve1.conf
 create mode 100644 src/test/cfg2cmd/q35-linux-hostpci-11.1+pve1.conf.cmd
 create mode 100644 src/test/cfg2cmd/q35-linux-hostpci-wrong.conf
 create mode 100644 src/test/cfg2cmd/q35-linux-hostpci-wrong.conf.cmd
 create mode 100644 src/test/cfg2cmd/simple-grouped-virtiofs-viommu.conf
 create mode 100644 src/test/cfg2cmd/simple-grouped-virtiofs-viommu.conf.cmd
 create mode 100644 src/test/cfg2cmd/simple-grouped.conf
 create mode 100644 src/test/cfg2cmd/simple-grouped.conf.cmd
 create mode 100644 src/test/cfg2cmd/spice-win-grouped.conf
 create mode 100644 src/test/cfg2cmd/spice-win-grouped.conf.cmd
 create mode 100644 src/test/cfg2cmd/virtiofs-viommu.conf
 create mode 100644 src/test/cfg2cmd/virtiofs-viommu.conf.cmd
 delete mode 100755 src/test/run_pci_addr_checks.pl


Summary over all repositories:
  49 files changed, 1166 insertions(+), 679 deletions(-)

-- 
Generated by murpp 0.11.0