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

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

Other changes from RFC2:
- 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)

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/i386/pc.c              |  1 +
 hw/i386/pc_q35.c          |  6 ++++-
 hw/ide/ahci.c             | 15 +++++++++++++
 hw/ide/ahci.h             |  2 ++
 hw/ide/core.c             | 12 +++++-----
 include/hw/boards.h       |  2 ++
 include/sysemu/blockdev.h |  5 +++++
 tests/bios-tables-test.c  | 10 ++++-----
 vl.c                      | 20 ++++++++++++++++-
 10 files changed, 115 insertions(+), 14 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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 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                      | 12 +++++++++++-
 3 files changed, 34 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 dc792fe..eaef240 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)
@@ -4457,6 +4462,11 @@ int main(int argc, char **argv, char **envp)
     if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
         exit(1);
 
+    /* anybody left over? */
+    if (drive_check_orphaned()) {
+        fprintf(stderr, "Warning: found drives without a backing device.\n");
+    }
+
     net_check_clients();
 
     ds = init_displaystate();
-- 
1.9.3

[Qemu-devel] [PATCH 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                | 35 ++++++++++++++++++++++++++++++++++-
 include/sysemu/blockdev.h |  3 +++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 81398e7..94562e9 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, "Warning: Cannot override units-per-bus property of"
+                    " the %s interface, because a drive of that type has"
+                    " already been added.\n", if_name[type]);
+            return;
+        }
+    }
+
+    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.
@@ -111,6 +135,15 @@ void blockdev_auto_del(BlockDriverState *bs)
     }
 }
 
+int if_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/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 80f768d..10719d5 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -46,10 +46,13 @@ 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);
 int drive_get_max_bus(BlockInterfaceType type);
+int if_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
-- 
1.9.3

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

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 2c2e9dc..ab578bf 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 eaef240..6fffa1f 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;
@@ -4377,6 +4378,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 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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6fba056..1e43d50 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
 void ide_drive_get(DriveInfo **hd, int max_bus)
 {
     int i;
+    int max_devs = if_get_max_devs(IF_IDE) * max_bus;
+    int buses = drive_get_max_bus(IF_IDE) + 1;
 
-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
-        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
-        exit(1);
+    if (buses > max_bus) {
+        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
+                buses, max_bus);
     }
 
-    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 < max_devs; i++) {
+        hd[i] = drive_get_by_index(IF_IDE, i);
     }
 }
-- 
1.9.3

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

A workaround for these command line options not previously
working as intended in the bios-tables-test is also removed.

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b28ddbb..1664a2d 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,8 @@ 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");
+    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 ba69de3..64f0d37 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1402,3 +1402,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 1/6] blockdev: Orphaned drive search

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 warn about drives added with if_none,

Judging from my testing, I suspect you mean "will not warn" ;)

> 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                      | 12 +++++++++++-
>  3 files changed, 34 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 dc792fe..eaef240 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)
> @@ -4457,6 +4462,11 @@ int main(int argc, char **argv, char **envp)
>      if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
>          exit(1);
>  
> +    /* anybody left over? */
> +    if (drive_check_orphaned()) {
> +        fprintf(stderr, "Warning: found drives without a backing device.\n");
> +    }
> +
>      net_check_clients();
>  
>      ds = init_displaystate();

Terminology: the device model is the front end, the thing created by
-drive is the back end.  I'd simply  drop this message.  If you want to
keep it, rephrase it to avoid "backing device".

Re: [Qemu-devel] [PATCH 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                | 35 ++++++++++++++++++++++++++++++++++-
>  include/sysemu/blockdev.h |  3 +++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 81398e7..94562e9 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, "Warning: Cannot override units-per-bus property of"
> +                    " the %s interface, because a drive of that type has"
> +                    " already been added.\n", if_name[type]);

Isn't this a programming error?  If yes: assert().

