Re: [Qemu-devel] [PATCH v2] 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, giving users the same
> command line experience as for scsi or ide.
>
> Signed-off-by: Alexander Graf <address@hidden>
>
> ---
>
> v1 -> v2:
>
>   - support more than a single drive per adapter
>   - support index= option
>   - treat IF_AHCI the same as IF_IDE

Inhowfar?  Not obvious to me from the patch, or the diff patch v1.

>   - add is_ata() helper to match AHCI || IDE

Not addressed:

Once we switch to q35, if=ahci will become a redundant wart: to add
drives to the board's AHCI controller, you'll have to use if=ide.
if=ahci will create new controllers, which is generally not what you
want.  Ugh.


> ---
>  blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  blockdev.h      |    7 +++++++
>  qemu-options.hx |    7 ++++++-
>  vl.c            |    2 ++
>  4 files changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9e0a72a..744a886 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] = {
> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>       */
>      [IF_IDE] = 2,
>      [IF_SCSI] = 7,
> +    [IF_AHCI] = 6,
>  };
>  
>  /*
> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int 
> default_to_scsi)
       if ((buf = qemu_opt_get(opts, "if")) != NULL) {
           for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
               ;
           if (type == IF_COUNT) {
               error_report("unsupported bus type '%s'", buf);
               return NULL;
           }
       } else {
           type = default_to_scsi ? IF_SCSI : IF_IDE;
       }

A board can't default to IF_AHCI.  I guess what such a board would do is
treat IF_IDE and IF_AHCI just the same.

Leads me this question: what do "if=ide", "if=ahci" and "no if given"
mean?  Let me try:

* "if=ide" means "if the board provides an IDE controller, create an IDE
  device attached to it.  What kind of IDE controller the board provides
  doesn't matter.  In particular, an AHCI controller is fine.

* "no if given" means "create a block device of the board's preferred
  kind" in theory, and "default to either if=ide or if=scsi" in current
  practice.

* "if=ahci" means "create an IDE device and attach it to a completely
  seperate set of ich9-ahci controllers specifically created for the
  "-drive if=ahci".  If the board provides an AHCI controller, it's not
  used for if=ahci.  It may still be used for if=ide (depends on board).

  Isn't this an embarrassment?

>      max_devs = if_max_devs[type];
>  
>      if (cyls || heads || secs) {
> -        if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
> +        if (cyls < 1 || (is_ata(type) && cyls > 16383)) {
>              error_report("invalid physical cyls number");
>           return NULL;
>       }
> -        if (heads < 1 || (type == IF_IDE && heads > 16)) {
> +        if (heads < 1 || (is_ata(type) && heads > 16)) {
>              error_report("invalid physical heads number");
>           return NULL;
>       }
> -        if (secs < 1 || (type == IF_IDE && secs > 63)) {
> +        if (secs < 1 || (is_ata(type) && secs > 63)) {
>              error_report("invalid physical secs number");
>           return NULL;
>       }

Trivial conflict with my "blockdev: Drop redundant CHS validation for
if=ide".  Don't worry about it.

A few more instances of IF_IDE:

       on_write_error = BLOCK_ERR_STOP_ENOSPC;
       if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
           if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type 
!= IF_NONE) {
               error_report("werror is not supported by this bus type");
               return NULL;
           }

           on_write_error = parse_block_error_action(buf, 0);
           if (on_write_error < 0) {
               return NULL;
           }
       }

       on_read_error = BLOCK_ERR_REPORT;
       if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
           if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type 
!= IF_NONE) {
               error_report("rerror is not supported by this bus type");
               return NULL;
           }

           on_read_error = parse_block_error_action(buf, 1);
           if (on_read_error < 0) {
               return NULL;
           }
       }

Are you sure you don't want to check for IF_AHCI?

> @@ -516,7 +518,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 (is_ata(type) || type == IF_SCSI)
>              mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>          if (max_devs)
>              snprintf(dinfo->id, 32, "%s%i%s%i",
> @@ -546,6 +548,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:
> @@ -628,6 +631,49 @@ err:
>      return NULL;
>  }
>  
> +static void drive_populate_ahci(void)
> +{
> +    int bus;
> +    QemuOpts *opts;
> +
> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
> +        char devname[] = "ahciXXX";
> +        int dev;
> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
> +
> +        /* add ahci host controller */
> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
> +        qemu_opt_set(opts, "driver", "ich9-ahci");

Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
ahci_init() correctly, it provides six.

Creates it even if the bus is unused.

While better than v1, which created one per -drive, it still wastes
precious PCI slots, doesn't it?

Hardcodes the AHCI controller device to "ich9-ahci".  Just we do for
IF_SCSI, only in generic code instead of board-specific code.  No
problem, we can always add another IF_ ;)  Just kidding; we'd add a way
for the board to define the preferred controller device.  SCSI could use
that, too.

> +        for (dev = 0; dev < if_max_devs[IF_AHCI]; dev++) {
> +            DriveInfo *dinfo = drive_get(IF_AHCI, bus, dev);
> +            char busname[] = "ahciXXX.XXX";
> +
> +            if (!dinfo) {
> +                continue;
> +            }
> +            snprintf(busname, sizeof(busname), "ahci%d.%d", bus, dev);
> +
> +            /* attach this 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);
> +        }
> +    }
> +}
> +
> +/*
> + * This function creates -device options out of IF_xxx elements,
> + * so that we don't have to mess with user friendly syntax parsing
> + * in device emulation code.
> + *
> + * For now, only AHCI is implemented here.
> + */
> +void drive_populate(void)
> +{
> +    drive_populate_ahci();
> +}
> +
>  void do_commit(Monitor *mon, const QDict *qdict)
>  {
>      const char *device = qdict_get_str(qdict, "device");
> diff --git a/blockdev.h b/blockdev.h
> index 260e16b..9d79558 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;
>  
> @@ -53,6 +54,12 @@ QemuOpts *drive_def(const char *optstr);
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
>  DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
> +void drive_populate(void);
> +
> +static inline bool is_ata(int type)
> +{
> +    return (type == IF_IDE) || (type == IF_AHCI);
> +}
>  
>  /* device-hotplug */
>  
> 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
> +

I still think the automatic controller creation should be documented.
*Especially* since it does that even when the board provides a perfectly
usable AHCI controller already.

>  Instead of @option{-fda}, @option{-fdb}, you can use:
>  @example
>  qemu-system-i386 -drive file=file,index=0,if=floppy
> diff --git a/vl.c b/vl.c
> index 1329c30..65410a0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3438,6 +3438,8 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_sdcard, snapshot, machine->use_scsi,
>                    IF_SD, 0, SD_OPTS);
>  
> +    drive_populate();
> +
>      register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live, NULL,
>                           ram_load, NULL);