[Libguestfs] [PATCH v2v] input: -i vmx: Add support for NVMe devices
Laszlo Ersek
lersek at redhat.com
Tue Apr 5 14:26:53 UTC 2022
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
>
More information about the Libguestfs
mailing list