John Snow <address@hidden> writes:
Instead of duplicating the logic for the if_ide
(bus,unit) mappings, rely on the blockdev layer
for managing those mappings for us, and use the
drive_get_by_index call instead.
This allows ide_drive_get to work for AHCI HBAs
as well, and can be used in the Q35 initialization.
Lastly, change the nature of the argument to
ide_drive_get so that represents the number of
total drives we can support, and not the total
number of buses. This will prevent array overflows
if the units-per-default-bus property ever needs
to be adjusted for compatibility reasons.
Signed-off-by: John Snow <address@hidden>
---
blockdev.c | 9 +++++++++
hw/alpha/dp264.c | 2 +-
hw/i386/pc_piix.c | 2 +-
hw/ide/core.c | 21 +++++++++++++++------
hw/mips/mips_fulong2e.c | 2 +-
hw/mips/mips_malta.c | 2 +-
hw/mips/mips_r4k.c | 2 +-
hw/ppc/mac_newworld.c | 2 +-
hw/ppc/mac_oldworld.c | 2 +-
hw/ppc/prep.c | 2 +-
hw/sparc64/sun4u.c | 2 +-
include/sysemu/blockdev.h | 1 +
12 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 9b05f1b..ffaad39 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -135,6 +135,15 @@ void blockdev_auto_del(BlockDriverState *bs)
}
}
+int drive_get_max_devs(BlockInterfaceType type)
+{
+ if (type >= IF_IDE && type < IF_COUNT) {
+ return if_max_devs[type];
+ }
+
+ return 0;
+}
+
drive_get_max_bus() returns -1 for a type without drives. Includes
invalid types. When it returns a non-negative number, a drive on that
bus exists.
Your drive_get_max_devs() has a similar name, but different semantics:
it returns a positive number when the implied HBA supports multiple
buses, else zero. The "else" includes invalid types. When it returns a
positive number, then the HBA can take that many units per bus.
No big deal, but functions comments would be nice.
Should invalid type be treated as a programming error instead?
static int drive_index_to_bus_id(BlockInterfaceType type, int index)
{
int max_devs = if_max_devs[type];
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index b178a03..ab61bb6 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -97,7 +97,7 @@ static void clipper_init(MachineState *machine)
/* IDE disk setup. */
{
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
- ide_drive_get(hd, MAX_IDE_BUS);
+ ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
More obviously correct would be
ide_drive_get(hd, ARRAY_SIZE(hd));
Less so here, because the declaration is right next to the use, more so
elsewhere, where it isn't.
pci_cmd646_ide_init(pci_bus, hd, 0);
}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 103d756..2760c81 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -239,7 +239,7 @@ static void pc_init1(MachineState *machine,
pc_nic_init(isa_bus, pci_bus);
- ide_drive_get(hd, MAX_IDE_BUS);
+ ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
if (pci_enabled) {
PCIDevice *dev;
if (xen_enabled()) {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 190700a..e7c1050 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2558,16 +2558,25 @@ const VMStateDescription vmstate_ide_bus = {
}
};
-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ide_drive_get(DriveInfo **hd, int n)
{
int i;
+ int highest_bus = drive_get_max_bus(IF_IDE) + 1;
Actually, this is the "highest bus" + 1 :)
+ int n_buses = n / drive_get_max_devs(IF_IDE);
What if drive_get_max_devs(IF_IDE) returns 0?
You could side-step the question by using drive_index_to_bus_id(n).
- if (drive_get_max_bus(IF_IDE) >= max_bus) {
- fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
- exit(1);
Before: fatal.
+ /* Note: The number of actual buses available is not known.
+ * We compute this based on the size of the DriveInfo* array, n.
+ * If it is less than (drive_get_max_devs(IF_IDE) * num_real_buses),
+ * We will stop looking for drives prematurely instead of overfilling
+ * the array. */
+
You might want to consider winged comments:
/*
* Note: The number of actual buses available is not known.
* We compute this based on the size of the DriveInfo* array, n.
* If it is less than (drive_get_max_devs(IF_IDE) * num_real_buses),
* We will stop looking for drives prematurely instead of overfilling
* the array.
*/
+ if (highest_bus > n_buses) {
+ error_report("Warning: Too many IDE buses defined (%d > %d)",
+ highest_bus, n_buses);
After: warning.
Why?