[Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[John Snow]

This is an extremely rough/quick sketch of
a -cdrom/-hda desugaring fix for Q35/AHCI.

Before I spent any time on it, I wanted feedback
from Markus or anyone else who had concerns about
how this problem would get fixed.

This is, then, rough approach #2.

Highlights:
(1) Add a board property (instead of a HBA property, sigh)
    that defines how we should map (index, (bus,unit)).

(2) Modify drive_new to accept the MachineClass instead of
    the default interface type. This does not affect how
    default drives get added, because any over-rides to
    the "default type" get handled in options, so while
    it appears we have removed the type of default drives,
    we have not.

(3) Create helpers for AHCI to assist the Q35 board in
    populating the AHCI device with the IDE drives.

(4) Create a helper to whine at us for oversights and
    help bug reporters give us more meaningful information.

John Snow (3):
  blockdev: Add function to search for orphaned drives
  Add units-per-idebus property
  ahci: implement -cdrom and -hd[a-d]

 blockdev.c                | 29 +++++++++++++++++++++++++++--
 device-hotplug.c          |  2 +-
 hw/i386/pc_q35.c          |  6 +++++-
 hw/ide/ahci.c             | 31 +++++++++++++++++++++++++++++++
 hw/ide/ahci.h             |  3 +++
 include/hw/boards.h       |  3 ++-
 include/sysemu/blockdev.h |  3 ++-
 vl.c                      | 24 ++++++++++++++++--------
 8 files changed, 87 insertions(+), 14 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC v2 1/3] blockdev: Add function to search for orphaned drives

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 19 +++++++++++++++++++
 include/sysemu/blockdev.h |  1 +
 vl.c                      |  5 +++++
 3 files changed, 25 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..5e7c93a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -166,6 +166,25 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
+DriveInfo *drive_check_orphaned(void)
+{
+    DriveInfo *dinfo;
+    DriveInfo *ret = NULL;
+
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        /* If dev is NULL, it has no device attached.
+         * If drv is non-NULL, it has a file attached.
+         * If both conditions are true, it is possibly an oversight. */
+        if ((dinfo->bdrv->dev == NULL) && (dinfo->bdrv->drv != NULL)) {
+            fprintf(stderr, "Orphaned drive: id=%s,if=%s,file=%s\n",
+                    dinfo->id, if_name[dinfo->type], dinfo->bdrv->filename);
+            ret = dinfo;
+        }
+    }
+
+    return ret;
+}
+
 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..25d52d2 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -46,6 +46,7 @@ struct DriveInfo {
 };
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
+DriveInfo *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 5db0d08..e095bcd 100644
--- a/vl.c
+++ b/vl.c
@@ -4457,6 +4457,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] [RFC v2 2/3] Add units-per-idebus property

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 10 ++++++++--
 device-hotplug.c          |  2 +-
 hw/i386/pc_q35.c          |  3 ++-
 include/hw/boards.h       |  3 ++-
 include/sysemu/blockdev.h |  2 +-
 vl.c                      | 19 +++++++++++--------
 6 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5e7c93a..6c524b7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -45,6 +45,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "sysemu/arch_init.h"
+#include "hw/boards.h"
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
@@ -643,7 +644,7 @@ QemuOptsList qemu_legacy_drive_opts = {
     },
 };
 
-DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
+DriveInfo *drive_new(QemuOpts *all_opts, MachineClass *mc)
 {
     const char *value;
     DriveInfo *dinfo = NULL;
@@ -651,6 +652,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     QemuOpts *legacy_opts;
     DriveMediaType media = MEDIA_DISK;
     BlockInterfaceType type;
+    BlockInterfaceType block_default_type = mc->block_default_type;
     int cyls, heads, secs, translation;
     int max_devs, bus_id, unit_id, index;
     const char *devaddr;
@@ -828,7 +830,11 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     unit_id = qemu_opt_get_number(legacy_opts, "unit", -1);
     index   = qemu_opt_get_number(legacy_opts, "index", -1);
 
-    max_devs = if_max_devs[type];
+    if (type == IF_IDE && mc->units_per_idebus) {
+        max_devs = mc->units_per_idebus;
+    } else {
+        max_devs = if_max_devs[type];
+    }
 
     if (index != -1) {
         if (bus_id != 0 || unit_id != -1) {
diff --git a/device-hotplug.c b/device-hotplug.c
index e6a1ffb..857ac53 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -40,7 +40,7 @@ DriveInfo *add_init_drive(const char *optstr)
         return NULL;
 
     mc = MACHINE_GET_CLASS(current_machine);
-    dinfo = drive_new(opts, mc->block_default_type);
+    dinfo = drive_new(opts, mc);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d4a907c..fd26fe1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -348,7 +348,8 @@ static void pc_q35_init_1_4(MachineState *machine)
 
 #define PC_Q35_2_2_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
-    .default_machine_opts = "firmware=bios-256k.bin"
+    .default_machine_opts = "firmware=bios-256k.bin",   \
+    .units_per_idebus = 1
 
 static QEMUMachine pc_q35_machine_v2_2 = {
     PC_Q35_2_2_MACHINE_OPTIONS,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index dfb6718..73e656f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -37,6 +37,7 @@ struct QEMUMachine {
         no_cdrom:1,
         no_sdcard:1;
     int is_default;
+    unsigned short units_per_idebus;
     const char *default_machine_opts;
     const char *default_boot_order;
     GlobalProperty *compat_props;
@@ -95,11 +96,11 @@ struct MachineClass {
         no_cdrom:1,
         no_sdcard:1;
     int is_default;
+    unsigned short units_per_idebus;
     const char *default_machine_opts;
     const char *default_boot_order;
     GlobalProperty *compat_props;
     const char *hw_version;
-
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 25d52d2..f7de0a0 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -55,7 +55,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
-DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
+DriveInfo *drive_new(QemuOpts *arg, MachineClass *mc);
 void drive_del(DriveInfo *dinfo);
 
 /* device-hotplug */
diff --git a/vl.c b/vl.c
index e095bcd..8359469 100644
--- a/vl.c
+++ b/vl.c
@@ -1151,9 +1151,9 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
 
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
-    BlockInterfaceType *block_default_type = opaque;
+    MachineClass *mc = opaque;
 
-    return drive_new(opts, *block_default_type) == NULL;
+    return drive_new(opts, mc) == NULL;
 }
 
 static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
@@ -1165,7 +1165,7 @@ static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
 }
 
 static void default_drive(int enable, int snapshot, BlockInterfaceType type,
-                          int index, const char *optstr)
+                          int index, const char *optstr, MachineClass *mc)
 {
     QemuOpts *opts;
 
@@ -1177,7 +1177,8 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
-    if (!drive_new(opts, type)) {
+
+    if (!drive_new(opts, mc)) {
         exit(1);
     }
 }
@@ -1583,6 +1584,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_idebus = qm->units_per_idebus;
     mc->max_cpus = qm->max_cpus;
     mc->no_serial = qm->no_serial;
     mc->no_parallel = qm->no_parallel;
@@ -4376,14 +4378,15 @@ int main(int argc, char **argv, char **envp)
     if (snapshot)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
     if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
-                          &machine_class->block_default_type, 1) != 0) {
+                          machine_class, 1) != 0) {
         exit(1);
     }
 
     default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
-                  CDROM_OPTS);
-    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
-    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
+                  CDROM_OPTS, machine_class);
+    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS,
+                  machine_class);
+    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS, machine_class);
 
     if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
                           NULL, 1) != 0) {
-- 
1.9.3

[Qemu-devel] [RFC v2 3/3] ahci: implement -cdrom and -hd[a-d]

[John Snow]

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index fd26fe1..0f33696 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");
+    ahci_drive_get(ahci, hd);
+    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..ae28de4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1402,3 +1402,34 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void ahci_drive_get(PCIDevice *dev, DriveInfo **tab)
+{
+    AHCIPCIState *d = ICH_AHCI(dev);
+    AHCIState *ahci = &d->ahci;
+    unsigned i;
+
+    if ((i = drive_get_max_bus(IF_IDE)) >= ahci->ports) {
+        fprintf(stderr, "AHCI: Too many IDE buses defined for AHCI (%d > %d)\n",
+                i, ahci->ports - 1);
+    }
+
+    for (i = 0; i < ahci->ports; ++i) {
+        tab[i] = drive_get_by_index(IF_IDE, i);
+    }
+}
+
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
+{
+    AHCIPCIState *d = ICH_AHCI(dev);
+    AHCIState *ahci = &d->ahci;
+    unsigned 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..06a18de 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -332,4 +332,7 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
+void ahci_drive_get(PCIDevice *dev, DriveInfo **tab);
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
+
 #endif /* HW_IDE_AHCI_H */
-- 
1.9.3

Re: [Qemu-devel] [RFC v2 1/3] blockdev: Add function to search for orphaned drives

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c                | 19 +++++++++++++++++++
>  include/sysemu/blockdev.h |  1 +
>  vl.c                      |  5 +++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index b361fbb..5e7c93a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -166,6 +166,25 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>      return NULL;
>  }
>  
> +DriveInfo *drive_check_orphaned(void)
> +{
> +    DriveInfo *dinfo;
> +    DriveInfo *ret = NULL;
> +
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        /* If dev is NULL, it has no device attached.
> +         * If drv is non-NULL, it has a file attached.
> +         * If both conditions are true, it is possibly an oversight. */

Suggest to spell out dinfo->bdrv->dev and dinfo->bdrv->drv.

"File attached" is imprecise.  BDS member drv is non-null betwen
bdrv_open() and bdrv_close().  A BDS with null drv means "empty", in the
sense of "no medium".


> +        if ((dinfo->bdrv->dev == NULL) && (dinfo->bdrv->drv != NULL)) {
> +            fprintf(stderr, "Orphaned drive: id=%s,if=%s,file=%s\n",
> +                    dinfo->id, if_name[dinfo->type], dinfo->bdrv->filename);
> +            ret = dinfo;
> +        }
> +    }

Please prefix "Warning:" to make the nature of this message more
explicit.

"Orphaned drive" might not be obvious to all users, but it's concise,
and no worse than the "has no peer" we use for NICs.

You warn when a non-empty drive is not used by a device model.

This warns when you create one with -drive if=none for future use in the
monitor.  I guess that's fine.

It doesn't warn for empty drives.  I doubt "empty" should make a
difference.

I think the condition to check is "has the board failed to pick up a
drive that is meant to be picked up by the board":

    dinfo->type != IF_NONE && !dinfo->bdrv->dev

I guess this can warn about default drives, because we blindly add them
whether the boards wants them or not.  Stupidest solution that could
possibly work: add a flag to DriveInfo to suppress the warning for them.

Better solution: don't add them unless the board wants them.  I tried
that before, but my solution[*] went nowhere.  If you're interested in
trying again, let me know, and I'll explain.

> +
> +    return ret;
> +}
> +
>  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..25d52d2 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -46,6 +46,7 @@ struct DriveInfo {
>  };
>  
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
> +DriveInfo *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 5db0d08..e095bcd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4457,6 +4457,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();


