On 14.06.2012, at 09:29, 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, 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 <ag...@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
