Libguestfs - Apr 2016 - [PATCH v3] v2v: add support for virtio-scsi

Virtio-SCSI offers a number of advantages over virtio-blk, in
particular, it supports SCSI UNMAP (aka trim) which is crucial for
keeping the virtual drive from wasting host disk space.

This patch adds support for virtio-scsi as the virtual disk connection
type both on input and on output of v2v.

Virtio-blk remains the default, so for now virtio-scsi-based guests can
only be produced in --in-place mode.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v3:
 - resend off a proper branch

v2:
 - corrected OVF and glance output
 - stopped abusing debug stringifier to obtain linux module name
 - droped unneeded braces

 v2v/OVF.ml                   |  4 +++-
 v2v/convert_linux.ml         | 20 ++++++++++++++------
 v2v/input_libvirtxml.ml      | 32 ++++++++++++++++++++------------
 v2v/output_glance.ml         |  5 +++++
 v2v/test-v2v-print-source.sh |  2 +-
 v2v/types.ml                 |  9 ++++++---
 v2v/types.mli                | 11 ++++-------
 v2v/v2v.ml                   | 13 ++++++++-----
 v2v/windows_virtio.ml        | 30 ++++++++++++++++++++++++------
 9 files changed, 85 insertions(+), 41 deletions(-)

diff --git a/v2v/OVF.ml b/v2v/OVF.ml
index f8afc8c..87ce561 100644
--- a/v2v/OVF.ml
+++ b/v2v/OVF.ml
@@ -454,7 +454,9 @@ and add_disks targets guestcaps output_alloc sd_uuid
image_uuids vol_uuids ovf            "ovf:format",
"http://en.wikipedia.org/wiki/Byte"; (* wtf? *)
           "ovf:disk-interface",
           (match guestcaps.gcaps_block_bus with
-          | Virtio_blk -> "VirtIO" | IDE -> "IDE");
+          | Virtio_blk -> "VirtIO"
+          | Virtio_SCSI -> "VirtIO_SCSI"
+          | IDE -> "IDE");
           "ovf:disk-type", "System"; (* RHBZ#744538 *)
           "ovf:boot", if is_boot_drive then "True" else
"False";
         ] in
diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 0a2e439..04b7739 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -803,8 +803,9 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps             * major number of vdX block devices. If we change it,
RHEL 3
            * KVM guests won't boot.
            *)
-          [ "virtio"; "virtio_ring";
"virtio_blk"; "virtio_net";
-            "virtio_pci" ]
+          List.filter (fun m -> List.mem m kernel.ki_modules)
+                      [ "virtio"; "virtio_ring";
"virtio_blk";
+                        "virtio_scsi"; "virtio_net";
"virtio_pci" ]
         else
           [ "sym53c8xx" (* XXX why not "ide"? *) ] in
 
@@ -1134,7 +1135,13 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps      (* Update 'alias scsi_hostadapter ...' *)
     let paths = augeas_modprobe ". =~
regexp('scsi_hostadapter.*')" in
     match block_type with
