thr3ads.net - Libguestfs - [Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping [Apr 2022]

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2022-Apr-14 15:07 UTC

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping

The current code handles some nonexistent cases (such as SCSI floppies,
virtio-block CD-ROMs), and does not create proper drives (that is,
back-ends with no media inserted) for removable devices (floppies,
CD-ROMs).

Rewrite the whole logic:

- handle only those scenarios that QEMU can support,

- separate the back-end (-drive) and front-end (-device) options,

- wherever / whenever a host-bus adapter front-end is needed
  (virtio-scsi-pci, isa-fdc), create it,

- assign front-ends to buses (= parent front-ends) and back-ends
  explicitly.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074805
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 output/output_qemu.ml | 197 ++++++++++++++++----
 1 file changed, 160 insertions(+), 37 deletions(-)

diff --git a/output/output_qemu.ml b/output/output_qemu.ml
index da7278b88b67..a25b49d86679 100644
--- a/output/output_qemu.ml
+++ b/output/output_qemu.ml
@@ -127,7 +127,7 @@ module QEMU = struct
          machine, false in
     let smm = secure_boot_required in
 
-    let machine +    let machine_str        match machine with
       | I440FX -> "pc"
       | Q35 -> "q35"
@@ -153,7 +153,7 @@ module QEMU = struct
         arg_list "-device" ["vmgenid"; sprintf
"guid=%s" genid; "id=vmgenid0"]
     );
 
-    arg_list "-machine" (machine ::
+    arg_list "-machine" (machine_str ::
                            (if smm then ["smm=on"] else []) @
                            ["accel=kvm:tcg"]);
 
@@ -184,52 +184,175 @@ module QEMU = struct
       );
     );
 
-    let make_disk if_name i = function
-      | BusSlotEmpty -> ()
+    (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies, we
+     * need host-bus adapters (HBAs) between these devices and the PCI(e) root
+     * bus. Some machine types create these HBAs automatically (despite
+     * "-no-user-config -nodefaults"), some don't...
+     *)
+    let parent_ctrl_needed target_bus dev_filter +      try ignore (List.find
dev_filter (Array.to_list target_bus)); true
+      with Not_found -> false
+    and disk_cdrom_filter +      function
+      | BusSlotDisk _
+      | BusSlotRemovable { s_removable_type = CDROM } -> true
+      | _ -> false
+    and floppy_filter +      function
+      | BusSlotRemovable { s_removable_type = Floppy } -> true
+      | _ -> false in
+    let ide_ctrl_needed +      parent_ctrl_needed target_buses.target_ide_bus
disk_cdrom_filter
+    and scsi_ctrl_needed +      parent_ctrl_needed target_buses.target_scsi_bus
disk_cdrom_filter
+    and floppy_ctrl_needed +      parent_ctrl_needed
target_buses.target_floppy_bus floppy_filter in
 
-      | BusSlotDisk d ->
-         (* Find the corresponding target disk. *)
-         let outdisk = disk_path output_storage output_name d.s_disk_id in
+    if ide_ctrl_needed then (
+      match machine with
+      | I440FX -> ()
+        (* The PC machine has a built-in controller of type
"piix3-ide"
+         * providing buses "ide.0" and "ide.1", with each
bus fitting two
+         * devices.
+         *)
+      | Q35 -> ()
+        (* The Q35 machine has a built-in controller of type
"ich9-ahci"
+         * providing buses "ide.0" through "ide.5", with
each bus fitting one
+         * device.
+         *)
+      | Virt -> warning (f_"The Virt machine has no support for IDE.
Please \
+                            report a bug for virt-v2v.")
+    );
 
-         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
-                            "if=" ^ if_name; "index=" ^
string_of_int i;
-                            "media=disk"]
+    if scsi_ctrl_needed then
+      (* We need to add the virtio-scsi HBA on all three machine types. The bus
+       * provided by this device will be called "scsi0.0".
+       *)
+      arg_list "-device" [ "virtio-scsi-pci";
"id=scsi0" ];
 
