On 04/15/22 11:13, Richard W.M. Jones wrote: > On Fri, Apr 15, 2022 at 10:47:28AM +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 <[email protected]> >> --- >> >> 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 > > A style point: When defining things which are functions (even if they > are curried) I usually put the "in" on the next line as a small > reminder that it's a function and not a value. eg: > > 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 <-- here > > There are a few places in the code that don't follow that style.
Yeah, I'll fix this up before pushing. Another style point / simplification has occurred to me the other day too: in the patch I use the following expression four times: ([ device_model ] @ common_props) that should be just (device_model :: common_props) IIUC. Thanks, Laszlo > > Reviewed-by: Richard W.M. Jones <[email protected]> > > Rich. > _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