> +            return;
> +        }
> +    }
> +
> +    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.
> @@ -111,6 +135,15 @@ void blockdev_auto_del(BlockDriverState *bs)
>      }
>  }
>  
> +int if_get_max_devs(BlockInterfaceType type)
> +{
> +    if (type >= IF_IDE && type < IF_COUNT) {
> +        return if_max_devs[type];
> +    }
> +
> +    return 0;
> +}
> +

The need for if_get_max_devs() isn't clear at this point.

>  static int drive_index_to_bus_id(BlockInterfaceType type, int index)
>  {
>      int max_devs = if_max_devs[type];
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 80f768d..10719d5 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -46,10 +46,13 @@ 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);
>  int drive_get_max_bus(BlockInterfaceType type);
> +int if_get_max_devs(BlockInterfaceType type);
>  DriveInfo *drive_get_next(BlockInterfaceType type);
>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);

Re: [Qemu-devel] [PATCH 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

A bit more detail would be nice here.  Suggest to give a -drive example,
and discuss its meaning before and after the patch with Q35: bogus unit#
before, sane one after.

> ---
>  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 2c2e9dc..ab578bf 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

Changes the (bus, unit) mapping for all Q35 machine types.

Overriding the overide by adding .units_per_default_bus = 0 to
PC_Q35_2_1_MACHINE_OPTIONS would limit the change to new machine types,
if we want that for bug compatibility.  I'm not sure it would be useful,
I'm just spelling it out for the benefit of other reviewers.

Any incompatible change to old machine types must feature *prominently*
in the commit message, even when it changes things from "not useful" to
"actually sane".

>  #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 eaef240..6fffa1f 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;
> @@ -4377,6 +4378,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);

Looks good.

Re: [Qemu-devel] [PATCH 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 6fba056..1e43d50 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
>  void ide_drive_get(DriveInfo **hd, int max_bus)
>  {
>      int i;
> +    int max_devs = if_get_max_devs(IF_IDE) * max_bus;

Okay, here you need if_get_max_devs().  Suggest to move its introduction
from PATCH 2 to this one.  Hmm, I guess we better change its name to
start with drive_.

> +    int buses = drive_get_max_bus(IF_IDE) + 1;
>  
> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
> -        exit(1);
> +    if (buses > max_bus) {
> +        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
> +                buses, max_bus);

New!  Error message now English!

Since you touch it, you could use error_report().  Not important, as
doesn't make much of a difference in this case.

>      }
>  
> -    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 < max_devs; i++) {
> +        hd[i] = drive_get_by_index(IF_IDE, i);
>      }
>  }

Maybe parameter max_bus should be replaced by the number of slots in
hd[].  What do you think?

Re: [Qemu-devel] [PATCH 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.
>
> A workaround for these command line options not previously
> working as intended in the bios-tables-test is also removed.

I figure this paragraph has since become PATCH 5, and should be dropped
here.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/i386/pc_q35.c |  3 +++
>  hw/ide/ahci.c    | 15 +++++++++++++++
>  hw/ide/ahci.h    |  2 ++
>  3 files changed, 20 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b28ddbb..1664a2d 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,8 @@ 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");
> +    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);

Superfluous parenthesis around ahci.