-      | BusSlotRemovable { s_removable_type = CDROM } ->
-         arg_list "-drive" ["format=raw"; "if=" ^
if_name;
-                            "index=" ^ string_of_int i;
"media=cdrom"]
+    if floppy_ctrl_needed then (
+      match machine with
+      | I440FX -> ()
+        (* The PC machine has a built-in controller of type "isa-fdc"
+         * providing bus "floppy-bus.0", fitting two devices.
+         *)
+      | Q35 -> arg_list "-device" [ "isa-fdc";
"id=floppy-bus" ]
+        (* On the Q35 machine, we need to add the same HBA manually. Note that
+         * the bus name will have ".0" appended automatically.
+         *)
+      | Virt -> warning (f_"The Virt machine has no support for
floppies. \
+                            Please report a bug for virt-v2v.")
+    );
 
-      | BusSlotRemovable { s_removable_type = Floppy } ->
-         arg_list "-drive" ["format=raw"; "if=" ^
if_name;
-                            "index=" ^ string_of_int i;
"media=floppy"]
-    in
-    Array.iteri (make_disk "virtio")
target_buses.target_virtio_blk_bus;
-    Array.iteri (make_disk "ide") target_buses.target_ide_bus;
+    let add_disk_backend disk_id backend_name +      (* Add a drive (back-end)
for a "virtio-blk-pci", "ide-hd", or "scsi-hd"
+       * device (front-end). The drive has a backing file, identified by
+       * "disk_id".
+       *)
+      let outdisk = disk_path output_storage output_name disk_id in
+      arg_list "-drive" [ "file=" ^ outdisk;
"format=" ^ output_format;
+                          "if=none"; "id=" ^ backend_name;
"media=disk" ]
 
-    let make_scsi i = function
-      | BusSlotEmpty -> ()
+    and add_cdrom_backend backend_name +      (* Add a drive (back-end) for an
"ide-cd" or "scsi-cd" device (front-end).
+       * The drive is empty -- there is no backing file.
+       *)
+      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=cdrom" ]
 
-      | BusSlotDisk d ->
-         (* Find the corresponding target disk. *)
-         let outdisk = disk_path output_storage output_name d.s_disk_id in
+    and add_floppy_backend backend_name +      (* Add a drive (back-end) for a
"floppy" device (front-end). The drive is
+       * empty -- there is no backing file. *)
+      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=disk" ] in
 
-         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
-                            "if=scsi"; "bus=0";
"unit=" ^ string_of_int i;
-                            "media=disk"]
+    let add_virtio_blk disk_id frontend_ctr +      (* Create a
"virtio-blk-pci" device (front-end), together with its drive
+       * (back-end). The disk identifier is mandatory.
+       *)
+      let backend_name = sprintf "drive-vblk-%d" frontend_ctr in
+      add_disk_backend disk_id backend_name;
+      arg_list "-device" [ "virtio-blk-pci";
"drive=" ^ backend_name ]
 
-      | BusSlotRemovable { s_removable_type = CDROM } ->
-         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
-                            "unit=" ^ string_of_int i;
"media=cdrom"]
+    and add_ide disk_id frontend_ctr +      (* Create an "ide-hd" or
"ide-cd" device (front-end), together with its
+       * drive (back-end). If a disk identifier is passed in, then
"ide-hd" is
+       * created (with a non-empty drive); otherwise, "ide-cd" is
created (with
+       * an empty drive).
+       *)
+      let backend_name = sprintf "drive-ide-%d" frontend_ctr
+      and ide_bus, ide_unit +        match machine with
+        | I440FX -> frontend_ctr / 2, frontend_ctr mod 2
+        | Q35 -> frontend_ctr, 0
+        | Virt -> 0, 0 (* should never happen, see warning above *) in
+      let common_props = [ sprintf "bus=ide.%d" ide_bus;
+                           sprintf "unit=%d" ide_unit;
+                           "drive=" ^ backend_name ] in
+      (match disk_id with
+       | Some id ->
+           add_disk_backend id backend_name;
+           arg_list "-device" ([ "ide-hd" ] @ common_props)
+       | None ->
+           add_cdrom_backend backend_name;
+           arg_list "-device" ([ "ide-cd" ] @
common_props))
+
+    and add_scsi disk_id frontend_ctr +      (* Create a "scsi-hd" or
"scsi-cd" device (front-end), together with its
+       * drive (back-end). If a disk identifier is passed in, then
"scsi-hd" is
+       * created (with a non-empty drive); otherwise, "scsi-cd" is
created (with
+       * an empty drive).
+       *)
+      let backend_name = sprintf "drive-scsi-%d" frontend_ctr in
+      let common_props = [ sprintf "lun=%d" frontend_ctr;
+                           "drive=" ^ backend_name ] in
+      (match disk_id with
+       | Some id ->
+           add_disk_backend id backend_name;
+           arg_list "-device" ([ "scsi-hd" ] @
common_props)
+       | None ->
+           add_cdrom_backend backend_name;
+           arg_list "-device" ([ "scsi-cd" ] @
common_props))
 
