Libguestfs - Feb 2023 - [COMMON PATCH 5/5] inject_virtio_win: make virtio-scsi the default block driver

Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"),
f0afc439
("Remove guestcaps_block_type Virtio_SCSI") support for installing
virtio-scsi driver is missing in virt-v2v.  AFAIU plans and demands for
bringing this feature back have been out there for a while.  E.g. I've
found a corresponding issue which is still open [1].

The code in b28cd1dc, f0afc439 was removed due to removing the old in-place
support.  However, having the new in-place support present and bringing
this same code (partially) back with several additions and improvements,
I'm able to successfully convert and boot a Win guest with a virtio-scsi
disk controller.  So please consider the following implementation of
this feature.

[1] https://github.com/libguestfs/virt-v2v/issues/12


v2v:

Andrey Drobyshev (2):
  Revert "Remove guestcaps_block_type Virtio_SCSI"
  convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block
    type

 convert/convert.ml                   | 2 +-
 convert/convert_linux.ml             | 9 +++++++--
 convert/convert_windows.ml           | 1 +
 convert/target_bus_assignment.ml     | 1 +
 lib/create_ovf.ml                    | 1 +
 lib/types.ml                         | 3 ++-
 lib/types.mli                        | 2 +-
 output/openstack_image_properties.ml | 7 +++++++
 9 files changed, 22 insertions(+), 6 deletions(-)

common:

Andrey Drobyshev (2):
  inject_virtio_win: add Virtio_SCSI to block_type
  inject_virtio_win: make virtio-scsi the default block driver

Roman Kagan (1):
  inject_virtio_win: match only vendor/device

 mlcustomize/inject_virtio_win.ml  | 25 ++++++++++++++++---------
 mlcustomize/inject_virtio_win.mli |  2 +-
 2 files changed, 17 insertions(+), 10 deletions(-)

--
2.31.1

This code is needed to check whether virtio-scsi driver was installed.

This reverts commit f0afc439524853508938b2bfc758896f053462e3.
---
 convert/convert.ml                   | 2 +-
 convert/convert_linux.ml             | 9 +++++++--
 convert/target_bus_assignment.ml     | 1 +
 lib/create_ovf.ml                    | 1 +
 lib/types.ml                         | 3 ++-
 lib/types.mli                        | 2 +-
 output/openstack_image_properties.ml | 7 +++++++
 7 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/convert/convert.ml b/convert/convert.ml