> +    ahci_ide_create_devs(ahci, hd);
>  
>      if (usb_enabled(false)) {
>          /* Should we create 6 UHCI according to ich9 spec? */
[...]

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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.
>>
>> A workaround for these command line options not previously
>> working as intended in the bios-tables-test is also removed.
>
> I figure this paragraph has since become PATCH 5, and should be dropped
> here.
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/i386/pc_q35.c |  3 +++
>>  hw/ide/ahci.c    | 15 +++++++++++++++
>>  hw/ide/ahci.h    |  2 ++
>>  3 files changed, 20 insertions(+)
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index b28ddbb..1664a2d 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,8 @@ 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");
>> +    ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
>
> Superfluous parenthesis around ahci.

Nevermind, I can't read.

>> +    ahci_ide_create_devs(ahci, hd);
>>  
>>      if (usb_enabled(false)) {
>>          /* Should we create 6 UHCI according to ich9 spec? */
> [...]

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

I'm pleased with your solution, and I think it's almost ready for merge.

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

[John Snow]

On 09/24/2014 10:35 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/ide/core.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6fba056..1e43d50 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
>>   void ide_drive_get(DriveInfo **hd, int max_bus)
>>   {
>>       int i;
>> +    int max_devs = if_get_max_devs(IF_IDE) * max_bus;
>
> Okay, here you need if_get_max_devs().  Suggest to move its introduction
> from PATCH 2 to this one.  Hmm, I guess we better change its name to
> start with drive_.
>
>> +    int buses = drive_get_max_bus(IF_IDE) + 1;
>>
>> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
>> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>> -        exit(1);
>> +    if (buses > max_bus) {
>> +        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
>> +                buses, max_bus);
>
> New!  Error message now English!
>
> Since you touch it, you could use error_report().  Not important, as
> doesn't make much of a difference in this case.
>
>>       }
>>
>> -    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 < max_devs; i++) {
>> +        hd[i] = drive_get_by_index(IF_IDE, i);
>>       }
>>   }
>
> Maybe parameter max_bus should be replaced by the number of slots in
> hd[].  What do you think?
>

This is your only recommendation I haven't implemented yet for V2. I 
think this makes sense from a mechanical perspective because it would be 
nice to have the hard guarantee of not running over the array boundary 
instead of relying on the numbers in different places to be consistent.

The only reason I didn't do this is because I didn't want to touch calls 
to drive_get in 10 boards.

On the other hand, if the numbers are out of alignment and we run over 
the end of the array here, it's a board property not aligning with how 
the board init function works -- Not really a runtime issue, and 
something we can avoid relatively easily.

However, this could be a problem if we decide NOT to make the 
units-per-bus property constant across all Q35 types, for example, if we 
don't also then update how we generate this HD array.

(If Q35 1.6 does not use this property, we'll have 12 max devices 
implied, which is clearly wrong and why I advocate instating this 
property backwards for all versions.)

I think I am inclined to leave it as-is for now provided we agree that 
creating this property for all versions makes sense. If we wish to add 
the ability to have different numbers of units-per-bus properties per 
version, then the ide drive pickup code on all boards will have to get 
smarter.

Thoughts?

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

[John Snow]

On 09/24/2014 10:06 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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 warn about drives added with if_none,
>
> Judging from my testing, I suspect you mean "will not warn" ;)

A fun story: I did actually mean what I wrote, but in my own testing, 
found that this produced warnings for the test suite, so I decided to 
change the behavior to not warn for IF_NONE.

I figured it would make sense if it gave a little warning that amounted 
to "Don't forget to add a device for this drive!" but I didn't want to 
see any more output on the test suite, so I nixed it.

I amended my commit message for V2.

>> 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                      | 12 +++++++++++-
>>   3 files changed, 34 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 dc792fe..eaef240 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)
>> @@ -4457,6 +4462,11 @@ int main(int argc, char **argv, char **envp)
>>       if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0)
>>           exit(1);
>>
>> +    /* anybody left over? */
>> +    if (drive_check_orphaned()) {
>> +        fprintf(stderr, "Warning: found drives without a backing device.\n");
>> +    }
>> +
>>       net_check_clients();
>>
>>       ds = init_displaystate();
>
> Terminology: the device model is the front end, the thing created by
> -drive is the back end.  I'd simply  drop this message.  If you want to
> keep it, rephrase it to avoid "backing device".
>

