Message ID | 44ade6cc409ff55e4623d0db05e1a79b5f00cab2.1350677361.git.jbaron@redhat.com |
---|---|
State | New |
Headers | show |
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()!
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); +}