[Qemu-devel] [PATCH v2 0/6] Q35: Implement -cdrom/-hda sugar

[Qemu-devel] [PATCH v2 0/6] Q35: Implement -cdrom/-hda sugar

[John Snow]

The Q35 board initialization does not currently bother to look
for any drives added by the various syntactical sugar shorthands
to be added to the AHCI HBA. These include -hda through -hdd,
-cdrom, and -drive if=ide shorthands.

An obstacle to having implemented this sooner is debate over
whether or not to add an additional interface type, and how to
manage the different units-per-bus mappings of various HBA
implementations.

This patch series:
(1) Does not add IF_AHCI, but reuses IF_IDE
(2) Allows the if_max_devs table to be overridden
(3) Adds this override to the Q35 board type.
(4) Finally, adds implementation to Q35 initialization.

History:

V2:
- Adjusted language in patch #1's commit message.
  (drive if=none will NOT trigger warnings)
- Removed superfluous warning with bad phrasing in patch #1
- Removed if_get_max_devs from patch #2 and added to patch #4
- Added an assertion to patch #2
- Added more detail to patch #3's commit message
- Specified that Patch #3 will affect old Q35 machine types
- Changed fprintf to error_report in patch #4
- Replaced max_bus parameter in ide_drive_get with 'n', size of array
- Updated calls to ide_drive_get in other boards
- Adjusted language in patch #6's commit message.
  (Removed reference to patch #5.)

V1:
- Re-uses ide_drive_get instead of ahci_drive_get
- Adds units-per-bus property to all Q35 machines
- Changes orphan scanning to exclude IF_NONE and
  automatically added drives
- Renames 'units-per-idebus' to 'units-per-default-bus'
  And allows override of any one IF type (block_default)

RFC2:
- Rewrote series to avoid the creation of IF_AHCI.

John Snow (6):
  blockdev: Orphaned drive search
  blockdev: Allow overriding if_max_dev property
  pc/vl: Add units-per-default-bus property
  ide: Update ide_drive_get to be HBA agnostic
  qtest/bios-tables: Correct Q35 command line
  q35/ahci: Pick up -cdrom and -hda options

 blockdev.c                | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
 hw/alpha/dp264.c          |  2 +-
 hw/i386/pc.c              |  1 +
 hw/i386/pc_piix.c         |  2 +-
 hw/i386/pc_q35.c          |  7 +++++-
 hw/ide/ahci.c             | 15 +++++++++++++
 hw/ide/ahci.h             |  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/hw/boards.h       |  2 ++
 include/sysemu/blockdev.h |  5 +++++
 tests/bios-tables-test.c  | 10 ++++-----
 vl.c                      | 18 ++++++++++++++-
 19 files changed, 131 insertions(+), 24 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/6] blockdev: Orphaned drive search

[John Snow]

When users use command line options like -hda, -cdrom,
or even -drive if=ide, it is up to the board initialization
routines to pick up these drives and create backing
devices for them.

Some boards, like Q35, have not been doing this.
However, there is no warning explaining why certain
drive specifications are just silently ignored,
so this function adds a check to print some warnings
to assist users in debugging these sorts of issues
in the future.

This patch will not warn about drives added with if_none,
for which it is not possible to tell in advance if
the omission of a backing device is an issue.

A warning in these cases is considered appropriate.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 21 +++++++++++++++++++++
 include/sysemu/blockdev.h |  2 ++
 vl.c                      | 10 +++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..81398e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -166,6 +166,27 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
