Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI

[Markus Armbruster]

Jason Baron <address@hidden> writes:

> On Wed, Oct 24, 2012 at 05:50:25PM +0200, Markus Armbruster wrote:
>> Jason Baron <address@hidden> writes:
>> 
>> > On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf wrote:
>> >> >> From: Jason Baron <address@hidden>
>> >> >>
>> >> >> Introduce IF_AHCI so that q35 can differentiate between ide
>> >> >> and ahci disks.
>> >> >> This allows q35 to specify its default disk type. It also allows q35 to
>> >> >> differentiate between ahci and ide disks, such that -drive
>> >> >> if=ide does not
>> >> >> result in the creating of an ahci disk. This is important, since
>> >> >> we don't want
>> >> >> to have the meaning of if=ide changing once q35 is introduced. Thus, 
>> >> >> its
>> >> >> important for this to be applied before we introduce q35.
>> 
>> This isn't the real argument for IF_AHCI.  The real argument is that the
>> (bus, unit) namespace for if=ide makes no sense for AHCI.
>> 
>> A board can have any number of IDE controllers.  Each IDE controller
>> provides one or two buses, and each bus takes up to two units.
>> Together, we get a a board-specific number of buses, where each bus
>> takes up to two units.
>> 
>> A board can have any number of AHCI controllers.  Each AHCI controller
>> provides a device-specific number of ports.  Together, we get a
>> board-specific number of buses, where each bus can takes a bus-specific
>> number of units.
>> 
>> Plain q35 doesn't really need to differentiate between IDE and AHCI; it
>> has only AHCI.  What it needs is a "one bus with six units" namespace.
>> 
>> We could fix this by relaxing if=ide's rigid "two units per bus" for new
>> machine types.
>> 
>> You fix it by introducing if=ahci.  Probably simpler.  But what's the
>> plan for the next generation of controller?  Yet another interface type?
>
> Next generation might require a new interface type. But taking you're
> suggestions, if=ahci is now machine dependent, so it shouldn't be so bad
> (see incremental patch at end).
>
>> 
>> Note that SCSI suffers from similar rigidity: seven units per bus.
>> That's fine for SCSI-1, but not for some of the other variants.
>> 
>> "Differentiate" comes into play when a board sports both AHCI and IDE
>> controllers.  Which q35 doesn't, does it?
>
> Yes, there is no ide controller by default. That said i've used the
> piix-ide controller to install windows xp, for example, on q35.

Sure, but only default controllers matter when it comes to -drive with
if!=none.

>> Then, the new if=ahci lets users select the kind of controller more
>> easily than bus numbers would.
>> 
>> Drawback: existing command lines upgrade from pc to plain q35 gracefully
>> only if they don't specify if=ide explicitly.  Upgrading to some q35
>> variant with IDE controller on board is even worse: it works, but
>> performance sucks.  Some regard this as a feature.
>> 
>
> well if=ide could give a warning on q35, suggesting using if=ahci, if
> we're not going to create an ide controller for if=ide (which I wasn't
> intending).

Sounds helpful.

We should find warn about -drive with if!=none the board doesn't use in
general.  Extra points for a special warning for pointing to if=ahci in
cases where it makes sense.