-      | BusSlotRemovable { s_removable_type = Floppy } ->
-         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
-                            "unit=" ^ string_of_int i;
"media=floppy"]
-    in
-    Array.iteri make_scsi target_buses.target_scsi_bus;
+    and add_floppy frontend_ctr +      (* Create a "floppy"
(front-end), together with its empty drive
+       * (back-end).
+       *)
+      let backend_name = sprintf "drive-floppy-%d" frontend_ctr in
+      add_floppy_backend backend_name;
+      arg_list "-device" [ "floppy";
"bus=floppy-bus.0";
+                           sprintf "unit=%d" frontend_ctr;
+                           "drive=" ^ backend_name ] in
 
-    (* XXX Highly unlikely that anyone cares, but the current
-     * code ignores target_buses.target_floppy_bus.
+    (* Add virtio-blk-pci devices for BusSlotDisk elements on
+     * "target_virtio_blk_bus".
      *)
+    Array.iteri
+      (fun frontend_ctr disk ->
+         match disk with
+         | BusSlotDisk d -> add_virtio_blk d.s_disk_id frontend_ctr
+         | _ -> ())
+      target_buses.target_virtio_blk_bus;
+
+    let add_disk_or_cdrom bus_adder frontend_ctr slot +      (* Add a disk or
CD-ROM front-end to the IDE or SCSI bus. *)
+      match slot with
+      | BusSlotDisk d ->
+          bus_adder (Some d.s_disk_id) frontend_ctr
+      | BusSlotRemovable { s_removable_type = CDROM } ->
+          bus_adder None frontend_ctr
+      | _ -> () in
+
+    (* Add disks and CD-ROMs to the IDE and SCSI buses. *)
+    Array.iteri (add_disk_or_cdrom add_ide) target_buses.target_ide_bus;
+    Array.iteri (add_disk_or_cdrom add_scsi) target_buses.target_scsi_bus;
+
+    (* Add floppies. *)
+    Array.iteri
+      (fun frontend_ctr disk ->
+         match disk with
+         | BusSlotRemovable { s_removable_type = Floppy } ->
+             add_floppy frontend_ctr
+         | _ -> ())
+      target_buses.target_floppy_bus;
 
     let net_bus        match guestcaps.gcaps_net_bus with

base-commit: e3226dd431e7476cae2d0ca54b2332344de2d201
-- 
2.19.1.3.g30247aa5d201

Laszlo Ersek

2022-Apr-14 15:29 UTC

head link

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping

On 04/14/22 17:07, Laszlo Ersek wrote:> The current code handles some nonexistent cases (such as SCSI floppies,
> virtio-block CD-ROMs), and does not create proper drives (that is,
> back-ends with no media inserted) for removable devices (floppies,
> CD-ROMs).
> 
> Rewrite the whole logic:
> 
> - handle only those scenarios that QEMU can support,
> 
> - separate the back-end (-drive) and front-end (-device) options,
> 
> - wherever / whenever a host-bus adapter front-end is needed
>   (virtio-scsi-pci, isa-fdc), create it,
> 
> - assign front-ends to buses (= parent front-ends) and back-ends
>   explicitly.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074805
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  output/output_qemu.ml | 197 ++++++++++++++++----
>  1 file changed, 160 insertions(+), 37 deletions(-)
> 
> diff --git a/output/output_qemu.ml b/output/output_qemu.ml
> index da7278b88b67..a25b49d86679 100644
> --- a/output/output_qemu.ml
> +++ b/output/output_qemu.ml
> @@ -127,7 +127,7 @@ module QEMU = struct
>           machine, false in
>      let smm = secure_boot_required in
>  
> -    let machine > +    let machine_str >        match machine with
>        | I440FX -> "pc"
>        | Q35 -> "q35"
> @@ -153,7 +153,7 @@ module QEMU = struct
>          arg_list "-device" ["vmgenid"; sprintf
"guid=%s" genid; "id=vmgenid0"]
>      );
>  
> -    arg_list "-machine" (machine ::
> +    arg_list "-machine" (machine_str ::
>                             (if smm then ["smm=on"] else []) @
>                             ["accel=kvm:tcg"]);
>  
> @@ -184,52 +184,175 @@ module QEMU = struct
>        );
>      );
>  
> -    let make_disk if_name i = function
> -      | BusSlotEmpty -> ()
> +    (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies,
we
> +     * need host-bus adapters (HBAs) between these devices and the PCI(e)
root
> +     * bus. Some machine types create these HBAs automatically (despite
> +     * "-no-user-config -nodefaults"), some don't...
> +     *)
> +    let parent_ctrl_needed target_bus dev_filter > +      try ignore
(List.find dev_filter (Array.to_list target_bus)); true
> +      with Not_found -> false
> +    and disk_cdrom_filter > +      function
> +      | BusSlotDisk _
> +      | BusSlotRemovable { s_removable_type = CDROM } -> true
> +      | _ -> false
> +    and floppy_filter > +      function
> +      | BusSlotRemovable { s_removable_type = Floppy } -> true
> +      | _ -> false in
> +    let ide_ctrl_needed > +      parent_ctrl_needed
target_buses.target_ide_bus disk_cdrom_filter
> +    and scsi_ctrl_needed > +      parent_ctrl_needed
target_buses.target_scsi_bus disk_cdrom_filter
> +    and floppy_ctrl_needed > +      parent_ctrl_needed
target_buses.target_floppy_bus floppy_filter in
>  
> -      | BusSlotDisk d ->
> -         (* Find the corresponding target disk. *)
> -         let outdisk = disk_path output_storage output_name d.s_disk_id in
> +    if ide_ctrl_needed then (
> +      match machine with
> +      | I440FX -> ()
> +        (* The PC machine has a built-in controller of type
"piix3-ide"
> +         * providing buses "ide.0" and "ide.1", with
each bus fitting two
> +         * devices.
> +         *)
> +      | Q35 -> ()
> +        (* The Q35 machine has a built-in controller of type
"ich9-ahci"
> +         * providing buses "ide.0" through "ide.5",
with each bus fitting one
> +         * device.
> +         *)
> +      | Virt -> warning (f_"The Virt machine has no support for
IDE. Please \
> +                            report a bug for virt-v2v.")
> +    );
>  
> -         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
> -                            "if=" ^ if_name; "index="
^ string_of_int i;
> -                            "media=disk"]
> +    if scsi_ctrl_needed then
> +      (* We need to add the virtio-scsi HBA on all three machine types.
The bus
> +       * provided by this device will be called "scsi0.0".
> +       *)
> +      arg_list "-device" [ "virtio-scsi-pci";
"id=scsi0" ];
>  
> -      | BusSlotRemovable { s_removable_type = CDROM } ->
> -         arg_list "-drive" ["format=raw";
"if=" ^ if_name;
> -                            "index=" ^ string_of_int i;
"media=cdrom"]
> +    if floppy_ctrl_needed then (
> +      match machine with
> +      | I440FX -> ()
> +        (* The PC machine has a built-in controller of type
"isa-fdc"
> +         * providing bus "floppy-bus.0", fitting two devices.
> +         *)
> +      | Q35 -> arg_list "-device" [ "isa-fdc";
"id=floppy-bus" ]
> +        (* On the Q35 machine, we need to add the same HBA manually. Note
that
> +         * the bus name will have ".0" appended automatically.
> +         *)
> +      | Virt -> warning (f_"The Virt machine has no support for
floppies. \
> +                            Please report a bug for virt-v2v.")
> +    );
>  
> -      | BusSlotRemovable { s_removable_type = Floppy } ->
> -         arg_list "-drive" ["format=raw";
"if=" ^ if_name;
> -                            "index=" ^ string_of_int i;
"media=floppy"]
> -    in
> -    Array.iteri (make_disk "virtio")
target_buses.target_virtio_blk_bus;
> -    Array.iteri (make_disk "ide") target_buses.target_ide_bus;
> +    let add_disk_backend disk_id backend_name > +      (* Add a drive
(back-end) for a "virtio-blk-pci", "ide-hd", or
"scsi-hd"
> +       * device (front-end). The drive has a backing file, identified by
> +       * "disk_id".
> +       *)
> +      let outdisk = disk_path output_storage output_name disk_id in
> +      arg_list "-drive" [ "file=" ^ outdisk;
"format=" ^ output_format;
> +                          "if=none"; "id=" ^
backend_name; "media=disk" ]
>  
> -    let make_scsi i = function
> -      | BusSlotEmpty -> ()
> +    and add_cdrom_backend backend_name > +      (* Add a drive
(back-end) for an "ide-cd" or "scsi-cd" device (front-end).
> +       * The drive is empty -- there is no backing file.
> +       *)
> +      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=cdrom" ]
>  
> -      | BusSlotDisk d ->
> -         (* Find the corresponding target disk. *)
> -         let outdisk = disk_path output_storage output_name d.s_disk_id in
> +    and add_floppy_backend backend_name > +      (* Add a drive
(back-end) for a "floppy" device (front-end). The drive is
> +       * empty -- there is no backing file. *)
> +      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=disk" ] in
>  
> -         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
> -                            "if=scsi"; "bus=0";
"unit=" ^ string_of_int i;
> -                            "media=disk"]
> +    let add_virtio_blk disk_id frontend_ctr > +      (* Create a
"virtio-blk-pci" device (front-end), together with its drive
> +       * (back-end). The disk identifier is mandatory.
> +       *)
> +      let backend_name = sprintf "drive-vblk-%d" frontend_ctr in
> +      add_disk_backend disk_id backend_name;
> +      arg_list "-device" [ "virtio-blk-pci";
"drive=" ^ backend_name ]
>  
> -      | BusSlotRemovable { s_removable_type = CDROM } ->
> -         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
> -                            "unit=" ^ string_of_int i;
"media=cdrom"]
> +    and add_ide disk_id frontend_ctr > +      (* Create an
"ide-hd" or "ide-cd" device (front-end), together with its
> +       * drive (back-end). If a disk identifier is passed in, then
"ide-hd" is
> +       * created (with a non-empty drive); otherwise, "ide-cd"
is created (with
> +       * an empty drive).
> +       *)
> +      let backend_name = sprintf "drive-ide-%d" frontend_ctr
> +      and ide_bus, ide_unit > +        match machine with
> +        | I440FX -> frontend_ctr / 2, frontend_ctr mod 2
> +        | Q35 -> frontend_ctr, 0
> +        | Virt -> 0, 0 (* should never happen, see warning above *) in
> +      let common_props = [ sprintf "bus=ide.%d" ide_bus;
> +                           sprintf "unit=%d" ide_unit;
> +                           "drive=" ^ backend_name ] in
> +      (match disk_id with
> +       | Some id ->
> +           add_disk_backend id backend_name;
> +           arg_list "-device" ([ "ide-hd" ] @
common_props)
> +       | None ->
> +           add_cdrom_backend backend_name;
> +           arg_list "-device" ([ "ide-cd" ] @
common_props))
> +
> +    and add_scsi disk_id frontend_ctr > +      (* Create a
"scsi-hd" or "scsi-cd" device (front-end), together with its
> +       * drive (back-end). If a disk identifier is passed in, then
"scsi-hd" is
> +       * created (with a non-empty drive); otherwise, "scsi-cd"
is created (with
> +       * an empty drive).
> +       *)
> +      let backend_name = sprintf "drive-scsi-%d" frontend_ctr in
> +      let common_props = [ sprintf "lun=%d" frontend_ctr;
> +                           "drive=" ^ backend_name ] in
... since posting, I've prepended "bus=scsi0.0" here.

