Alexander Graf <ag...@suse.de> writes:
> We've had support for creating AHCI devices using -device for a while now,
> but it's cumbersome to users. We really should provide an easier way for
> them to leverage the power of AHCI!
>
> So let's introduce a new if= option to -drive, bumping it en par with virtio.
If I understand your patch correctly, this makes if=ahci work like
if=virtio and unlike if=ide:
* if=virtio: configure a new PCI device.
* if=ide: instruct the board to add an IDE device to its IDE controller.
For -M pc, the board's IDE controller happens to be piix3-ide.
For -M q35, I'd expect the board's IDE controller to be an ich9-ahci.
Once we switch to q35, if=ahci will become a redundant wart: to add
drives to the existing AHCI controller, you'll have to use if=ide.
if=ahci will create a new controller, which is generally not what you
want. Ugh.
A few questions inline.
> Signed-off-by: Alexander Graf <ag...@suse.de>
> ---
> blockdev.c | 25 +++++++++++++++++++++++--
> blockdev.h | 1 +
> qemu-options.hx | 7 ++++++-
> 3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 622ecba..5405f6c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -32,6 +32,7 @@ static const char *const if_name[IF_COUNT] = {
> [IF_SD] = "sd",
> [IF_VIRTIO] = "virtio",
> [IF_XEN] = "xen",
> + [IF_AHCI] = "ahci",
> };
>
> static const int if_max_devs[IF_COUNT] = {
> @@ -519,7 +520,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> } else {
> /* no id supplied -> create one */
> dinfo->id = g_malloc0(32);
> - if (type == IF_IDE || type == IF_SCSI)
> + if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
> mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
> if (max_devs)
> snprintf(dinfo->id, 32, "%s%i%s%i",
> @@ -549,6 +550,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> case IF_IDE:
> case IF_SCSI:
> case IF_XEN:
> + case IF_AHCI:
> case IF_NONE:
> switch(media) {
> case MEDIA_DISK:
> @@ -582,6 +584,25 @@ DriveInfo *drive_init(QemuOpts *opts, int
> default_to_scsi)
> default:
> abort();
> }
> +
> + if (type == IF_AHCI) {
> + static int ahci_bus = 0;
> + char devname[] = "ahciXXX";
> + char busname[] = "ahciXXX.0";
> + snprintf(devname, sizeof(devname), "ahci%d", ahci_bus);
> + snprintf(busname, sizeof(busname), "ahci%d.0", ahci_bus++);
> +
> + /* add ahci host controller */
> + opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
> + qemu_opt_set(opts, "driver", "ich9-ahci");
> +
> + /* and attach a single ata disk to its bus */
> + opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
> + qemu_opt_set(opts, "driver", "ide-drive");
> + qemu_opt_set(opts, "bus", busname);
> + qemu_opt_set(opts, "drive", dinfo->id);
> + }
> +
Doesn't this create a new ich9-ahci controller per -drive?
If yes, it's problematic in practice, as you'll run out of PCI slots
pretty darn fast. That problem made us replace virtio-blk by
virtio-scsi. Let's not re-create it.
IF_VIRTIO device option creation is done in the preceeding switch. You
don't do that for IF_AHCI because you already do something else there:
handling the media option. I think one switch for media and a separate
one for device options would be clearer.
> if (!file || !*file) {
> return dinfo;
> }
> @@ -604,7 +625,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> ro = 1;
> } else if (ro == 1) {
> if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> - type != IF_NONE && type != IF_PFLASH) {
> + type != IF_NONE && type != IF_PFLASH && type != IF_AHCI) {
> error_report("readonly not supported by this bus type");
> goto err;
> }
Are you sure AHCI can handle read-only?
[...]
> diff --git a/blockdev.h b/blockdev.h
> index 260e16b..e14c1d5 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -23,6 +23,7 @@ typedef enum {
> IF_DEFAULT = -1, /* for use with drive_add() only */
> IF_NONE,
> IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> + IF_AHCI,
> IF_COUNT
> } BlockInterfaceType;
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8b66264..9527c51 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -160,7 +160,7 @@ Special files such as iSCSI devices can be specified
> using protocol
> specific URLs. See the section for "Device URL Syntax" for more information.
> @item if=@var{interface}
> This option defines on which type on interface the drive is connected.
> -Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
> +Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio, ahci.
> @item bus=@var{bus},unit=@var{unit}
> These options define where is connected the drive by defining the bus number
> and
> the unit id.
> @@ -260,6 +260,11 @@ You can connect a SCSI disk with unit ID 6 on the bus #0:
> qemu-system-i386 -drive file=file,if=scsi,bus=0,unit=6
> @end example
>
> +You can attach a SATA disk using AHCI:
> +@example
> +qemu-system-i386 -drive file=file,if=ahci
> +@end example
> +
If I'm reading drive_init() correctly, if=ahci doesn't attach a disk, it
creates a controller with a disk. So this is somewhat misleading.
> Instead of @option{-fda}, @option{-fdb}, you can use:
> @example
> qemu-system-i386 -drive file=file,index=0,if=floppy