>> >> >> This patch also adds:
>> >> >>
>> >> >> pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
>> >> >>
>> >> >> Which provides a convient way of attaching ahci drives to an
>> >> >> ahci controller.
>> >> >>
>> >> >> Reviewed-by: Paolo Bonzini <address@hidden>
>> >> >> Signed-off-by: Jason Baron <address@hidden>
>> >> >> ---
>> >> > 
>> >> > Kevin, could you review/ack this patch pls?
>> >> > 
>> >> >>  blockdev.c    |   13 ++++++++++++-
>> >> >>  blockdev.h    |    2 ++
>> >> >>  hw/ide.h      |    6 ++++++
>> >> >>  hw/ide/ahci.c |   18 ++++++++++++++++++
>> >> >>  hw/ide/core.c |   23 ++++++++++++++++++-----
>> >> >>  5 files changed, 56 insertions(+), 6 deletions(-)
>> >> >>
>> >> >> diff --git a/blockdev.c b/blockdev.c
>> >> >> index c9a49c8..b684348 100644
>> >> >> --- a/blockdev.c
>> >> >> +++ b/blockdev.c
>> >> >> @@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
>> >> >>      [IF_SD] = "sd",
>> >> >>      [IF_VIRTIO] = "virtio",
>> >> >>      [IF_XEN] = "xen",
>> >> >> +    [IF_AHCI] = "ahci",
>> >> >>  };
>> >> >>  
>> >> >>  static const int if_max_devs[IF_COUNT] = {
>> >> >> @@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {
>>            /*
>>             * Do not change these numbers!  They govern how drive option
>>             * index maps to unit and bus.  That mapping is ABI.
>>             *
>>             * All controllers used to imlement if=T drives need to support
>>             * if_max_devs[T] units, for any T with if_max_devs[T] != 0.
>>             * Otherwise, some index values map to "impossible" bus, unit
>>             * values.
>>             *
>>             * For instance, if you change [IF_SCSI] to 255, -drive
>>             * if=scsi,index=12 no longer means bus=1,unit=5, but
>>             * bus=0,unit=12.  With an lsi53c895a controller (7 units max),
>>             * the drive can't be set up.  Regression.
>> >> >>       */
>> >> >>      [IF_IDE] = 2,
>> >> >>      [IF_SCSI] = 7,
>> >> >> +    [IF_AHCI] = 6,
>> >> >>  };
>> >> 
>> >> What are the implications of this if we decided to add another AHCI
>> >> controller which had a different number of ports? I suspect that a
>> >> controller with less than 6 ports breaks when you add more drives than a
>> >> single controller can handle, and one with more than 6 ports doesn't use
>> >> up all of its ports before it adds another controller.
>> >> 
>> >> Markus?
>> >> 
>> >
>> > My plan was to make this field, machine dependent if/when we
>> > wanted a different
>> > size. I don't think it breaks anything to make this change at a later
>> > point. But please correct me, if I am wrong.
>> 
>> I'm afraid you are.  See the comment immediately above, and commit
>> 27d6bf40.
>> 
>> As far as I can see, the least bad solution is leaving
>> if_max_devs[IF_AHCI] zero.  Makes index=N an alias for unit=N,bus=0, and
>> leaves rejecting invalid (bus,unit) to the board.
>> 
>
> Ok. I've done that. And it makes if=ahci machine dependent.

Uh, in what sense?

>> >> >> +int get_if_max_devs(BlockInterfaceType if_type)
>> >> >> +{
>> >> >> +    assert(if_type < IF_COUNT);
>> >> >> +    assert(if_type >= IF_DEFAULT);
>> >> >> +
>> >> >> +    return if_max_devs[if_type];
>> >> >> +}
>> >> 
>> >> if_max_devs has a specific obvious meaning within blockdev.c, but
>> >> outside it's not as obvious. So this function could use a rename.
>> >
>> > ok.
>> 
>> No, get rid of it.  if_max_devs[]'s purpose is mapping between index and
>> (bus, unit), no more.  See below.
>> 
>
> done.
>
>> >> >>  /*
>> >> >>   * We automatically delete the drive when a device using it gets
>> >> >>   * unplugged.  Questionable feature, but we can't just drop it.
>> >> >> @@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
>> >> >>      } else {
>> >> >>          /* no id supplied -> create one */
>> >> >>          dinfo->id = g_malloc0(32);
>> >> >> -        if (type == IF_IDE || type == IF_SCSI)
>> >> >> +        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
>> >> >>              mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>> >> >>          if (max_devs)
>> >> >>              snprintf(dinfo->id, 32, "%s%i%s%i",
>> >> >> @@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
>> >> >>  
>> >> >>      switch(type) {
>> >> >>      case IF_IDE:
>> >> >> +    case IF_AHCI:
>> >> >>      case IF_SCSI:
>> >> >>      case IF_XEN:
>> >> >>      case IF_NONE:
>> >> >> diff --git a/blockdev.h b/blockdev.h
>> >> >> index 8b126ad..bbd1017 100644
>> >> >> --- a/blockdev.h
>> >> >> +++ b/blockdev.h
>> >> >> @@ -21,6 +21,7 @@ typedef enum {
>> >> >>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>> >> >>      IF_NONE,
>> >> >>      IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, 
>> >> >> IF_XEN,
>> >> >> +    IF_AHCI,
>> >> >>      IF_COUNT
>> >> >>  } BlockInterfaceType;
>> >> >>  
>> >> >> @@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
>> >> >>      return mach_if;
>> >> >>  }
>> >> >>  
>> >> >> +int get_if_max_devs(BlockInterfaceType if_type);
>> >> >>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>> >> >>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>> >> >>  int drive_get_max_bus(BlockInterfaceType type);
>> >> >> diff --git a/hw/ide.h b/hw/ide.h
>> >> >> index 2db4079..0b7e000 100644
>> >> >> --- a/hw/ide.h
>> >> >> +++ b/hw/ide.h
>> >> >> @@ -4,6 +4,7 @@
>> >> >>  #include "isa.h"
>> >> >>  #include "pci.h"
>> >> >>  #include "memory.h"
>> >> >> +#include "blockdev.h"
>> >> >>  
>> >> >>  #define MAX_IDE_DEVS  2
>> >> >>  
>> >> >> @@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
>> >> >>  int ide_get_bios_chs_trans(BusState *bus, int unit);
>> >> >>  
>> >> >>  /* ide/core.c */
>> >> >> +void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType 
>> >> >> type);
>> >> >>  void ide_drive_get(DriveInfo **hd, int max_bus);
>> >> >> +void ahci_drive_get(DriveInfo **hd, int max_bus);
>> >> >> +
>> >> >> +/* ide/ahci.c */
>> >> >> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
>> 
>> Shouldn't this go next to pci_ide_create_devs()'s declaration?
>> Currently in hw/ide/pci.h.
>> 
>
> done.
>
>> >> >>  
>> >> >>  #endif /* HW_IDE_H */
>> >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> >> >> index 68671bc..824b86f 100644
>> >> >> --- a/hw/ide/ahci.c
>> >> >> +++ b/hw/ide/ahci.c
>> >> >> @@ -26,6 +26,7 @@
>> >> >>  #include <hw/pc.h>
>> >> >>  #include <hw/pci.h>
>> >> >>  #include <hw/sysbus.h>
>> >> >> +#include <blockdev.h>
>> >> >>  
>> >> >>  #include "monitor.h"
>> >> >>  #include "dma.h"
>> >> >> @@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
>> >> >>  }
>> >> >>  
>> >> >>  type_init(sysbus_ahci_register_types)
>> >> >> +
>> >> >> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
>> >> >> +{
>> >> >> + struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState,
>> >> >> card, pci_dev);
>> >> >> +    int i;
>> >> >> +    DriveInfo *drive;
>> >> >> +
>> >> >> +    for (i = 0; i < dev->ahci.ports; i++) {
>> >> >> +        if (hd_table[i] == NULL) {
>> >> >> +            continue;
>> >> >> +        }
>> >> >> +        drive = hd_table[i];
>> >> >> +        assert(drive->type == IF_AHCI);
>> >> >> +        ide_create_drive(&dev->ahci.dev[i].port, 0,
>> >> >> +                         hd_table[i]);
>> >> >> +    }
>> >> >> +}
>> 
>> Shouldn't this go next to pci_ide_create_devs()'s definition?  Currently
>> in hw/ide/pci.c.
>> 
>> Consider adding a parameter for the number of elements in hd_table[],
>> for robustness.  As is, the caller has to ensure hd_table[] has at least
>> dev->ahci.ports elements, which is not obvious from the function
>> signature.
>> 
>> >> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> >> >> index d683a8c..044da3c 100644
>> >> >> --- a/hw/ide/core.c
>> >> >> +++ b/hw/ide/core.c
>> >> >> @@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
>> >> >>      }
>> >> >>  };
>> >> >>  
>> >> >> -void ide_drive_get(DriveInfo **hd, int max_bus)
>> >> >> +void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType 
>> >> >> type)
>> >> 
>> >> Could be static?
>> >> 
>> >
>> > Yes.
>> >
>> >> >>  {
>> >> >>      int i;
>> >> >> +    int max_devs;
>> >> >> +
>> >> >> +    assert((type == IF_IDE) || type == IF_AHCI);
>> >> 
>> >> Interesting use of brackets.
>> >
>> > oops.
>> >
>> >> 
>> >> >>  
>> >> >> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
>> >> >> +    if (drive_get_max_bus(type) >= max_bus) {
>> >> >>          fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>> >> 
>> >> "Too many %s buses", type == IF_IDE ? "IDE" : "AHCI"
>> >
>> > ok.
>> >
>> >> 
>> >> >>          exit(1);
>> >> >>      }
>> >> >> -
>> >> >> -    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
>> >> >> -        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
>> >> >> +    max_devs = get_if_max_devs(type);
>> >> >> +    for (i = 0; i < max_bus * max_devs; i++) {
>> >> >> +        hd[i] = drive_get(type, i / max_devs, i % max_devs);
>> >> >>      }
>> >> >>  }
>> >> >> +
>> >> >> +void ide_drive_get(DriveInfo **hd, int max_bus)
>> >> >> +{
>> >> >> +    ata_drive_get(hd, max_bus, IF_IDE);
>> >> >> +}
>> >> >> +
>> >> >> +void ahci_drive_get(DriveInfo **hd, int max_bus)
>> >> >> +{
>> >> >> +    ata_drive_get(hd, max_bus, IF_AHCI);
>> >> >> +}
>> 
>> For IDE, we always have two devices per bus.  The boards number of buses
>> varies in theory, and is always two in practice.  Thus we have "#define
>> MAX_IDE_DEVS 2" in ide.h, and "#define MAX_IDE_BUS 2" in all the
>> <board>.c.  The latter is passed to ide_drive_get() as parameter.
>> Getting the well-known MAX_IDE_DEVS from get_if_max_devs() buys us
>> nothing.
>> 
>> For AHCI, I figure we have a device-specific number of ports per device,
>> and a board-specific number of devices (typically one).  If there are
>> multiple devices, they don't necessarily sport the same number of ports.
>> Thus, a nested loop does not work.
>> 
>> Suggest
>> 
>>     void ahci_drive_get(DriveInfo **hd, int bus, int max_ports)
>> 
>> which the board can call once for each device, with the device's true
>> number of ports as argument.  Look ma, no get_if_max_devs()!
>
> Agreed.
>
> So here is an incremental patch. If its too hard to read I'll send a
> full diff - I'd like to include something like this in my next q35
> series. thoughts?
>
> Thanks,
>
> -Jason
>
>
> diff --git a/blockdev.c b/blockdev.c
> index b684348..e17016e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -53,17 +53,9 @@ static const int if_max_devs[IF_COUNT] = {
>       */
>      [IF_IDE] = 2,
>      [IF_SCSI] = 7,
> -    [IF_AHCI] = 6,
> +    [IF_AHCI] = 0,
>  };

