* [pve-devel] [WIP qemu-server 2/3] nvme: make it somewhat work with current version
2024-07-31 13:40 ` [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation Aaron Lauterer
@ 2024-07-31 13:40 ` Aaron Lauterer
2024-07-31 13:40 ` [pve-devel] [WIP manager 3/3] gui: add nvme as a bus type for creating disks Aaron Lauterer
2024-07-31 15:30 ` [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation Thomas Lamprecht
2 siblings, 0 replies; 5+ messages in thread
From: Aaron Lauterer @ 2024-07-31 13:40 UTC (permalink / raw)
To: pve-devel
this patch is not meant to be applied but captures my efforts to make
the old series work (somewhat).
This way I was able to achieve hot-plugging on an i440 VM.
Hot plugging on a q35 VM did not work in my tests, but I think it is
related to how the NVME device was attached to the PCI tree of the VM.
By controlling this more directly, we should be able to attach it to a
PCI port/bridge that allows hot-plugging.
For some reason that I need to investigate further, giving the nvme
device the id `nvmeX` will cause issues when unplugging. It seems our
code will still see the `drive-nvmeX` drive and will add it to the list
of devices present. Therefore, the unplug code will think it did not
remove successfully. I worked around that for now by calling the nvme
device `id=nvmecontX` instead of `id=nvmeX`.
Live migration is still a blocker as the NVME QEMU device does not allow
it.
It fails with the following errors:
2024-07-31 09:31:14 migrate uri => unix:/run/qemu-server/102.migrate failed: VM 102 qmp command 'migrate' failed - State blocked by non-migratable device '0000:00:04.0/nvme'
2024-07-31 09:31:15 ERROR: online migrate failure - VM 102 qmp command 'migrate' failed - State blocked by non-migratable device '0000:00:04.0/nvme'
We need to check (with upstream) what is missing. Also if there are
plans upstream to implement the missing pieces, or if we could do it
ourselves.
I found the following older thread on the qemu mailing list:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05788.html
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
PVE/QemuServer.pm | 15 +++++++++------
PVE/QemuServer/Drive.pm | 1 +
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 7d8d75b..6dc3eb2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1436,7 +1436,7 @@ sub print_drivedevice_full {
} elsif ($drive->{interface} eq 'nvme') {
my $path = $drive->{file};
$drive->{serial} //= "$drive->{interface}$drive->{index}"; # serial is mandatory for nvme
- $device = "nvme,drive=drive-$drive->{interface}$drive->{index},id=$drive->{interface}$drive->{index}";
+ $device = "nvme,drive=drive-$drive->{interface}$drive->{index},id=$drive->{interface}cont$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);
@@ -4292,11 +4292,10 @@ sub vm_deviceplug {
warn $@ if $@;
die $err;
}
- } elsif ($deviceid =~ m/^(nvme)(\d+)$/) {
-
+ } elsif ($deviceid =~ m/^nvme(\d+)$/) {
qemu_driveadd($storecfg, $vmid, $device);
- my $devicefull = print_drivedevice_full($storecfg, $conf, $vmid, $device, $arch, $machine_type);
+ my $devicefull = print_drivedevice_full($storecfg, $conf, $vmid, $device, undef, $arch, $machine_type);
eval { qemu_deviceadd($vmid, $devicefull); };
if (my $err = $@) {
eval { qemu_drivedel($vmid, $deviceid); };
@@ -4375,8 +4374,12 @@ 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);
+ } elsif ($deviceid =~ m/^nvme(\d+)$/) {
+ my $device = parse_drive($deviceid, $conf->{$deviceid});
+
+ my $nvmecont = "nvmecont$1";
+ qemu_devicedel($vmid, $nvmecont);
+ qemu_devicedelverify($vmid, $nvmecont);
qemu_drivedel($vmid, $deviceid);
} elsif ($deviceid =~ m/^(net)(\d+)$/) {
qemu_devicedel($vmid, $deviceid);
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index f05ad26..b055674 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -532,6 +532,7 @@ for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++) {
for (my $i = 0; $i < $MAX_NVME_DISKS; $i++) {
$drivedesc_hash->{"nvme$i"} = $nvmedesc;
+ $drivedesc_hash_with_alloc->{"nvme$i"} = $desc_with_alloc->('nvme', $nvmedesc);
}
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [WIP manager 3/3] gui: add nvme as a bus type for creating disks
2024-07-31 13:40 ` [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation Aaron Lauterer
2024-07-31 13:40 ` [pve-devel] [WIP qemu-server 2/3] nvme: make it somewhat work with current version Aaron Lauterer
@ 2024-07-31 13:40 ` Aaron Lauterer
2024-07-31 15:30 ` [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation Thomas Lamprecht
2 siblings, 0 replies; 5+ messages in thread
From: Aaron Lauterer @ 2024-07-31 13:40 UTC (permalink / raw)
To: pve-devel
From: Oguz Bektas <o.bektas@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
www/manager6/Utils.js | 3 ++-
www/manager6/form/BusTypeSelector.js | 2 ++
www/manager6/form/ControllerSelector.js | 2 +-
www/manager6/qemu/CloudInit.js | 4 ++--
www/mobile/QemuSummary.js | 2 +-
5 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index db86fa9a..aba72135 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -13,7 +13,7 @@ Ext.define('PVE.Utils', {
toolkit: undefined, // (extjs|touch), set inside Toolkit.js
- bus_match: /^(ide|sata|virtio|scsi)(\d+)$/,
+ bus_match: /^(ide|sata|virtio|scsi|nvme)\d+$/,
log_severity_hash: {
0: "panic",
@@ -1560,6 +1560,7 @@ Ext.define('PVE.Utils', {
ide: 4,
sata: 6,
scsi: 31,
+ nvme: 8,
virtio: 16,
unused: 256,
},
diff --git a/www/manager6/form/BusTypeSelector.js b/www/manager6/form/BusTypeSelector.js
index 0f040229..fbc6f222 100644
--- a/www/manager6/form/BusTypeSelector.js
+++ b/www/manager6/form/BusTypeSelector.js
@@ -16,6 +16,8 @@ Ext.define('PVE.form.BusTypeSelector', {
me.comboItems.push(['scsi', 'SCSI']);
+ me.comboItems.push(['nvme', 'NVMe']);
+
if (me.withUnused) {
me.comboItems.push(['unused', 'Unused']);
}
diff --git a/www/manager6/form/ControllerSelector.js b/www/manager6/form/ControllerSelector.js
index d7c2625d..8c766598 100644
--- a/www/manager6/form/ControllerSelector.js
+++ b/www/manager6/form/ControllerSelector.js
@@ -39,7 +39,7 @@ Ext.define('PVE.form.ControllerSelector', {
deviceid.setValue(2);
return;
}
- clist = ['ide', 'scsi', 'sata'];
+ clist = ['ide', 'scsi', 'sata', 'nvme'];
} else {
// in most cases we want to add a disk to the same controller we previously used
clist = PVE.Utils.sortByPreviousUsage(me.vmconfig);
diff --git a/www/manager6/qemu/CloudInit.js b/www/manager6/qemu/CloudInit.js
index 49519726..4ea58641 100644
--- a/www/manager6/qemu/CloudInit.js
+++ b/www/manager6/qemu/CloudInit.js
@@ -135,7 +135,7 @@ Ext.define('PVE.qemu.CloudInit', {
var id = record.data.key;
var value = record.data.value;
var ciregex = new RegExp("vm-" + me.pveSelNode.data.vmid + "-cloudinit");
- if (id.match(/^(ide|scsi|sata)\d+$/) && ciregex.test(value)) {
+ if (id.match(/^(ide|scsi|sata|nvme)\d+$/) && ciregex.test(value)) {
found = id;
me.ciDriveId = found;
me.ciDrive = value;
@@ -330,7 +330,7 @@ Ext.define('PVE.qemu.CloudInit', {
};
}
- PVE.Utils.forEachBus(['ide', 'scsi', 'sata'], function(type, id) {
+ PVE.Utils.forEachBus(['ide', 'scsi', 'sata', 'nvme'], function(type, id) {
me.rows[type+id] = {
visible: false,
};
diff --git a/www/mobile/QemuSummary.js b/www/mobile/QemuSummary.js
index c416ea7d..a484fcb6 100644
--- a/www/mobile/QemuSummary.js
+++ b/www/mobile/QemuSummary.js
@@ -12,7 +12,7 @@ Ext.define('PVE.QemuSummary', {
config_keys: [
'name', 'memory', 'sockets', 'cores', 'ostype', 'bootdisk', /^net\d+/,
- /^ide\d+/, /^virtio\d+/, /^sata\d+/, /^scsi\d+/, /^unused\d+/,
+ /^ide\d+/, /^virtio\d+/, /^sata\d+/, /^scsi\d+/, /^nvme\d+/, /^unused\d+/
],
initialize: function() {
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation
2024-07-31 13:40 ` [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation Aaron Lauterer
2024-07-31 13:40 ` [pve-devel] [WIP qemu-server 2/3] nvme: make it somewhat work with current version Aaron Lauterer
2024-07-31 13:40 ` [pve-devel] [WIP manager 3/3] gui: add nvme as a bus type for creating disks Aaron Lauterer
@ 2024-07-31 15:30 ` Thomas Lamprecht
2 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2024-07-31 15:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Aaron Lauterer
Am 31/07/2024 um 15:40 schrieb Aaron Lauterer:
> From: Oguz Bektas <o.bektas@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@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@proxmox.com>
[ AL: rebased, ... ]
Signed-off-by: Aaron Lauterer <a.lauterer@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');
> }
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread