Re: [Libguestfs] [PATCH v2v] input: -i vmx: Add support for NVMe devices

On 04/04/22 17:15, Richard W.M. Jones wrote: > We model NVMe devices in the source hypervisor. > > We currently assume that no one is using the namespaces feature of > NVMe, ie. that each source device will appear in a Linux guest as > /dev/nvme0n1, /dev/nvme1n1, etc. We could fix this if it is a > problem, but it requires adjusting the current assumption for > removable devices that slots are simple integers. > > The devices are mapped to virtio-blk, so in the target the device name > has to change from /dev/nvme0 to /dev/vda (etc.) > > Reported-by: Ming Xie > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2070530 > --- > convert/convert_linux.ml | 28 ++++++----- > convert/target_bus_assignment.ml | 4 +- > input/parse_domain_from_vmx.ml | 26 +++++++++- > lib/types.ml | 3 +- > lib/types.mli | 2 +- > tests/test-v2v-i-vmx-6.expected | 22 +++++++++ > tests/test-v2v-i-vmx-6.vmx | 84 ++++++++++++++++++++++++++++++++ > tests/test-v2v-i-vmx.sh | 4 +- > 8 files changed, 154 insertions(+), 19 deletions(-) > > diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml > index 58a2a2a84d..d5beb309c8 100644 > --- a/convert/convert_linux.ml > +++ b/convert/convert_linux.ml > @@ -1022,17 +1022,23 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = > > let map = > List.mapi ( > - fun i disk -> > - let block_prefix_before_conversion = > - match disk.s_controller with > - | Some Source_IDE -> ide_block_prefix > - | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) -> "sd" > - | Some Source_virtio_blk -> "vd" > - | None -> > - (* This is basically a guess. It assumes the source used IDE. *) > - ide_block_prefix in > - let source_dev = block_prefix_before_conversion ^ drive_name i in > - let target_dev = block_prefix_after_conversion ^ drive_name i in > + fun i { s_controller } -> > + let device_name_before_conversion i = > + match s_controller with > + | None > + (* If we don't have a controller, guess. Assumes the > + * source used IDE. > + *) > + | Some Source_IDE -> > + ide_block_prefix ^ drive_name i > + | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) -> > + "sd" ^ drive_name i > + | Some Source_virtio_blk -> "vd" ^ drive_name i > + | Some Source_NVME -> > + (* For NVMe assume no one is using namespaces. *) > + "nvme" ^ drive_name i ^ "n1" in > + let source_dev = device_name_before_conversion i in > + let target_dev = device_name_before_conversion i in (1) I find this change a bit too heavy-weight. Before, "block_prefix_before_conversion" was a string, and we used that string as a part of the concatenation in source_dev. Afterwards, "device_name_before_conversion" is a *function*, which contains the concatenations internally for each pattern, BUT this function also handles Source_NVME. I think that's what I would have preferred to see in separation -- first reworking the existing logic from "block_prefix_before_conversion" (string) to "device_name_before_conversion" (function), without observable changes in behavior, then adding the NVME branch. ... I guess if someone is more used to reading OCaml, doing both things at the same time may not be a big deal. (2) The computation of "target_dev" looks fishy. First, with the patch applied, we now only use "block_prefix_after_conversion" for mapping Xen disks (xvd), so the "block_prefix_after_conversion" name becomes too generic. Second, the effect on "target_dev" seems to conflict with the commit message (namely the statement about mapping NVME disks to virtio-blk), PLUS it seems to regress the current logic -- it would now map (e.g.) SCSI disks to sdX, not vdX (virtio-block). In other words, I think the "target_dev" calculation should be preserved. In turn, I think there is no use for the "i" parameter of "device_name_before_conversion", we could just compute "source_dev" as a string, using the outer "i". Something like: fun i { s_controller } -> let source_dev = match s_controller with | None (* If we don't have a controller, guess. Assumes the * source used IDE. *) | Some Source_IDE -> ide_block_prefix ^ drive_name i | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) -> "sd" ^ drive_name i | Some Source_virtio_blk -> "vd" ^ drive_name i | Some Source_NVME -> (* For NVMe assume no one is using namespaces. *) "nvme" ^ drive_name i ^ "n1" and target_dev = block_prefix_after_conversion ^ drive_name i in To summarize (1) and (2): I'd likely write a patch first just for sinking (= multiplying) the ^ drive_name i operation into the various pattern matches (calculting "source_dev" directly, without "block_prefix_before_conversion"), not touching "target_dev". Then a different patch for extending the patterns with NVME.

> source_dev, target_dev > ) source.s_disks in > > diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml > index 4b56a6e171..f8675cf29b 100644 > --- a/convert/target_bus_assignment.ml > +++ b/convert/target_bus_assignment.ml > @@ -73,8 +73,8 @@ let rec target_bus_assignment source_disks source_removables guestcaps = > | None -> ide_bus (* Wild guess, but should be safe. *) > | Some Source_virtio_blk -> virtio_blk_bus > | Some Source_IDE -> ide_bus > - | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA) -> > - scsi_bus in > + | Some (Source_virtio_SCSI | Source_SCSI | Source_SATA | > + Source_NVME) -> scsi_bus in > > match r.s_removable_slot with > | None -> (3) Not sure if *all* of the preexistent code is very useful here; I've never seen a virtio-blk CD-ROM (only IDE/SATA, or (virtio-)SCSI). I also can't really imagine an NVMe CD-ROM. So I guess the intent of this hunk is just to give the pattern match 100% coverage?

(Personally I'd be tempted to assert that a CDROM is never on virtio-blk, or NVMe.)

The rest looks OK to me. Thanks Laszlo > diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml > index 730f1177f7..b812edeb81 100644 > --- a/input/parse_domain_from_vmx.ml > +++ b/input/parse_domain_from_vmx.ml > @@ -103,6 +103,7 @@ let remote_file_exists uri path = > > let rec find_disks vmx vmx_source = > find_scsi_disks vmx vmx_source > + @ find_nvme_disks vmx vmx_source > @ find_ide_disks vmx vmx_source > > (* Find all SCSI hard disks. > @@ -129,6 +130,27 @@ and find_scsi_disks vmx vmx_source = > get_scsi_controller_target is_scsi_controller_target > scsi_device_types scsi_controller > > +(* Find all NVMe hard disks. > + * > + * In the VMX file: > + * nvme0.pcislotnumber = "192" > + * nvme0:0.fileName = "guest.vmdk" > + *) > +and find_nvme_disks vmx vmx_source = > + let get_nvme_controller_target ns = > + sscanf ns "nvme%d:%d" (fun c t -> c, t) > + in > + let is_nvme_controller_target ns = > + try ignore (get_nvme_controller_target ns); true > + with Scanf.Scan_failure _ | End_of_file | Failure _ -> false > + in > + let nvme_device_types = [ None ] in > + let nvme_controller = Source_NVME in > + > + find_hdds vmx vmx_source > + get_nvme_controller_target is_nvme_controller_target > + nvme_device_types nvme_controller > + > (* Find all IDE hard disks. > * > * In the VMX file: > @@ -153,12 +175,12 @@ and find_ide_disks vmx vmx_source = > and find_hdds vmx vmx_source > get_controller_target is_controller_target > device_types controller = > - (* Find namespaces matching '(ide|scsi)X:Y' with suitable deviceType. *) > + (* Find namespaces matching '(ide|scsi|nvme)X:Y' with suitable deviceType. *) > let hdds = > Parse_vmx.select_namespaces ( > function > | [ns] -> > - (* Check the namespace is '(ide|scsi)X:Y' *) > + (* Check the namespace is '(ide|scsi|nvme)X:Y' *) > if not (is_controller_target ns) then false > else ( > (* Check the deviceType is one we are looking for. *) > diff --git a/lib/types.ml b/lib/types.ml > index 5804c7c79f..92ed0e5251 100644 > --- a/lib/types.ml > +++ b/lib/types.ml > @@ -56,7 +56,7 @@ and source_disk = { > s_disk_id : int; > s_controller : s_controller option; > } > -and s_controller = Source_IDE | Source_SATA | Source_SCSI | > +and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME | > Source_virtio_blk | Source_virtio_SCSI > and source_removable = { > s_removable_type : s_removable_type; > @@ -194,6 +194,7 @@ and string_of_controller = function > | Source_IDE -> "ide" > | Source_SATA -> "sata" > | Source_SCSI -> "scsi" > + | Source_NVME -> "nvme" > | Source_virtio_blk -> "virtio-blk" > | Source_virtio_SCSI -> "virtio-scsi" > > diff --git a/lib/types.mli b/lib/types.mli > index dd2fe5925d..37238cd755 100644 > --- a/lib/types.mli > +++ b/lib/types.mli > @@ -103,7 +103,7 @@ and source_disk = { > } > (** A source disk. *) > > -and s_controller = Source_IDE | Source_SATA | Source_SCSI | > +and s_controller = Source_IDE | Source_SATA | Source_SCSI | Source_NVME | > Source_virtio_blk | Source_virtio_SCSI > (** Source disk controller. *) > > diff --git a/tests/test-v2v-i-vmx-6.expected b/tests/test-v2v-i-vmx-6.expected > new file mode 100644 > index 0000000000..1793b6b953 > --- /dev/null > +++ b/tests/test-v2v-i-vmx-6.expected > @@ -0,0 +1,22 @@ > + > +Source guest information (--print-source option): > + > + source name: esx6.7-rhel8.6-nvme-disk > +hypervisor type: vmware > + VM genid: > + memory: 2147483648 (bytes) > + nr vCPUs: 1 > + CPU vendor: > + CPU model: > + CPU topology: > + CPU features: > + firmware: bios > + display: > + sound: > +disks: > + 0 [nvme] > +removable media: > + > +NICs: > + Network "VM Network" mac: 00:50:56:ac:cf:51 [vmxnet3] > + > diff --git a/tests/test-v2v-i-vmx-6.vmx b/tests/test-v2v-i-vmx-6.vmx > new file mode 100644 > index 0000000000..9203c0cfcb > --- /dev/null > +++ b/tests/test-v2v-i-vmx-6.vmx > @@ -0,0 +1,84 @@ > +.encoding = "UTF-8" > +config.version = "8" > +virtualHW.version = "14" > +nvram = "esx6.7-rhel8.6-nvme-disk.nvram" > +pciBridge0.present = "TRUE" > +svga.present = "TRUE" > +pciBridge4.present = "TRUE" > +pciBridge4.virtualDev = "pcieRootPort" > +pciBridge4.functions = "8" > +pciBridge5.present = "TRUE" > +pciBridge5.virtualDev = "pcieRootPort" > +pciBridge5.functions = "8" > +pciBridge6.present = "TRUE" > +pciBridge6.virtualDev = "pcieRootPort" > +pciBridge6.functions = "8" > +pciBridge7.present = "TRUE" > +pciBridge7.virtualDev = "pcieRootPort" > +pciBridge7.functions = "8" > +vmci0.present = "TRUE" > +hpet0.present = "TRUE" > +floppy0.present = "FALSE" > +svga.vramSize = "8388608" > +memSize = "2048" > +powerType.powerOff = "default" > +powerType.suspend = "default" > +powerType.reset = "default" > +tools.upgrade.policy = "manual" > +sched.cpu.units = "mhz" > +sched.cpu.affinity = "all" > +vm.createDate = "1648710632879834" > +sata0.present = "TRUE" > +nvme0.present = "TRUE" > +nvme0:0.fileName = "nvme-disk.vmdk" > +sched.nvme0:0.shares = "normal" > +sched.nvme0:0.throughputCap = "off" > +nvme0:0.present = "TRUE" > +ethernet0.virtualDev = "vmxnet3" > +ethernet0.networkName = "VM Network" > +ethernet0.addressType = "vpx" > +ethernet0.generatedAddress = "00:50:56:ac:cf:51" > +ethernet0.uptCompatibility = "TRUE" > +ethernet0.present = "TRUE" > +sata0:0.startConnected = "FALSE" > +sata0:0.deviceType = "cdrom-raw" > +sata0:0.clientDevice = "TRUE" > +sata0:0.fileName = "emptyBackingString" > +sata0:0.present = "TRUE" > +displayName = "esx6.7-rhel8.6-nvme-disk" > +guestOS = "rhel8-64" > +toolScripts.afterPowerOn = "TRUE" > +toolScripts.afterResume = "TRUE" > +toolScripts.beforeSuspend = "TRUE" > +toolScripts.beforePowerOff = "TRUE" > +uuid.bios = "42 2c 17 6d 0b 31 4e 16-82 97 f0 b4 99 0b 86 09" > +vc.uuid = "50 2c fc c7 be fb b1 48-fc 7d 4b 39 b3 f5 de b5" > +migrate.hostLog = "esx6.7-rhel8.6-nvme-disk-7585773e.hlog" > +sched.cpu.min = "0" > +sched.cpu.shares = "normal" > +sched.mem.min = "0" > +sched.mem.minSize = "0" > +sched.mem.shares = "normal" > +migrate.encryptionMode = "opportunistic" > +numa.autosize.cookie = "10001" > +numa.autosize.vcpu.maxPerVirtualNode = "1" > +sched.swap.derivedName = "/vmfs/volumes/02155034-b2df70e7/esx6.7-rhel8.6-nvme-disk/esx6.7-rhel8.6-nvme-disk-34965f24.vswp" > +uuid.location = "56 4d c5 90 b1 ca f0 32-64 99 32 a7 d8 97 27 3a" > +nvme0:0.redo = "" > +pciBridge0.pciSlotNumber = "17" > +pciBridge4.pciSlotNumber = "21" > +pciBridge5.pciSlotNumber = "22" > +pciBridge6.pciSlotNumber = "23" > +pciBridge7.pciSlotNumber = "24" > +ethernet0.pciSlotNumber = "160" > +vmci0.pciSlotNumber = "32" > +sata0.pciSlotNumber = "33" > +nvme0.pciSlotNumber = "192" > +vmci0.id = "-1727298039" > +monitor.phys_bits_used = "43" > +vmotion.checkpointFBSize = "8388608" > +vmotion.checkpointSVGAPrimarySize = "8388608" > +cleanShutdown = "TRUE" > +softPowerOff = "TRUE" > +svga.guestBackedPrimaryAware = "TRUE" > +tools.syncTime = "FALSE" > \ No newline at end of file > diff --git a/tests/test-v2v-i-vmx.sh b/tests/test-v2v-i-vmx.sh > index db870bea05..d74ddfaaf8 100755 > --- a/tests/test-v2v-i-vmx.sh > +++ b/tests/test-v2v-i-vmx.sh > @@ -39,14 +39,14 @@ rm -f test-v2v-i-vmx-*.actual > # For the tests to succeed we need at least the fileName (VMDK input > # files) to exist. > > -fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk MSEdge-Win10_preview.vmdk" > +fns="BZ1308535_21disks.vmdk Fedora-20.vmdk RHEL-7.1-UEFI.vmdk Windows-7-x64.vmdk MSEdge-Win10_preview.vmdk nvme-disk.vmdk" > for fn in BZ1308535_21disks_{1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20}.vmdk; do > fns="$fns $fn" > done > > for fn in $fns; do qemu-img create -f vmdk $fn 512; done > > -for i in 1 2 3 4 5; do > +for i in 1 2 3 4 5 6; do > $VG virt-v2v --debug-gc \ > -i vmx test-v2v-i-vmx-$i.vmx \ > --print-source > test-v2v-i-vmx-$i.actual >