On 09.07.2012, at 10:50, Markus Armbruster wrote:
> Alexander Graf <ag...@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, giving users the same
>> command line experience as for scsi or ide.
>>
>> Signed-off-by: Alexander Graf <ag...@suse.de>
>>
>> ---
>>
>> 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 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
>> +
>
> 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
