Libguestfs - Apr 2016 - [PATCH] 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>
---
 v2v/OVF.ml                   |  4 +++-
 v2v/convert_linux.ml         | 15 +++++++++------
 v2v/input_libvirtxml.ml      | 32 ++++++++++++++++++++------------
 v2v/output_glance.ml         |  1 +
 v2v/test-v2v-print-source.sh |  2 +-
 v2v/types.ml                 |  9 ++++++---
 v2v/types.mli                | 14 +++++++-------
 v2v/v2v.ml                   | 13 ++++++++-----
 v2v/windows_virtio.ml        | 30 ++++++++++++++++++++++++------
 9 files changed, 79 insertions(+), 41 deletions(-)

diff --git a/v2v/OVF.ml b/v2v/OVF.ml
index f8afc8c..dc1e971 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 -> "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..90355fb 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,7 @@ 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 ->
       if paths <> [] then (
         (* There's only 1 scsi controller in the converted guest.
          * Convert only the first scsi_hostadapter entry to virtio
@@ -1148,14 +1149,15 @@ 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")
+                  (string_of_block_type block_type)
       ) 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"
+                  (string_of_block_type block_type)
       )
     | IDE ->
       (* There is no scsi controller in an IDE guest. *)
@@ -1247,6 +1249,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 +1258,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..6f325e6 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..2572de6 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
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..71cfa31 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,10 +215,13 @@ 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
 
+val string_of_block_type : guestcaps_block_type -> string
+val string_of_net_type : guestcaps_net_type -> string
+val string_of_video : guestcaps_video_type -> string
 val string_of_guestcaps : guestcaps -> string
 val string_of_requested_guestcaps : requested_guestcaps -> string
 
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..a54dbca 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 Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan
wrote:
>            (match guestcaps.gcaps_block_bus with
> -          | Virtio_blk -> "VirtIO" | IDE ->
"IDE");
> +          | Virtio_blk -> "VirtIO"
> +          | Virtio_SCSI -> "SCSI"
I believe this should be "VirtIO_SCSI", although it's difficult to
get
a straight answer from the massive and convoluted Java sources of
ovirt-engine ...
> @@ -1148,14 +1149,15 @@ 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")
> +                  (string_of_block_type block_type)
Here ...
>        ) 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"
> +                  (string_of_block_type block_type)
>        )
... and here don't seem right.

Firstly, string_of_block_type returns "virtio-blk" (not
"virtio_blk").
Maybe for this configuration file it doesn't matter.

But, the real problem is that string_of_block_type is essentially an
internal debugging function.

What we should really have is a new function (eg.
"linux_module_of_block_type") which converts the block type to a Linux
module name.
> +  (* Presence of virtio-scsi controller. *)
> +  let has_virtio_scsi > +    let obj = Xml.xpath_eval_expression
xpathctx
> +               
"/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK.  A true test would involve checking the
<target bus="scsi"> on each disk and matching it back to the
controller.  In other words, a huge pain!  Maybe you can add an "XXX"
comment in the source about this.
> +    (Xml.xpathobj_nr_nodes obj) > 0 in
       ^                         ^
You don't need these parentheses.  In functional languages, function
application always binds tightest of all, eg:

  f x + 1

is the same as:

  (f x) + 1
> diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml
> index 9487f0b..2572de6 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
This is wrong, or at least, incomplete.

Apparently you need:

  hw_disk_bus=scsi     (as above)
  hw_scsi_model=virtio-scsi  (additional property)

See:
http://www.sebastien-han.fr/blog/2015/02/02/openstack-and-ceph-rbd-discard/
>    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) }
I don't think you need parens there.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Apr 13, 2016 at 07:45:42PM +0300, Roman Kagan
wrote:
> On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote:
> > On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
> > > +  (* Presence of virtio-scsi controller. *)
> > > +  let has_virtio_scsi > > > +    let obj =
Xml.xpath_eval_expression xpathctx
> > > +               
"/domain/devices/controller[@model='virtio-scsi']" in
> > 
> > I guess this short cut is OK.  A true test would involve checking the
> > <target bus="scsi"> on each disk and matching it back
to the
> > controller.  In other words, a huge pain!  Maybe you can add an
"XXX"
> > comment in the source about this.
> 
> On a second look, I'm not sure I got your comment right.
> 
> AFAICS there's no way in libvirt xml to define multiple scsi buses,
I think this is possible, although massively unlikely in practice.  I
think the different controllers would have different
index="0"/index="1"/... attributes.
> so all the matching I can do is just to notice the presence of
> virtio-scsi controller, and attribute all SCSI drives to it.  This
> is exactly what happens in the patch.
>
> Am I missing something?
So first off I think this part of the patch and your assumption is
fine, there's no need to change the code.

However there is a possibility that the source guest might have
multiple mixed virtio-blk + virtio-scsi disks.  In that case the disks
would look like this:

    <disk type='file' device='disk'>
      <driver name='qemu' type='raw'/>
      <source file=...>
      <target dev='vda' bus='virtio'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw'/>
      <source file=...>
      <target dev='sda' bus='scsi'/>
    </disk>
    <controller type="scsi" index="0"
model="virtio-scsi"/>

It's not something that we should worry about, and a simple comment
with "XXX" in it is fine, if you agree with my analysis.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Wed, Apr 13, 2016 at 09:04:54PM +0300, Roman Kagan
wrote:
> On Wed, Apr 13, 2016 at 06:39:55PM +0100, Richard W.M. Jones wrote:
> > I think this is possible, although massively unlikely in practice.  I
> > think the different controllers would have different
> > index="0"/index="1"/... attributes.
> 
> How the binding of drives to controllers is done, then?  Is it specified
> in drive's "address"?
Yes, apparently using <address/> under <disk/>.
> This case is handled fine in the patch, isn't it?  has_virtio_scsi is a
> local variable which is only consulted when populating per-drive
> s_controller field: for drives with bus='scsi' it differentiates
between
> Source_virtio_SCSI and Source_SCSI.
Oh I see, target_bus is consulted for each drive.  So this is right
(except for the scsi/virtio-scsi case, but who knows about that).
> The place where we make up a single disk type is
> v2v.ml:rcaps_from_source, but there I explicitly error out if the guest
> has multiple disk types (which may be too harsh but simplifies things
> for now).
Yup.

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/