+bool drive_check_orphaned(void)
+{
+    DriveInfo *dinfo;
+    bool rs = false;
+
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
+        /* Unless this is a default drive, this may be an oversight. */
+        if (!dinfo->bdrv->dev && !dinfo->is_default &&
+            dinfo->type != IF_NONE) {
+            fprintf(stderr, "Warning: Orphaned drive without device: "
+                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
+                    dinfo->id, dinfo->bdrv->filename, if_name[dinfo->type],
+                    dinfo->bus, dinfo->unit);
+            rs = true;
+        }
+    }
+
+    return rs;
+}
+
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
 {
     return drive_get(type,
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 23a5d10..80f768d 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -38,6 +38,7 @@ struct DriveInfo {
     int unit;
     int auto_del;               /* see blockdev_mark_auto_del() */
     bool enable_auto_del;       /* Only for legacy drive_new() */
+    bool is_default;            /* Added by default_drive() ?  */
     int media_cd;
     int cyls, heads, secs, trans;
     QemuOpts *opts;
@@ -46,6 +47,7 @@ struct DriveInfo {
 };
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
+bool drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
diff --git a/vl.c b/vl.c
index dbdca59..6500472 100644
--- a/vl.c
+++ b/vl.c
@@ -1168,6 +1168,7 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
                           int index, const char *optstr)
 {
     QemuOpts *opts;
+    DriveInfo *dinfo;
 
     if (!enable || drive_get_by_index(type, index)) {
         return;
@@ -1177,9 +1178,13 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
-    if (!drive_new(opts, type)) {
+
+    dinfo = drive_new(opts, type);
+    if (!dinfo) {
         exit(1);
     }
+    dinfo->is_default = true;
+
 }
 
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
@@ -4458,6 +4463,9 @@ int main(int argc, char **argv, char **envp)
     if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
         exit(1);
 
+    /* Did we create any drives that we failed to create a device for? */
+    drive_check_orphaned();
+
     net_check_clients();
 
     ds = init_displaystate();
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/6] blockdev: Allow overriding if_max_dev property

[John Snow]

The if_max_devs table as in the past been an immutable
default that controls the mapping of index => (bus,unit)
for all boards and all HBAs for each interface type.

Since adding this mapping information to the HBA device
itself is currently unwieldly from the perspective of
retrieving this information at option parsing time
(e.g, within drive_new), we consider the alternative
of marking the if_max_devs table mutable so that
later configuration and initialization can adjust the
mapping at will, but only up until a drive is added,
at which point the mapping is finalized.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 26 +++++++++++++++++++++++++-
 include/sysemu/blockdev.h |  2 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 81398e7..9b05f1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -60,7 +60,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_XEN] = "xen",
 };
 
-static const int if_max_devs[IF_COUNT] = {
+static 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.
@@ -79,6 +79,30 @@ static const int if_max_devs[IF_COUNT] = {
     [IF_SCSI] = 7,
 };
 
+/**
+ * Boards may call this to offer board-by-board overrides
+ * of the default, global values.
+ */
+void override_max_devs(BlockInterfaceType type, int max_devs)
+{
+    DriveInfo *dinfo;
+
+    if (max_devs <= 0) {
+        return;
+    }
+
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        if (dinfo->type == type) {
+            fprintf(stderr, "Cannot override units-per-bus property of"
+                    " the %s interface, because a drive of that type has"
+                    " already been added.\n", if_name[type]);
+            g_assert_not_reached();
+        }
+    }
+
+    if_max_devs[type] = max_devs;
+}
+
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 80f768d..f23d495 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -46,6 +46,8 @@ struct DriveInfo {
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
+void override_max_devs(BlockInterfaceType type, int max_devs);
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 bool drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/6] pc/vl: Add units-per-default-bus property

[John Snow]

This patch adds the 'units_per_default_bus' property which
allows individual boards to declare their desired
index => (bus,unit) mapping for their default HBA, so that
boards such as Q35 can specify that its default if_ide HBA,
AHCI, only accepts one unit per bus.

This property only overrides the mapping for drives matching
the block_default_type interface.

This patch also adds this property to *all* past and present
Q35 machine types. This retroactive addition is justified
because the previous erroneous index=>(bus,unit) mappings
caused by lack of such a property were not utilized due to
lack of initialization code in the Q35 init routine.

Further, semantically, the Q35 board type has always had the
property that its default HBA, AHCI, only accepts one unit per
bus. The new code added to add devices to drives relies upon
the accuracy of this mapping. Thus, the property is applied
retroactively to reduce complexity of allowing IDE HBAs with
different units per bus.

Examples:

Prior to this patch, all IDE HBAs were assumed to use 2 units
per bus (Master, Slave). When using Q35 and AHCI, however, we
only allow one unit per bus.

-hdb foo.qcow2 would become index=1, or bus=0,unit=1.
-hdd foo.qcow2 would become index=3, or bus=1,unit=1.
-drive file=foo.qcow2,index=5 becomes bus=2,unit=1.

These are invalid for AHCI. They now become, under Q35 only:

-hdb foo.qcow2 --> index=1, bus=1, unit=0.
-hdd foo.qcow2 --> index=3, bus=3, unit=0.
-drive file=foo.qcow2,index=5 --> bus=5,unit=0.

The mapping is adjusted based on the fact that the default IF
for the Q35 machine type is IF_IDE, and units-per-default-bus
overrides the IDE mapping from its default of 2 units per bus
to just 1 unit per bus.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/i386/pc.c        | 1 +
 hw/i386/pc_q35.c    | 3 ++-
 include/hw/boards.h | 2 ++
 vl.c                | 8 ++++++++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 82a7daa..d045e8b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1524,6 +1524,7 @@ static void pc_generic_machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = qm->hot_add_cpu;
     mc->kvm_type = qm->kvm_type;
     mc->block_default_type = qm->block_default_type;
+    mc->units_per_default_bus = qm->units_per_default_bus;
     mc->max_cpus = qm->max_cpus;
     mc->no_serial = qm->no_serial;
     mc->no_parallel = qm->no_parallel;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d4a907c..b28ddbb 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -344,7 +344,8 @@ static void pc_q35_init_1_4(MachineState *machine)
 #define PC_Q35_MACHINE_OPTIONS \
     PC_DEFAULT_MACHINE_OPTIONS, \
     .desc = "Standard PC (Q35 + ICH9, 2009)", \
-    .hot_add_cpu = pc_hot_add_cpu
+    .hot_add_cpu = pc_hot_add_cpu, \
+    .units_per_default_bus = 1
 
 #define PC_Q35_2_2_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
diff --git a/include/hw/boards.h b/include/hw/boards.h
index dfb6718..663f16a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -28,6 +28,7 @@ struct QEMUMachine {
     QEMUMachineHotAddCPUFunc *hot_add_cpu;
     QEMUMachineGetKvmtypeFunc *kvm_type;
     BlockInterfaceType block_default_type;
+    int units_per_default_bus;
     int max_cpus;
     unsigned int no_serial:1,
         no_parallel:1,
@@ -86,6 +87,7 @@ struct MachineClass {
     int (*kvm_type)(const char *arg);
 
     BlockInterfaceType block_default_type;
+    int units_per_default_bus;
     int max_cpus;
     unsigned int no_serial:1,
         no_parallel:1,
diff --git a/vl.c b/vl.c
index 6500472..940b149 100644
--- a/vl.c
+++ b/vl.c
@@ -1588,6 +1588,7 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = qm->hot_add_cpu;
     mc->kvm_type = qm->kvm_type;
     mc->block_default_type = qm->block_default_type;
+    mc->units_per_default_bus = qm->units_per_default_bus;
     mc->max_cpus = qm->max_cpus;
     mc->no_serial = qm->no_serial;
     mc->no_parallel = qm->no_parallel;
@@ -4378,6 +4379,13 @@ int main(int argc, char **argv, char **envp)
     blk_mig_init();
     ram_mig_init();
 
+    /* If the currently selected machine wishes to override the units-per-bus
+     * property of its default HBA interface type, do so now. */
+    if (machine_class->units_per_default_bus) {
+        override_max_devs(machine_class->block_default_type,
+                          machine_class->units_per_default_bus);
+    }
+
     /* open the virtual block devices */
     if (snapshot)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic

[John Snow]

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 <jsnow@redhat.com>
---
 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;
+}
+
 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);
 
         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;
+    int n_buses = n / drive_get_max_devs(IF_IDE);
 
-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
-        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
-        exit(1);
+    /* 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);
     }
 
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    for (i = 0; i < n; i++) {
+        hd[i] = drive_get_by_index(IF_IDE, i);
     }
+
 }
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index be286da..0239438 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -350,7 +350,7 @@ static void mips_fulong2e_init(MachineState *machine)
     pci_bus = bonito_init((qemu_irq *)&(env->irq[2]));
 
     /* South bridge */
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
 
     isa_bus = vt82c686b_init(pci_bus, PCI_DEVFN(FULONG2E_VIA_SLOT, 0));
     if (!isa_bus) {
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 2d87de9..e19a125 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1147,7 +1147,7 @@ void mips_malta_init(MachineState *machine)
     pci_bus = gt64120_register(isa_irq);
 
     /* Southbridge */
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
 
     piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
 
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index e219766..0d6f7ed 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -294,7 +294,7 @@ void mips_r4k_init(MachineState *machine)
     if (nd_table[0].used)
         isa_ne2000_init(isa_bus, 0x300, 9, &nd_table[0]);
 
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
     for(i = 0; i < MAX_IDE_BUS; i++)
         isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
                      hd[MAX_IDE_DEVS * i],
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 1626db4..d291fee 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -400,7 +400,7 @@ static void ppc_core99_init(MachineState *machine)
     macio_init(macio, pic_mem, escc_bar);
 
     /* We only emulate 2 out of 3 IDE controllers for now */
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
 
     macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
                                                         "ide[0]"));
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index be9a194..9dd7940 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -278,7 +278,7 @@ static void ppc_heathrow_init(MachineState *machine)
         pci_nic_init_nofail(&nd_table[i], pci_bus, "ne2k_pci", NULL);
 
 
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
 
     macio = pci_create(pci_bus, -1, TYPE_OLDWORLD_MACIO);
     dev = DEVICE(macio);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index f0ef1af..3e87cf9 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -519,7 +519,7 @@ static void ppc_prep_init(MachineState *machine)
         }
     }
 
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
     for(i = 0; i < MAX_IDE_BUS; i++) {
         isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i], ide_irq[i],
                      hd[2 * i],
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 9c77e18..c1b84bc 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -864,7 +864,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     for(i = 0; i < nb_nics; i++)
         pci_nic_init_nofail(&nd_table[i], pci_bus, "ne2k_pci", NULL);
 
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, MAX_IDE_BUS * MAX_IDE_DEVS);
 
     pci_cmd646_ide_init(pci_bus, hd, 1);
 
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index f23d495..81ec4fb 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -52,6 +52,7 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 bool drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
+int drive_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 5/6] qtest/bios-tables: Correct Q35 command line

