[pve-devel] [RFC PATCH qemu-server] add win7 pcie quirk

[Thomas Lamprecht]

On 12/7/18 3:04 PM, Dominik Csapak wrote:
> win7 is very picky about pcie assignments and fails with
> 'error 12' the way we add hospci devices
> 
> to combat that, we simply give the hostpci device a normal port
> instead
> 
> since it is not possible to do live migration/snapshots with ram
> with pci passthrough, we do not have be particularly careful
> with backwards compatibility here
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> sending as rfc, because i am unsure about the way i do it
> the alternative would be to expand the parameters of print_pcie_addr
> to include e.g. the ostype or config and pull the information there
> which seems a big change for a small workaround
> 
>  PVE/QemuServer.pm     | 8 +++++++-
>  PVE/QemuServer/PCI.pm | 5 +++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index a162db9..24a9d38 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3524,7 +3524,13 @@ sub config_to_command {
>  	my $pcie = $d->{pcie};
>  	if($pcie){
>  	    die "q35 machine model is not enabled" if !$q35;
> -	    $pciaddr = print_pcie_addr("hostpci$i");
> +	    # win7 is very picky about pcie assignments
> +	    # the way we do it normally does not work there
> +	    if ($winversion == 7) {
> +		$pciaddr = print_pcie_addr("hostpci${i}win7");
> +	    } else {
> +		$pciaddr = print_pcie_addr("hostpci$i");
> +	    }
>  	}else{
>  	    $pciaddr = print_pci_addr("hostpci$i", $bridges, $arch, $machine_type);
>  	}
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 29b8943..2d49070 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -145,6 +145,11 @@ sub print_pcie_addr {
>  	hostpci1 => { bus => "ich9-pcie-port-2", addr => 0 },
>  	hostpci2 => { bus => "ich9-pcie-port-3", addr => 0 },
>  	hostpci3 => { bus => "ich9-pcie-port-4", addr => 0 },
> +	# win7 is more picky about pcie assignments

hmm, could we use a less "win7" bound name, i.e., one which describes this
port better? AFAICT, those do not directly connect to the PCIe root complex
but rather to the bus0 switch? 

'hostpci0bus0', 'hostpci1bus0', ...

or (switch instead of bus?). Because while Win7 is picky I do not really like
to use it in the PCI address schema, as another OS (version) could just be/become
picky as well.

> +	hostpci0win7 => { bus => "pcie.0", addr => 16 },

why staring from 16? Could you please add a reason for this at least in the
commit message?

Oh, and I do not like the fact that we have $devices redefined here, even if we use
it already on module level for PCI devices, maybe rename this in $pci_devices and
$pcie_devices? Additionally a simple test ensuring that we do not reuse addresses
could be nice, the PCI list is quite long and one could not detect a reuse of a bit
obscure/seldom used address before deployment. But that's can be done another time
just fine.. :)

> +	hostpci1win7 => { bus => "pcie.0", addr => 17 },
> +	hostpci2win7 => { bus => "pcie.0", addr => 18 },
> +	hostpci3win7 => { bus => "pcie.0", addr => 19 },
>      };
>  
>      if (defined($devices->{$id}->{bus}) && defined($devices->{$id}->{addr})) {
>