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

[Alexander Graf]

On 19.11.2010, at 14:46, Kevin Wolf wrote:

> Am 19.11.2010 14:08, schrieb Alexander Graf:
>> 
>> On 19.11.2010, at 10:15, Kevin Wolf wrote:
>> 
>>> Am 18.11.2010 19:43, schrieb Alexander Graf:
>>>>> 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.
> 
> Fair enough. I'll accept that we can't get it sorted out now, but let's
> try to do the part that we can do. Let's change the split to SATA
> (ahci.c), Legacy IDE (ide.c?), common code (ata.c) and "don't know yet"
> (core.c).
> 
> A start for that would be if in Patch 2 you moved the function to ata.c
> instead of just reindenting. We're also probably pretty sure that, for
> example, the I/O port handling won't need to be shared and can be moved
> to ide.c. Whenever it becomes clear for a part in which category it
> belongs, we would move it out of core.c and eventually, I hope, core.c
> could be removed.

I can certainly move out obviously pata specific pieces to a new file called 
pata.c. As for the split between ata.c and core.c, I don't think it's useful. 
Once we moved everything pata specific out or core.c, that file will 
essentially be ata.c.

> 
>> 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.
> 
> Hm, breaking migration isn't really an option. I'm not familiar with
> migration code, but maybe Juan can contribute some magic?
> 
> Speaking of migration, that seems to be missing for the AHCI yet, too.
> Are you planning to complete the functionality first before you start
> with that?

I'm planning to take baby steps so others can contribute and I don't keep a 
patch set lying around become more invasive and thus more prone to bitrot every 
day :). I myself just don't scale well enough for a feature this intrusive and 
important.

I had the code bitrot for quite a bit already btw. GSoC ended a couple months 
ago and I just didn't get around to polish the code well enough for upstream 
submission. And believe me, it rots very fast.

Vmstate is an issue we need to solve. I'm not sure what the right way forward 
for that would be. I certainly would not recommend declaring the migration 
protocol for sata even remotely stable for the time being, as we're missing 
crucial pieces still that might require major restructuring or even duplicating 
of core ide code. And as long as that's the case, I'm not sure how much sense 
it really makes to have any at all.

Basically, if there was a CONFIG_EXPERIMENTAL flag, I would set it on ahci. The 
code is not and will not be 100% stable and well structures and reliable within 
the next probably 1/2 year to year. But we need to start walking into a 
direction where it can finally end up being there some day, and that only works 
by multiple people working together on this, preferably upstream, so we don't 
collide with other possible ide work.

Of course there's some chance that it won't get there. If there is interest in 
it though, it will. And from what I've gathered so far, there is interest, as 
it's a speedup for a lot of guests without the need for special drivers. If it 
just lies around without getting improved, rip it out again :). If nobody works 
on it, nobody uses it.


Alex