[John Snow]

If the Q35 board types are to begin recognizing
and decoding syntactic sugar for drive/device
declarations, then workarounds found within
the qtests suite need to be adjusted to prevent
any test failures after the fix.

bios-tables-test improperly uses this cli:
-drive file=etc,id=hd -device ide-hd,drive=hd

Which will create a drive and device due to
the lack of specifying if=none. Then, it will
attempt to create a second device and fail.

This patch corrects this test to always use
the full, non-sugared -device/-drive syntax
for both PC and Q35.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/bios-tables-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 602932b..9e4d205 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -714,14 +714,12 @@ static void test_acpi_one(const char *params, test_data *data)
     uint8_t signature_high;
     uint16_t signature;
     int i;
-    const char *device = "";
 
-    if (!g_strcmp0(data->machine, MACHINE_Q35)) {
-        device = ",id=hd -device ide-hd,drive=hd";
-    }
+    args = g_strdup_printf("-net none -display none %s "
+                           "-drive id=hd0,if=none,file=%s "
+                           "-device ide-hd,drive=hd0 ",
+                           params ? params : "", disk);
 
-    args = g_strdup_printf("-net none -display none %s -drive file=%s%s,",
-                           params ? params : "", disk, device);
     qtest_start(args);
 
    /* Wait at most 1 minute */
