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

[Markus Armbruster]

Alexander Graf <address@hidden> writes:

> On 12.07.2012, at 10:17, Markus Armbruster wrote:
>
>> [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?
>
> I agree that they should behave similar within their scope, yes.
>
>> 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.
>
> At the point in time when -drive is passed in, we don't know what
> controllers the board might create yet, no?

Correct.

>> 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
>
> Sure, which renders if=ide completely useless on machines that don't
> spawn an IDE controller by default, like -M mpc8544ds.

Correct.  Having it attempt to create a controller would be nicer, and
consistent with how you want if=ahci behave (I think).

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

The technical reason for this behavior: -drive if=ide creates the
backends immediately, but leaves creating device models to the board
initialization function.  If the board doesn't know about IDE...

>> 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.
>
> I think we have 2 options
>
> 1) Keep the creation process 2-tiered. Parse the cmdline before
> machine create. Plug disks on after/during machine create. This can
> get ugly pretty quick.
>
> 2) Make the process be linear. Command line parsing goes first. Then
> goes drive parsing which creates -device parameters. Then machine
> creation. During the time we parse the drives we should be able to
> evaluate machine info, so we can put bits in there like "spawns the
> first 2 IDE buses itself".

I'm afraid this is a bit too terse and abstract for me to follow...

[...]
>>>>> +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.
>
> How is that different from if=scsi?

First of all, if=scsi isn't exactly a shining example of how to do
things right.

But one thing it does right is not creating unnecessary controllers
unless ordered.  If you don't specify bus, unit, you get one controller
per seven -drive.  That's how it should be; the LSI controller supports
seven units.

Your patch appears to create one AHCI controller per -drive.  But
ich9-ahci has six ports, doesn't it?  Shouldn't you create one
controller per six -drive?