Re: [Qemu-devel] [PATCH 00/10] AHCI emulation support v2

[Kevin Wolf]

Am 18.11.2010 19:43, schrieb Alexander Graf:
> 
> On 18.11.2010, at 14:26, Kevin Wolf <address@hidden> wrote:
> 
>> Hi Alex,
>>
>> Am 18.11.2010 04:27, schrieb Alexander Graf:
>>> This patch adds support for AHCI emulation. I have tested and verified it 
>>> works
>>> in Linux, OpenBSD, Windows Vista and Windows 7. This AHCI emulation supports
>>> NCQ, so multiple read or write requests can be outstanding at the same time.
>>>
>>> The code is however not fully optimized yet. I'm fairly sure that there are
>>> low hanging performance fruits to be found still :). In my simple benchmarks
>>> I achieved about 2/3rd of virtio performance.
>>>
>>> Also, this AHCI emulation layer does not support legacy mode. So if you're
>>> using a disk with this emulation, you do not get it exposed using the legacy
>>> IDE interfaces.
>>>
>>> Another nitpick is CD-ROM support in Windows. Somehow it doesn't detect a
>>> CD-ROM drive attached to AHCI. At least it doesn't list it.
>>>
>>> To attach an AHCI disk to your VM, please use
>>>
>>>  -drive file=...,if=sata
>>>
>>> This should do the trick for x86. On other platforms, you might need to add
>>> the ahci host controller using -device.
>>>
>>>
>>> This patch set is based on work done during the Google Summer of Code. I was
>>> mentoring a student, Roland Elek, who wrote most of the AHCI emulation code
>>> based on a patch from Chong Qiao. A bunch of other people were also 
>>> involved,
>>> so everybody who I didn't mention - thanks a lot!
>>
>> I'm not completely sure about the relationship between the AHCI
>> emulation and our existing IDE emulation. First thing I noticed is that
>> AHCI wants to be independent and resides in hw/ instead of hw/ide/, but
>> it still include ide/internal.h. Do you think it would make sense to
>> move AHCI into hw/ide?
> 
> Both ahci and ide implement ata. I guess the best thing to do would be to 
> completely refactor all ide code into ata and pata code, then add ahci (sata) 
> to the game. Estimated working time: ~1 month. :)
> 
> As I would rather have something working we can base on in the tree, so 
> whoever volunteers for the refactoring (hint!) knows how to design the 
> interfaces, I am not sure how much is reasonable within this patch set.

I guess I have to read this as: You want to drop the code into the
repository, no matter what mess it creates, and leave the cleanup to
some volunteer that will never appear. So whoever is in the unfortunate
position of having to touch the IDE code afterwards will have the pain
because this series is doing only the half of what should be done.

I'm sure that with a working time of only a few days the result could be
substantially improved, even if a month would be needed for perfection.

> Moving the file to ide/ does sound like a good idea however.
> 
>>
>> Then I believe that core.c is now a mixture of some generic ATA code
>> (that is also used by SATA) and the Legacy IDE code. SATA doesn't seem
>> to interact with the generic code through clean interfaces, but by
>> accessing internal data structures and calls to somewhere in the middle
>> of the existing IDE emultion. I think we should get a clean abstraction
>> there and have a clean split between SATA, PATA and common code, with
>> each of them sitting in its own file in hw/ide.
>>
>> I haven't reviewed the patches in detail but just had a quick look at
>> them, so my impressions might be wrong. If so, please correct me.
> 
> No, you're completely right. We're in a chicken and egg situation. We don't 
> have ahci, but the ide code is ugly. We would probably do a bad job at 
> refactoring the ata code if we don't know which interfaces to design for.

That problem is solved. You have posted patches, so you're aware what
interfaces you need for AHCI. This awareness doesn't come from putting
the code into git master.

> So IMHO the only way we can really go is to implement sata, take the uglyness 
> it adds to the already ugly ide code and use all of that for a working state 
> we can start to refactor from.
> 
> So yes, while I agree with your obversations, I do not believe we will get 
> there until at least 0.16 or so. And I'd rather like to see a fast default 
> block driver in gueast sooner than later :)

Note that not doing the refactoring in this patch series means not only
that it won't happen, but also that your patches are very hard to
review. The order in which I would expect changes is to do the
refactoring and then base the SATA code on these clean interfaces - not
first creating a mess and then (maybe some time) trying to clean it up.

You always say "we" will refactor and "we" will clean up, so who is this
"we"? Will you do it or are you rather trying to put it on my todo list?
I must admit that I wouldn't be happy about the latter.

Kevin