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

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

[Alexander Graf]

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.

Signed-off-by: Alexander Graf <agraf@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);
+    }
+
     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;
         }
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
+
 Instead of @option{-fda}, @option{-fdb}, you can use:
 @example
 qemu-system-i386 -drive file=file,index=0,if=floppy
-- 
1.6.0.2

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

[Markus Armbruster]

Alexander Graf <agraf@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 <agraf@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

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

[Alexander Graf]

On 14.06.2012, at 09:29, Markus Armbruster wrote:

> Alexander Graf <agraf@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.

Yeah, I couldn't come up with anything else that's not completely ugly like the IF_SCSI implementation.

> 
> A few questions inline.
> 
>> Signed-off-by: Alexander Graf <agraf@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.

Hrm. If you have a great idea on how to implement it, I'm all open for it. Talking about it from a high level perspective I had the same feelings at first. Looking at how to implement index= and bus= for real, I quickly withdrew myself from the approach.

The good news is that the limitation here is only a -drive if=ahci limitation. It does not apply to -device. There you can still plug up to 6 ahci devices onto a single HBA.

> 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.

We can do that, yeah :).

> 
>>     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?

Isn't read-only handled in generic ATA code?

> 
> [...]
>> 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.

It creates an HBA with a disk, yes. Which for almost everyone is the same thing as attaching a disk. People who want to create more sophisticated setups are still free to use the complex -device based syntax :).


Alex

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

[Kevin Wolf]

Am 14.06.2012 09:34, schrieb Alexander Graf:
> 
> On 14.06.2012, at 09:29, Markus Armbruster wrote:
> 
>> Alexander Graf <agraf@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.
> 
> Yeah, I couldn't come up with anything else that's not completely ugly like the IF_SCSI implementation.
> 
>>
>> A few questions inline.
>>
>>> Signed-off-by: Alexander Graf <agraf@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.
> 
> Hrm. If you have a great idea on how to implement it, I'm all open for it. Talking about it from a high level perspective I had the same feelings at first. Looking at how to implement index= and bus= for real, I quickly withdrew myself from the approach.
> 
> The good news is that the limitation here is only a -drive if=ahci limitation. It does not apply to -device. There you can still plug up to 6 ahci devices onto a single HBA.

That I get two controllers when I have two drivers is really not
something I would expect as a user. If we can, we should make it use up
any existing controllers. If we can't, maybe -device has to be good enough.

Kevin

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

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 14.06.2012, at 09:29, Markus Armbruster wrote:
>
>> Alexander Graf <agraf@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.
>
> Yeah, I couldn't come up with anything else that's not completely ugly like the IF_SCSI implementation.

Here's a non-ugly solution: finish the q35 job, and if=ide just works :)

Except for old VMs that still have piix3-ide.  And for those VMs an easy
way to add AHCI controllers and drives isn't exactly a priority.

>> A few questions inline.
>> 
>>> Signed-off-by: Alexander Graf <agraf@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.
>
> Hrm. If you have a great idea on how to implement it, I'm all open for it. Talking about it from a high level perspective I had the same feelings at first. Looking at how to implement index= and bus= for real, I quickly withdrew myself from the approach.

I'm afraid the sane way to do this is going to be complicated, just like
if=scsi.  I understand your reluctance to do that; I feel the same.
That's why I'm suggesting to make the problem go away via -M q35.

> The good news is that the limitation here is only a -drive if=ahci limitation. It does not apply to -device. There you can still plug up to 6 ahci devices onto a single HBA.

A good convenience option covers a useful subset of common cases neatly.

Your -drive if=ahci covers one common case: add the first AHCI drive
(automatically adding the controller for it as well).  It doesn't cover
the common case of adding a second AHCI drive (to the same controller,
of course).  If I have to resort to -device for something as simple as
that, then I wonder why it's worth having -drive if=ahci at all.

>> 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.
>
> We can do that, yeah :).
>
>> 
>>>     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?
>
> Isn't read-only handled in generic ATA code?

I think it is.  But treating IF_AHCI and IF_IDE differently here is
confusing, isn't it?

>> [...]
>>> 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.
>
> It creates an HBA with a disk, yes. Which for almost everyone is the same thing as attaching a disk. People who want to create more sophisticated setups are still free to use the complex -device based syntax :).

I think we'd owe users a fair warning that if=ahci does something rather
unexpected, namely adding a controller.