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

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

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.

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 :)


Alex

>