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

[Alexander Graf]

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.

Ah, I think I understand the main issue now: You're thinking of AHCI as an IDE 
controller.

It isn't. AHCI is on the same level as IDE. They both speak ATA, but the guest 
os interface is completely different. You can write a generic IDE driver, but 
that won't be able to talk to an AHCI controller in AHCI mode. You can write a 
generic AHCI driver, but that won't be able to talk to an IDE controller.


Alex