[Qemu-devel] AHCI support integration

[Qemu-devel] AHCI support integration

[Sebastian Herbszt]

The ICH6 AHCI implementation submitted by Chong is an all-in-one attempt (ahci.c).
It includes all needed parts of the ICH6, AHCI, SATA and ATA specification.
The code in hw/ide/* on the other hand is split (or could be split) into smaller parts like
port based and bus master access, IDE and ATA core.
I think it might be reasonable to split ahci.c into ICH6, AHCI and SATA parts and strip the
limited ATA support and reuse it from the ide core.
This would give us something like the following:

hw/ide/piix.c (PIIX3 and PIIX4)
hw/ide/pci.c and core.c (IDE, BM)
hw/ata-core.c (ATA)
hw/sata/ich6.c (ICH6)
hw/sata-core.c (SATA)
hw/ahci-core.c (AHCI)

Should this be a goal or am i over-engineering here?

Sebastian

[Qemu-devel] Re: AHCI support integration

[Alexander Graf]

Sebastian Herbszt wrote:
> The ICH6 AHCI implementation submitted by Chong is an all-in-one
> attempt (ahci.c).
> It includes all needed parts of the ICH6, AHCI, SATA and ATA
> specification.
> The code in hw/ide/* on the other hand is split (or could be split)
> into smaller parts like
> port based and bus master access, IDE and ATA core.
> I think it might be reasonable to split ahci.c into ICH6, AHCI and
> SATA parts and strip the
> limited ATA support and reuse it from the ide core.
> This would give us something like the following:
>
> hw/ide/piix.c (PIIX3 and PIIX4)
> hw/ide/pci.c and core.c (IDE, BM)
> hw/ata-core.c (ATA)
> hw/sata/ich6.c (ICH6)
> hw/sata-core.c (SATA)
> hw/ahci-core.c (AHCI)
>
> Should this be a goal or am i over-engineering here?

[CC'ing Tejun - he volunteered to help out on this topic as well]

I think there's no need to split sata and ahci.
Apart from that, I think we should take things incrementally. For now
there's no need to split IDE further until we hit a technical limit. I
have yet to see a patch trying to reuse the IDE command processing, so
depending on how the respective person implements that, I'm open to
suggenstions.

So as far as I'm concerned, I'd prefer to model things after they're
developed. That way we know the pitfalls.


Alex

[Qemu-devel] Re: AHCI support integration

[Tejun Heo]

Hello,

On 05/09/2010 09:11 PM, Alexander Graf wrote:
> Sebastian Herbszt wrote:
>> The ICH6 AHCI implementation submitted by Chong is an all-in-one
>> attempt (ahci.c).  It includes all needed parts of the ICH6, AHCI,
>> SATA and ATA specification.  The code in hw/ide/* on the other hand
>> is split (or could be split) into smaller parts like port based and
>> bus master access, IDE and ATA core.  I think it might be
>> reasonable to split ahci.c into ICH6, AHCI and SATA parts and strip
>> the limited ATA support and reuse it from the ide core.  This would
>> give us something like the following:
>>
>> hw/ide/piix.c (PIIX3 and PIIX4)
>> hw/ide/pci.c and core.c (IDE, BM)
>> hw/ata-core.c (ATA)
>> hw/sata/ich6.c (ICH6)
>> hw/sata-core.c (SATA)
>> hw/ahci-core.c (AHCI)
>>
>> Should this be a goal or am i over-engineering here?
> 
> [CC'ing Tejun - he volunteered to help out on this topic as well]
> 
> I think there's no need to split sata and ahci.
> Apart from that, I think we should take things incrementally. For now
> there's no need to split IDE further until we hit a technical limit. I
> have yet to see a patch trying to reuse the IDE command processing, so
> depending on how the respective person implements that, I'm open to
> suggenstions.

I don't know the code but here are a few things which might worth
considering.

* ahci and the IDE interfaces are pretty much independent in most
  implementations.

* On host emulation side, there might not be too much to separate out
  for generic sata part from ahci at this point.  Link state emulation
  should be pretty simple and I suppose multiple command processing
  would be a bit strongly tied into specific host emulation.

* If necessary, a separate IDE layer below PIIX3/4, ICH whatever would
  be nice.  Most IDE controllers behave about the same after all and
  depending on implementation except for link emulation and NCQ
  handling, ahci can probably just wrap the IDE layer for other stuff
  too.

* If necessary, implementing generic ahci would be nice.  Most ahci
  implementations only have small quirks on top of generic ahci
  behavior (even less deviance compared to IDE), so separating it out
  shouldn't be difficult at all.

Thanks.

-- 
tejun

Re: [Qemu-devel] AHCI support integration

[Christoph Egger]

On Sunday 09 May 2010 18:01:50 Sebastian Herbszt wrote:
> The ICH6 AHCI implementation submitted by Chong is an all-in-one attempt
> (ahci.c). It includes all needed parts of the ICH6, AHCI, SATA and ATA
> specification. The code in hw/ide/* on the other hand is split (or could be
> split) into smaller parts like port based and bus master access, IDE and
> ATA core.
> I think it might be reasonable to split ahci.c into ICH6, AHCI and SATA
> parts and strip the limited ATA support and reuse it from the ide core.
> This would give us something like the following:
>
> hw/ide/piix.c (PIIX3 and PIIX4)
> hw/ide/pci.c and core.c (IDE, BM)
> hw/ata-core.c (ATA)
> hw/sata/ich6.c (ICH6)
> hw/sata-core.c (SATA)
> hw/ahci-core.c (AHCI)
>
> Should this be a goal or am i over-engineering here?

I think this will result in code much easier to read and to
maintain. You can even split out FIS code
into a seperate file since not every SATA controller is
an AHCI controller.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] Re: AHCI support integration

[Christoph Egger]

On Sunday 09 May 2010 21:11:17 Alexander Graf wrote:
> Sebastian Herbszt wrote:
> > The ICH6 AHCI implementation submitted by Chong is an all-in-one
> > attempt (ahci.c).
> > It includes all needed parts of the ICH6, AHCI, SATA and ATA
> > specification.
> > The code in hw/ide/* on the other hand is split (or could be split)
> > into smaller parts like
> > port based and bus master access, IDE and ATA core.
> > I think it might be reasonable to split ahci.c into ICH6, AHCI and
> > SATA parts and strip the
> > limited ATA support and reuse it from the ide core.
> > This would give us something like the following:
> >
> > hw/ide/piix.c (PIIX3 and PIIX4)
> > hw/ide/pci.c and core.c (IDE, BM)
> > hw/ata-core.c (ATA)
> > hw/sata/ich6.c (ICH6)
> > hw/sata-core.c (SATA)
> > hw/ahci-core.c (AHCI)
> >
> > Should this be a goal or am i over-engineering here?
>
> [CC'ing Tejun - he volunteered to help out on this topic as well]
>
> I think there's no need to split sata and ahci.
> Apart from that, I think we should take things incrementally. For now
> there's no need to split IDE further until we hit a technical limit. I
> have yet to see a patch trying to reuse the IDE command processing, so
> depending on how the respective person implements that, I'm open to
> suggenstions.
>
> So as far as I'm concerned, I'd prefer to model things after they're
> developed. That way we know the pitfalls.

You missed Sebastians point. His question is related to software design.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[Qemu-devel] Re: AHCI support integration

[Joerg Roedel]

On Sun, May 09, 2010 at 09:11:17PM +0200, Alexander Graf wrote:
> Sebastian Herbszt wrote:
> > The ICH6 AHCI implementation submitted by Chong is an all-in-one
> > attempt (ahci.c).
> > It includes all needed parts of the ICH6, AHCI, SATA and ATA
> > specification.
> > The code in hw/ide/* on the other hand is split (or could be split)
> > into smaller parts like
> > port based and bus master access, IDE and ATA core.
> > I think it might be reasonable to split ahci.c into ICH6, AHCI and
> > SATA parts and strip the
> > limited ATA support and reuse it from the ide core.
> > This would give us something like the following:
> >
> > hw/ide/piix.c (PIIX3 and PIIX4)
> > hw/ide/pci.c and core.c (IDE, BM)
> > hw/ata-core.c (ATA)
> > hw/sata/ich6.c (ICH6)
> > hw/sata-core.c (SATA)
> > hw/ahci-core.c (AHCI)
> >
> > Should this be a goal or am i over-engineering here?
> 
> [CC'ing Tejun - he volunteered to help out on this topic as well]
> 
> I think there's no need to split sata and ahci.
> Apart from that, I think we should take things incrementally. For now
> there's no need to split IDE further until we hit a technical limit. I
> have yet to see a patch trying to reuse the IDE command processing, so
> depending on how the respective person implements that, I'm open to
> suggenstions.
> 
> So as far as I'm concerned, I'd prefer to model things after they're
> developed. That way we know the pitfalls.

Agreed. We should only split code if there is a good reason for doing
so. I currently don't see the need to implement more SATA host controler
emulations beside AHCI. So there is currently no point in splitting the
code (for now). More important is completeness and support for as many
guest operating systems as possible.

	Joerg