index 0aa0e5cd..084619c8 100644
--- a/convert/convert.ml
+++ b/convert/convert.ml
@@ -252,7 +252,7 @@ and do_convert g source inspect i_firmware
keep_serial_console interfaces    (* Did we manage to install virtio drivers? *)
   if not (quiet ()) then (
     match guestcaps.gcaps_block_bus with
-    | Virtio_blk ->
+    | Virtio_blk | Virtio_SCSI ->
         info (f_"This guest has virtio drivers installed.")
     | IDE ->
         info (f_"This guest does not have virtio drivers installed.")
diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
index d5c0f24d..dab4f36d 100644
--- a/convert/convert_linux.ml
+++ b/convert/convert_linux.ml
@@ -1068,8 +1068,12 @@ let convert (g : G.guestfs) source inspect i_firmware
keep_serial_console _      (* Update 'alias scsi_hostadapter ...' *)
     let paths = augeas_modprobe ". =~
regexp('scsi_hostadapter.*')" in
     (match block_type with
-    | Virtio_blk ->
-      let block_module = "virtio_blk" in
+    | Virtio_blk | Virtio_SCSI ->
+      let block_module +        match block_type with
+        | Virtio_blk -> "virtio_blk"
+        | Virtio_SCSI -> "virtio_scsi"
+        | IDE -> assert false in
 
       if paths <> [] then (
         (* There's only 1 scsi controller in the converted guest.
@@ -1142,6 +1146,7 @@ let convert (g : G.guestfs) source inspect i_firmware
keep_serial_console _      let block_prefix_after_conversion        match
block_type with
       | Virtio_blk -> "vd"
+      | Virtio_SCSI -> "sd"
       | IDE -> ide_block_prefix in
 
     let map diff --git a/convert/target_bus_assignment.ml
b/convert/target_bus_assignment.ml
index 54c9516b..d13340c7 100644
--- a/convert/target_bus_assignment.ml
+++ b/convert/target_bus_assignment.ml
@@ -35,6 +35,7 @@ let rec target_bus_assignment source_disks source_removables
guestcaps      let bus        match guestcaps.gcaps_block_bus with
       | Virtio_blk -> virtio_blk_bus
+      | Virtio_SCSI -> scsi_bus
       | IDE -> ide_bus in
     List.iteri (
       fun i d ->
diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml
index 5e444868..f0b32e01 100644
--- a/lib/create_ovf.ml
+++ b/lib/create_ovf.ml
@@ -920,6 +920,7 @@ and add_disks sizes guestcaps output_alloc output_format
           "ovf:disk-interface",
           (match guestcaps.gcaps_block_bus with
           | Virtio_blk -> "VirtIO"
+          | Virtio_SCSI -> "VirtIO_SCSI"
           | IDE -> "IDE");
           "ovf:disk-type", "System"; (* RHBZ#744538 *)
           "ovf:boot", if is_bootable_drive then "True" else
"False";
diff --git a/lib/types.ml b/lib/types.ml
index e16da007..75c14fd4 100644
--- a/lib/types.ml
+++ b/lib/types.ml
@@ -400,12 +400,13 @@ type guestcaps = {
   gcaps_arch_min_version : int;
   gcaps_virtio_1_0 : bool;
 }
-and guestcaps_block_type = Virtio_blk | IDE
+and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE
 and guestcaps_net_type = Virtio_net | E1000 | RTL8139
 and guestcaps_machine = I440FX | Q35 | Virt
 
 let string_of_block_type = function
   | Virtio_blk -> "virtio-blk"
+  | Virtio_SCSI -> "virtio-scsi"
   | IDE -> "ide"
 let string_of_net_type = function
   | Virtio_net -> "virtio-net"
diff --git a/lib/types.mli b/lib/types.mli
index 4a183dd3..65ef2e35 100644
--- a/lib/types.mli
+++ b/lib/types.mli
@@ -280,7 +280,7 @@ type guestcaps = {
 }
 (** Guest capabilities after conversion.  eg. Was virtio found or installed? *)
 
-and guestcaps_block_type = Virtio_blk | IDE
+and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE
 and guestcaps_net_type = Virtio_net | E1000 | RTL8139
 and guestcaps_machine = I440FX | Q35 | Virt
 
diff --git a/output/openstack_image_properties.ml
b/output/openstack_image_properties.ml
index c75d72fe..c76ad913 100644
--- a/output/openstack_image_properties.ml
+++ b/output/openstack_image_properties.ml
@@ -35,6 +35,7 @@ let create source inspect { target_buses; guestcaps;
target_firmware }      "hw_disk_bus",
     (match guestcaps.gcaps_block_bus with
      | Virtio_blk -> "virtio"
+     | Virtio_SCSI -> "scsi"
      | IDE -> "ide");
     "hw_vif_model",
     (match guestcaps.gcaps_net_bus with
@@ -69,6 +70,12 @@ let create source inspect { target_buses; guestcaps;
target_firmware }        List.push_back properties ("hw_cpu_threads",
string_of_int threads);
   );
 
+  (match guestcaps.gcaps_block_bus with
+   | Virtio_SCSI ->
+      List.push_back properties ("hw_scsi_model",
"virtio-scsi")
+   | Virtio_blk | IDE -> ()
+  );
+
   (match inspect.i_major_version, inspect.i_minor_version with
    | 0, 0 -> ()
    | x, 0 -> List.push_back properties ("os_version",
string_of_int x)
-- 
2.31.1

From: Roman Kagan <rkagan at virtuozzo.com>

Since different hypervisor vendors are allowed to use their own vendor-id
as PCI subsystem-vendor-id for virtio devices, during v2v conversion it
makes sense to only match the vendor/device and not the full device
"path"
in the Windows registry.  This way the code will remain universal but will
work for different hypervisor vendors.

Signed-off-by: Roman Kagan <rkagan at virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
---
 mlcustomize/inject_virtio_win.ml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml
index 5f4aab7..acb3a93 100644
--- a/mlcustomize/inject_virtio_win.ml
+++ b/mlcustomize/inject_virtio_win.ml
@@ -110,10 +110,10 @@ and get_inspection g root      virtio_win = "";
was_set = false }
 
 let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}"
-let viostor_legacy_pciid =
"VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00"
-let viostor_modern_pciid =
"VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01"
-let vioscsi_legacy_pciid =
"VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00"
-let vioscsi_modern_pciid =
"VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01"
+let viostor_legacy_pciid = "VEN_1AF4&DEV_1001"
+let viostor_modern_pciid = "VEN_1AF4&DEV_1042"
+let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004"
+let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048"
 
 let rec inject_virtio_win_drivers ({ g } as t) reg    (* Copy the virtio
drivers to the guest. *)
-- 
2.31.1

Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
---
 mlcustomize/inject_virtio_win.ml  | 2 +-
 mlcustomize/inject_virtio_win.mli | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml
index acb3a93..62f7710 100644
--- a/mlcustomize/inject_virtio_win.ml
+++ b/mlcustomize/inject_virtio_win.ml
@@ -51,7 +51,7 @@ type t = {
       the user to select where they want to get drivers from. XXX *)
 }
 
-type block_type = Virtio_blk | IDE
+type block_type = Virtio_blk | Virtio_SCSI | IDE
 and net_type = Virtio_net | E1000 | RTL8139
 and machine_type = I440FX | Q35 | Virt
 
diff --git a/mlcustomize/inject_virtio_win.mli
b/mlcustomize/inject_virtio_win.mli
index 0ced02e..58169e1 100644
--- a/mlcustomize/inject_virtio_win.mli
+++ b/mlcustomize/inject_virtio_win.mli
@@ -20,7 +20,7 @@
 
 type t (** Handle *)
 
-type block_type = Virtio_blk | IDE
+type block_type = Virtio_blk | Virtio_SCSI | IDE
 and net_type = Virtio_net | E1000 | RTL8139
 and machine_type = I440FX | Q35 | Virt
 
-- 
2.31.1

If virtio-scsi is present among the tools for this version of Windows,
prefer it over virtio-blk.

Originally-by: Roman Kagan <rkagan at virtuozzo.com>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
---
 mlcustomize/inject_virtio_win.ml | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml
index 62f7710..751a9b0 100644
--- a/mlcustomize/inject_virtio_win.ml
+++ b/mlcustomize/inject_virtio_win.ml
@@ -176,7 +176,8 @@ let rec inject_virtio_win_drivers ({ g } as t) reg    else (
     (* Can we install the block driver? *)
     let block : block_type -      let filenames = ["virtio_blk";
"vrtioblk"; "viostor"] in
+      (* First list item is the default driver. *)
+      let filenames = ["vioscsi"; "virtio_blk";
"vrtioblk"; "viostor"] in
       let viostor_driver = try (
         Some (
           List.find (
@@ -200,10 +201,16 @@ let rec inject_virtio_win_drivers ({ g } as t) reg        
let target = sprintf "%s/system32/drivers/%s.sys"
                              t.i_windows_systemroot driver_name in
         let target = g#case_sensitive_path target in
+        let legacy_pciid, modern_pciid = (
+          if driver_name = "vioscsi" then
+            vioscsi_legacy_pciid, vioscsi_modern_pciid
+          else
+            viostor_legacy_pciid, viostor_modern_pciid
+        ) in
         g#cp source target;
-        add_guestor_to_registry t reg driver_name viostor_legacy_pciid;
-        add_guestor_to_registry t reg driver_name viostor_modern_pciid;
-        Virtio_blk in
+        add_guestor_to_registry t reg driver_name legacy_pciid;
+        add_guestor_to_registry t reg driver_name modern_pciid;
+        (if driver_name = "vioscsi" then Virtio_SCSI else Virtio_blk)
in
 
     (* Can we install the virtio-net driver? *)
     let net : net_type -- 
2.31.1

On 2/22/23 19:20, Andrey Drobyshev wrote:
> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"),
f0afc439
> ("Remove guestcaps_block_type Virtio_SCSI") support for
installing
> virtio-scsi driver is missing in virt-v2v.  AFAIU plans and demands for
> bringing this feature back have been out there for a while.  E.g. I've
> found a corresponding issue which is still open [1].
> 
> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place
> support.  However, having the new in-place support present and bringing
> this same code (partially) back with several additions and improvements,
> I'm able to successfully convert and boot a Win guest with a
virtio-scsi
> disk controller.  So please consider the following implementation of
> this feature.
> 
> [1] https://github.com/libguestfs/virt-v2v/issues/12
(Preamble: I'm 100% deferring to Rich on this, so take my comments for
what they are worth.)

In my opinion, the argument made is weak. This cover letter does not say
"why" -- it does not explain why virtio-blk is insufficient for
*Virtuozzo*.

Second, reference [1] -- issue #12 -- doesn't sound too convincing. It
writes, "opinionated qemu-based VMs that exclusively use UEFI and only
virtio devices". "Opinionated" is the key word there. They're
entitled
to an opinion, they're not entitled to others conforming to their
opinion. I happen to be opinionated as well, and I hold the opposite view.

(BTW even if they insist on UEFI + virtio, which I do sympathize with,
requiring virtio-scsi exclusively is hard to sell. In particular,
virtio-blk nowadays has support for trim/discard, so the main killer
feature of virtio-scsi is no longer unique to virtio-scsi. Virtio-blk is
also simpler code and arguably faster. Note: I don't want to convince
anyone about what *they* support, just pointing out that virt-v2v
outputting solely virtio-blk disks is entirely fine, as far as
virt-v2v's mission is concerned -- "salvage 'pet' (not
'cattle') VMs
from proprietary hypervisors, and make sure they boot". Virtio-blk is
sufficient for booting, further tweaks are up to the admin (again,
virt-v2v is not for mass/cattle conversions). The "Hetzner Cloud" is
not
a particular output module of virt-v2v, so I don't know why virt-v2v's
mission should extend to making the converted VM bootable on "Hetzner
Cloud".)

Rich has recently added tools for working with the virtio devices in
windows guests; maybe those can be employed as extra (manual) steps
before or after the conversion.

Third, the last patch in the series is overreaching IMO; it switches the
default. That causes a behavior change for such conversions that have
been working well, and have been thoroughly tested. It doesn't just add
a new use case, it throws away an existent use case for the new one's
sake, IIUC. I don't like that.

Again -- fully deferring to Rich on the final verdict (and the review).

Laszlo
> 
> 
> v2v:
> 
> Andrey Drobyshev (2):
>   Revert "Remove guestcaps_block_type Virtio_SCSI"
>   convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block
>     type
> 
>  convert/convert.ml                   | 2 +-
>  convert/convert_linux.ml             | 9 +++++++--
>  convert/convert_windows.ml           | 1 +
>  convert/target_bus_assignment.ml     | 1 +
>  lib/create_ovf.ml                    | 1 +
>  lib/types.ml                         | 3 ++-
>  lib/types.mli                        | 2 +-
>  output/openstack_image_properties.ml | 7 +++++++
>  9 files changed, 22 insertions(+), 6 deletions(-)
> 
> common:
> 
> Andrey Drobyshev (2):
>   inject_virtio_win: add Virtio_SCSI to block_type
>   inject_virtio_win: make virtio-scsi the default block driver
> 
> Roman Kagan (1):
>   inject_virtio_win: match only vendor/device
> 
>  mlcustomize/inject_virtio_win.ml  | 25 ++++++++++++++++---------
>  mlcustomize/inject_virtio_win.mli |  2 +-
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> --
> 2.31.1
>

On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev
wrote:
> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"),
f0afc439
> ("Remove guestcaps_block_type Virtio_SCSI") support for
installing
> virtio-scsi driver is missing in virt-v2v.  AFAIU plans and demands for
> bringing this feature back have been out there for a while.  E.g. I've
> found a corresponding issue which is still open [1].
> 
> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place
> support.  However, having the new in-place support present and bringing
> this same code (partially) back with several additions and improvements,
> I'm able to successfully convert and boot a Win guest with a
virtio-scsi
> disk controller.  So please consider the following implementation of
> this feature.
> 
> [1] https://github.com/libguestfs/virt-v2v/issues/12
So you're right that we ended up removing virtio-scsi support because
in-place conversion was temporarily removed, and it wasn't restored
when the new virt-v2v-in-place tool was added.

I looked at the patches, and I don't have any objection to 1-4.

Patch 5 is the tricky one because it changes the default, but we
(Red Hat) are likely to stick with virtio-blk for better or worse.

Could we make this configurable?  We've traditionally shied away from
adding virt-v2v command line options which are purely for fine-tuning
conversions.  The reason for this is that when you're doing bulk
conversions of hundreds of VMs from VMware, you want virt-v2v to do
"the best it can", and it's not really feasible to hand configure
every conversion.  (There are some historical exceptions to this, like
--root, but don't look at those!)  I know the bulk conversion case
doesn't apply to Virtuozzo, but it does matter for us.

It could be argued that this isn't the same, because if we're bulk
converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM,
but per target hypervisor.  So a command line option would be OK in
this case.

BTW previously we "configured" this preference through the input
libvirt XML.  That was a terrible idea, so I'd prefer to avoid it this
time around.

Anyway if you can think of a better version of patch 5 then I think we
can accept this.

Thanks,

Rich.
> 
> v2v:
> 
> Andrey Drobyshev (2):
>   Revert "Remove guestcaps_block_type Virtio_SCSI"
>   convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block
>     type
> 
>  convert/convert.ml                   | 2 +-
>  convert/convert_linux.ml             | 9 +++++++--
>  convert/convert_windows.ml           | 1 +
>  convert/target_bus_assignment.ml     | 1 +
>  lib/create_ovf.ml                    | 1 +
>  lib/types.ml                         | 3 ++-
>  lib/types.mli                        | 2 +-
>  output/openstack_image_properties.ml | 7 +++++++
>  9 files changed, 22 insertions(+), 6 deletions(-)
> 
> common:
> 
> Andrey Drobyshev (2):
>   inject_virtio_win: add Virtio_SCSI to block_type
>   inject_virtio_win: make virtio-scsi the default block driver
> 
> Roman Kagan (1):
>   inject_virtio_win: match only vendor/device
> 
>  mlcustomize/inject_virtio_win.ml  | 25 ++++++++++++++++---------
>  mlcustomize/inject_virtio_win.mli |  2 +-
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> --
> 2.31.1
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit