Alexander Graf <ag...@suse.de> writes:
> 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.
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 <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.
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.