-- 
1.9.3

[Qemu-devel] [PATCH v2 6/6] q35/ahci: Pick up -cdrom and -hda options

[John Snow]

This patch implements the backend for the Q35 board
for us to be able to pick up and use drives defined
by the -cdrom, -hda, or -drive if=ide shorthand options.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/i386/pc_q35.c |  4 ++++
 hw/ide/ahci.c    | 15 +++++++++++++++
 hw/ide/ahci.h    |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b28ddbb..bb0dc8e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -86,6 +86,7 @@ static void pc_q35_init(MachineState *machine)
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
+    DriveInfo *hd[MAX_SATA_PORTS];
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -253,6 +254,9 @@ static void pc_q35_init(MachineState *machine)
                                            true, "ich9-ahci");
     idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
     idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
+    g_assert_cmpint(MAX_SATA_PORTS, ==, ICH_AHCI(ahci)->ahci.ports);
+    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
+    ahci_ide_create_devs(ahci, hd);
 
     if (usb_enabled(false)) {
         /* Should we create 6 UHCI according to ich9 spec? */
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8978643..79abb6a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1419,3 +1419,18 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
+{
+    AHCIPCIState *d = ICH_AHCI(dev);
+    AHCIState *ahci = &d->ahci;
+    int i;
+
+    for (i = 0; i < ahci->ports; i++) {
+        if (tab[i] == NULL) {
+            continue;
+        }
+        ide_create_drive(&ahci->dev[i].port, 0, tab[i]);
+    }
+
+}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1543df7..b6dc64e 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -332,4 +332,6 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
+
 #endif /* HW_IDE_AHCI_H */
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/6] blockdev: Allow overriding if_max_dev property

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The if_max_devs table as in the past been an immutable
> default that controls the mapping of index => (bus,unit)
> for all boards and all HBAs for each interface type.
>
> Since adding this mapping information to the HBA device
> itself is currently unwieldly from the perspective of
> retrieving this information at option parsing time
> (e.g, within drive_new), we consider the alternative
> of marking the if_max_devs table mutable so that
> later configuration and initialization can adjust the
> mapping at will, but only up until a drive is added,
> at which point the mapping is finalized.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c                | 26 +++++++++++++++++++++++++-
>  include/sysemu/blockdev.h |  2 ++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 81398e7..9b05f1b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -60,7 +60,7 @@ static const char *const if_name[IF_COUNT] = {
>      [IF_XEN] = "xen",
>  };
>  
> -static const int if_max_devs[IF_COUNT] = {
> +static 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.
> @@ -79,6 +79,30 @@ static const int if_max_devs[IF_COUNT] = {
>      [IF_SCSI] = 7,
>  };
>  
> +/**
> + * Boards may call this to offer board-by-board overrides
> + * of the default, global values.
> + */
> +void override_max_devs(BlockInterfaceType type, int max_devs)
> +{
> +    DriveInfo *dinfo;
> +
> +    if (max_devs <= 0) {
> +        return;
> +    }
> +
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        if (dinfo->type == type) {
> +            fprintf(stderr, "Cannot override units-per-bus property of"
> +                    " the %s interface, because a drive of that type has"
> +                    " already been added.\n", if_name[type]);
> +            g_assert_not_reached();
> +        }

I'd prefer a straightforward

    QTAILQ_FOREACH(dinfo, &drives, next) {
        assert(dinfo->type != type);
    }

or even

    assert(QTAILQ_EMPTY(drives));

assuming it actually holds.

> +    }
> +
> +    if_max_devs[type] = max_devs;
> +}
> +
>  /*
>   * We automatically delete the drive when a device using it gets
>   * unplugged.  Questionable feature, but we can't just drop it.
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 80f768d..f23d495 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -46,6 +46,8 @@ struct DriveInfo {
>      QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> +void override_max_devs(BlockInterfaceType type, int max_devs);
> +
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>  bool drive_check_orphaned(void);
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);

Re: [Qemu-devel] [PATCH v2 3/6] pc/vl: Add units-per-default-bus property

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch adds the 'units_per_default_bus' property which
> allows individual boards to declare their desired
> index => (bus,unit) mapping for their default HBA, so that
> boards such as Q35 can specify that its default if_ide HBA,
> AHCI, only accepts one unit per bus.
>
> This property only overrides the mapping for drives matching
> the block_default_type interface.
>
> This patch also adds this property to *all* past and present
> Q35 machine types. This retroactive addition is justified
> because the previous erroneous index=>(bus,unit) mappings
> caused by lack of such a property were not utilized due to
> lack of initialization code in the Q35 init routine.
>
> Further, semantically, the Q35 board type has always had the
> property that its default HBA, AHCI, only accepts one unit per
> bus. The new code added to add devices to drives relies upon
> the accuracy of this mapping. Thus, the property is applied
> retroactively to reduce complexity of allowing IDE HBAs with
> different units per bus.
>
> Examples:
>
> Prior to this patch, all IDE HBAs were assumed to use 2 units
> per bus (Master, Slave). When using Q35 and AHCI, however, we
> only allow one unit per bus.
>
> -hdb foo.qcow2 would become index=1, or bus=0,unit=1.
> -hdd foo.qcow2 would become index=3, or bus=1,unit=1.
> -drive file=foo.qcow2,index=5 becomes bus=2,unit=1.
>
> These are invalid for AHCI. They now become, under Q35 only:
>
> -hdb foo.qcow2 --> index=1, bus=1, unit=0.
> -hdd foo.qcow2 --> index=3, bus=3, unit=0.
> -drive file=foo.qcow2,index=5 --> bus=5,unit=0.
>
> The mapping is adjusted based on the fact that the default IF
> for the Q35 machine type is IF_IDE, and units-per-default-bus
> overrides the IDE mapping from its default of 2 units per bus
> to just 1 unit per bus.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Nice examples, thank you!

Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic

[Markus Armbruster]

John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
> ---
>  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?

>      }
>  
> -    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
> -        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
> +    for (i = 0; i < n; i++) {
> +        hd[i] = drive_get_by_index(IF_IDE, i);
>      }
> +

Stray blank line.

>  }

Function is much nicer now, thanks!

[...]

Re: [Qemu-devel] [PATCH v2 6/6] q35/ahci: Pick up -cdrom and -hda options

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch implements the backend for the Q35 board
> for us to be able to pick up and use drives defined
> by the -cdrom, -hda, or -drive if=ide shorthand options.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/i386/pc_q35.c |  4 ++++
>  hw/ide/ahci.c    | 15 +++++++++++++++
>  hw/ide/ahci.h    |  2 ++
>  3 files changed, 21 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b28ddbb..bb0dc8e 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -86,6 +86,7 @@ static void pc_q35_init(MachineState *machine)
>      DeviceState *icc_bridge;
>      PcGuestInfo *guest_info;
>      ram_addr_t lowmem;
> +    DriveInfo *hd[MAX_SATA_PORTS];
>  
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
> @@ -253,6 +254,9 @@ static void pc_q35_init(MachineState *machine)
>                                             true, "ich9-ahci");
>      idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
>      idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
> +    g_assert_cmpint(MAX_SATA_PORTS, ==, ICH_AHCI(ahci)->ahci.ports);
> +    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
> +    ahci_ide_create_devs(ahci, hd);

The assertion is new since v1, and a bit more interesting than it looks
on first glance.

It protects the two calls following it, by ensuring the array has space
for the ports.

MAX_SATA_PORTS is defined to 6 in this file.

ICH_AHCI(ahci)->ahci.ports is initialized by ahci_init() to its ports
argument.  pci_ich9_ahci_init() passes literal 6.  Oookay.

The assertion is more restrictive than required for correctness: >=
would do.  I don't mind.

It's tempting to do

    ide_drive_get(hd, ARRAY_SIZE(hd));

for more obvious correctness, except that'll screw with the detection of
extra drives if ahci.ports ever becomes < MAX_SATA_PORTS.

Good.

>  
>      if (usb_enabled(false)) {
>          /* Should we create 6 UHCI according to ich9 spec? */
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 8978643..79abb6a 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1419,3 +1419,18 @@ static void sysbus_ahci_register_types(void)
>  }
>  
>  type_init(sysbus_ahci_register_types)
> +
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)

