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

[Qemu-devel] [PATCH v3 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:

V3:

- renamed a variable for consistency ("tab" to "hd")
- Now uses ARRAY_SIZE() for calls to ide_drive_get where applicable
- Re-added a call to exit() for the error pathway of ide_drive_get
- Adjusted drive_get_max_devs semantics to be similar to drive_get_max_bus
  and added documentation
- Removed any possibility of a divide-by-zero in ide_drive_get

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                | 64 ++++++++++++++++++++++++++++++++++++++++++++++-
 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             | 22 ++++++++++++----
 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, 141 insertions(+), 23 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 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 ad43648..d2ad065 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 abec381..3040286 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 v3 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 d2ad065..1d9ab7f 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 3040286..a4033d4 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 v3 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 v3 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                | 17 +++++++++++++++++
 hw/alpha/dp264.c          |  2 +-
 hw/i386/pc_piix.c         |  2 +-
 hw/ide/core.c             | 22 +++++++++++++++++-----
 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, 44 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1d9ab7f..4489090 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -135,6 +135,23 @@ void blockdev_auto_del(BlockDriverState *bs)
     }
 }
 
+/**
+ * Returns the current mapping of how many units per bus
+ * a particular interface can support.
+ *
+ *  A positive integer indicates n units per bus.
+ *  0 implies the mapping has not been established.
+ * -1 indicates an invalid BlockInterfaceType was given.
+ */
+int drive_get_max_devs(BlockInterfaceType type)
+{
+    if (type >= IF_IDE && type < IF_COUNT) {
+        return if_max_devs[type];
+    }
+
+    return -1;
+}
+
 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..84a55e4 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, ARRAY_SIZE(hd));
 
         pci_cmd646_ide_init(pci_bus, hd, 0);
     }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 103d756..4384633 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, ARRAY_SIZE(hd));
     if (pci_enabled) {
         PCIDevice *dev;
         if (xen_enabled()) {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 190700a..ae85428 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2558,16 +2558,28 @@ 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 max_devs = drive_get_max_devs(IF_IDE);
+    int n_buses = max_devs ? (n / max_devs) : n;
 
-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
-        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
+    /*
+     * 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 max_devs * <num_real_buses>,
+     * We will stop looking for drives prematurely instead of overfilling
+     * the array.
+     */
+
+    if (highest_bus > n_buses) {
+        error_report("Too many IDE buses defined (%d > %d)",
+                     highest_bus, n_buses);
         exit(1);
     }
 
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    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..29cd708 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, ARRAY_SIZE(hd));
 
     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..b20807c 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, ARRAY_SIZE(hd));
 
     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..93606a4 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, ARRAY_SIZE(hd));
     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..4094beb 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, ARRAY_SIZE(hd));
 
     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..ebd87fb 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, ARRAY_SIZE(hd));
 
     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..9f484cd 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, ARRAY_SIZE(hd));
     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..871a0ea 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, ARRAY_SIZE(hd));
 
     pci_cmd646_ide_init(pci_bus, hd, 1);
 
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index a4033d4..09716de 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 v3 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 v3 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..063730e 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 **hd)
+{
+    AHCIPCIState *d = ICH_AHCI(dev);
+    AHCIState *ahci = &d->ahci;
+    int i;
+
+    for (i = 0; i < ahci->ports; i++) {
+        if (hd[i] == NULL) {
+            continue;
+        }
+        ide_create_drive(&ahci->dev[i].port, 0, hd[i]);
+    }
+
+}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1543df7..e223258 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 **hd);
+
 #endif /* HW_IDE_AHCI_H */
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Michael S. Tsirkin]

On Wed, Oct 01, 2014 at 02:19:26PM -0400, John Snow wrote:
> 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>


Reviewed-by: Michael S. Tsirkin <mst@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

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

[Michael S. Tsirkin]

On Wed, Oct 01, 2014 at 02:19:28PM -0400, John Snow wrote:
> 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>

Reviewed-by: Michael S. Tsirkin <mst@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

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

[Michael S. Tsirkin]

On Wed, Oct 01, 2014 at 02:19:29PM -0400, John Snow wrote:
> 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>

Reviewed-by: Michael S. Tsirkin <mst@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..063730e 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 **hd)
> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    int i;
> +
> +    for (i = 0; i < ahci->ports; i++) {
> +        if (hd[i] == NULL) {
> +            continue;
> +        }
> +        ide_create_drive(&ahci->dev[i].port, 0, hd[i]);
> +    }
> +
> +}
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 1543df7..e223258 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 **hd);
> +
>  #endif /* HW_IDE_AHCI_H */
> -- 
> 1.9.3

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

On Wed, Oct 01, 2014 at 02:19:23PM -0400, John Snow wrote:
> 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:
> 
> V3:
> 
> - renamed a variable for consistency ("tab" to "hd")
> - Now uses ARRAY_SIZE() for calls to ide_drive_get where applicable
> - Re-added a call to exit() for the error pathway of ide_drive_get
> - Adjusted drive_get_max_devs semantics to be similar to drive_get_max_bus
>   and added documentation
> - Removed any possibility of a divide-by-zero in ide_drive_get
> 
> 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                | 64 ++++++++++++++++++++++++++++++++++++++++++++++-
>  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             | 22 ++++++++++++----
>  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, 141 insertions(+), 23 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]