2:13 a.m.
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 <lersek(a)redhat.com>
> ---
>
> 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 <rjones(a)redhat.com>
Rich.