From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38766) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQMTq-0004Kb-1n for qemu-devel@nongnu.org; Mon, 22 Oct 2012 14:12:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQMTo-0001ou-9M for qemu-devel@nongnu.org; Mon, 22 Oct 2012 14:12:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49814) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQMTo-0001oq-0O for qemu-devel@nongnu.org; Mon, 22 Oct 2012 14:12:00 -0400 Date: Mon, 22 Oct 2012 14:11:55 -0400 From: Jason Baron Message-ID: <20121022181155.GB23972@redhat.com> References: <44ade6cc409ff55e4623d0db05e1a79b5f00cab2.1350677361.git.jbaron@redhat.com> <20121022104806.GB28828@redhat.com> <508530A5.3040508@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <508530A5.3040508@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: aliguori@us.ibm.com, juzhang@redhat.com, "Michael S. Tsirkin" , jan.kiszka@siemens.com, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, yamahata@valinux.co.jp, alex.williamson@redhat.com, kevin@koconnor.net, avi@redhat.com, mkletzan@redhat.com, pbonzini@redhat.com, lcapitulino@redhat.com, afaerber@suse.de, armbru@redhat.com, kraxel@redhat.com On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf wrote: > >> From: Jason Baron > >> > >> 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 > >> Signed-off-by: Jason Baron > >> --- > > > > 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 > >> #include > >> #include > >> +#include > >> > >> #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