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

[Kevin Wolf]

Am 14.06.2012 09:34, schrieb 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.

That I get two controllers when I have two drivers is really not
something I would expect as a user. If we can, we should make it use up
any existing controllers. If we can't, maybe -device has to be good enough.

Kevin