Consider dropping the IF_AHCI line, for consistency with the other
interface types that want zero value here.

>  
> -int get_if_max_devs(BlockInterfaceType if_type)
> -{
> -    assert(if_type < IF_COUNT);
> -    assert(if_type >= IF_DEFAULT);
> -
> -    return if_max_devs[if_type];
> -}
> -
>  /*
>   * We automatically delete the drive when a device using it gets
>   * unplugged.  Questionable feature, but we can't just drop it.
> @@ -168,6 +160,20 @@ int drive_get_max_bus(BlockInterfaceType type)
>      return max_bus;
>  }
>  
> +int drive_get_max_unit(BlockInterfaceType type)
> +{
> +    int max_unit;
> +    DriveInfo *dinfo;
> +
> +    max_unit = -1;
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        if(dinfo->type == type &&
> +           dinfo->unit > max_unit)
> +            max_unit = dinfo->unit;
> +    }
> +    return max_unit;
> +}
> +

This returns the largest unit number on any bus.  Wouldn't a function to
return the largest unit number on a specific bus (given by parameter int
bus) be more useful?

Anyway, there's so caller, yet.  Suggest to drop until it's needed.

Oh, maybe you need it to call your ahci_drive_get().  Patch doesn't
include that part.

>  /* Get a block device.  This should only be used for single-drive devices
>     (e.g. SD/Floppy/MTD).  Multi-disk devices (scsi/ide) should use the
>     appropriate bus.  */
> diff --git a/blockdev.h b/blockdev.h
> index bbd1017..250f9d8 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -61,6 +61,7 @@ int get_if_max_devs(BlockInterfaceType if_type);
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>  int drive_get_max_bus(BlockInterfaceType type);
> +int drive_get_max_unit(BlockInterfaceType type);
>  DriveInfo *drive_get_next(BlockInterfaceType type);
>  void drive_get_ref(DriveInfo *dinfo);
>  void drive_put_ref(DriveInfo *dinfo);
> diff --git a/hw/ide.h b/hw/ide.h
> index 0b7e000..54e485f 100644
> --- a/hw/ide.h
> +++ b/hw/ide.h