[*] https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg02993.html

Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c                | 10 ++++++++--
>  device-hotplug.c          |  2 +-
>  hw/i386/pc_q35.c          |  3 ++-
>  include/hw/boards.h       |  3 ++-
>  include/sysemu/blockdev.h |  2 +-
>  vl.c                      | 19 +++++++++++--------
>  6 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5e7c93a..6c524b7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -45,6 +45,7 @@
>  #include "qmp-commands.h"
>  #include "trace.h"
>  #include "sysemu/arch_init.h"
> +#include "hw/boards.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -643,7 +644,7 @@ QemuOptsList qemu_legacy_drive_opts = {
>      },
>  };
>  
> -DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> +DriveInfo *drive_new(QemuOpts *all_opts, MachineClass *mc)
>  {
>      const char *value;
>      DriveInfo *dinfo = NULL;
> @@ -651,6 +652,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      QemuOpts *legacy_opts;
>      DriveMediaType media = MEDIA_DISK;
>      BlockInterfaceType type;
> +    BlockInterfaceType block_default_type = mc->block_default_type;
>      int cyls, heads, secs, translation;
>      int max_devs, bus_id, unit_id, index;
>      const char *devaddr;
> @@ -828,7 +830,11 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      unit_id = qemu_opt_get_number(legacy_opts, "unit", -1);
>      index   = qemu_opt_get_number(legacy_opts, "index", -1);
>  
> -    max_devs = if_max_devs[type];
> +    if (type == IF_IDE && mc->units_per_idebus) {
> +        max_devs = mc->units_per_idebus;
> +    } else {
> +        max_devs = if_max_devs[type];
> +    }

This overrides if_max_devs[IF_IDE] in one out of three places.

if_max_devs[type] governs the mapping between index and (bus, unit).

If it's zero, then (bus, unit) = (0, index).

Else, (bus, unit) = (index / max_devs, index % max_devs).

Overriding it just here affects these things:

* Picking a default when the user specifies neither index nor unit

* Range checking unit

* Default ID, but let's ignore that for now

It does *not* affect drive_index_to_bus_id(), drive_index_to_unit_id(),
i.e. the actual mapping between index and (bus, unit)!  index=1 is still
mapped to (bus, unit) = (0, 1).  No good.

Testing (needs an incremental fix, see below) confirms:

    qemu: -drive if=ide,media=cdrom,index=1: unit 1 too big (max is 0)

You have to override if_max_devs[] consistently.

You provide for overriding if_max_devs[IF_IDE] only.  It'll do for now.

>  
>      if (index != -1) {
>          if (bus_id != 0 || unit_id != -1) {
> diff --git a/device-hotplug.c b/device-hotplug.c
> index e6a1ffb..857ac53 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -40,7 +40,7 @@ DriveInfo *add_init_drive(const char *optstr)
>          return NULL;
>  
>      mc = MACHINE_GET_CLASS(current_machine);
> -    dinfo = drive_new(opts, mc->block_default_type);
> +    dinfo = drive_new(opts, mc);
>      if (!dinfo) {
>          qemu_opts_del(opts);
>          return NULL;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d4a907c..fd26fe1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -348,7 +348,8 @@ static void pc_q35_init_1_4(MachineState *machine)
>  
>  #define PC_Q35_2_2_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
> -    .default_machine_opts = "firmware=bios-256k.bin"
> +    .default_machine_opts = "firmware=bios-256k.bin",   \
> +    .units_per_idebus = 1
>  

I figrue this keeps -drive if=ide for older Q35 machines compatibly
broken.  If that's what we want to do...

>  static QEMUMachine pc_q35_machine_v2_2 = {
>      PC_Q35_2_2_MACHINE_OPTIONS,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index dfb6718..73e656f 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -37,6 +37,7 @@ struct QEMUMachine {
>          no_cdrom:1,
>          no_sdcard:1;
>      int is_default;
> +    unsigned short units_per_idebus;
>      const char *default_machine_opts;
>      const char *default_boot_order;
>      GlobalProperty *compat_props;

if_max_devs[] and the max_devs variables are all int.  I'd rather not
mix signed and unsigned without need

> @@ -95,11 +96,11 @@ struct MachineClass {
>          no_cdrom:1,
>          no_sdcard:1;
>      int is_default;
> +    unsigned short units_per_idebus;
>      const char *default_machine_opts;
>      const char *default_boot_order;
>      GlobalProperty *compat_props;
>      const char *hw_version;
> -
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
>  };

Let's keep the blank line separating the instance variables from the
method.

> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 25d52d2..f7de0a0 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -55,7 +55,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
>  QemuOpts *drive_def(const char *optstr);
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
> -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> +DriveInfo *drive_new(QemuOpts *arg, MachineClass *mc);
>  void drive_del(DriveInfo *dinfo);
>  
>  /* device-hotplug */
> diff --git a/vl.c b/vl.c
> index e095bcd..8359469 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1151,9 +1151,9 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
>  
>  static int drive_init_func(QemuOpts *opts, void *opaque)
>  {
> -    BlockInterfaceType *block_default_type = opaque;
> +    MachineClass *mc = opaque;
>  
> -    return drive_new(opts, *block_default_type) == NULL;
> +    return drive_new(opts, mc) == NULL;
>  }
>  
>  static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
> @@ -1165,7 +1165,7 @@ static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
>  }
>  
>  static void default_drive(int enable, int snapshot, BlockInterfaceType type,
> -                          int index, const char *optstr)
> +                          int index, const char *optstr, MachineClass *mc)
>  {
>      QemuOpts *opts;
>  
> @@ -1177,7 +1177,8 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
>      if (snapshot) {
>          drive_enable_snapshot(opts, NULL);
>      }
> -    if (!drive_new(opts, type)) {
> +
> +    if (!drive_new(opts, mc)) {
>          exit(1);
>      }
>  }
> @@ -1583,6 +1584,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_idebus = qm->units_per_idebus;
>      mc->max_cpus = qm->max_cpus;
>      mc->no_serial = qm->no_serial;
>      mc->no_parallel = qm->no_parallel;

Not sufficient!  You missed the duplicated code in
pc_generic_machine_class_init().  That something was missing was quite
obvious in my testing, although what was missing was not.  This is the
fix I mentioned above.

Marcel, you touched both copies recently.  Do you know why we need two
of them?  And why do we copy from QEMUMachine to MachineClass member by
member?  Why can't we just point from MachineClass to QEMUMachine?  Or
at least embed the struct so we can copy it wholesale?

> @@ -4376,14 +4378,15 @@ int main(int argc, char **argv, char **envp)
>      if (snapshot)
>          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
>      if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> -                          &machine_class->block_default_type, 1) != 0) {
> +                          machine_class, 1) != 0) {
>          exit(1);
>      }
>  
>      default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
> -                  CDROM_OPTS);
> -    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> -    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> +                  CDROM_OPTS, machine_class);
> +    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS,
> +                  machine_class);
> +    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS, machine_class);
>  
>      if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
>                            NULL, 1) != 0) {

Re: [Qemu-devel] [RFC v2 3/3] ahci: implement -cdrom and -hd[a-d]

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/i386/pc_q35.c |  3 +++
>  hw/ide/ahci.c    | 31 +++++++++++++++++++++++++++++++
>  hw/ide/ahci.h    |  3 +++
>  3 files changed, 37 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index fd26fe1..0f33696 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");
> +    ahci_drive_get(ahci, hd);
> +    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..ae28de4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1402,3 +1402,34 @@ static void sysbus_ahci_register_types(void)
>  }
>  
>  type_init(sysbus_ahci_register_types)
> +
> +void ahci_drive_get(PCIDevice *dev, DriveInfo **tab)
> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    unsigned i;
> +
> +    if ((i = drive_get_max_bus(IF_IDE)) >= ahci->ports) {

I might be one of the strongest advocates for brevity on this list, but
even I frown on embedding assignments in conditionals without a genuine
need, and on reusing loop counters for unrelated purposes.

Moreover, you're mixing signed and unsigned: drive_get_max_bus() returns
int, ahci->ports is int32_t, but your i is unsigned.  Breaks when
drive_get_max_bus() returns -1 because no IF_IDE drives are defined:

    $ qemu -vnc :0 -M q35 -nodefaults
    AHCI: Too many IDE buses defined for AHCI (-1 > 5)

Stick to int.

    int n, i;

    n = drive_get_max_bus(IF_IDE);
    if (n >= ahci->ports) {

> +        fprintf(stderr, "AHCI: Too many IDE buses defined for AHCI (%d > %d)\n",
> +                i, ahci->ports - 1);
> +    }
> +
> +    for (i = 0; i < ahci->ports; ++i) {

Compares unsigned i with signed ahci->ports.  Stick to int.

> +        tab[i] = drive_get_by_index(IF_IDE, i);
> +    }
> +}
> +
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab)
> +{
> +    AHCIPCIState *d = ICH_AHCI(dev);
> +    AHCIState *ahci = &d->ahci;
> +    unsigned i;
> +
> +    for (i = 0; i < ahci->ports; i++) {

Likewise.

> +        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..06a18de 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -332,4 +332,7 @@ void ahci_uninit(AHCIState *s);
>  
>  void ahci_reset(AHCIState *s);
>  
> +void ahci_drive_get(PCIDevice *dev, DriveInfo **tab);
> +void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **tab);
> +
>  #endif /* HW_IDE_AHCI_H */

Re: [Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is an extremely rough/quick sketch of
> a -cdrom/-hda desugaring fix for Q35/AHCI.
>
> Before I spent any time on it, I wanted feedback
> from Markus or anyone else who had concerns about
> how this problem would get fixed.
>
> This is, then, rough approach #2.
>
> Highlights:
> (1) Add a board property (instead of a HBA property, sigh)
>     that defines how we should map (index, (bus,unit)).

Imperfect, but it'll do for now.  The place in the boards that sets it
should point to the HBA in a comment.

> (2) Modify drive_new to accept the MachineClass instead of
>     the default interface type. This does not affect how
>     default drives get added, because any over-rides to
>     the "default type" get handled in options, so while
>     it appears we have removed the type of default drives,
>     we have not.
>
> (3) Create helpers for AHCI to assist the Q35 board in
>     populating the AHCI device with the IDE drives.
>
> (4) Create a helper to whine at us for oversights and
>     help bug reporters give us more meaningful information.

General approach looks good to me; I can see only coding bugs, not
design flaws.

Re: [Qemu-devel] [RFC v2 1/3] blockdev: Add function to search for orphaned drives

[John Snow]

On 09/19/2014 04:28 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c                | 19 +++++++++++++++++++
>>   include/sysemu/blockdev.h |  1 +
>>   vl.c                      |  5 +++++
>>   3 files changed, 25 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index b361fbb..5e7c93a 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -166,6 +166,25 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>       return NULL;
>>   }
>>
>> +DriveInfo *drive_check_orphaned(void)
>> +{
>> +    DriveInfo *dinfo;
>> +    DriveInfo *ret = NULL;
>> +
>> +    QTAILQ_FOREACH(dinfo, &drives, next) {
>> +        /* If dev is NULL, it has no device attached.
>> +         * If drv is non-NULL, it has a file attached.
>> +         * If both conditions are true, it is possibly an oversight. */
>
> Suggest to spell out dinfo->bdrv->dev and dinfo->bdrv->drv.
>
> "File attached" is imprecise.  BDS member drv is non-null betwen
> bdrv_open() and bdrv_close().  A BDS with null drv means "empty", in the
> sense of "no medium".
>
>
>> +        if ((dinfo->bdrv->dev == NULL) && (dinfo->bdrv->drv != NULL)) {
>> +            fprintf(stderr, "Orphaned drive: id=%s,if=%s,file=%s\n",
>> +                    dinfo->id, if_name[dinfo->type], dinfo->bdrv->filename);
>> +            ret = dinfo;
>> +        }
>> +    }
>
> Please prefix "Warning:" to make the nature of this message more
> explicit.
>
> "Orphaned drive" might not be obvious to all users, but it's concise,
> and no worse than the "has no peer" we use for NICs.
>
> You warn when a non-empty drive is not used by a device model.
>
> This warns when you create one with -drive if=none for future use in the
> monitor.  I guess that's fine.
>
> It doesn't warn for empty drives.  I doubt "empty" should make a
> difference.

I didn't want it to warn about things like the SD drive and the floppy 
drive, created by default, but you're right. Your suggestion below fixes 
this problem.

> I think the condition to check is "has the board failed to pick up a
> drive that is meant to be picked up by the board":
>
>      dinfo->type != IF_NONE && !dinfo->bdrv->dev
>
> I guess this can warn about default drives, because we blindly add them
> whether the boards wants them or not.  Stupidest solution that could
> possibly work: add a flag to DriveInfo to suppress the warning for them.

Hm. Actually, the default drives are added by their specific interfaces. 
IF_IDE, IF_FLOPPY and IF_SD. This is a good improvement.
(Well, cdrom is actually added via block_default_type which is usually 
unset, and happens to coincide with IF_IDE.)

> Better solution: don't add them unless the board wants them.  I tried
> that before, but my solution[*] went nowhere.  If you're interested in
> trying again, let me know, and I'll explain.
>
>> +
>> +    return ret;
>> +}
>> +
>>   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..25d52d2 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -46,6 +46,7 @@ struct DriveInfo {
>>   };
>>
>>   DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>> +DriveInfo *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 5db0d08..e095bcd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4457,6 +4457,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();
>
>
> [*] https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg02993.html
>

Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property

[Marcel Apfelbaum]

On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  blockdev.c                | 10 ++++++++--
> >  device-hotplug.c          |  2 +-
> >  hw/i386/pc_q35.c          |  3 ++-
> >  include/hw/boards.h       |  3 ++-
> >  include/sysemu/blockdev.h |  2 +-
> >  vl.c                      | 19 +++++++++++--------
> >  6 files changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 5e7c93a..6c524b7 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -45,6 +45,7 @@
> >  #include "qmp-commands.h"
> >  #include "trace.h"
> >  #include "sysemu/arch_init.h"
> > +#include "hw/boards.h"
> >  
> >  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
> >  
> > @@ -643,7 +644,7 @@ QemuOptsList qemu_legacy_drive_opts = {
> >      },
> >  };
> >  
> > -DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> > +DriveInfo *drive_new(QemuOpts *all_opts, MachineClass *mc)
> >  {
> >      const char *value;
> >      DriveInfo *dinfo = NULL;
> > @@ -651,6 +652,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> >      QemuOpts *legacy_opts;
> >      DriveMediaType media = MEDIA_DISK;
> >      BlockInterfaceType type;
> > +    BlockInterfaceType block_default_type = mc->block_default_type;
> >      int cyls, heads, secs, translation;
> >      int max_devs, bus_id, unit_id, index;
> >      const char *devaddr;
> > @@ -828,7 +830,11 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
> >      unit_id = qemu_opt_get_number(legacy_opts, "unit", -1);
> >      index   = qemu_opt_get_number(legacy_opts, "index", -1);
> >  
> > -    max_devs = if_max_devs[type];
> > +    if (type == IF_IDE && mc->units_per_idebus) {
> > +        max_devs = mc->units_per_idebus;
> > +    } else {
> > +        max_devs = if_max_devs[type];
> > +    }
> 
> This overrides if_max_devs[IF_IDE] in one out of three places.
> 
> if_max_devs[type] governs the mapping between index and (bus, unit).
> 
> If it's zero, then (bus, unit) = (0, index).
> 
> Else, (bus, unit) = (index / max_devs, index % max_devs).
> 
> Overriding it just here affects these things:
> 
> * Picking a default when the user specifies neither index nor unit
> 
> * Range checking unit
> 
> * Default ID, but let's ignore that for now
> 
> It does *not* affect drive_index_to_bus_id(), drive_index_to_unit_id(),
> i.e. the actual mapping between index and (bus, unit)!  index=1 is still
> mapped to (bus, unit) = (0, 1).  No good.
> 
> Testing (needs an incremental fix, see below) confirms:
> 
>     qemu: -drive if=ide,media=cdrom,index=1: unit 1 too big (max is 0)
> 
> You have to override if_max_devs[] consistently.
> 
> You provide for overriding if_max_devs[IF_IDE] only.  It'll do for now.
> 
> >  
> >      if (index != -1) {
> >          if (bus_id != 0 || unit_id != -1) {
> > diff --git a/device-hotplug.c b/device-hotplug.c
> > index e6a1ffb..857ac53 100644
> > --- a/device-hotplug.c
> > +++ b/device-hotplug.c
> > @@ -40,7 +40,7 @@ DriveInfo *add_init_drive(const char *optstr)
> >          return NULL;
> >  
> >      mc = MACHINE_GET_CLASS(current_machine);
> > -    dinfo = drive_new(opts, mc->block_default_type);
> > +    dinfo = drive_new(opts, mc);
> >      if (!dinfo) {
> >          qemu_opts_del(opts);
> >          return NULL;
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index d4a907c..fd26fe1 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -348,7 +348,8 @@ static void pc_q35_init_1_4(MachineState *machine)
> >  
> >  #define PC_Q35_2_2_MACHINE_OPTIONS                      \
> >      PC_Q35_MACHINE_OPTIONS,                             \
> > -    .default_machine_opts = "firmware=bios-256k.bin"
> > +    .default_machine_opts = "firmware=bios-256k.bin",   \
> > +    .units_per_idebus = 1
> >  
> 
> I figrue this keeps -drive if=ide for older Q35 machines compatibly
> broken.  If that's what we want to do...
> 
> >  static QEMUMachine pc_q35_machine_v2_2 = {
> >      PC_Q35_2_2_MACHINE_OPTIONS,
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index dfb6718..73e656f 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -37,6 +37,7 @@ struct QEMUMachine {
> >          no_cdrom:1,
> >          no_sdcard:1;
> >      int is_default;
> > +    unsigned short units_per_idebus;
> >      const char *default_machine_opts;
> >      const char *default_boot_order;
> >      GlobalProperty *compat_props;
> 
> if_max_devs[] and the max_devs variables are all int.  I'd rather not
> mix signed and unsigned without need
> 
> > @@ -95,11 +96,11 @@ struct MachineClass {
> >          no_cdrom:1,
> >          no_sdcard:1;
> >      int is_default;
> > +    unsigned short units_per_idebus;
> >      const char *default_machine_opts;
> >      const char *default_boot_order;
> >      GlobalProperty *compat_props;
> >      const char *hw_version;
> > -
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >                                             DeviceState *dev);
> >  };
> 
> Let's keep the blank line separating the instance variables from the
> method.
> 
> > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> > index 25d52d2..f7de0a0 100644
> > --- a/include/sysemu/blockdev.h
> > +++ b/include/sysemu/blockdev.h
> > @@ -55,7 +55,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
> >  QemuOpts *drive_def(const char *optstr);
> >  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
> >                      const char *optstr);
> > -DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> > +DriveInfo *drive_new(QemuOpts *arg, MachineClass *mc);
> >  void drive_del(DriveInfo *dinfo);
> >  
> >  /* device-hotplug */
> > diff --git a/vl.c b/vl.c
> > index e095bcd..8359469 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1151,9 +1151,9 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
> >  
> >  static int drive_init_func(QemuOpts *opts, void *opaque)
> >  {
> > -    BlockInterfaceType *block_default_type = opaque;
> > +    MachineClass *mc = opaque;
> >  
> > -    return drive_new(opts, *block_default_type) == NULL;
> > +    return drive_new(opts, mc) == NULL;
> >  }
> >  
> >  static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
> > @@ -1165,7 +1165,7 @@ static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
> >  }
> >  
> >  static void default_drive(int enable, int snapshot, BlockInterfaceType type,
> > -                          int index, const char *optstr)
> > +                          int index, const char *optstr, MachineClass *mc)
> >  {
> >      QemuOpts *opts;
> >  
> > @@ -1177,7 +1177,8 @@ static void default_drive(int enable, int snapshot, BlockInterfaceType type,
> >      if (snapshot) {
> >          drive_enable_snapshot(opts, NULL);
> >      }
> > -    if (!drive_new(opts, type)) {
> > +
> > +    if (!drive_new(opts, mc)) {
> >          exit(1);
> >      }
> >  }
> > @@ -1583,6 +1584,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_idebus = qm->units_per_idebus;
> >      mc->max_cpus = qm->max_cpus;
> >      mc->no_serial = qm->no_serial;
> >      mc->no_parallel = qm->no_parallel;
> 
> Not sufficient!  You missed the duplicated code in
> pc_generic_machine_class_init().  That something was missing was quite
> obvious in my testing, although what was missing was not.  This is the
> fix I mentioned above.
> 
> Marcel, you touched both copies recently.  Do you know why we need two
> of them?  And why do we copy from QEMUMachine to MachineClass member by
> member?  Why can't we just point from MachineClass to QEMUMachine?  Or
> at least embed the struct so we can copy it wholesale?
Hi Markus,

I'll try to explain the design:
1. MachineClass should not be aware of QEMUMachine. The objective here is to
   eliminate QEMUMachine and it should not be part of MachineClass at any cost.
2. The plan is like this:
   - The machine types that are not yet QOMified will be QOMified on the fly
     by qemu_register_machine (vl.c) that calls machine_class_init and matches
     QEMUMachine fields with MachineClass fields.
     - This mechanism will be removed when all machines are QOMified. (never? :) )
   - Machines that are QOMified will not reach this code at all, but follow
     the normal QOM path.
As you can see, by design there is no duplication.

Now let's get to PC machines case:
- This is a "weird" use case of hybrid QOMifying.
- All that the author wanted was to have all the PC machines
  derive from type TYPE_PC_MACHINE, but didn't have the time/resources/will
  to go over every PC machine and QOMify it. But he did need the common class.
- His implementation:
  - He couldn't use the generic qemu_register_machine because the PC machines
    would not have derived from MACHINE_PC_TYPE.
  - So he used the following logic:
    - from the vl.c point of view, the PC machines are QOMified, so the
      PC machines registration *does not reach vl.c".
    - from the PC machines point of view, they remained not QOMified.
    - qemu_register_pc_machine mimics vl.c registration *only for pc machines*
      and has to copy the fields by itself. Plus, it gives the PC machines a common
      base class, the thing he was interested in in the first place.

I hope it helped,
Marcel

> 
> > @@ -4376,14 +4378,15 @@ int main(int argc, char **argv, char **envp)
> >      if (snapshot)
> >          qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
> >      if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
> > -                          &machine_class->block_default_type, 1) != 0) {
> > +                          machine_class, 1) != 0) {
> >          exit(1);
> >      }
> >  
> >      default_drive(default_cdrom, snapshot, machine_class->block_default_type, 2,
> > -                  CDROM_OPTS);
> > -    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> > -    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> > +                  CDROM_OPTS, machine_class);
> > +    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS,
> > +                  machine_class);
> > +    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS, machine_class);
> >  
> >      if (qemu_opts_foreach(qemu_find_opts("numa"), numa_init_func,
> >                            NULL, 1) != 0) {

Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property

[Markus Armbruster]

Marcel Apfelbaum <marcel.a@redhat.com> writes:

> On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>> > Signed-off-by: John Snow <jsnow@redhat.com>
[...]
>> > @@ -1583,6 +1584,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_idebus = qm->units_per_idebus;
>> >      mc->max_cpus = qm->max_cpus;
>> >      mc->no_serial = qm->no_serial;
>> >      mc->no_parallel = qm->no_parallel;
>> 
>> Not sufficient!  You missed the duplicated code in
>> pc_generic_machine_class_init().  That something was missing was quite
>> obvious in my testing, although what was missing was not.  This is the
>> fix I mentioned above.
>> 
>> Marcel, you touched both copies recently.  Do you know why we need two
>> of them?  And why do we copy from QEMUMachine to MachineClass member by
>> member?  Why can't we just point from MachineClass to QEMUMachine?  Or
>> at least embed the struct so we can copy it wholesale?
> Hi Markus,
>
> I'll try to explain the design:
> 1. MachineClass should not be aware of QEMUMachine. The objective here is to
>    eliminate QEMUMachine and it should not be part of MachineClass at any cost.
> 2. The plan is like this:
>    - The machine types that are not yet QOMified will be QOMified on the fly
>      by qemu_register_machine (vl.c) that calls machine_class_init and matches
>      QEMUMachine fields with MachineClass fields.
>      - This mechanism will be removed when all machines are QOMified. (never? :) )