-- 
—js

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/24/2014 10:35 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.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   hw/ide/core.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 6fba056..1e43d50 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
>>>   void ide_drive_get(DriveInfo **hd, int max_bus)
>>>   {
>>>       int i;
>>> +    int max_devs = if_get_max_devs(IF_IDE) * max_bus;
>>
>> Okay, here you need if_get_max_devs().  Suggest to move its introduction
>> from PATCH 2 to this one.  Hmm, I guess we better change its name to
>> start with drive_.
>>
>>> +    int buses = drive_get_max_bus(IF_IDE) + 1;
>>>
>>> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
>>> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>>> -        exit(1);
>>> +    if (buses > max_bus) {
>>> +        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
>>> +                buses, max_bus);
>>
>> New!  Error message now English!
>>
>> Since you touch it, you could use error_report().  Not important, as
>> doesn't make much of a difference in this case.
>>
>>>       }
>>>
>>> -    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 < max_devs; i++) {
>>> +        hd[i] = drive_get_by_index(IF_IDE, i);
>>>       }
>>>   }
>>
>> Maybe parameter max_bus should be replaced by the number of slots in
>> hd[].  What do you think?
>>
>
> This is your only recommendation I haven't implemented yet for V2. I
> think this makes sense from a mechanical perspective because it would
> be nice to have the hard guarantee of not running over the array
> boundary instead of relying on the numbers in different places to be
> consistent.
>
> The only reason I didn't do this is because I didn't want to touch
> calls to drive_get in 10 boards.

Leaving this idea to a followup patch is fine, even if we decide now we
do want it.

> On the other hand, if the numbers are out of alignment and we run over
> the end of the array here, it's a board property not aligning with how
> the board init function works -- Not really a runtime issue, and
> something we can avoid relatively easily.

An overrun is clearly a programming error.

The intent of replacing the parameter is to make the code clearer, not
to change its behavior.

> However, this could be a problem if we decide NOT to make the
> units-per-bus property constant across all Q35 types, for example, if
> we don't also then update how we generate this HD array.
>
> (If Q35 1.6 does not use this property, we'll have 12 max devices
> implied, which is clearly wrong and why I advocate instating this
> property backwards for all versions.)
>
> I think I am inclined to leave it as-is for now provided we agree that
> creating this property for all versions makes sense. If we wish to add
> the ability to have different numbers of units-per-bus properties per
> version, then the ide drive pickup code on all boards will have to get
> smarter.

Okay.  Let's figure out whether we want to keep the (bus, unit) mapping
bug-compatibly broken for old machine types, and get this series
wrapped.  Then, if we still care about making ide_drive_get() clearer,
try my idea to see how it works out.

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

[John Snow]