Thanks
Laszlo
> +      (match disk_id with
> +       | Some id ->
> +           add_disk_backend id backend_name;
> +           arg_list "-device" ([ "scsi-hd" ] @
common_props)
> +       | None ->
> +           add_cdrom_backend backend_name;
> +           arg_list "-device" ([ "scsi-cd" ] @
common_props))
>  
> -      | BusSlotRemovable { s_removable_type = Floppy } ->
> -         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
> -                            "unit=" ^ string_of_int i;
"media=floppy"]
> -    in
> -    Array.iteri make_scsi target_buses.target_scsi_bus;
> +    and add_floppy frontend_ctr > +      (* Create a "floppy"
(front-end), together with its empty drive
> +       * (back-end).
> +       *)
> +      let backend_name = sprintf "drive-floppy-%d" frontend_ctr
in
> +      add_floppy_backend backend_name;
> +      arg_list "-device" [ "floppy";
"bus=floppy-bus.0";
> +                           sprintf "unit=%d" frontend_ctr;
> +                           "drive=" ^ backend_name ] in
>  
> -    (* XXX Highly unlikely that anyone cares, but the current
> -     * code ignores target_buses.target_floppy_bus.
> +    (* Add virtio-blk-pci devices for BusSlotDisk elements on
> +     * "target_virtio_blk_bus".
>       *)
> +    Array.iteri
> +      (fun frontend_ctr disk ->
> +         match disk with
> +         | BusSlotDisk d -> add_virtio_blk d.s_disk_id frontend_ctr
> +         | _ -> ())
> +      target_buses.target_virtio_blk_bus;
> +
> +    let add_disk_or_cdrom bus_adder frontend_ctr slot > +      (* Add a
disk or CD-ROM front-end to the IDE or SCSI bus. *)
> +      match slot with
> +      | BusSlotDisk d ->
> +          bus_adder (Some d.s_disk_id) frontend_ctr
> +      | BusSlotRemovable { s_removable_type = CDROM } ->
> +          bus_adder None frontend_ctr
> +      | _ -> () in
> +
> +    (* Add disks and CD-ROMs to the IDE and SCSI buses. *)
> +    Array.iteri (add_disk_or_cdrom add_ide) target_buses.target_ide_bus;
> +    Array.iteri (add_disk_or_cdrom add_scsi) target_buses.target_scsi_bus;
> +
> +    (* Add floppies. *)
> +    Array.iteri
> +      (fun frontend_ctr disk ->
> +         match disk with
> +         | BusSlotRemovable { s_removable_type = Floppy } ->
> +             add_floppy frontend_ctr
> +         | _ -> ())
> +      target_buses.target_floppy_bus;
>  
>      let net_bus >        match guestcaps.gcaps_net_bus with
> 
> base-commit: e3226dd431e7476cae2d0ca54b2332344de2d201
>

Richard W.M. Jones

2022-Apr-14 15:40 UTC

head link

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping

On Thu, Apr 14, 2022 at 05:07:29PM +0200, Laszlo Ersek
wrote:> The current code handles some nonexistent cases (such as SCSI floppies,
> virtio-block CD-ROMs), and does not create proper drives (that is,
> back-ends with no media inserted) for removable devices (floppies,
> CD-ROMs).
> 
> Rewrite the whole logic:
> 
> - handle only those scenarios that QEMU can support,
> 
> - separate the back-end (-drive) and front-end (-device) options,
> 
> - wherever / whenever a host-bus adapter front-end is needed
>   (virtio-scsi-pci, isa-fdc), create it,
> 
> - assign front-ends to buses (= parent front-ends) and back-ends
>   explicitly.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2074805
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  output/output_qemu.ml | 197 ++++++++++++++++----
>  1 file changed, 160 insertions(+), 37 deletions(-)
> 
> diff --git a/output/output_qemu.ml b/output/output_qemu.ml
> index da7278b88b67..a25b49d86679 100644
> --- a/output/output_qemu.ml
> +++ b/output/output_qemu.ml
> @@ -127,7 +127,7 @@ module QEMU = struct
>           machine, false in
>      let smm = secure_boot_required in
>  
> -    let machine > +    let machine_str >        match machine with
>        | I440FX -> "pc"
>        | Q35 -> "q35"
> @@ -153,7 +153,7 @@ module QEMU = struct
>          arg_list "-device" ["vmgenid"; sprintf
"guid=%s" genid; "id=vmgenid0"]
>      );
>  
> -    arg_list "-machine" (machine ::
> +    arg_list "-machine" (machine_str ::
>                             (if smm then ["smm=on"] else []) @
>                             ["accel=kvm:tcg"]);
>  
> @@ -184,52 +184,175 @@ module QEMU = struct
>        );
>      );
>  
> -    let make_disk if_name i = function
> -      | BusSlotEmpty -> ()
> +    (* For IDE disks, IDE CD-ROMs, SCSI disks, SCSI CD-ROMs, and floppies,
we
> +     * need host-bus adapters (HBAs) between these devices and the PCI(e)
root
> +     * bus. Some machine types create these HBAs automatically (despite
> +     * "-no-user-config -nodefaults"), some don't...
> +     *)
> +    let parent_ctrl_needed target_bus dev_filter > +      try ignore
(List.find dev_filter (Array.to_list target_bus)); true
> +      with Not_found -> false
You can Array.exists here, but it might also be worth converting
everything to a list earlier on in the file, eg:

  let target_virtio_blk_bus = Array.to_list target_buses.target_virtio_blk_bus
  and target_ide_bus = Array.to_list target_buses.target_ide_bus
  [etc]
