[pve-devel] [PATCH common/qemu-server v5] improve sysfs write behaviour

[pve-devel] [PATCH common/qemu-server v5] improve sysfs write behaviour

[Dominik Csapak]

and fix passthrough regressions

As i feared previously in [0], making it a hard error when encountering
errors during sysfs writes uncovered some situations where our code was
too strict to keep some setups working.

One such case is resetting devices, which is seemingly not necessary
at all times, so this series

* donwgrades that error to warning
* adds some more logging to `file_write` to be able to better debug

Another case that broke was passing through similar devices with the
same vendor/modelid since the write to vfio-pci's 'new_id' works only
once for the same vendor/modelid.

To fix that make EEXIST errors ignorable for file_write

changes from v4:
* extract $! into variable before close

changes from v3:
* common patch 1/2 is now only adding the logging again
* add a single option for ignoring EEXIST, if we need more in the future

changes from v2:
* improve comment on file_write
* shorten code with suggestions from stoiko
* fix commit message

changes from v1:
* also include error ignore list
* ignore EEXIST for writing to new_id

0: https://lore.proxmox.com/pve-devel/20240723082925.934603-1-d.csapak@proxmox.com/

pve-common:

Dominik Csapak (2):
  sysfstools: file_write: extend with logging
  sysfstools: fix regression on binding to vfio-pci

 src/PVE/SysFSTools.pm | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

qemu-server:

Dominik Csapak (1):
  pci: don't hard require resetting devices for passthrough

 PVE/QemuServer/PCI.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH common v5 1/2] sysfstools: file_write: extend with logging

[Dominik Csapak]

the actual error and path is useful to know when trying to debug or
figure out what did not work, so warn here if there was an error.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v4:
* extract $! before close

 src/PVE/SysFSTools.pm | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index 0bde6d7..428e6ec 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -211,17 +211,24 @@ sub check_iommu_support{
     return PVE::Tools::dir_glob_regex('/sys/class/iommu/', "[^\.].*");
 }
 
+# writes $buf into $filename, on success returns 1, on error returns 0 and warns
 sub file_write {
     my ($filename, $buf) = @_;
 
     my $fh = IO::File->new($filename, "w");
     return undef if !$fh;
 
-    my $res = defined(syswrite($fh, $buf)) ? 1 : 0;
-
+    my $res = syswrite($fh, $buf);
+    my $syserr = $!; # only relevant if $res is undefined
     $fh->close();
 
-    return $res;
+    if (defined($res)) {
+	return 1;
+    } elsif ($syserr) {
+	warn "error writing '$buf' to '$filename': $syserr\n";
+    }
+
+    return 0;
 }
 
 sub pci_device_info {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH common v5 2/2] sysfstools: fix regression on binding to vfio-pci

[Dominik Csapak]

when starting a vm with passthrough, we have to bind all normal pci
devices to vfio-pci. This happens by

* unbinding from current driver
* telling vfio-pci the 'vendorid modelid' combo so it knows this device
  class can use the driver (by writing to 'new_id')
* actually binding the device to vfio-pci

if there are multiple devices of the same 'vendorid modelid' class on
the host (and passed through), only the first write to 'new_id' is
successful, all subsequent ones return EEXIST.

This could happen e.g. for setups with multiple GPUs that have the same
audio chip.

To fix this, ignore the EEXIST error for this write to new_id, by adding
a new parameter to file_write for this.

If we need to ignore other errors in the future, we can still make this
more generic.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v4:
* adapt to $! extraction

 src/PVE/SysFSTools.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index 428e6ec..74f3242 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use IO::File;
+use POSIX qw(EEXIST);
 
 use PVE::Tools qw(file_read_firstline dir_glob_foreach);
 
@@ -212,8 +213,9 @@ sub check_iommu_support{
 }
 
 # writes $buf into $filename, on success returns 1, on error returns 0 and warns