-    | Virtio_blk ->
+    | Virtio_blk | Virtio_SCSI ->
+      let block_module +        match block_type with
+        | Virtio_blk -> "virtio_blk"
+        | Virtio_SCSI -> "virtio_scsi"
+        | _ -> raise Not_found in
+
       if paths <> [] then (
         (* There's only 1 scsi controller in the converted guest.
          * Convert only the first scsi_hostadapter entry to virtio
@@ -1148,14 +1155,14 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps          List.iter (fun path -> ignore (g#aug_rm path))
           (List.rev paths_to_delete);
 
-        g#aug_set (path ^ "/modulename") "virtio_blk"
+        g#aug_set (path ^ "/modulename") block_module
       ) else (
         (* We have to add a scsi_hostadapter. *)
         let modpath = discover_modpath () in
         g#aug_set (sprintf "/files%s/alias[last()+1]" modpath)
           "scsi_hostadapter";
         g#aug_set (sprintf "/files%s/alias[last()]/modulename"
modpath)
-          "virtio_blk"
+          block_module
       )
     | IDE ->
       (* There is no scsi controller in an IDE guest. *)
@@ -1247,6 +1254,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps      let block_prefix_after_conversion        match
block_type with
       | Virtio_blk -> "vd"
+      | Virtio_SCSI -> "sd"
       | IDE -> ide_block_prefix in
 
     let map @@ -1255,7 +1263,7 @@ let rec convert ~keep_serial_console (g :
G.guestfs) inspect source rcaps            let block_prefix_before_conversion   
match disk.s_controller with
             | Some Source_IDE -> ide_block_prefix
-            | Some Source_SCSI -> "sd"
+            | Some Source_virtio_SCSI | Some Source_SCSI -> "sd"
             | Some Source_virtio_blk -> "vd"
             | None ->
               (* This is basically a guess.  It assumes the source used IDE. *)
diff --git a/v2v/input_libvirtxml.ml b/v2v/input_libvirtxml.ml
index 9d8963d..231931f 100644
--- a/v2v/input_libvirtxml.ml
+++ b/v2v/input_libvirtxml.ml
@@ -181,6 +181,12 @@ let parse_libvirt_xml ?conn xml           None
     ) in
 
+  (* Presence of virtio-scsi controller. *)
+  let has_virtio_scsi +    let obj = Xml.xpath_eval_expression xpathctx
+               
"/domain/devices/controller[@model='virtio-scsi']" in
+    Xml.xpathobj_nr_nodes obj > 0 in
+
   (* Non-removable disk devices. *)
   let disks      let get_disks, add_disk @@ -208,12 +214,13 @@ let
parse_libvirt_xml ?conn xml
       let controller          let target_bus = xpath_string
"target/@bus" in
-        match target_bus with
-        | None -> None
-        | Some "ide" -> Some Source_IDE
-        | Some "scsi" -> Some Source_SCSI
-        | Some "virtio" -> Some Source_virtio_blk
-        | Some _ -> None in
+        match target_bus, has_virtio_scsi with
+        | None, _ -> None
+        | Some "ide", _ -> Some Source_IDE
+        | Some "scsi", true -> Some Source_virtio_SCSI
+        | Some "scsi", false -> Some Source_SCSI
+        | Some "virtio", _ -> Some Source_virtio_blk
+        | Some _, _ -> None in
 
       let format          match xpath_string "driver/@type" with
@@ -297,12 +304,13 @@ let parse_libvirt_xml ?conn xml  
       let controller          let target_bus = xpath_string
"target/@bus" in
-        match target_bus with
-        | None -> None
-        | Some "ide" -> Some Source_IDE
-        | Some "scsi" -> Some Source_SCSI
-        | Some "virtio" -> Some Source_virtio_blk
-        | Some _ -> None in
+        match target_bus, has_virtio_scsi with
+        | None, _ -> None
+        | Some "ide", _ -> Some Source_IDE
+        | Some "scsi", true -> Some Source_virtio_SCSI
+        | Some "scsi", false -> Some Source_SCSI
+        | Some "virtio", _ -> Some Source_virtio_blk
+        | Some _, _ -> None in
 
       let slot          let target_dev = xpath_string "target/@dev"
in
diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml
index 9487f0b..106c99a 100644
--- a/v2v/output_glance.ml
+++ b/v2v/output_glance.ml
@@ -86,6 +86,7 @@ object
           "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
@@ -105,6 +106,10 @@ object
           )
         ] in
         let properties +          match guestcaps.gcaps_block_bus with
+          | Virtio_SCSI -> ("hw_scsi_model",
"virtio-scsi") :: properties
+          | _ -> properties in
+        let properties            match inspect.i_major_version,
inspect.i_minor_version with
           | 0, 0 -> properties
           | x, 0 -> ("os_version", string_of_int x) :: properties
diff --git a/v2v/test-v2v-print-source.sh b/v2v/test-v2v-print-source.sh
index 8af6104..789555d 100755
--- a/v2v/test-v2v-print-source.sh
+++ b/v2v/test-v2v-print-source.sh
@@ -63,7 +63,7 @@ hypervisor type: test
           video: qxl
           sound: 
 disks:
-	/windows.img (raw) [virtio]
+	/windows.img (raw) [virtio-blk]
 removable media:
 NICs:
 	Network \"default\" mac: 00:11:22:33:44:55 [virtio]" ]; then
diff --git a/v2v/types.ml b/v2v/types.ml
index 821b7ec..9a23b4a 100644
--- a/v2v/types.ml
+++ b/v2v/types.ml
@@ -55,7 +55,8 @@ and source_disk = {
   s_format : string option;
   s_controller : s_controller option;
 }
