[pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation

[Thomas Lamprecht]

Am 31/07/2024 um 15:40 schrieb Aaron Lauterer:
> From: Oguz Bektas <o.bektas at proxmox.com>
> 
> now we can add nvme drives;
> 
> nvme0: local-lvm:vm-103-disk-0,size=32G
> 
> or
> 
> qm set VMID --nvme0 local-lvm:32
> 
> max number is 8 for now, as most real hardware has 1-3 nvme slots and
> can have a few more with pcie. most cases won't go over 8, if there's an
> actual usecase at some point this can always be increased without
> breaking anything (although the same isn't valid for decreasing it).
> 
> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>

A S-o-b from you hear would be good to have, you can also describe the
changes done on top of original patch like:

Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
 [ AL: rebased, ... ]
Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>


> ---
>  PVE/QemuServer.pm       | 23 ++++++++++++++++++++---
>  PVE/QemuServer/Drive.pm | 21 +++++++++++++++++++++
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index b26da50..7d8d75b 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -437,7 +437,7 @@ EODESC
>  	optional => 1,
>  	type => 'string', format => 'pve-qm-bootdisk',
>  	description => "Enable booting from specified disk. Deprecated: Use 'boot: order=foo;bar' instead.",
> -	pattern => '(ide|sata|scsi|virtio)\d+',
> +	pattern => '(ide|sata|scsi|virtio|nvme)\d+',
>      },
>      smp => {
>  	optional => 1,
> @@ -1433,7 +1433,10 @@ sub print_drivedevice_full {
>  		$device .= ",product=$product";
>  	    }
>  	}
> -
> +    } elsif ($drive->{interface} eq 'nvme') {
> +	my $path = $drive->{file};

^- this variable seems to be unused? 

> +	$drive->{serial} //= "$drive->{interface}$drive->{index}"; # serial is mandatory for nvme
> +	$device = "nvme,drive=drive-$drive->{interface}$drive->{index},id=$drive->{interface}$drive->{index}";
>      } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') {
>  	my $maxdev = ($drive->{interface} eq 'sata') ? $PVE::QemuServer::Drive::MAX_SATA_DISKS : 2;
>  	my $controller = int($drive->{index} / $maxdev);
> @@ -2396,7 +2399,7 @@ sub parse_vm_config {
>  	    } else {
>  		$key = 'ide2' if $key eq 'cdrom';
>  		my $fmt = $confdesc->{$key}->{format};
> -		if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata)$/) {
> +		if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata|nvme)$/) {
>  		    my $v = parse_drive($key, $value);
>  		    if (my $volid = filename_to_volume_id($vmid, $v->{file}, $v->{media})) {
>  			$v->{file} = $volid;
> @@ -4289,6 +4292,17 @@ sub vm_deviceplug {
>  	    warn $@ if $@;
>  	    die $err;
>          }
> +    } elsif ($deviceid =~ m/^(nvme)(\d+)$/) {
> +
> +	qemu_driveadd($storecfg, $vmid, $device);
> +
> +	my $devicefull = print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);

Would prefer having a word separator: my $device_full = ...

> +	eval { qemu_deviceadd($vmid, $devicefull); };
> +	if (my $err = $@) {
> +	    eval { qemu_drivedel($vmid, $deviceid); };
> +	    warn $@ if $@;
> +	    die $err;
> +        }
>      } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>  	return if !qemu_netdevadd($vmid, $conf, $arch, $device, $deviceid);
>  
> @@ -4361,6 +4375,9 @@ sub vm_deviceunplug {
>  
>  	qemu_iothread_del($vmid, "virtioscsi$device->{index}", $device)
>  	    if $conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single');
> +    } elsif ($deviceid =~ m/^(nvme)(\d+)$/) {
> +	qemu_devicedel($vmid, $deviceid);
> +	qemu_drivedel($vmid, $deviceid);
>      } elsif ($deviceid =~ m/^(net)(\d+)$/) {
>  	qemu_devicedel($vmid, $deviceid);
>  	qemu_devicedelverify($vmid, $deviceid);
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 6e98c09..f05ad26 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -33,6 +33,7 @@ PVE::JSONSchema::register_standard_option('pve-qm-image-format', {
>  
>  my $MAX_IDE_DISKS = 4;
>  my $MAX_SCSI_DISKS = 31;
> +my $MAX_NVME_DISKS = 8;
>  my $MAX_VIRTIO_DISKS = 16;
>  our $MAX_SATA_DISKS = 6;
>  our $MAX_UNUSED_DISKS = 256;
> @@ -315,6 +316,20 @@ my $scsidesc = {
>  };
>  PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
>  
> +my $nvme_fmt = {
> +    %drivedesc_base,
> +    %ssd_fmt,
> +    %wwn_fmt,
> +};
> +
> +my $nvmedesc = {

The existing `foodesc` use is ugly, not sure how much I'd care about style
compat here though, so IMO it would be slightly nicer to use:
my $nvme_desc = ...

> +    optional => 1,
> +    type => 'string', format => $nvme_fmt,
> +    description => "Use volume as NVME disk (n is 0 to " . ($MAX_NVME_DISKS -1) . ").",
> +};
> +
> +PVE::JSONSchema::register_standard_option("pve-qm-nvme", $nvmedesc);
> +
>  my $sata_fmt = {
>      %drivedesc_base,
>      %ssd_fmt,
> @@ -515,6 +530,11 @@ for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++)  {
>      $drivedesc_hash_with_alloc->{"scsi$i"} = $desc_with_alloc->('scsi', $scsidesc);
>  }
>  
> +for (my $i = 0; $i < $MAX_NVME_DISKS; $i++)  {
> +    $drivedesc_hash->{"nvme$i"} = $nvmedesc;
> +}
> +
> +
>  for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++)  {
>      $drivedesc_hash->{"virtio$i"} = $virtiodesc;
>      $drivedesc_hash_with_alloc->{"virtio$i"} = $desc_with_alloc->('virtio', $virtiodesc);
> @@ -541,6 +561,7 @@ sub valid_drive_names {
>              (map { "scsi$_" } (0 .. ($MAX_SCSI_DISKS - 1))),
>              (map { "virtio$_" } (0 .. ($MAX_VIRTIO_DISKS - 1))),
>              (map { "sata$_" } (0 .. ($MAX_SATA_DISKS - 1))),
> +            (map { "nvme$_" } (0 .. ($MAX_NVME_DISKS - 1))),
>              'efidisk0',
>              'tpmstate0');
>  }