On 09/25/2014 02:13 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 09/24/2014 10:35 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.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    hw/ide/core.c | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index 6fba056..1e43d50 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -2551,13 +2551,15 @@ const VMStateDescription vmstate_ide_bus = {
>>>>    void ide_drive_get(DriveInfo **hd, int max_bus)
>>>>    {
>>>>        int i;
>>>> +    int max_devs = if_get_max_devs(IF_IDE) * max_bus;
>>>
>>> Okay, here you need if_get_max_devs().  Suggest to move its introduction
>>> from PATCH 2 to this one.  Hmm, I guess we better change its name to
>>> start with drive_.
>>>
>>>> +    int buses = drive_get_max_bus(IF_IDE) + 1;
>>>>
>>>> -    if (drive_get_max_bus(IF_IDE) >= max_bus) {
>>>> -        fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
>>>> -        exit(1);
>>>> +    if (buses > max_bus) {
>>>> +        fprintf(stderr, "Warning: Too many IDE buses defined (%d > %d)\n",
>>>> +                buses, max_bus);
>>>
>>> New!  Error message now English!
>>>
>>> Since you touch it, you could use error_report().  Not important, as
>>> doesn't make much of a difference in this case.
>>>
>>>>        }
>>>>
>>>> -    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 < max_devs; i++) {
>>>> +        hd[i] = drive_get_by_index(IF_IDE, i);
>>>>        }
>>>>    }
>>>
>>> Maybe parameter max_bus should be replaced by the number of slots in
>>> hd[].  What do you think?
>>>
>>
>> This is your only recommendation I haven't implemented yet for V2. I
>> think this makes sense from a mechanical perspective because it would
>> be nice to have the hard guarantee of not running over the array
>> boundary instead of relying on the numbers in different places to be
>> consistent.
>>
>> The only reason I didn't do this is because I didn't want to touch
>> calls to drive_get in 10 boards.
>
> Leaving this idea to a followup patch is fine, even if we decide now we
> do want it.
>
>> On the other hand, if the numbers are out of alignment and we run over
>> the end of the array here, it's a board property not aligning with how
>> the board init function works -- Not really a runtime issue, and
>> something we can avoid relatively easily.
>
> An overrun is clearly a programming error.
>
> The intent of replacing the parameter is to make the code clearer, not
> to change its behavior.
>
>> However, this could be a problem if we decide NOT to make the
>> units-per-bus property constant across all Q35 types, for example, if
>> we don't also then update how we generate this HD array.
>>
>> (If Q35 1.6 does not use this property, we'll have 12 max devices
>> implied, which is clearly wrong and why I advocate instating this
>> property backwards for all versions.)
>>
>> I think I am inclined to leave it as-is for now provided we agree that
>> creating this property for all versions makes sense. If we wish to add
>> the ability to have different numbers of units-per-bus properties per
>> version, then the ide drive pickup code on all boards will have to get
>> smarter.
>
> Okay.  Let's figure out whether we want to keep the (bus, unit) mapping
> bug-compatibly broken for old machine types, and get this series
> wrapped.  Then, if we still care about making ide_drive_get() clearer,
> try my idea to see how it works out.

On the subject of compatibly broken:
Is there any circumstance where the mapping has *any* effect on the 
current working behavior? Since we do not support the shorthand syntax 
at all currently, there is no code that USES this mapping to do anything.

Even if you specify -drive file=...,index=42; the -device that you 
currently *must* add simply ignores any index/bus/unit mappings you've 
already given the drive -- it doesn't seem to touch or use any 
information within the DriveInfo structure at all.

That said, why not make this property retroactive? It won't affect anything.

Or rather, the property alone won't. What *will* break older command 
lines is the fact that we are now supporting the syntactic sugar at all, 
but I think this is clearly a bug, and we should not go out of our way 
to tolerate broken syntax.

Based on the above, I think I will:

(A) Implement the property for all versions
(B) Change the behavior of ide_drive_get to take the number of elements 
in the array, and imply the number of buses instead. If we decide to 
roll back older versions to be "compatibly broken," this will in effect 
limit us to three drives instead of six (index 0, 2, and 4) being mapped 
to ports 0, 1, and 2.

This requires me touching the board init to adjust the call to drive_get 
slightly.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On the subject of compatibly broken:
> Is there any circumstance where the mapping has *any* effect on the
> current working behavior? Since we do not support the shorthand syntax
> at all currently, there is no code that USES this mapping to do
> anything.
>
> Even if you specify -drive file=...,index=42; the -device that you
> currently *must* add simply ignores any index/bus/unit mappings you've
> already given the drive -- it doesn't seem to touch or use any
> information within the DriveInfo structure at all.

I didn't think this through, and I knew it, so I chose to err on the
side of excessive caution by posing the bug-compatibility question.
You're right: since -drive if=ide was ignored before, we can change the
mapping.

> That said, why not make this property retroactive? It won't affect anything.
>
> Or rather, the property alone won't. What *will* break older command
> lines is the fact that we are now supporting the syntactic sugar at
> all, but I think this is clearly a bug, and we should not go out of
> our way to tolerate broken syntax.

Agreed.

> Based on the above, I think I will:
>
> (A) Implement the property for all versions

Yes, please.

> (B) Change the behavior of ide_drive_get to take the number of
> elements in the array, and imply the number of buses instead. If we
> decide to roll back older versions to be "compatibly broken," this
> will in effect limit us to three drives instead of six (index 0, 2,
> and 4) being mapped to ports 0, 1, and 2.
>
> This requires me touching the board init to adjust the call to
> drive_get slightly.

Thanks!