+# if $allow_existing is set, an EEXIST error will be handled as success
 sub file_write {
-    my ($filename, $buf) = @_;
+    my ($filename, $buf, $allow_existing) = @_;
 
     my $fh = IO::File->new($filename, "w");
     return undef if !$fh;
@@ -225,6 +227,7 @@ sub file_write {
     if (defined($res)) {
 	return 1;
     } elsif ($syserr) {
+	return 1 if $allow_existing && $syserr == EEXIST;
 	warn "error writing '$buf' to '$filename': $syserr\n";
     }
 
@@ -310,7 +313,8 @@ sub pci_dev_bind_to_vfio {
     return 1 if -d $testdir;
 
     my $data = "$dev->{vendor} $dev->{device}";
-    return undef if !file_write("$vfio_basedir/new_id", $data);
+    # allow EEXIST for multiple devices with the same vendor/modelid
+    return undef if !file_write("$vfio_basedir/new_id", $data, 1);
 
     my $fn = "$pcisysfs/devices/$name/driver/unbind";
     if (!file_write($fn, $name)) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v5 1/1] pci: don't hard require resetting devices for passthrough

[Dominik Csapak]

Since pve-common commit:

 eff5957 (sysfstools: file_write: properly catch errors)

this check here fails now when the reset does not work. It turns out
that resetting the device is not always necessary, and we previously
ignored most errors when trying to do so.

To restore that functionality, downgrade this `die` to a warning.

If the device really needs a reset to work, it will either fail later
during startup, or not work correctly in the guest, but that behavior
existed before and is AFAIK not really detectable from our side.

Also improve the warning message a bit to not scare users and explain
that we're continuing.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes since v4

 PVE/QemuServer/PCI.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index 75eac134..8c94fdd8 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -728,7 +728,7 @@ sub prepare_pci_device {
     } else {
 	die "can't unbind/bind PCI group to VFIO '$pciid'\n"
 	    if !PVE::SysFSTools::pci_dev_group_bind_to_vfio($pciid);
-	die "can't reset PCI device '$pciid'\n"
+	warn "couldn't reset PCI device '$pciid', trying to continue (not all devices need a reset).\n"
 	    if $info->{has_fl_reset} && !PVE::SysFSTools::pci_dev_reset($info);
     }
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied-series: [PATCH common/qemu-server v5] improve sysfs write behaviour

[Thomas Lamprecht]

Am 11.11.24 um 12:05 schrieb Dominik Csapak:
> and fix passthrough regressions
> 
> As i feared previously in [0], making it a hard error when encountering
> errors during sysfs writes uncovered some situations where our code was
> too strict to keep some setups working.
> 
> One such case is resetting devices, which is seemingly not necessary
> at all times, so this series
> 
> * donwgrades that error to warning
> * adds some more logging to `file_write` to be able to better debug
> 
> Another case that broke was passing through similar devices with the
> same vendor/modelid since the write to vfio-pci's 'new_id' works only
> once for the same vendor/modelid.
> 
> To fix that make EEXIST errors ignorable for file_write
> 
> changes from v4:
> * extract $! into variable before close
> 
> changes from v3:
> * common patch 1/2 is now only adding the logging again
> * add a single option for ignoring EEXIST, if we need more in the future
> 
> changes from v2:
> * improve comment on file_write
> * shorten code with suggestions from stoiko
> * fix commit message
> 
> changes from v1:
> * also include error ignore list
> * ignore EEXIST for writing to new_id
> 
> 0: https://lore.proxmox.com/pve-devel/20240723082925.934603-1-d.csapak@proxmox.com/
> 
> pve-common:
> 
> Dominik Csapak (2):
>   sysfstools: file_write: extend with logging
>   sysfstools: fix regression on binding to vfio-pci
> 
>  src/PVE/SysFSTools.pm | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> qemu-server:
> 
> Dominik Csapak (1):
>   pci: don't hard require resetting devices for passthrough
> 
>  PVE/QemuServer/PCI.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 


applied all three patches with some minor opinionated clean-ups, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel