Re: [pve-devel] [PATCH qemu-server] pci: allow override of PCI vendor/device ids

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

Hi, Thanks for the patch!

the approach looks good (thanks for adding tests!),
but i have some comments (most inline)

I'd prefer to have the text from the cover letter (i.e.
the rationale for this) in the commit message here. That
way we have it in the git history and stumbling upon the
commit directly gives the reason (instead of having to search
the mailing list)

gui part looks fine, remaining comments inline

On 1/18/22 09:43, nick@nicksherlock.com wrote:
> From: Nicholas Sherlock <n.sherlock@gmail.com>
> 
> e.g. hostpci0: 03:00,x-pci-vendor-id=0x8086,x-pci-device-id=0x10f6
> 
> Signed-off-by: Nicholas Sherlock <n.sherlock@gmail.com>
> ---
>   PVE/QemuServer/PCI.pm                         | 31 ++++++++++++++++
>   .../q35-linux-hostpci-x-pci-overrides.conf    | 16 +++++++++
>   ...q35-linux-hostpci-x-pci-overrides.conf.cmd | 35 +++++++++++++++++++
>   3 files changed, 82 insertions(+)
>   create mode 100644 test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf
>   create mode 100644 test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
> 
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 4033784..45790b6 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -77,6 +77,34 @@ The type of mediated device to use.
>   An instance of this type will be created on startup of the VM and
>   will be cleaned up when the VM stops.
>   EODESCR
> +    },
> +    'x-pci-vendor-id' => {

since we are here in pci context anyway, i'd drop the
'x-pci-' prefix altogether and simply
have it named 'vendor-id', 'device-id' and so on.

makes the config a bit shorter and more readable
(would need gui patch adaption too)

> +	type => 'string',
> +	pattern => qr/^0x[0-9a-fA-F]{4}$/,
> +	format_description => 'hex id',
> +	optional => 1,
> +	description => "Override PCI vendor ID visible to guest"
> +    },
> +    'x-pci-device-id' => {
> +	type => 'string',
> +	pattern => qr/^0x[0-9a-fA-F]{4}$/,
> +	format_description => 'hex id',
> +	optional => 1,
> +	description => "Override PCI device ID visible to guest"
> +    },
> +    'x-pci-sub-vendor-id' => {
> +	type => 'string',
> +	pattern => qr/^0x[0-9a-fA-F]{4}$/,
> +	format_description => 'hex id',
> +	optional => 1,
> +	description => "Override PCI sub-vendor ID visible to guest"
> +    },
> +    'x-pci-sub-device-id' => {
> +	type => 'string',
> +	pattern => qr/^0x[0-9a-fA-F]{4}$/,
> +	format_description => 'hex id',
> +	optional => 1,
> +	description => "Override PCI sub-device ID visible to guest"
>       }
>   };
>   PVE::JSONSchema::register_format('pve-qm-hostpci', $hostpci_fmt);
> @@ -457,6 +485,9 @@ sub print_hostpci_devices {
>   		$devicestr .= ",multifunction=on" if $multifunction;
>   		$devicestr .= ",romfile=/usr/share/kvm/$d->{romfile}" if $d->{romfile};
>   		$devicestr .= ",bootindex=$bootorder->{$id}" if $bootorder->{$id};
> +		foreach ("x-pci-vendor-id","x-pci-device-id","x-pci-sub-vendor-id","x-pci-sub-device-id") {
> +		    $devicestr .= ",$_=$d->{$_}" if $d->{$_};
> +		}

while its valid perl, it does not really use our style (see [0])

i'd use something like this (not tested though):
---8<---
for my $option (qw(vendor-id device-id sub-vendor-id sub-device-id)) {
     $devicestr .= ",x-pci-$option=$d->{$option}" if $d->{$option};
}
--->8---

0: https://pve.proxmox.com/wiki/Perl_Style_Guide

>   	    }
>   
>   	    push @$devices, '-device', $devicestr;
> diff --git a/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf
> new file mode 100644
> index 0000000..c5ab9bd
> --- /dev/null
> +++ b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf
> @@ -0,0 +1,16 @@
> +# TEST: Overriding PCI vendor/device IDs reported to guest
> +bios: ovmf
> +bootdisk: scsi0
> +cores: 1
> +efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
> +hostpci0: 00:ff.1,x-pci-vendor-id=0x1234,x-pci-device-id=0x5678,x-pci-sub-vendor-id=0x2233,x-pci-sub-device-id=0x0000
> +hostpci1: d0:13.0,pcie=1,x-pci-vendor-id=0x1234,x-pci-device-id=0x5678
> +machine: q35
> +memory: 512
> +net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
> +numa: 1
> +ostype: l26
> +scsihw: virtio-scsi-pci
> +smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
> +sockets: 2
> +vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
> diff --git a/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
> new file mode 100644
> index 0000000..7a215c9
> --- /dev/null
> +++ b/test/cfg2cmd/q35-linux-hostpci-x-pci-overrides.conf.cmd
> @@ -0,0 +1,35 @@
> +/usr/bin/kvm \
> +  -id 8006 \
> +  -name vm8006 \
> +  -no-shutdown \
> +  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
> +  -mon 'chardev=qmp,mode=control' \
> +  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
> +  -mon 'chardev=qmp-event,mode=control' \
> +  -pidfile /var/run/qemu-server/8006.pid \
> +  -daemonize \
> +  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
> +  -drive 'if=pflash,unit=0,format=raw,readonly=on,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
> +  -global 'ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off' \
> +  -smp '2,sockets=2,cores=1,maxcpus=2' \
> +  -nodefaults \
> +  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
> +  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
> +  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
> +  -m 512 \
> +  -object 'memory-backend-ram,id=ram-node0,size=256M' \
> +  -numa 'node,nodeid=0,cpus=0,memdev=ram-node0' \
> +  -object 'memory-backend-ram,id=ram-node1,size=256M' \
> +  -numa 'node,nodeid=1,cpus=1,memdev=ram-node1' \
> +  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
> +  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
> +  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
> +  -device 'vfio-pci,host=0000:00:ff.1,id=hostpci0,bus=pci.0,addr=0x10,x-pci-vendor-id=0x1234,x-pci-device-id=0x5678,x-pci-sub-vendor-id=0x2233,x-pci-sub-device-id=0x0000' \
> +  -device 'vfio-pci,host=0000:d0:13.0,id=hostpci1,bus=ich9-pcie-port-2,addr=0x0,x-pci-vendor-id=0x1234,x-pci-device-id=0x5678' \
> +  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
> +  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
> +  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
> +  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
> +  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
> +  -machine 'type=q35+pve0'