Re: [Qemu-devel] [PATCH] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <address@hidden> 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 <address@hidden>
> ---
>  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 address@hidden
>  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 address@hidden,address@hidden
>  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:
> address@hidden
> +qemu-system-i386 -drive file=file,if=ahci
> address@hidden 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