Elsewhere, we call it DriveInfo **hd.  I'd stick to the common name.

> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    int i;
> +
> +    for (i = 0; i < ahci->ports; i++) {
> +        if (tab[i] == NULL) {
> +            continue;
> +        }
> +        ide_create_drive(&ahci->dev[i].port, 0, tab[i]);
> +    }
> +
> +}
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 1543df7..b6dc64e 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -332,4 +332,6 @@ void ahci_uninit(AHCIState *s);
>  
>  void ahci_reset(AHCIState *s);
>  
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
> +
>  #endif /* HW_IDE_AHCI_H */

Re: [Qemu-devel] [PATCH v2 0/6] Q35: Implement -cdrom/-hda sugar

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The Q35 board initialization does not currently bother to look
> for any drives added by the various syntactical sugar shorthands
> to be added to the AHCI HBA. These include -hda through -hdd,
> -cdrom, and -drive if=ide shorthands.
>
> An obstacle to having implemented this sooner is debate over
> whether or not to add an additional interface type, and how to
> manage the different units-per-bus mappings of various HBA
> implementations.
>
> This patch series:
> (1) Does not add IF_AHCI, but reuses IF_IDE
> (2) Allows the if_max_devs table to be overridden
> (3) Adds this override to the Q35 board type.
> (4) Finally, adds implementation to Q35 initialization.