Okay %-)

>    - Machines that are QOMified will not reach this code at all, but follow
>      the normal QOM path.
> As you can see, by design there is no duplication.
>
> Now let's get to PC machines case:
> - This is a "weird" use case of hybrid QOMifying.
> - All that the author wanted was to have all the PC machines
>   derive from type TYPE_PC_MACHINE, but didn't have the time/resources/will
>   to go over every PC machine and QOMify it. But he did need the common class.
> - His implementation:
>   - He couldn't use the generic qemu_register_machine because the PC machines
>     would not have derived from MACHINE_PC_TYPE.
>   - So he used the following logic:
>     - from the vl.c point of view, the PC machines are QOMified, so the
>       PC machines registration *does not reach vl.c".
>     - from the PC machines point of view, they remained not QOMified.
>     - qemu_register_pc_machine mimics vl.c registration *only for pc machines*
>       and has to copy the fields by itself. Plus, it gives the PC machines a common
>       base class, the thing he was interested in in the first place.

Artifact of this hackery: two identical static functions: vl.c's
machine_class_init() and pc.c's pc_generic_machine_class_init().  Trap
for the unwary, and it caught John.

Unless there are plans to get rid of pc_generic_machine_class_init()
fairly soon, I'd recommend to give machine_class_init() external linkage
with a suitable comment, and drop pc_generic_machine_class_init().

