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

[Markus Armbruster]

[Alex's illegibly long lines wrapped]

Alexander Graf <address@hidden> writes:

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

Okay, we got two competing ideas here.

1. -drive doesn't give you any control over the kind of IDE controller.
You can select an IDE bus by number (bus=...), and you get whatever
existing controller provides this bus.  If none exists, a board-specific
one may be created for your convenience.  If you need more control, use
-device to set up controllers the way you want.

2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
you get an existing AHCI controller.  If none exists, a board-specific
one may be created for your convenience.  With if=ide, you get an
existing non-AHCI controller.  If none exists, a board-specific one may
be created for your convenience.  If you need more control, use -device
to set up controllers the way you want.

The question to answer is whether the difference between AHCI and
non-AHCI is so important that we want to provide control in -drive.

What I'd like to avoid is casual users setting up new guests with our
shiny new q35 board getting their IDE drives connected to some slow, old
controller just because they haven't discovered that if=ide doesn't cut
it anymore, and they need to use if=ahci now.

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

The controller is an integral part of the board.  Chipset even.  It
should always be created, drive or no drives.

Here's a meaning that's consistent with the one you proposed for if=ide:
if the board provides an AHCI controller, create an IDE device attached
to it.  If it does not provide one, create one first.

See, I really don't want yet another if=FOO with its own special
behavior.  I'd be much happier with a set of if=... that behaves pretty
much the same.  Ideally, all of them, but that's a tall order.  However,
let's not make it worse by adding new special behaviors.  I'm trying to
find a way to make if=ide and if=ahci behave pretty much the same.  Do
you agree with this goal in principle?

Let me refine.  If the controller identified by if and bus exists, use
it.  Else, if such a controller can be created, create and use it.
Else, fail.

if=ide is already consistent with this meaning, in a somewhat degenerate
form: "can be created" is always false.  Only the board creates IDE
controllers.  All our boards create zero or two IDE buses.  If it
creates two, attempts to use more fail.  Wart: if it creates none,
attempts to use any are silently ignored.  Wart: if the user adds IDE
controllers with -device, we don't use them.

if=scsi isn't consistent with this meaning.  It could be made
consistent, but I'm not asking you do that work now.

Your if=ahci isn't quite consistent with this meaning, because it never
uses existing controllers.

I'm open to other ways to make if=ide and if=ahci consistent.

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

You're welcome ;)

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

With "-drive if=ahci,bus=5,...", drive_get_max_bus() returns 5, and the
loop executes six times, creating six AHCI controllers, doesn't it?
Each of them provides six buses, for a total of 36.

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

Agree.

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

Let's revisit this when we agree on how if=ahci should work.