Libguestfs - Apr 2022 - [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

On Thu, Apr 14, 2022 at 04:40:25PM +0100, Richard W.M. Jones
wrote:
> > +    (* 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
       ^ use
> 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 then using List.exists here and List.iter{,i} below.

[warning telling people to file a bug]
> 
> 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).
Actually I'm having second thoughts about encouraging people to file a
bug at all.  If lots of people see this message and file bugs, is that
something we actually would need to fix?  If yes then OK, otherwise
we'll just get more bugs on the backlog.

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 04/14/22 17:40, Richard W.M. Jones wrote:
> On Thu, Apr 14, 2022 at 05:07:29PM +0200, Laszlo Ersek wrote:
>> +    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.
I prefer sticking with Array.iteri: "frontend_ctr" is used in various
front-end properties, such as the primary/secondary master/slave
placement on (i440fx) IDE, the (q35) SATA port selection, the LUNs for
scsi-cd / scsi-hd, the unit number for floppies; so "frontend_ctr"
should iterate over consecutive, and small, integers.

Thanks!
Laszlo