-and s_controller = Source_IDE | Source_SCSI | Source_virtio_blk
+and s_controller = Source_IDE | Source_SCSI |
+                   Source_virtio_blk | Source_virtio_SCSI
 and source_removable = {
   s_removable_type : s_removable_type;
   s_removable_controller : s_controller option;
@@ -187,7 +188,8 @@ and string_of_source_disk { s_qemu_uri = qemu_uri; s_format
= format;
 and string_of_controller = function
   | Source_IDE -> "ide"
   | Source_SCSI -> "scsi"
-  | Source_virtio_blk -> "virtio"
+  | Source_virtio_blk -> "virtio-blk"
+  | Source_virtio_SCSI -> "virtio-scsi"
 
 and string_of_source_removable { s_removable_type = typ;
                                  s_removable_controller = controller;
@@ -366,13 +368,14 @@ and requested_guestcaps = {
   rcaps_net_bus : guestcaps_net_type option;
   rcaps_video : guestcaps_video_type option;
 }
-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_video_type = QXL | Cirrus
 
 let string_of_block_type block_type    (match block_type with
    | Virtio_blk -> "virtio-blk"
+   | Virtio_SCSI -> "virtio-scsi"
    | IDE -> "ide")
 let string_of_net_type net_type    (match net_type with
diff --git a/v2v/types.mli b/v2v/types.mli
index 4595a1f..dde0d29 100644
--- a/v2v/types.mli
+++ b/v2v/types.mli
@@ -65,12 +65,9 @@ and source_disk = {
 }
 (** A source disk. *)
 
-and s_controller = Source_IDE | Source_SCSI | Source_virtio_blk
-(** Source disk controller.
-
-    For the purposes of this field, we can treat virtio-scsi as
-    [SCSI].  However we don't support conversions from virtio in any
-    case so virtio is here only to make it work for testing. *)
+and s_controller = Source_IDE | Source_SCSI |
+                   Source_virtio_blk | Source_virtio_SCSI
+(** Source disk controller. *)
 
 and source_removable = {
   s_removable_type : s_removable_type;  (** Type.  *)
@@ -218,7 +215,7 @@ and requested_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_video_type = QXL | Cirrus
 
diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 4d0d525..647ae11 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -731,10 +731,11 @@ and do_convert g inspect source keep_serial_console rcaps
   (* Did we manage to install virtio drivers? *)
   if not (quiet ()) then (
-    if guestcaps.gcaps_block_bus = Virtio_blk then
-      info (f_"This guest has virtio drivers installed.")
-    else
-      info (f_"This guest does not have virtio drivers installed.");
+    match guestcaps.gcaps_block_bus with
+    | Virtio_blk | Virtio_SCSI ->
+        info (f_"This guest has virtio drivers installed.")
+    | _ ->
+        info (f_"This guest does not have virtio drivers installed.")
   );
 
   guestcaps
@@ -939,6 +940,7 @@ and target_bus_assignment source targets guestcaps       
let t = BusSlotTarget t in
       match guestcaps.gcaps_block_bus with
       | Virtio_blk -> insert virtio_blk_bus i t
+      | Virtio_SCSI -> insert scsi_bus i t
       | IDE -> insert ide_bus i t
   ) targets;
 
@@ -952,7 +954,7 @@ and target_bus_assignment source targets guestcaps         
| None -> ide_bus (* Wild guess, but should be safe. *)
         | Some Source_virtio_blk -> virtio_blk_bus
         | Some Source_IDE -> ide_bus
-        | Some Source_SCSI -> scsi_bus in
+        | Some Source_virtio_SCSI | Some Source_SCSI -> scsi_bus in
       match r.s_removable_slot with
       | None -> ignore (insert_after bus 0 (BusSlotRemovable r))
       | Some desired_slot_nr ->
@@ -992,6 +994,7 @@ and rcaps_from_source source    let block_type      match
source_block_type with
     | Some Source_virtio_blk -> Some Virtio_blk
+    | Some Source_virtio_SCSI -> Some Virtio_SCSI
     | Some Source_IDE -> Some IDE
     | Some t -> error (f_"source has unsupported hard disk type
'%s'")
                       (string_of_controller t)
diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml
index 7ca4068..6b78420 100644
--- a/v2v/windows_virtio.ml
+++ b/v2v/windows_virtio.ml
@@ -35,6 +35,7 @@ let virtio_win  
 let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}"
 let viostor_pciid =
"VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00"
+let vioscsi_pciid =
"VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00"
 
 let rec install_drivers g inspect systemroot root current_cs rcaps    (* Copy
the virtio drivers to the guest. *)
@@ -43,7 +44,7 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps
   if not (copy_drivers g inspect driverdir) then (
     match rcaps with
-    | { rcaps_block_bus = Some Virtio_blk }
+    | { rcaps_block_bus = Some Virtio_blk | Some Virtio_SCSI }
     | { rcaps_net_bus = Some Virtio_net }
     | { rcaps_video = Some QXL } ->
       error (f_"there are no virtio drivers available for this version of
Windows (%d.%d %s %s).  virt-v2v looks for drivers in %s")
@@ -66,19 +67,25 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps      (* Can we install the block driver? *)
     let block : guestcaps_block_type        let has_viostor = g#exists
(driverdir // "viostor.inf") in
-      match rcaps.rcaps_block_bus, has_viostor with
-      | Some Virtio_blk, false ->
+      let has_vioscsi = g#exists (driverdir // "vioscsi.inf") in
+      match rcaps.rcaps_block_bus, has_viostor, has_vioscsi with
+      | Some Virtio_blk, false, _ ->
         error (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
               inspect.i_major_version inspect.i_minor_version
               inspect.i_arch virtio_win
 
-      | None, false ->
+      | Some Virtio_SCSI, _, false ->
+        error (f_"there is no vioscsi (virtio SCSI passthrough) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
+              inspect.i_major_version inspect.i_minor_version
+              inspect.i_arch virtio_win
+
+      | None, false, _ ->
         warning (f_"there is no viostor (virtio block device) driver for
this version of Windows (%d.%d %s).  virt-v2v looks for this driver in %s\n\nThe
guest will be configured to use a slower emulated device.")
                 inspect.i_major_version inspect.i_minor_version
                 inspect.i_arch virtio_win;
         IDE
 
-      | (Some Virtio_blk | None), true ->
+      | (Some Virtio_blk | None), true, _ ->
         (* Block driver needs tweaks to allow booting; the rest is set up by
PnP
          * manager *)
         let source = driverdir // "viostor.sys" in
@@ -89,7 +96,18 @@ let rec install_drivers g inspect systemroot root current_cs
rcaps                                  viostor_pciid;
         Virtio_blk
 
-      | Some IDE, _ ->
+      | Some Virtio_SCSI, _, true ->
+        (* Block driver needs tweaks to allow booting; the rest is set up by
PnP
+         * manager *)
+        let source = driverdir // "vioscsi.sys" in
+        let target = sprintf "%s/system32/drivers/vioscsi.sys"
systemroot in
+        let target = g#case_sensitive_path target in
+        g#cp source target;
+        add_guestor_to_registry g root current_cs "vioscsi"
+                                vioscsi_pciid;
+        Virtio_SCSI
+
+      | Some IDE, _, _ ->
         IDE in
 
     (* Can we install the virtio-net driver? *)
-- 
2.5.5

On Thu, Apr 14, 2016 at 12:01:30PM +0300, Roman Kagan
wrote:
> @@ -1134,7 +1135,13 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >      (* Update 'alias scsi_hostadapter ...' *)
>      let paths = augeas_modprobe ". =~
regexp('scsi_hostadapter.*')" in
>      match block_type with
> -    | Virtio_blk ->
> +    | Virtio_blk | Virtio_SCSI ->
> +      let block_module > +        match block_type with
> +        | Virtio_blk -> "virtio_blk"
> +        | Virtio_SCSI -> "virtio_scsi"
> +        | _ -> raise Not_found in
It's a bad idea to raise Not_found unless you're going to catch it
immediately.  As this is an impossible condition you should use
`assert false' here.
> @@ -105,6 +106,10 @@ object
>            )
>          ] in
>          let properties > +          match guestcaps.gcaps_block_bus
with
> +          | Virtio_SCSI -> ("hw_scsi_model",
"virtio-scsi") :: properties
> +          | _ -> properties in
Since there are only two other cases, it's better to match them
explicitly, ie:

  | Virtio_blk | IDE -> properties in

The reason for this is if we add another block type in future the
compiler will error out because of the missing case, forcing us to
analyze whether we need to add some other property for it.
> diff --git a/v2v/v2v.ml b/v2v/v2v.ml
> index 4d0d525..647ae11 100644
> --- a/v2v/v2v.ml
> +++ b/v2v/v2v.ml
> @@ -731,10 +731,11 @@ and do_convert g inspect source keep_serial_console
rcaps >
>    (* Did we manage to install virtio drivers? *)
>    if not (quiet ()) then (
> -    if guestcaps.gcaps_block_bus = Virtio_blk then
> -      info (f_"This guest has virtio drivers installed.")
> -    else
> -      info (f_"This guest does not have virtio drivers
installed.");
> +    match guestcaps.gcaps_block_bus with
> +    | Virtio_blk | Virtio_SCSI ->
> +        info (f_"This guest has virtio drivers installed.")
> +    | _ ->
> +        info (f_"This guest does not have virtio drivers
installed.")
As above, use `| IDE ->' instead of this wildcard match, so that in
future we are forced to analyze this code if we add another block
type.
> +      | Some Virtio_SCSI, _, false ->
> +        error (f_"there is no vioscsi (virtio SCSI passthrough)
driver for this version of Windows (%d.%d %s).  virt-v2v looks for this driver
in %s\n\nThe guest will be configured to use a slower emulated device.")
'passthrough'?  I know that the virtio SCSI driver passes SCSI
commands through, but I think this might be confused with PCI / host
adapter passthrough which is a completely different thing.  Probably
better to drop this word.

-----

ACK if you make all of the changes above.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/