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

[Jason Baron <jbaron@redhat.com>]

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