[pve-devel] [PATCH guest-common/qemu-server/manager/docs] enable experimental support for pci live migration

[pve-devel] [PATCH guest-common/qemu-server/manager/docs] enable experimental support for pci live migration

[Dominik Csapak]

for mapped resources. This requires driver and hardware support,
but aside from nvidia vgpus there don't seem to be many drivers
(if any) that do support that.

qemu already supports that for vfio-pci devices, so nothing to be
done there besides actively enabling it.

Since we currently can't properly test it here and very much depends on
hardware/driver support, mark it as experimental everywhere (docs/api/gui).
(though i tested the live-migration part manually here by using
"exec:cat > /tmp/test" for the migration target, and "exec: cat
/tmp/test" as the 'incoming' parameter for a new vm start, which worked ;) )

i opted for marking them migratable at the mapping level, but we could
theoretically also put it in the hostpciX config instead.
(though imho it fits better in the cluster-wide resource mapping config)

also the naming/texts could probably be improved, but i think
'live-migration-capable' is very descriptive and i didn't want to
use an overly short name for it (which can be confusing, see the
'shared' flag for storages)

note that patch 1 of qemu-server is only a cosmetic fix that i
encountered while testing and can be applied independently

pve-guest-common:

Dominik Csapak (2):
  mapping: pci: add 'live-migration-capable' flag to mappings
  mapping: pci: optionally return the config in 'find_on_current_node'

 src/PVE/Mapping/PCI.pm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

qemu-server:

Dominik Csapak (4):
  usb: fix undef error on string match
  pci: set 'enable-migration' to on for live-migration marked mapped
    devices
  check_local_resources: add more info per mapped device
  api: enable live migration for marked mapped pci devices

 PVE/API2/Qemu.pm      |  7 ++++++-
 PVE/QemuMigrate.pm    | 13 +++++++++----
 PVE/QemuServer.pm     | 12 +++++++-----
 PVE/QemuServer/PCI.pm |  8 +++++++-
 4 files changed, 29 insertions(+), 11 deletions(-)

pve-docs:

Dominik Csapak (2):
  qm: resource mapping: add description for `mdev` option
  qm: resource mapping: document `live-migration-capable` setting

 qm.adoc | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

pve-manager:

Dominik Csapak (1):
  ui: allow configuring and live migration of mapped pci resources

 www/manager6/window/Migrate.js    | 22 +++++++++++++++++++---
 www/manager6/window/PCIMapEdit.js | 12 ++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH guest-common 1/2] mapping: pci: add 'live-migration-capable' flag to mappings

[Dominik Csapak]

so that we can decide in qemu-server to allow live-migration.
the driver and qemu must be capable of that, and it's the
admins responsibility to know and configure that