> I hope it helped,

Sure did!

I checked the CodeTransitions Wiki page.  It covers this work, and
points to http://wiki.qemu.org/Features/QOM/Machine for more
information.  Good.

Re: [Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[John Snow]

On 09/19/2014 05:53 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This is an extremely rough/quick sketch of
>> a -cdrom/-hda desugaring fix for Q35/AHCI.
>>
>> Before I spent any time on it, I wanted feedback
>> from Markus or anyone else who had concerns about
>> how this problem would get fixed.
>>
>> This is, then, rough approach #2.
>>
>> Highlights:
>> (1) Add a board property (instead of a HBA property, sigh)
>>      that defines how we should map (index, (bus,unit)).
>
> Imperfect, but it'll do for now.  The place in the boards that sets it
> should point to the HBA in a comment.
>
>> (2) Modify drive_new to accept the MachineClass instead of
>>      the default interface type. This does not affect how
>>      default drives get added, because any over-rides to
>>      the "default type" get handled in options, so while
>>      it appears we have removed the type of default drives,
>>      we have not.
>>
>> (3) Create helpers for AHCI to assist the Q35 board in
>>      populating the AHCI device with the IDE drives.
>>
>> (4) Create a helper to whine at us for oversights and
>>      help bug reporters give us more meaningful information.
>
> General approach looks good to me; I can see only coding bugs, not
> design flaws.
>

I rewrote this series and was about to send it out, but it does fail the 
bios-tables-test because this test uses this command line:

-net none -display none -machine q35,accel=tcg -drive 
file=tests/acpi-test-disk.raw,id=hd0 -device ide-hd,drive=hd0,

Notice it doesn't say if=none for the drive, so after fixing Q35, this 
actually creates a new failure in this test because we will create the 
drive (and device), then fail when trying to create the second device 
attached to the same drive.

I think this test is at fault, but I wanted to be duly diligent and ask 
the question: "Is it a big deal if I break backwards compatibility with 
broken scripts?"

-- 
—js

Re: [Qemu-devel] [RFC v2 0/3] Q35/AHCI -cdrom/-hda desugaring

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/19/2014 05:53 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> This is an extremely rough/quick sketch of
>>> a -cdrom/-hda desugaring fix for Q35/AHCI.
>>>
>>> Before I spent any time on it, I wanted feedback
>>> from Markus or anyone else who had concerns about
>>> how this problem would get fixed.
>>>
>>> This is, then, rough approach #2.
>>>
>>> Highlights:
>>> (1) Add a board property (instead of a HBA property, sigh)
>>>      that defines how we should map (index, (bus,unit)).
>>
>> Imperfect, but it'll do for now.  The place in the boards that sets it
>> should point to the HBA in a comment.
>>
>>> (2) Modify drive_new to accept the MachineClass instead of
>>>      the default interface type. This does not affect how
>>>      default drives get added, because any over-rides to
>>>      the "default type" get handled in options, so while
>>>      it appears we have removed the type of default drives,
>>>      we have not.
>>>
>>> (3) Create helpers for AHCI to assist the Q35 board in
>>>      populating the AHCI device with the IDE drives.
>>>
>>> (4) Create a helper to whine at us for oversights and
>>>      help bug reporters give us more meaningful information.
>>
>> General approach looks good to me; I can see only coding bugs, not
>> design flaws.
>>
>
> I rewrote this series and was about to send it out, but it does fail
> the bios-tables-test because this test uses this command line:
>
> -net none -display none -machine q35,accel=tcg -drive
> file=tests/acpi-test-disk.raw,id=hd0 -device ide-hd,drive=hd0,

qemu: -device ide-hd,drive=hd: Property 'ide-hd.drive' can't take value 'hd', it's in use

> Notice it doesn't say if=none for the drive, so after fixing Q35, this
> actually creates a new failure in this test because we will create the
> drive (and device), then fail when trying to create the second device
> attached to the same drive.

Exactly.

Code in question:

    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 file=%s%s,",
                           params ? params : "", disk, device);
    qtest_start(args);

