Re: [Libguestfs] [v2v PATCH] 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(a)redhat.com&gt; --- Notes: v1: - do not pick up Rich's R-b yet, because of the changes below - replace "parent_ctrl_needed" with Array.exists [Rich] - reference 'virt-v2v(1) section "BUGS"' in the "this should not happen" warnings [Rich] - add "bus=scsi0.0" property to "scsi-hd" and "scsi-cd" devices - created test scenarios <https://bugzilla.redhat.com/show_bug.cgi?id=2074805#c17> and tested them 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..da8bd475e56e 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 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 = + Array.exists disk_cdrom_filter target_buses.target_ide_bus + and scsi_ctrl_needed = + Array.exists disk_cdrom_filter target_buses.target_scsi_bus + and floppy_ctrl_needed = + Array.exists floppy_filter target_buses.target_floppy_bus 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 -- refer to virt-v2v(1) \ + section \"BUGS\".") + ); - 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 -- refer to \ + virt-v2v(1) section \"BUGS\".") + ); - | 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 = [ "bus=scsi0.0"; + 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: b3a9dd6442ea2aff31bd93e85aa130f432e24932