> +    and disk_cdrom_filter > +      function
> +      | BusSlotDisk _
> +      | BusSlotRemovable { s_removable_type = CDROM } -> true
> +      | _ -> false
> +    and floppy_filter > +      function
> +      | BusSlotRemovable { s_removable_type = Floppy } -> true
> +      | _ -> false in
> +    let ide_ctrl_needed > +      parent_ctrl_needed
target_buses.target_ide_bus disk_cdrom_filter
> +    and scsi_ctrl_needed > +      parent_ctrl_needed
target_buses.target_scsi_bus disk_cdrom_filter
> +    and floppy_ctrl_needed > +      parent_ctrl_needed
target_buses.target_floppy_bus floppy_filter in
>  
> -      | BusSlotDisk d ->
> -         (* Find the corresponding target disk. *)
> -         let outdisk = disk_path output_storage output_name d.s_disk_id in
> +    if ide_ctrl_needed then (
> +      match machine with
> +      | I440FX -> ()
> +        (* The PC machine has a built-in controller of type
"piix3-ide"
> +         * providing buses "ide.0" and "ide.1", with
each bus fitting two
> +         * devices.
> +         *)
> +      | Q35 -> ()
> +        (* The Q35 machine has a built-in controller of type
"ich9-ahci"
> +         * providing buses "ide.0" through "ide.5",
with each bus fitting one
> +         * device.
> +         *)
> +      | Virt -> warning (f_"The Virt machine has no support for
IDE. Please \
> +                            report a bug for virt-v2v.")
So it's good that it's a warning (dropping a floppy or CD-ROM when
converting is not a reason to fail), however it could be a bit more
actionable.  I would include a reference to the manual, "BUGS" section
in virt-v2v(1).
> +    );
>  
> -         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
> -                            "if=" ^ if_name; "index="
^ string_of_int i;
> -                            "media=disk"]
> +    if scsi_ctrl_needed then
> +      (* We need to add the virtio-scsi HBA on all three machine types.
The bus
> +       * provided by this device will be called "scsi0.0".
> +       *)
> +      arg_list "-device" [ "virtio-scsi-pci";
"id=scsi0" ];
>  
> -      | BusSlotRemovable { s_removable_type = CDROM } ->
> -         arg_list "-drive" ["format=raw";
"if=" ^ if_name;
> -                            "index=" ^ string_of_int i;
"media=cdrom"]
> +    if floppy_ctrl_needed then (
> +      match machine with
> +      | I440FX -> ()
> +        (* The PC machine has a built-in controller of type
"isa-fdc"
> +         * providing bus "floppy-bus.0", fitting two devices.
> +         *)
> +      | Q35 -> arg_list "-device" [ "isa-fdc";
"id=floppy-bus" ]
> +        (* On the Q35 machine, we need to add the same HBA manually. Note
that
> +         * the bus name will have ".0" appended automatically.
> +         *)
> +      | Virt -> warning (f_"The Virt machine has no support for
floppies. \
> +                            Please report a bug for virt-v2v.")
Same here.
> +    );
>  
> -      | BusSlotRemovable { s_removable_type = Floppy } ->
> -         arg_list "-drive" ["format=raw";
"if=" ^ if_name;
> -                            "index=" ^ string_of_int i;
"media=floppy"]
> -    in
> -    Array.iteri (make_disk "virtio")
target_buses.target_virtio_blk_bus;
> -    Array.iteri (make_disk "ide") target_buses.target_ide_bus;
> +    let add_disk_backend disk_id backend_name > +      (* Add a drive
(back-end) for a "virtio-blk-pci", "ide-hd", or
"scsi-hd"
> +       * device (front-end). The drive has a backing file, identified by
> +       * "disk_id".
> +       *)
> +      let outdisk = disk_path output_storage output_name disk_id in
> +      arg_list "-drive" [ "file=" ^ outdisk;
"format=" ^ output_format;
> +                          "if=none"; "id=" ^
backend_name; "media=disk" ]
>  
> -    let make_scsi i = function
> -      | BusSlotEmpty -> ()
> +    and add_cdrom_backend backend_name > +      (* Add a drive
(back-end) for an "ide-cd" or "scsi-cd" device (front-end).
> +       * The drive is empty -- there is no backing file.
> +       *)
> +      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=cdrom" ]
>  
> -      | BusSlotDisk d ->
> -         (* Find the corresponding target disk. *)
> -         let outdisk = disk_path output_storage output_name d.s_disk_id in
> +    and add_floppy_backend backend_name > +      (* Add a drive
(back-end) for a "floppy" device (front-end). The drive is
> +       * empty -- there is no backing file. *)
> +      arg_list "-drive" [ "if=none"; "id=" ^
backend_name; "media=disk" ] in
>  
> -         arg_list "-drive" ["file=" ^ outdisk;
"format=" ^ output_format;
> -                            "if=scsi"; "bus=0";
"unit=" ^ string_of_int i;
> -                            "media=disk"]
> +    let add_virtio_blk disk_id frontend_ctr > +      (* Create a
"virtio-blk-pci" device (front-end), together with its drive
> +       * (back-end). The disk identifier is mandatory.
> +       *)
> +      let backend_name = sprintf "drive-vblk-%d" frontend_ctr in
If you just need a source of unique numbers you can use ?unique ()?
(from Std_utils).  A possible issue with this is that you won't get
the same output on every run, especially if something unrelated inside
virt-v2v changes, but that probably doesn't matter here.  Especially
since we are printing the process ID in the output already, so that
ship has sailed.

This means you wouldn't need to call Array.iteri below.
> +      add_disk_backend disk_id backend_name;
> +      arg_list "-device" [ "virtio-blk-pci";
"drive=" ^ backend_name ]
>  
> -      | BusSlotRemovable { s_removable_type = CDROM } ->
> -         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
> -                            "unit=" ^ string_of_int i;
"media=cdrom"]
> +    and add_ide disk_id frontend_ctr > +      (* Create an
"ide-hd" or "ide-cd" device (front-end), together with its
> +       * drive (back-end). If a disk identifier is passed in, then
"ide-hd" is
> +       * created (with a non-empty drive); otherwise, "ide-cd"
is created (with
> +       * an empty drive).
> +       *)
> +      let backend_name = sprintf "drive-ide-%d" frontend_ctr
> +      and ide_bus, ide_unit > +        match machine with
> +        | I440FX -> frontend_ctr / 2, frontend_ctr mod 2
I remember master/slave IDE ...
> +        | Q35 -> frontend_ctr, 0
> +        | Virt -> 0, 0 (* should never happen, see warning above *) in
> +      let common_props = [ sprintf "bus=ide.%d" ide_bus;
> +                           sprintf "unit=%d" ide_unit;
> +                           "drive=" ^ backend_name ] in
> +      (match disk_id with
> +       | Some id ->
> +           add_disk_backend id backend_name;
> +           arg_list "-device" ([ "ide-hd" ] @
common_props)
> +       | None ->
> +           add_cdrom_backend backend_name;
> +           arg_list "-device" ([ "ide-cd" ] @
common_props))
> +
> +    and add_scsi disk_id frontend_ctr > +      (* Create a
"scsi-hd" or "scsi-cd" device (front-end), together with its
> +       * drive (back-end). If a disk identifier is passed in, then
"scsi-hd" is
> +       * created (with a non-empty drive); otherwise, "scsi-cd"
is created (with
> +       * an empty drive).
> +       *)
> +      let backend_name = sprintf "drive-scsi-%d" frontend_ctr in
> +      let common_props = [ sprintf "lun=%d" frontend_ctr;
> +                           "drive=" ^ backend_name ] in
> +      (match disk_id with
> +       | Some id ->
> +           add_disk_backend id backend_name;
> +           arg_list "-device" ([ "scsi-hd" ] @
common_props)
> +       | None ->
> +           add_cdrom_backend backend_name;
> +           arg_list "-device" ([ "scsi-cd" ] @
common_props))
>  
> -      | BusSlotRemovable { s_removable_type = Floppy } ->
> -         arg_list "-drive" ["format=raw";
"if=scsi"; "bus=0";
> -                            "unit=" ^ string_of_int i;
"media=floppy"]
> -    in
> -    Array.iteri make_scsi target_buses.target_scsi_bus;
> +    and add_floppy frontend_ctr > +      (* Create a "floppy"
(front-end), together with its empty drive
> +       * (back-end).
> +       *)
> +      let backend_name = sprintf "drive-floppy-%d" frontend_ctr
in
> +      add_floppy_backend backend_name;
> +      arg_list "-device" [ "floppy";
"bus=floppy-bus.0";
> +                           sprintf "unit=%d" frontend_ctr;
> +                           "drive=" ^ backend_name ] in
>  
> -    (* XXX Highly unlikely that anyone cares, but the current
> -     * code ignores target_buses.target_floppy_bus.
> +    (* Add virtio-blk-pci devices for BusSlotDisk elements on
> +     * "target_virtio_blk_bus".
>       *)
> +    Array.iteri
> +      (fun frontend_ctr disk ->
> +         match disk with
> +         | BusSlotDisk d -> add_virtio_blk d.s_disk_id frontend_ctr
> +         | _ -> ())
> +      target_buses.target_virtio_blk_bus;
> +
> +    let add_disk_or_cdrom bus_adder frontend_ctr slot > +      (* Add a
disk or CD-ROM front-end to the IDE or SCSI bus. *)
> +      match slot with
> +      | BusSlotDisk d ->
> +          bus_adder (Some d.s_disk_id) frontend_ctr
> +      | BusSlotRemovable { s_removable_type = CDROM } ->
> +          bus_adder None frontend_ctr
> +      | _ -> () in
> +
> +    (* Add disks and CD-ROMs to the IDE and SCSI buses. *)
> +    Array.iteri (add_disk_or_cdrom add_ide) target_buses.target_ide_bus;
> +    Array.iteri (add_disk_or_cdrom add_scsi) target_buses.target_scsi_bus;
> +
> +    (* Add floppies. *)
> +    Array.iteri
> +      (fun frontend_ctr disk ->
> +         match disk with
> +         | BusSlotRemovable { s_removable_type = Floppy } ->
> +             add_floppy frontend_ctr
> +         | _ -> ())
> +      target_buses.target_floppy_bus;
It's definitely not as complicated as I thought it could be.

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Libguestfs - Apr 2022 - [virt-v2v RFC] output_qemu: rewrite output disk mapping

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping

[Libguestfs] [virt-v2v RFC] output_qemu: rewrite output disk mapping