General: commit messages' line lengths on the short side (~50 rather
than the customary ~70), but that's better than too long.

I think PATCH 4 could be improved further, but the only issue serious
enough to make me withhold my R-by is the unexplained change from fatal
error to warning there.

Almost there :)

Re: [Qemu-devel] [PATCH v2 6/6] q35/ahci: Pick up -cdrom and -hda options

[John Snow]

On 09/30/2014 03:54 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This patch implements the backend for the Q35 board
>> for us to be able to pick up and use drives defined
>> by the -cdrom, -hda, or -drive if=ide shorthand options.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/i386/pc_q35.c |  4 ++++
>>   hw/ide/ahci.c    | 15 +++++++++++++++
>>   hw/ide/ahci.h    |  2 ++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index b28ddbb..bb0dc8e 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -86,6 +86,7 @@ static void pc_q35_init(MachineState *machine)
>>       DeviceState *icc_bridge;
>>       PcGuestInfo *guest_info;
>>       ram_addr_t lowmem;
>> +    DriveInfo *hd[MAX_SATA_PORTS];
>>
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>>        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>> @@ -253,6 +254,9 @@ static void pc_q35_init(MachineState *machine)
>>                                              true, "ich9-ahci");
>>       idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
>>       idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
>> +    g_assert_cmpint(MAX_SATA_PORTS, ==, ICH_AHCI(ahci)->ahci.ports);
>> +    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
>> +    ahci_ide_create_devs(ahci, hd);
>
> The assertion is new since v1, and a bit more interesting than it looks
> on first glance.
>
> It protects the two calls following it, by ensuring the array has space
> for the ports.
>
> MAX_SATA_PORTS is defined to 6 in this file.
>
> ICH_AHCI(ahci)->ahci.ports is initialized by ahci_init() to its ports
> argument.  pci_ich9_ahci_init() passes literal 6.  Oookay.
>
> The assertion is more restrictive than required for correctness: >=
> would do.  I don't mind.

