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

[Kevin Wolf]

Am 21.11.2010 03:19, schrieb 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.

The reason why I suggested ata.c is that core.c would serve as kind of a
todo list. But I don't really mind if you wan to keep it in core.c, the
important part is getting the split between core/ata and pata.

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

Okay, I think that's a good point.

I was asking because I'm not sure if it wouldn't be easier to have
migration working early and then incrementally change it with whatever
is added, compared to developing everything and adding migration as the
last thing. I haven't done either yet, so I might be wrong.

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

Not disagreeing with that. Also, we have a lot of hardware that has only
a few users. That's never been a reason for dropping it.

The one important point to me is that AHCI touches IDE and shouldn't
leave it in a worse state than before. If AHCI itself in its current
state really works well all the time is secondary for me, because, as
you say, there's still time to fix and enhance it when it's in.

Kevin