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

[Dominik Csapak <d.csapak@proxmox.com>]

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 => {},
>>       };
>>   }