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

[Alexander Graf]

On 09.07.2012, at 10:50, 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, 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.

I don't think this is what we want it to mean. What we want is:

"if=ide" means "if the board provides an IDE controller, create an IDE device 
attached to it. If it does not provide one, create one".

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

Yes. This should be ide for -M pc, scsi for -M pseries and ahci for -M q35.

> * "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).

The board should simply not create one then, no?

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

Oh? Must have missed those...

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

I don't think I understand?

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

Why would it? If no -drive if=ahci is give, drive_get_max_bus(IF_AHCI) returns 
0, so no device gets created.

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

I agree, we need some hint from the machine to tell us which devices it would 
prefer for its defaults. We have a hack that does something similar for virtio 
and s390, so we can say "virtio-blk" and get "virtio-blk-s390" on s390, but 
"virtio-blk-pci" on pci capable platforms. But this logic really should be 
machine, not architecture specific.

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

We don't have any board today that provides a controller.


Alex