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