Suspect you can also back out he hunk adding #include "blockdev.h".

> @@ -35,11 +35,7 @@ int ide_get_geometry(BusState *bus, int unit,
>  int ide_get_bios_chs_trans(BusState *bus, int unit);
>  
>  /* ide/core.c */
> -void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
>  void ide_drive_get(DriveInfo **hd, int max_bus);
> -void ahci_drive_get(DriveInfo **hd, int max_bus);
> -
> -/* ide/ahci.c */
> -void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
> +void ahci_drive_get(DriveInfo **hd, int max_bus, int max_ports);
>  
>  #endif /* HW_IDE_H */
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 824b86f..1e389aa 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1261,20 +1261,3 @@ static void sysbus_ahci_register_types(void)
>  }
>  
>  type_init(sysbus_ahci_register_types)
> -
> -void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
> -{
> -    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
> -    int i;
> -    DriveInfo *drive;
> -
> -    for (i = 0; i < dev->ahci.ports; i++) {
> -        if (hd_table[i] == NULL) {
> -            continue;
> -        }
> -        drive = hd_table[i];
> -        assert(drive->type == IF_AHCI);
> -        ide_create_drive(&dev->ahci.dev[i].port, 0,
> -                         hd_table[i]);
> -    }
> -}
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 044da3c..6fc2626 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2341,18 +2341,17 @@ const VMStateDescription vmstate_ide_bus = {
>      }
>  };
>  
> -void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)
> +static void ata_drive_get(DriveInfo **hd, int max_bus, int max_devs,
> +                          BlockInterfaceType type)
>  {
>      int i;
> -    int max_devs;
>  
> -    assert((type == IF_IDE) || type == IF_AHCI);
> +    assert((type == IF_IDE) || (type == IF_AHCI));
>  
> -    if (drive_get_max_bus(type) >= max_bus) {
> +    if ((type == IF_IDE) && (drive_get_max_bus(type) >= max_bus)) {
>          fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>          exit(1);
>      }
> -    max_devs = get_if_max_devs(type);
>      for (i = 0; i < max_bus * max_devs; i++) {
>          hd[i] = drive_get(type, i / max_devs, i % max_devs);
>      }
> @@ -2360,10 +2359,10 @@ void ata_drive_get(DriveInfo **hd, int max_bus, 
> BlockInterfaceType type)
>  
>  void ide_drive_get(DriveInfo **hd, int max_bus)
>  {
> -    ata_drive_get(hd, max_bus, IF_IDE);
> +    ata_drive_get(hd, max_bus, MAX_IDE_DEVS, IF_IDE);
>  }
>  
> -void ahci_drive_get(DriveInfo **hd, int max_bus)
> +void ahci_drive_get(DriveInfo **hd, int max_bus, int max_ports)
>  {
> -    ata_drive_get(hd, max_bus, IF_AHCI);
> +    ata_drive_get(hd, max_bus, max_ports, IF_AHCI);
>  }

Looks like this now:

static void ata_drive_get(DriveInfo **hd, int max_bus, int max_devs,
                          BlockInterfaceType type)
{
    int i;

    assert((type == IF_IDE) || (type == IF_AHCI));

    if ((type == IF_IDE) && (drive_get_max_bus(type) >= max_bus)) {
        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
        exit(1);
    }
    for (i = 0; i < max_bus * max_devs; i++) {
        hd[i] = drive_get(type, i / max_devs, i % max_devs);
    }
}

void ide_drive_get(DriveInfo **hd, int max_bus)
{
    ata_drive_get(hd, max_bus, MAX_IDE_DEVS, IF_IDE);
}

void ahci_drive_get(DriveInfo **hd, int max_bus, int max_ports)
{
    ata_drive_get(hd, max_bus, max_ports, IF_AHCI);
}

What I had in mind is this:

void ide_drive_get(DriveInfo **hd, int max_bus)
{
    int i;

    if (drive_get_max_bus(IF_IDE) >= max_bus) {
        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
        exit(1);
    }

    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
    }
}

void ahci_drive_get(DriveInfo **hd, int bus, int nports)
{
    for(i = 0; i < nports; i++) {
        hd[i] = drive_get(IF_IDE, bus, i);
    }
}

ide_drive_get() is unchanged.

The board knows exactly how many AHCI controllers it creates, and how
many ports they have.  Say it creates two, one with 6 ports, the other
with eight.  It can get the drives like this:

    DriveInfo *hd0[6], *hd1[8];

    ahci_drive_get(hd0, 0, ARRAY_SIZE(hd0));
    ahci_drive_get(hd1, 1, ARRAY_SIZE(hd1));

> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 644533f..2df6b57 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -30,6 +30,7 @@
>  #include "dma.h"
>  
>  #include <hw/ide/pci.h>
> +#include <hw/ide/ahci.h>
>  
>  #define BMDMA_PAGE_SIZE 4096
>  
> @@ -504,6 +505,24 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo 
> **hd_table)
>      }
>  }
>  
> +
> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table, int 
> table_size)
> +{
> +    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
> +    int i;
> +    DriveInfo *drive;
> +
> +    for (i = 0; i < table_size; i++) {
> +        if (hd_table[i] == NULL) {
> +            continue;
> +        }
> +        drive = hd_table[i];
> +        assert(drive->type == IF_AHCI);
> +        ide_create_drive(&dev->ahci.dev[i].port, 0,
> +                         hd_table[i]);

Note that the drive with bus=b,unit=u is connected to the b-th AHCI
controller's u-th IDE bus.  I'm *not* objecting to that, because I think
-drive makes little sense, and improving it is a lost cause.

> +    }
> +}
> +
>  static const struct IDEDMAOps bmdma_ops = {
>      .start_dma = bmdma_start_dma,
>      .start_transfer = bmdma_start_transfer,
> diff --git a/hw/ide/pci.h b/hw/ide/pci.h
> index a694e54..6a0d500 100644
> --- a/hw/ide/pci.h
> +++ b/hw/ide/pci.h
> @@ -58,6 +58,8 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState 
> *d);
>  void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>  extern MemoryRegionOps bmdma_addr_ioport_ops;
>  void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
> +void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table, int 
> table_size);
> +
>  
>  extern const VMStateDescription vmstate_ide_pci;
>  #endif