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

[Markus Armbruster]

Alexander Graf <address@hidden> writes:

> On 12.07.2012, at 13:09, Markus Armbruster wrote:
>
>> 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.
>>> 
>>> 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.
>> 
>> Yes, but does the naive command line user care?
>> 
>> -serial configures a serial device.  The kind of device depends on the
>> board: 16550A UARTs with -M pc, Exynos 4210 UARTs with -M nuri, ColdFire
>> UARTs with -M an5206, ...  You can't write a generic serial device
>> driver.
>> 
>> Any thoughts on the remainder of my message, behavior of if=ahci in
>> particular?
>
> I think this is the core of the disagreement we're having. I believe
> that IDE and AHCI are as much different as IDE and SCSI, while you're
> trying to see them as a single thing with different implementations.

Actually, this is *one* topic.  There's a second one further down, which
you haven't replied to at all.  Namely: assuming we want if=ahci, how
should it behave?

Your patch makes it behave unlike any other if=FOO.  I explained why I
don't like that, and what could be done about it.  Please reply to my
comments.  Thanks.

[...]