[v3,02/26] blockdev: Introduce IF_AHCI

From: Jason Baron <jbaron@redhat.com>

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 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 <pbonzini@redhat.com>
Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 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(-)

On Fri, Oct 19, 2012 at 04:43:27PM -0400, Jason Baron wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> 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 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 <pbonzini@redhat.com>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---

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] = {
>       */
>      [IF_IDE] = 2,
>      [IF_SCSI] = 7,
> +    [IF_AHCI] = 6,
>  };
>  
> +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.
> @@ -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);
>  
>  #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]);
> +    }
> +}
> 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)
>  {
>      int i;
> +    int max_devs;
> +
> +    assert((type == IF_IDE) || type == IF_AHCI);
>  
> -    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);
>          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);
> +}
> -- 
> 1.7.1

Am 22.10.2012 12:48, schrieb Michael S. Tsirkin:
> On Fri, Oct 19, 2012 at 04:43:27PM -0400, Jason Baron wrote:
>> From: Jason Baron <jbaron@redhat.com>
>>
>> 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 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 <pbonzini@redhat.com>
>> Signed-off-by: Jason Baron <jbaron@redhat.com>
>> ---
> 
> 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] = {
>>       */
>>      [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?

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

>>  /*
>>   * 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);
>>  
>>  #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]);
>> +    }
>> +}
>> 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?

>>  {
>>      int i;
>> +    int max_devs;
>> +
>> +    assert((type == IF_IDE) || type == IF_AHCI);

Interesting use of brackets.

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

>>          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);
>> +}

Kevin

On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf wrote:
> >> From: Jason Baron <jbaron@redhat.com>
> >>
> >> 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 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 <pbonzini@redhat.com>
> >> Signed-off-by: Jason Baron <jbaron@redhat.com>
> >> ---
> > 
> > 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] = {
> >>       */
> >>      [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.


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

> 
> >>  /*
> >>   * 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);
> >>  
> >>  #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]);
> >> +    }
> >> +}
> >> 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);
> >> +}
> 
> Kevin

Thanks,

-Jason

Jason Baron <jbaron@redhat.com> writes:

> On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf wrote:
>> >> From: Jason Baron <jbaron@redhat.com>
>> >>
>> >> 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?

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?

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.

>> >> 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 <pbonzini@redhat.com>
>> >> Signed-off-by: Jason Baron <jbaron@redhat.com>
>> >> ---
>> > 
>> > 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.

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

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

>> >>  
>> >>  #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()!