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

[Alexander Graf]

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

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

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

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

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