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

[pve-devel] [PATCH common/qemu-server v4] 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 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 | 20 +++++++++++++++-----
 1 file changed, 15 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 v4 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 v3:
* only add the logging in this patch, not the error ignore handling

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

diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index 0bde6d7..772f6d6 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -211,17 +211,23 @@ 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);
     $fh->close();
 
-    return $res;
+    if (defined($res)) {
+	return 1;
+    } elsif (my $err = $!) {
+	warn "error writing '$buf' to '$filename': $err\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 v4 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 v3:
* add ignoring to file_write in this patch
* only add a single option for ignoring EEXIST, we can add more/make it
  more generic later too if we need it.


 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 772f6d6..8f39da3 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;
@@ -224,6 +226,7 @@ sub file_write {
     if (defined($res)) {
 	return 1;
     } elsif (my $err = $!) {
+	return 1 if $allow_existing && $err == EEXIST;
 	warn "error writing '$buf' to '$filename': $err\n";
     }
 
@@ -309,7 +312,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 v4 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>
---
changes from v3:
* expand the warning wording a bit

 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

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

[Thomas Lamprecht]

Am 11.11.24 um 11:17 schrieb 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 v3:
> * only add the logging in this patch, not the error ignore handling
> 
>  src/PVE/SysFSTools.pm | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
> index 0bde6d7..772f6d6 100644
> --- a/src/PVE/SysFSTools.pm
> +++ b/src/PVE/SysFSTools.pm
> @@ -211,17 +211,23 @@ 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);
>      $fh->close();

just to be sure: and the fh->close cannot alter `$!` ?

Otherwise, we probably want to assign my $sys_err = $! before the $fh->close()
call, and maybe comment that it only is relevant if $res is not defined.

>  
> -    return $res;
> +    if (defined($res)) {
> +	return 1;
> +    } elsif (my $err = $!) {
> +	warn "error writing '$buf' to '$filename': $err\n";
> +    }
> +
> +    return 0;
>  }
>  
>  sub pci_device_info {

Korrigieren

Schließen

Grammatik

This sentence does not start with an uppercase letter.

BeforeIgnorieren



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

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

[Dominik Csapak]

On 11/11/24 11:29, Thomas Lamprecht wrote:
> Am 11.11.24 um 11:17 schrieb 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 v3:
>> * only add the logging in this patch, not the error ignore handling
>>
>>   src/PVE/SysFSTools.pm | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
>> index 0bde6d7..772f6d6 100644
>> --- a/src/PVE/SysFSTools.pm
>> +++ b/src/PVE/SysFSTools.pm
>> @@ -211,17 +211,23 @@ 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);
>>       $fh->close();
> 
> just to be sure: and the fh->close cannot alter `$!` ?
> 
> Otherwise, we probably want to assign my $sys_err = $! before the $fh->close()
> call, and maybe comment that it only is relevant if $res is not defined.
> 

no, you're right, close can set $! too...

i'll send a v5 for extracting the syserr before


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

Re: [pve-devel] [PATCH common/qemu-server v4] improve sysfs write behaviour

[Dominik Csapak]

sent a v5: https://lore.proxmox.com/pve-devel/20241111110520.1803627-1-d.csapak@proxmox.com/


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