Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
Jason Baron jba...@redhat.com writes:
On Wed, Oct 24, 2012 at 05:50:25PM +0200, Markus Armbruster wrote:
Jason Baron jba...@redhat.com writes:
On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf 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 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.
Sure, but only default controllers matter when it comes to -drive with
if!=none.
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).
Sounds helpful.
We should find warn about -drive with if!=none the board doesn't use in
general. Extra points for a special warning for pointing to if=ahci in
cases where it makes sense.
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] = {
/*
* 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
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
Jason Baron jba...@redhat.com writes:
On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf 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 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 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] = {
/*
* 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
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
On Wed, Oct 24, 2012 at 05:50:25PM +0200, Markus Armbruster wrote:
Jason Baron jba...@redhat.com writes:
On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf 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 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 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] = {
/*
* 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
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
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,
};
+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,
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
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_DEVS2
@@ -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
Re: [Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
On Mon, Oct 22, 2012 at 01:40:21PM +0200, Kevin Wolf 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?
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)
}
[Qemu-devel] [PATCH v3 02/26] blockdev: Introduce IF_AHCI
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
---
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,