Mark the option as experimental in the description.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Mapping/PCI.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
index 19ace98..0866175 100644
--- a/src/PVE/Mapping/PCI.pm
+++ b/src/PVE/Mapping/PCI.pm
@@ -100,8 +100,16 @@ my $defaultData = {
 	    maxLength => 4096,
 	},
 	mdev => {
+	    description => "Marks the device(s) as being capable of providing mediated devices.",
 	    type => 'boolean',
 	    optional => 1,
+	    default => 0,
+	},
+	'live-migration-capable' => {
+	    description => "Marks the device(s) as being able to be live-migrated (Experimental).",
+	    type => 'boolean',
+	    optional => 1,
+	    default => 0,
 	},
 	map => {
 	    type => 'array',
@@ -123,6 +131,7 @@ sub options {
     return {
 	description => { optional => 1 },
 	mdev => { optional => 1 },
+	'live-migration-capable' => { optional => 1 },
 	map => {},
     };
 }
-- 
2.39.2

[pve-devel] [PATCH guest-common 2/2] mapping: pci: optionally return the config in 'find_on_current_node'

[Dominik Csapak]

this is useful to get to the config without having to parse it again

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Mapping/PCI.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
index 0866175..9d8a4a7 100644
--- a/src/PVE/Mapping/PCI.pm
+++ b/src/PVE/Mapping/PCI.pm
@@ -219,10 +219,11 @@ sub find_on_current_node {
 sub get_node_mapping {
     my ($cfg, $id, $nodename) = @_;
 
-    return undef if !defined($cfg->{ids}->{$id});
+    my $map_config = $cfg->{ids}->{$id};
+    return undef if !defined($map_config);
 
     my $res = [];
-    for my $map ($cfg->{ids}->{$id}->{map}->@*) {
+    for my $map ($map_config->{map}->@*) {
 	my $entry = eval { parse_property_string($map_fmt, $map) };
 	warn $@ if $@;
 	if ($entry && $entry->{node} eq $nodename) {
@@ -230,7 +231,7 @@ sub get_node_mapping {
 	}
     }
 
-    return $res;
+    return wantarray ? ($res, $map_config) : $res;
 }
 
 PVE::Mapping::PCI->register();
-- 
2.39.2

[pve-devel] [PATCH qemu-server 1/4] usb: fix undef error on string match

[Dominik Csapak]

'$entry->{host}' can be empty, so we have to check for that before
doing a regex check, otherwise we get ugly errors in the log

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8d0ed22c..6e2c8052 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2597,7 +2597,7 @@ sub check_local_resources {
     foreach my $k (keys %$conf) {
 	if ($k =~ m/^usb/) {
 	    my $entry = parse_property_string('pve-qm-usb', $conf->{$k});
-	    next if $entry->{host} =~ m/^spice$/i;
+	    next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
 	    if ($entry->{mapping}) {
 		$add_missing_mapping->('usb', $k, $entry->{mapping});
 		push @$mapped_res, $k;
-- 
2.39.2

[pve-devel] [PATCH qemu-server 2/4] pci: set 'enable-migration' to on for live-migration marked mapped devices

[Dominik Csapak]

the default is 'auto', but for those which are marked as capable for
live migration, we want to explicitly enable that, so we get an early
error on start if the driver does not support that.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer/PCI.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index 1673041b..402b4e7a 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -429,9 +429,11 @@ sub parse_hostpci {
 
     if ($mapping) {
 	# we have no ordinary pci id, must be a mapping
-	my $devices = PVE::Mapping::PCI::find_on_current_node($mapping);
+	my ($devices, $config) = PVE::Mapping::PCI::find_on_current_node($mapping);
 	die "PCI device mapping not found for '$mapping'\n" if !$devices || !scalar($devices->@*);
 
+	$res->{'live-migration-capable'} = 1 if $config->{'live-migration-capable'};
+
 	for my $device ($devices->@*) {
 	    eval { PVE::Mapping::PCI::assert_valid($mapping, $device) };
 	    die "PCI device mapping invalid (hardware probably changed): $@\n" if $@;
@@ -632,6 +634,10 @@ sub print_hostpci_devices {
 		$devicestr .= ",host=$pcidevice->{id}";
 	    }
 
+	    if ($d->{'live-migration-capable'}) {
+		$devicestr .= ",enable-migration=on"
+	    }
+
 	    my $mf_addr = $multifunction ? ".$j" : '';
 	    $devicestr .= ",id=${id}${mf_addr}${pciaddr}${mf_addr}";
 
-- 
2.39.2

[pve-devel] [PATCH qemu-server 3/4] check_local_resources: add more info per mapped device

[Dominik Csapak]

such as the mapping name and if it's marked for live-migration (pci only)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Qemu.pm   |  2 +-
 PVE/QemuMigrate.pm |  5 +++--
 PVE/QemuServer.pm  | 10 ++++++----
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 497987ff..4ecaeb91 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4516,7 +4516,7 @@ __PACKAGE__->register_method({
 	$res->{local_disks} = [ values %$local_disks ];;
 
 	$res->{local_resources} = $local_resources;
-	$res->{'mapped-resources'} = $mapped_resources;
+	$res->{'mapped-resources'} =  [ map { "$_->{key}" } $mapped_resources->@* ];
 
 	return $res;
 
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 8d9b35ae..6fe8157d 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -232,7 +232,7 @@ sub prepare {
     my ($loc_res, $mapped_res, $missing_mappings_by_node) = PVE::QemuServer::check_local_resources($conf, 1);
     my $blocking_resources = [];
     for my $res ($loc_res->@*) {
-	if (!grep($res, $mapped_res->@*)) {
+	if (!grep { $_->{key} eq $res } $mapped_res->@*) {
 	    push $blocking_resources->@*, $res;
 	}
     }
@@ -246,8 +246,9 @@ sub prepare {
 
     if (scalar($mapped_res->@*)) {
 	my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
+	my $mapped_text = join(", ", map { $_->{key} } $mapped_res->@*);
 	if ($running) {
-	    die "can't migrate running VM which uses mapped devices: " . join(", ", $mapped_res->@*) . "\n";
+	    die "can't migrate running VM which uses mapped devices: " . $mapped_text . "\n";
 	} elsif (scalar($missing_mappings->@*)) {
 	    die "can't migrate to '$self->{node}': missing mapped devices " . join(", ", $missing_mappings->@*) . "\n";
 	} else {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6e2c8052..ef3aee20 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2600,14 +2600,16 @@ sub check_local_resources {
 	    next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
 	    if ($entry->{mapping}) {
 		$add_missing_mapping->('usb', $k, $entry->{mapping});
-		push @$mapped_res, $k;
+		push @$mapped_res, { key => $k, device => $entry->{mapping} };
 	    }
 	}
 	if ($k =~ m/^hostpci/) {
 	    my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
-	    if ($entry->{mapping}) {
-		$add_missing_mapping->('pci', $k, $entry->{mapping});
-		push @$mapped_res, $k;
+	    if (my $name = $entry->{mapping}) {
+		$add_missing_mapping->('pci', $k, $name);
+		my $mapped_device = { key => $k, device => $name };
+		$mapped_device->{'live-migration'} = 1 if $pci_map->{ids}->{$name}->{'live-migration-capable'};
+		push @$mapped_res, $mapped_device;
 	    }
 	}
 	# sockets are safe: they will recreated be on the target side post-migrate
-- 
2.39.2

[pve-devel] [PATCH qemu-server 4/4] api: enable live migration for marked mapped pci devices

[Dominik Csapak]

They have to be marked as 'live-migration-capable' in the mapping
config, and the driver and qemu must support it.

For the gui checks, we now return a list of 'mapped-with-live-migration'
entries in the migration preflight api call too.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Qemu.pm   |  5 +++++
 PVE/QemuMigrate.pm | 12 ++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 4ecaeb91..8581a529 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4453,6 +4453,10 @@ __PACKAGE__->register_method({
 		type => 'array',
 		description => "List of mapped resources e.g. pci, usb"
 	    },
+	    'mapped-with-live-migration' => {
+		type => 'array',
+		description => "List of mapped resources that are marked as capable of live-migration",
+	    },
 	},
     },
     code => sub {
@@ -4517,6 +4521,7 @@ __PACKAGE__->register_method({
 
 	$res->{local_resources} = $local_resources;
 	$res->{'mapped-resources'} =  [ map { "$_->{key}" } $mapped_resources->@* ];
+	$res->{'mapped-with-live-migration'} =  [ map { $_->{'live-migration'} ? "$_->{key}" : () } $mapped_resources->@* ];
 
 	return $res;
 
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 6fe8157d..b3570770 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -246,11 +246,15 @@ sub prepare {
 
     if (scalar($mapped_res->@*)) {
 	my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
-	my $mapped_text = join(", ", map { $_->{key} } $mapped_res->@*);
-	if ($running) {
-	    die "can't migrate running VM which uses mapped devices: " . $mapped_text . "\n";
-	} elsif (scalar($missing_mappings->@*)) {
+	my $missing_live_mappings = [];
+	for my $res ($mapped_res->@*) {
+	    my $name = "$res->{key}:$res->{device}";
+	    push $missing_live_mappings->@*, $name if !$res->{'live-migration'};
+	}
+	if (scalar($missing_mappings->@*)) {
 	    die "can't migrate to '$self->{node}': missing mapped devices " . join(", ", $missing_mappings->@*) . "\n";
+	} elsif ($running && scalar($missing_live_mappings->@*)) {
+	    die "can't live migrate running VM which uses following mapped devices: " . join(", ", $missing_live_mappings->@*) . "\n";
 	} else {
 	    $self->log('info', "migrating VM which uses mapped local devices");
 	}
-- 
2.39.2

[pve-devel] [PATCH docs 1/2] qm: resource mapping: add description for `mdev` option

[Dominik Csapak]

in a new section about additional options

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qm.adoc | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index 1170dd1..c146ce9 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1734,6 +1734,19 @@ To create mappings `Mapping.Modify` on `/mapping/<type>/<name>` is necessary
 To use these mappings, `Mapping.Use` on `/mapping/<type>/<name>` is necessary
 (in addition to the normal guest privileges to edit the configuration).
 
+Additional Options
+^^^^^^^^^^^^^^^^^^
+
+There are additional options when defining a cluster wide resource mapping.
+Currently there are the following options:
+
+* `mdev` (PCI): This marks the PCI device as being capable of providing
+  `mediated devices`. When this is enabled, you can select a type when
+  configuring it on the guest. If multiple PCI devices are selected for
+  the mapping, the mediated device will be create on the first one where
+  there are any available instances of the selected type.
+
+
 Managing Virtual Machines with `qm`
 ------------------------------------
 
-- 
2.39.2

[pve-devel] [PATCH docs 2/2] qm: resource mapping: document `live-migration-capable` setting

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 qm.adoc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index c146ce9..c77cb7b 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1746,6 +1746,12 @@ Currently there are the following options:
   the mapping, the mediated device will be create on the first one where
   there are any available instances of the selected type.
 
+* `live-migration-capable` (PCI): This marks the device as being capable of
+  being live migrated between nodes. This requires driver and hardware support.
+  Only NVIDIA GPUs with recent kernel are known to support this. Note that
+  live migrating passed through devices is an experimental feature and may
+  not work or cause issues.
+
 
 Managing Virtual Machines with `qm`
 ------------------------------------
-- 
2.39.2

[pve-devel] [PATCH manager 1/1] ui: allow configuring and live migration of mapped pci resources

[Dominik Csapak]

if the hardware/driver is capable, the admin can now mark a pci device
as 'live-migration-capable', which then tries enabling live migration
for such devices.

mark it as experimental when configuring and in the migrate window

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/window/Migrate.js    | 22 +++++++++++++++++++---
 www/manager6/window/PCIMapEdit.js | 12 ++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 5473821b..21806d50 100644
--- a/www/manager6/window/Migrate.js
+++ b/www/manager6/window/Migrate.js
@@ -245,6 +245,7 @@ Ext.define('PVE.window.Migrate', {
 
 	    let blockingResources = [];
 	    let mappedResources = migrateStats['mapped-resources'] ?? [];
+	    let mappedWithLiveMigration = migrateStats['mapped-with-live-migration'] ?? [];
 
 	    for (const res of migrateStats.local_resources) {
 		if (mappedResources.indexOf(res) === -1) {
@@ -271,14 +272,29 @@ Ext.define('PVE.window.Migrate', {
 		}
 	    }
 
-	    if (mappedResources && mappedResources.length) {
-		if (vm.get('running')) {
+	    if (mappedResources && mappedResources.length && vm.get('running')) {
+		let allowed = [];
+		let notAllowed = [];
+		for (const resource of mappedResources) {
+		    if (mappedWithLiveMigration.indexOf(resource) === -1) {
+			notAllowed.push(resource);
+		    } else {
+			allowed.push(resource);
+		    }
+		}
+		if (notAllowed.length > 0) {
 		    migration.possible = false;
 		    migration.preconditions.push({
 			text: Ext.String.format('Can\'t migrate running VM with mapped resources: {0}',
-			mappedResources.join(', ')),
+			notAllowed.join(', ')),
 			severity: 'error',
 		    });
+		} else if (allowed.length > 0) {
+		    migration.preconditions.push({
+			text: Ext.String.format('Live-migrating running VM with mapped resources (Experimental): {0}',
+			allowed.join(', ')),
+			severity: 'warning',
+		    });
 		}
 	    }
 
diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
index d43f04eb..731269a0 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -242,6 +242,18 @@ Ext.define('PVE.window.PCIMapEditWindow', {
 			disabled: '{hideComment}',
 		    },
 		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    fieldLabel: gettext('Live Migration Capable'),
+		    labelWidth: 200,
+		    boxLabel: `<i class="fa fa-exclamation-triangle warning"></i> ${gettext('Experimental')}`,
+		    reference: 'live-migration-capable',
+		    name: 'live-migration-capable',
+		    cbind: {
+			deleteEmpty: '{!isCreate}',
+			disabled: '{hideComment}',
+		    },
+		},
 	    ],
 
 	    columnB: [
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server 1/4] usb: fix undef error on string match

[Fiona Ebner]

Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> '$entry->{host}' can be empty, so we have to check for that before
> doing a regex check, otherwise we get ugly errors in the log
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>

applied this one, thanks!

Re: [pve-devel] [PATCH guest-common 1/2] mapping: pci: add 'live-migration-capable' flag to mappings

[Fiona Ebner]

Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> so that we can decide in qemu-server to allow live-migration.
> the driver and qemu must be capable of that, and it's the
> admins responsibility to know and configure that
> 

Nit: "The" and "QEMU" should be capitalized like this. "admins" ->
"admin's". Missing period at the end.

> Mark the option as experimental in the description.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/Mapping/PCI.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index 19ace98..0866175 100644
> --- a/src/PVE/Mapping/PCI.pm
> +++ b/src/PVE/Mapping/PCI.pm
> @@ -100,8 +100,16 @@ my $defaultData = {
>  	    maxLength => 4096,
>  	},
>  	mdev => {
> +	    description => "Marks the device(s) as being capable of providing mediated devices.",
>  	    type => 'boolean',
>  	    optional => 1,
> +	    default => 0,
> +	},

Above should be its own patch. Most likely, I'm missing it, but where
exactly does the 'mdev' property from the mapping have an effect? Just
in the UI? At least telling from 'qm showcmd', the 'mdev' property for a
'hostpciN' VM config entry will not be ignored even if the mapping has
'mdev=0'. And it's also possible to run 'qm set 112 --hostpci0
mapping=bar,mdev=foo' without any warning if the mapping has 'mdev=0'.

> +	'live-migration-capable' => {
> +	    description => "Marks the device(s) as being able to be live-migrated (Experimental).",

The bit about QEMU and the driver needing to support it should be
mentioned here.

> +	    type => 'boolean',
> +	    optional => 1,
> +	    default => 0,
>  	},
>  	map => {
>  	    type => 'array',
> @@ -123,6 +131,7 @@ sub options {
>      return {
>  	description => { optional => 1 },
>  	mdev => { optional => 1 },
> +	'live-migration-capable' => { optional => 1 },
>  	map => {},
>      };
>  }

Re: [pve-devel] [PATCH qemu-server 3/4] check_local_resources: add more info per mapped device

[Fiona Ebner]

Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> such as the mapping name and if it's marked for live-migration (pci only)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm   |  2 +-
>  PVE/QemuMigrate.pm |  5 +++--
>  PVE/QemuServer.pm  | 10 ++++++----
>  3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 497987ff..4ecaeb91 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4516,7 +4516,7 @@ __PACKAGE__->register_method({
>  	$res->{local_disks} = [ values %$local_disks ];;
>  
>  	$res->{local_resources} = $local_resources;
> -	$res->{'mapped-resources'} = $mapped_resources;
> +	$res->{'mapped-resources'} =  [ map { "$_->{key}" } $mapped_resources->@* ];

Or should it become a hash? Then you could use 'keys' instead of map and
a 'key' property.

>  
>  	return $res;
>  
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 8d9b35ae..6fe8157d 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -232,7 +232,7 @@ sub prepare {
>      my ($loc_res, $mapped_res, $missing_mappings_by_node) = PVE::QemuServer::check_local_resources($conf, 1);
>      my $blocking_resources = [];
>      for my $res ($loc_res->@*) {
> -	if (!grep($res, $mapped_res->@*)) {

Seems like this is currently broken. I.e. it should be $res eq $_

> +	if (!grep { $_->{key} eq $res } $mapped_res->@*) {
>  	    push $blocking_resources->@*, $res;
>  	}
>      }
> @@ -246,8 +246,9 @@ sub prepare {
>  
>      if (scalar($mapped_res->@*)) {
>  	my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
> +	my $mapped_text = join(", ", map { $_->{key} } $mapped_res->@*);
>  	if ($running) {
> -	    die "can't migrate running VM which uses mapped devices: " . join(", ", $mapped_res->@*) . "\n";
> +	    die "can't migrate running VM which uses mapped devices: " . $mapped_text . "\n";

Style nit: no need for the concatenation anymore, can just use the
variable inside the string.

>  	} elsif (scalar($missing_mappings->@*)) {
>  	    die "can't migrate to '$self->{node}': missing mapped devices " . join(", ", $missing_mappings->@*) . "\n";
>  	} else {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 6e2c8052..ef3aee20 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2600,14 +2600,16 @@ sub check_local_resources {
>  	    next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
>  	    if ($entry->{mapping}) {
>  		$add_missing_mapping->('usb', $k, $entry->{mapping});
> -		push @$mapped_res, $k;
> +		push @$mapped_res, { key => $k, device => $entry->{mapping} };

Should we name the second key 'name'/'mapping'/'id' instead of 'device'?

>  	    }
>  	}
>  	if ($k =~ m/^hostpci/) {
>  	    my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
> -	    if ($entry->{mapping}) {
> -		$add_missing_mapping->('pci', $k, $entry->{mapping});
> -		push @$mapped_res, $k;
> +	    if (my $name = $entry->{mapping}) {
> +		$add_missing_mapping->('pci', $k, $name);
> +		my $mapped_device = { key => $k, device => $name };
> +		$mapped_device->{'live-migration'} = 1 if $pci_map->{ids}->{$name}->{'live-migration-capable'};

Style nit: line too long

> +		push @$mapped_res, $mapped_device;
>  	    }
>  	}
>  	# sockets are safe: they will recreated be on the target side post-migrate

Re: [pve-devel] [PATCH qemu-server 4/4] api: enable live migration for marked mapped pci devices

[Fiona Ebner]

Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> They have to be marked as 'live-migration-capable' in the mapping
> config, and the driver and qemu must support it.
> 
> For the gui checks, we now return a list of 'mapped-with-live-migration'
> entries in the migration preflight api call too.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm   |  5 +++++
>  PVE/QemuMigrate.pm | 12 ++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 4ecaeb91..8581a529 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4453,6 +4453,10 @@ __PACKAGE__->register_method({
>  		type => 'array',
>  		description => "List of mapped resources e.g. pci, usb"
>  	    },
> +	    'mapped-with-live-migration' => {
> +		type => 'array',
> +		description => "List of mapped resources that are marked as capable of live-migration",
> +	    },

Should we merge this with 'mapped-resources' for the next major release
(and add a reminder comment here)? I.e. make that return the objects
with the additional information.

>  	},
>      },
>      code => sub {

Re: [pve-devel] [PATCH guest-common 2/2] mapping: pci: optionally return the config in 'find_on_current_node'

[Fiona Ebner]

Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> this is useful to get to the config without having to parse it again
> 

You could also adapt the call sites that need it to use
PVE::Mapping::PCI::config() and PVE::Mapping::PCI::get_node_mapping()
instead of PVE::Mapping::PCI::find_on_current_node().

That would avoid overloading the return value. IMHO
find_on_current_node() doesn't sound like it should return the full map
config, even if just optionally.

Re: [pve-devel] [PATCH guest-common 1/2] mapping: pci: add 'live-migration-capable' flag to mappings

[Dominik Csapak]

On 3/22/24 14:37, Fiona Ebner wrote:
> Am 18.03.24 um 12:18 schrieb Dominik Csapak:
>> so that we can decide in qemu-server to allow live-migration.
>> the driver and qemu must be capable of that, and it's the
>> admins responsibility to know and configure that
>>
> 
> Nit: "The" and "QEMU" should be capitalized like this. "admins" ->
> "admin's". Missing period at the end.
> 
>> Mark the option as experimental in the description.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/Mapping/PCI.pm | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
>> index 19ace98..0866175 100644
>> --- a/src/PVE/Mapping/PCI.pm
>> +++ b/src/PVE/Mapping/PCI.pm
>> @@ -100,8 +100,16 @@ my $defaultData = {
>>   	    maxLength => 4096,
>>   	},
>>   	mdev => {
>> +	    description => "Marks the device(s) as being capable of providing mediated devices.",
>>   	    type => 'boolean',
>>   	    optional => 1,
>> +	    default => 0,
>> +	},
> 
> Above should be its own patch. Most likely, I'm missing it, but where
> exactly does the 'mdev' property from the mapping have an effect? Just
> in the UI? At least telling from 'qm showcmd', the 'mdev' property for a
> 'hostpciN' VM config entry will not be ignored even if the mapping has
> 'mdev=0'. And it's also possible to run 'qm set 112 --hostpci0
> mapping=bar,mdev=foo' without any warning if the mapping has 'mdev=0'.
> 

yeah sorry, i added that but overlooked it when adding the hunks...

AFAIR i intended to use the mdev property to safeguard the mapping
like we do with the iommu groups, so when it changes, warn the user that
the mapping changed (using it with mdevs wouldn't work anyway if that
was set but the device wouldn't provide one)

aside from that, yes it currently only has effects on the gui,
do we maybe want to make that stricter? (would be a breaking change imho)

>> +	'live-migration-capable' => {
>> +	    description => "Marks the device(s) as being able to be live-migrated (Experimental).",
> 
> The bit about QEMU and the driver needing to support it should be
> mentioned here.
> 
>> +	    type => 'boolean',
>> +	    optional => 1,
>> +	    default => 0,
>>   	},
>>   	map => {
>>   	    type => 'array',
>> @@ -123,6 +131,7 @@ sub options {
>>       return {
>>   	description => { optional => 1 },
>>   	mdev => { optional => 1 },
>> +	'live-migration-capable' => { optional => 1 },
>>   	map => {},
>>       };
>>   }

Re: [pve-devel] [PATCH guest-common 2/2] mapping: pci: optionally return the config in 'find_on_current_node'

[Dominik Csapak]

On 3/22/24 14:38, Fiona Ebner wrote:
> Am 18.03.24 um 12:18 schrieb Dominik Csapak:
>> this is useful to get to the config without having to parse it again
>>
> 
> You could also adapt the call sites that need it to use
> PVE::Mapping::PCI::config() and PVE::Mapping::PCI::get_node_mapping()
> instead of PVE::Mapping::PCI::find_on_current_node().
> 
> That would avoid overloading the return value. IMHO
> find_on_current_node() doesn't sound like it should return the full map
> config, even if just optionally.

yes that's a better idea.... as it stands we only use the 'find_on_current_node'
call only once anyway, so when we move the implementation to the current call
site, we could remove it here (if we'd ever need it again, it should
be trivial to introduce it then)

Re: [pve-devel] [PATCH qemu-server 3/4] check_local_resources: add more info per mapped device

[Dominik Csapak]

On 3/22/24 14:37, Fiona Ebner wrote:
> Am 18.03.24 um 12:18 schrieb Dominik Csapak:
>> such as the mapping name and if it's marked for live-migration (pci only)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm   |  2 +-
>>   PVE/QemuMigrate.pm |  5 +++--
>>   PVE/QemuServer.pm  | 10 ++++++----
>>   3 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 497987ff..4ecaeb91 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -4516,7 +4516,7 @@ __PACKAGE__->register_method({
>>   	$res->{local_disks} = [ values %$local_disks ];;
>>   
>>   	$res->{local_resources} = $local_resources;
>> -	$res->{'mapped-resources'} = $mapped_resources;
>> +	$res->{'mapped-resources'} =  [ map { "$_->{key}" } $mapped_resources->@* ];
> 
> Or should it become a hash? Then you could use 'keys' instead of map and
> a 'key' property.

yes that makes sense. Since we change the internal structure, we have
to adapt all call sites anyway...


> 
>>   
>>   	return $res;
>>   
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 8d9b35ae..6fe8157d 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -232,7 +232,7 @@ sub prepare {
>>       my ($loc_res, $mapped_res, $missing_mappings_by_node) = PVE::QemuServer::check_local_resources($conf, 1);
>>       my $blocking_resources = [];
>>       for my $res ($loc_res->@*) {
>> -	if (!grep($res, $mapped_res->@*)) {
> 
> Seems like this is currently broken. I.e. it should be $res eq $_

yep, seems i have to improve my tests running with that
(maybe i can whip up some automated ones to send along)

> 
>> +	if (!grep { $_->{key} eq $res } $mapped_res->@*) {
>>   	    push $blocking_resources->@*, $res;
>>   	}
>>       }
>> @@ -246,8 +246,9 @@ sub prepare {
>>   
>>       if (scalar($mapped_res->@*)) {
>>   	my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
>> +	my $mapped_text = join(", ", map { $_->{key} } $mapped_res->@*);
>>   	if ($running) {
>> -	    die "can't migrate running VM which uses mapped devices: " . join(", ", $mapped_res->@*) . "\n";
>> +	    die "can't migrate running VM which uses mapped devices: " . $mapped_text . "\n";
> 
> Style nit: no need for the concatenation anymore, can just use the
> variable inside the string.
> 
>>   	} elsif (scalar($missing_mappings->@*)) {
>>   	    die "can't migrate to '$self->{node}': missing mapped devices " . join(", ", $missing_mappings->@*) . "\n";
>>   	} else {
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 6e2c8052..ef3aee20 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -2600,14 +2600,16 @@ sub check_local_resources {
>>   	    next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
>>   	    if ($entry->{mapping}) {
>>   		$add_missing_mapping->('usb', $k, $entry->{mapping});
>> -		push @$mapped_res, $k;
>> +		push @$mapped_res, { key => $k, device => $entry->{mapping} };
> 
> Should we name the second key 'name'/'mapping'/'id' instead of 'device'?

yeah name is probably better here...

> 
>>   	    }
>>   	}
>>   	if ($k =~ m/^hostpci/) {
>>   	    my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
>> -	    if ($entry->{mapping}) {
>> -		$add_missing_mapping->('pci', $k, $entry->{mapping});
>> -		push @$mapped_res, $k;
>> +	    if (my $name = $entry->{mapping}) {
>> +		$add_missing_mapping->('pci', $k, $name);
>> +		my $mapped_device = { key => $k, device => $name };
>> +		$mapped_device->{'live-migration'} = 1 if $pci_map->{ids}->{$name}->{'live-migration-capable'};
> 
> Style nit: line too long
> 
>> +		push @$mapped_res, $mapped_device;
>>   	    }
>>   	}
>>   	# sockets are safe: they will recreated be on the target side post-migrate

Re: [pve-devel] [PATCH qemu-server 4/4] api: enable live migration for marked mapped pci devices

[Dominik Csapak]

On 3/22/24 14:37, Fiona Ebner wrote:
> Am 18.03.24 um 12:18 schrieb Dominik Csapak:
>> They have to be marked as 'live-migration-capable' in the mapping
>> config, and the driver and qemu must support it.
>>
>> For the gui checks, we now return a list of 'mapped-with-live-migration'
>> entries in the migration preflight api call too.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm   |  5 +++++
>>   PVE/QemuMigrate.pm | 12 ++++++++----
>>   2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 4ecaeb91..8581a529 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -4453,6 +4453,10 @@ __PACKAGE__->register_method({
>>   		type => 'array',
>>   		description => "List of mapped resources e.g. pci, usb"
>>   	    },
>> +	    'mapped-with-live-migration' => {
>> +		type => 'array',
>> +		description => "List of mapped resources that are marked as capable of live-migration",
>> +	    },
> 
> Should we merge this with 'mapped-resources' for the next major release
> (and add a reminder comment here)? I.e. make that return the objects
> with the additional information.

yeah i thought about that too, i general i'd really like to revampt the
whole api call and it's return values a bit, but that has to wait for
a major release since it will be a breaking change (if we don't want
to duplicate code and keep the old one around...)

i'll add a reminder comment to the api call

> 
>>   	},
>>       },
>>       code => sub {

Re: [pve-devel] [PATCH guest-common 1/2] mapping: pci: add 'live-migration-capable' flag to mappings

[Fiona Ebner]

Am 02.04.24 um 11:30 schrieb Dominik Csapak:
>>> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
>>> index 19ace98..0866175 100644
>>> --- a/src/PVE/Mapping/PCI.pm
>>> +++ b/src/PVE/Mapping/PCI.pm
>>> @@ -100,8 +100,16 @@ my $defaultData = {
>>>           maxLength => 4096,
>>>       },
>>>       mdev => {
>>> +        description => "Marks the device(s) as being capable of
>>> providing mediated devices.",
>>>           type => 'boolean',
>>>           optional => 1,
>>> +        default => 0,
>>> +    },
>>
>> Above should be its own patch. Most likely, I'm missing it, but where
>> exactly does the 'mdev' property from the mapping have an effect? Just
>> in the UI? At least telling from 'qm showcmd', the 'mdev' property for a
>> 'hostpciN' VM config entry will not be ignored even if the mapping has
>> 'mdev=0'. And it's also possible to run 'qm set 112 --hostpci0
>> mapping=bar,mdev=foo' without any warning if the mapping has 'mdev=0'.
>>
> 
> yeah sorry, i added that but overlooked it when adding the hunks...
> 
> AFAIR i intended to use the mdev property to safeguard the mapping
> like we do with the iommu groups, so when it changes, warn the user that
> the mapping changed (using it with mdevs wouldn't work anyway if that
> was set but the device wouldn't provide one)
> 
> aside from that, yes it currently only has effects on the gui,
> do we maybe want to make that stricter? (would be a breaking change imho)
> 

Maybe it's enough to mention in the description that the mdev property
from the hostpciN config will win? If we want to go one step further, we
could add warnings when mdev from the mapping and whether mdev in the
hostpciN is set do not agree. But the latter probably only makes sense
if we do want to enforce it at some point.

>>> +    'live-migration-capable' => {
>>> +        description => "Marks the device(s) as being able to be
>>> live-migrated (Experimental).",
>>
>> The bit about QEMU and the driver needing to support it should be
>> mentioned here.
>>
>>> +        type => 'boolean',
>>> +        optional => 1,
>>> +        default => 0,
>>>       },
>>>       map => {
>>>           type => 'array',
>>> @@ -123,6 +131,7 @@ sub options {
>>>       return {
>>>       description => { optional => 1 },
>>>       mdev => { optional => 1 },
>>> +    'live-migration-capable' => { optional => 1 },
>>>       map => {},
>>>       };
>>>   }
> 
>