Called twice:

1. data->machine = MACHINE_PC (really: "pc")
   params = "-machine accel=tcg"

   Thus, args = "-net none -display none -machine accel=tcg"
   " -drive file=tests/acpi-test-disk.raw,"

   Neither if, bus, unit, nor index specified with -drive, so this asks
   board code to set up an IDE disk (bus, unit) = (0,0), and the board
   code complies.

2. data->machine == MACHINE_Q35 (really: "q35",
   params = "-machine q35,accel=tcg"

   Thus, args = ""-net none -display none -machine q35,accel=tcg"
   " -drive file=tests/acpi-test-disk.raw,id=hd -device ide-hd,drive=hd,"

   Again, this asks board code for IDE disk (0,0).  Before your patch,
   board code silently ignores the request.  Afterwards, it complies.

   Additionally, it sets up another IDE disk (0,0) with device,
   exploiting the misfeature that -device *can* use a drive with if=ide.
   Uses the same drive with two device models, which is unsafe and duly
   rejected.

   The special case for q35 is for coping with the lack of a working
   -drive if=ide.  A working if=ide makes it unnecessary and brings out
   its flaws.

   Even before your patch, the special case is unnecessary: "-drive
   if=none,id=hd,file=... -drive ide-hd,drive=hd" would do for both
   machine types.  I'd make that change in a preliminary patch, because
   that avoids polluting the meat of your patching with correcting test
   bugs.

> I think this test is at fault, but I wanted to be duly diligent and
> ask the question: "Is it a big deal if I break backwards compatibility
> with broken scripts?"

Where "broken scripts" means "our users' broken scripts (if any)".

On the one hand, we've always told users that -device goes with a -drive
if=none.  On the other hand, using a drive not picked up by board code
has always worked anyway.

If we want to remain bug-compatible, we need to make the q35 board code
pick up the IF_IDE drives only for new machine types.  Feasible.  But is
it worthwhile?