From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C172961B7E for ; Tue, 18 Jan 2022 13:06:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B16882962B for ; Tue, 18 Jan 2022 13:06:28 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D5AA31C299 for ; Tue, 18 Jan 2022 12:58:45 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4189F42F69; Tue, 18 Jan 2022 12:58:45 +0100 (CET) Message-ID: <23447ce2-7231-db80-75a1-5f3853fc5f41@proxmox.com> Date: Tue, 18 Jan 2022 12:58:43 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Thunderbird/97.0 Content-Language: en-US To: Proxmox VE development discussion , nick@nicksherlock.com References: <20220118084312.67133-1-nick@nicksherlock.com> <0100017e6c5af7cd-3bbf78a7-5935-4259-8fff-886fab782269-000000@email.amazonses.com> From: Dominik Csapak In-Reply-To: <0100017e6c5af7cd-3bbf78a7-5935-4259-8fff-886fab782269-000000@email.amazonses.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.162 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [8006.pid, pci.pm, proxmox.com] Subject: Re: [pve-devel] [PATCH qemu-server] pci: allow override of PCI vendor/device ids X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2022 12:06:58 -0000 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 > > e.g. hostpci0: 03:00,x-pci-vendor-id=0x8086,x-pci-device-id=0x10f6 > > Signed-off-by: Nicholas Sherlock > --- > 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'