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

[Alexander Graf]

On 19.11.2010, at 10:15, Kevin Wolf wrote:

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

I guess you should go back and read the "this doesn't work yet" list. There is 
a lot of stuff that I'm not sure we have all correctly sorted out. The most 
intrusive one on that side is the legacy IDE compatibility. I don't know what 
interfaces and what changes we will need for that to become realistic.

Also to catch up on Gerd's point - whatever refactoring we do, we will 
basically have to break migration. There is no way we can change all the 
internal state and structure and maintain binary compatibility with the old 
save states.


Alex