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

[Gerd Hoffmann <kraxel@redhat.com>]

   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.

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

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.

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.

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.  Also having a
unused slave state doesn't make sense at all for sata.  Fixing that
without breaking migration could be non-trivial though.

cheers,
   Gerd