From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR6ko-00006t-O4 for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:36:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TR6ki-0003Ve-HC for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:36:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22940) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR6ki-0003VM-4l for qemu-devel@nongnu.org; Wed, 24 Oct 2012 15:36:32 -0400 Date: Wed, 24 Oct 2012 15:36:25 -0400 From: Jason Baron Message-ID: <20121024193625.GC7952@redhat.com> References: <44ade6cc409ff55e4623d0db05e1a79b5f00cab2.1350677361.git.jbaron@redhat.com> <20121022104806.GB28828@redhat.com> <508530A5.3040508@redhat.com> <20121022181155.GB23972@redhat.com> <87d307yim6.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87d307yim6.fsf@blackfin.pond.sub.org> 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: Markus Armbruster Cc: Kevin Wolf , aliguori@us.ibm.com, alex.williamson@redhat.com, "Michael S. Tsirkin" , jan.kiszka@siemens.com, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, yamahata@valinux.co.jp, juzhang@redhat.com, kevin@koconnor.net, avi@redhat.com, mkletzan@redhat.com, pbonzini@redhat.com, lcapitulino@redhat.com, afaerber@suse.de, kraxel@redhat.com On Wed, Oct 24, 2012 at 05:50:25PM +0200, Markus Armbruster wrote: > Jason Baron writes: > > > 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 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. > > 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). > >> >> 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] = { > /* > * 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. > >> >> +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 > >> >> #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]); > >> >> + } > >> >> +} > > 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 > .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, }; -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; +} + /* 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 @@ -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); } 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 +#include #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]); + } +} + 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