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

[Kevin Wolf <kwolf@redhat.com>]

Am 19.11.2010 12:56, schrieb Gerd Hoffmann:
>    Hi,
> 
>>> 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.
> 
> Well, this is how IDE gets cleaned up right now.  I was one of the 
> victims ;)
> 
> IDE is in a noticeable better state than it used to be two years ago.
> It is still quite messy though.

Sure that's how it works today. We have no choice because we can't
change the mistakes that were made in the past. But we can avoid to
contribute new mess to it or we'll need even more victims. ;-)

>> 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.
> 
> [ disclaimer: didn't look at v3 yet ]
> 
> Adding ahci should at least not make it more messy that it already is.
> 
> Doing the controller-depending dispatching through a function pointer
> table (aka IDEBusOps) looks sane to me.  SCSI does the same, although
> without a Ops struct as it needs a single function pointer only.

Yes, I suppose (still haven't looked at it in detail) these interfaces
are generally sane, based on your comments and a quick look.

This is also what makes me think that doing the next step shouldn't be
too hard. The abstraction is basically in place, the rest should be
mostly code movement and maybe splitting up some IDE data structures to
make things look more symmetric between PATA and SATA.

> Moving code shared between sata/ahci and ide to another source file
> (ata.c ?) makes sense and would be a good start to make clear which code
> is used for what.

I agree (and ata.c would have been my suggestion, too).

> IDEBus must learn some things about sata.  For example there is no such
> thing as a slave device, so a IDEBus representing a sata port must not
> allow slave devices being attached.

Hm, do the generic functions (i.e. what would end up in ata.c) even need
access to the IDEBus? If not, we could leave IDEBus as it is for Legacy
IDE and introduce a completely new SATABus.

Most occurrences in core.c seem to use the bus only for ide_set_irq.

> Fixing the IDEState mess would be great.  Right now IDEBus carries a
> array of IDEStates for master and slave for historical reasons:  The ide
> code (used to?) assume that the IDEStates for master and slave are
> allocated together and that it can easily jump from one to the other and
> back using pointer arithmetics on the IDEState struct.  IDEState doesn't
> belong there though, it should be part of IDEDrive. 

Okay, that makes sense.

Kevin