It's more of a warning that these two values are, for now, considered 
equivalent. If that should change in the future for some unthinkable 
reason, this assertion will help remind whoever changes it.

For now, Q35 uses ICH9. ICH9's AHCI controller has 6 ports. This should 
always be the case.

I could, I suppose, make the HD array in the heap, and ask the ICH9 how 
many ports it has, etc... The assertion was a quick, equally 
authoritative way that is not likely to need changing.

> It's tempting to do
>
>      ide_drive_get(hd, ARRAY_SIZE(hd));
>
> for more obvious correctness, except that'll screw with the detection of
> extra drives if ahci.ports ever becomes < MAX_SATA_PORTS.
>
> Good.
>
>>
>>       if (usb_enabled(false)) {
>>           /* Should we create 6 UHCI according to ich9 spec? */
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 8978643..79abb6a 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1419,3 +1419,18 @@ static void sysbus_ahci_register_types(void)
>>   }
>>
>>   type_init(sysbus_ahci_register_types)
>> +
>> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
>
> Elsewhere, we call it DriveInfo **hd.  I'd stick to the common name.
>
>> +{
>> +    AHCIPCIState *d = ICH_AHCI(dev);
>> +    AHCIState *ahci = &d->ahci;
>> +    int i;
>> +
>> +    for (i = 0; i < ahci->ports; i++) {
>> +        if (tab[i] == NULL) {
>> +            continue;
>> +        }
>> +        ide_create_drive(&ahci->dev[i].port, 0, tab[i]);
>> +    }
>> +
>> +}
>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>> index 1543df7..b6dc64e 100644
>> --- a/hw/ide/ahci.h
>> +++ b/hw/ide/ahci.h
>> @@ -332,4 +332,6 @@ void ahci_uninit(AHCIState *s);
>>
>>   void ahci_reset(AHCIState *s);
>>
>> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
>> +
>>   #endif /* HW_IDE_AHCI_H */

Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic

[John Snow]

On 09/30/2014 03:38 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>> ---
>>   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?

I'll fix the divide by zero and address the other comments. I can adjust 
the semantics to match the other function -- sometimes I don't realize 
I've accidentally created a function that is similar to, but behaves 
differently from, another.

The reasoning behind a warning instead of a hard error was that if we 
provide less slots in the HD table than is necessary for covering the 
full number of buses/units in the board, we're going to stop early. It's 
not necessarily a fatal error, so I replaced the hard exit() with a warning.

If we do want a hard exit, I should probably start baking in the error 
pathways down this far to do it appropriately instead of terminating 
execution.

>>       }
>>
>> -    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
>> -        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
>> +    for (i = 0; i < n; i++) {
>> +        hd[i] = drive_get_by_index(IF_IDE, i);
>>       }
>> +
>
> Stray blank line.
>
>>   }
>
> Function is much nicer now, thanks!
>
> [...]
>

Re: [Qemu-devel] [PATCH v2 4/6] ide: Update ide_drive_get to be HBA agnostic

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/30/2014 03:38 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>>> ---
>>>   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?
>
> I'll fix the divide by zero and address the other comments. I can
> adjust the semantics to match the other function -- sometimes I don't
> realize I've accidentally created a function that is similar to, but
> behaves differently from, another.

Easy to miss :)

> The reasoning behind a warning instead of a hard error was that if we
> provide less slots in the HD table than is necessary for covering the
> full number of buses/units in the board, we're going to stop
> early. It's not necessarily a fatal error, so I replaced the hard
> exit() with a warning.

I have no strong opinion on fatal vs. warning here.  But such a change
should be a separate patch.

> If we do want a hard exit, I should probably start baking in the error
> pathways down this far to do it appropriately instead of terminating
> execution.

Calling exit() deep down a call chain is never nice, but it's not
actually a problem in ide_drive_get(), because it gets called only
during board initialization.  That said, if you *want* to make it nice,
go right ahead :)

[...]