q35 qemu support

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +2 -2

index 99828ad..c9a49c8 100644
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
     return true;
 }
 
-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +19 -0

index 5f27b64..8b126ad 100644
@@ -40,6 +40,22 @@ struct DriveInfo {
     int refcount;
 };
 
+/*
+ * Each qemu machine type defines a mach_if field for its default
+ * interface type. If its unspecified, we set it to IF_IDE.
+ */
+static inline int get_mach_if(int mach_if)
+{
+    assert(mach_if < IF_COUNT);
+    assert(mach_if >= IF_DEFAULT);
+
+    if ((mach_if == IF_NONE) || (mach_if == IF_DEFAULT)) {
+        return IF_IDE;
+    }
+
+    return mach_if;
+}
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

@@ -61,4 +77,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+
+
 #endif

hw/boards.h +1 -1

index a2e0a54..969fd67 100644
@@ -20,7 +20,7 @@ typedef struct QEMUMachine {
     const char *desc;
     QEMUMachineInitFunc *init;
     QEMUMachineResetFunc *reset;
-    int use_scsi;
+    int mach_if;
     int max_cpus;
     unsigned int no_serial:1,
         no_parallel:1,

hw/device-hotplug.c +1 -1

index eec0fe3..33302f9 100644
@@ -39,7 +39,7 @@ DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->use_scsi);
+    dinfo = drive_init(opts, current_machine->mach_if);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;

hw/highbank.c +1 -1

index 11aa131..35cef06 100644
@@ -324,7 +324,7 @@ static QEMUMachine highbank_machine = {
     .name = "highbank",
     .desc = "Calxeda Highbank (ECX-1000)",
     .init = highbank_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/leon3.c +1 -1

index 7a9729d..cf9dcf8 100644
@@ -214,7 +214,7 @@ static QEMUMachine leon3_generic_machine = {
     .name     = "leon3_generic",
     .desc     = "Leon-3 generic",
     .init     = leon3_generic_hw_init,
-    .use_scsi = 0,
+    .mach_if = IF_DEFAULT,
 };
 
 static void leon3_machine_init(void)

hw/mips_jazz.c +2 -2

index db927f1..1c7a725 100644
@@ -325,14 +325,14 @@ static QEMUMachine mips_magnum_machine = {
     .name = "magnum",
     .desc = "MIPS Magnum",
     .init = mips_magnum_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine mips_pica61_machine = {
     .name = "pica61",
     .desc = "Acer Pica 61",
     .init = mips_pica61_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void mips_jazz_machine_init(void)

hw/pc_sysfw.c +1 -1

index b45f0ac..b8a03a6 100644
@@ -98,7 +98,7 @@ static void pc_fw_add_pflash_drv(void)
       return;
     }
 
-    drive_init(opts, machine->use_scsi);
+    drive_init(opts, machine->mach_if);
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,

hw/puv3.c +1 -1

index 43f7216..f68bb61 100644
@@ -120,7 +120,7 @@ static QEMUMachine puv3_machine = {
     .desc = "PKUnity Version-3 based on UniCore32",
     .init = puv3_init,
     .is_default = 1,
-    .use_scsi = 0,
+    .mach_if = IF_DEFAULT,
 };
 
 static void puv3_machine_init(void)

hw/realview.c +3 -3

index 19db4d0..7613f68 100644
@@ -382,14 +382,14 @@ static QEMUMachine realview_eb_machine = {
     .name = "realview-eb",
     .desc = "ARM RealView Emulation Baseboard (ARM926EJ-S)",
     .init = realview_eb_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine realview_eb_mpcore_machine = {
     .name = "realview-eb-mpcore",
     .desc = "ARM RealView Emulation Baseboard (ARM11MPCore)",
     .init = realview_eb_mpcore_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -403,7 +403,7 @@ static QEMUMachine realview_pbx_a9_machine = {
     .name = "realview-pbx-a9",
     .desc = "ARM RealView Platform Baseboard Explore for Cortex-A9",
     .init = realview_pbx_a9_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/spapr.c +1 -1

index 09b8e99..be8129e 100644
@@ -913,7 +913,7 @@ static QEMUMachine spapr_machine = {
     .reset = ppc_spapr_reset,
     .max_cpus = MAX_CPUS,
     .no_parallel = 1,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void spapr_machine_init(void)

hw/sun4m.c +12 -12

index a04b485..101d552 100644
@@ -1400,7 +1400,7 @@ static QEMUMachine ss5_machine = {
     .name = "SS-5",
     .desc = "Sun4m platform, SPARCstation 5",
     .init = ss5_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .is_default = 1,
 };
 

@@ -1408,7 +1408,7 @@ static QEMUMachine ss10_machine = {
     .name = "SS-10",
     .desc = "Sun4m platform, SPARCstation 10",
     .init = ss10_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1416,7 +1416,7 @@ static QEMUMachine ss600mp_machine = {
     .name = "SS-600MP",
     .desc = "Sun4m platform, SPARCserver 600MP",
     .init = ss600mp_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1424,7 +1424,7 @@ static QEMUMachine ss20_machine = {
     .name = "SS-20",
     .desc = "Sun4m platform, SPARCstation 20",
     .init = ss20_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1432,35 +1432,35 @@ static QEMUMachine voyager_machine = {
     .name = "Voyager",
     .desc = "Sun4m platform, SPARCstation Voyager",
     .init = vger_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine ss_lx_machine = {
     .name = "LX",
     .desc = "Sun4m platform, SPARCstation LX",
     .init = ss_lx_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine ss4_machine = {
     .name = "SS-4",
     .desc = "Sun4m platform, SPARCstation 4",
     .init = ss4_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine scls_machine = {
     .name = "SPARCClassic",
     .desc = "Sun4m platform, SPARCClassic",
     .init = scls_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine sbook_machine = {
     .name = "SPARCbook",
     .desc = "Sun4m platform, SPARCbook",
     .init = sbook_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static const struct sun4d_hwdef sun4d_hwdefs[] = {

@@ -1677,7 +1677,7 @@ static QEMUMachine ss1000_machine = {
     .name = "SS-1000",
     .desc = "Sun4d platform, SPARCserver 1000",
     .init = ss1000_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 8,
 };
 

@@ -1685,7 +1685,7 @@ static QEMUMachine ss2000_machine = {
     .name = "SS-2000",
     .desc = "Sun4d platform, SPARCcenter 2000",
     .init = ss2000_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 20,
 };
 

@@ -1861,7 +1861,7 @@ static QEMUMachine ss2_machine = {
     .name = "SS-2",
     .desc = "Sun4c platform, SPARCstation 2",
     .init = ss2_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void sun4m_register_types(void)

hw/versatilepb.c +2 -2

index 7b1b025..af5120f 100644
@@ -374,14 +374,14 @@ static QEMUMachine versatilepb_machine = {
     .name = "versatilepb",
     .desc = "ARM Versatile/PB (ARM926EJ-S)",
     .init = vpb_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine versatileab_machine = {
     .name = "versatileab",
     .desc = "ARM Versatile/AB (ARM926EJ-S)",
     .init = vab_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void versatile_machine_init(void)

hw/vexpress.c +2 -2

index 3596d1e..3c7c012 100644
@@ -495,7 +495,7 @@ static QEMUMachine vexpress_a9_machine = {
     .name = "vexpress-a9",
     .desc = "ARM Versatile Express for Cortex-A9",
     .init = vexpress_a9_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -503,7 +503,7 @@ static QEMUMachine vexpress_a15_machine = {
     .name = "vexpress-a15",
     .desc = "ARM Versatile Express for Cortex-A15",
     .init = vexpress_a15_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/xilinx_zynq.c +1 -1

index fd46ba2..c70eb69 100644
@@ -178,7 +178,7 @@ static QEMUMachine zynq_machine = {
     .name = "xilinx-zynq-a9",
     .desc = "Xilinx Zynq Platform Baseboard for Cortex-A9",
     .init = zynq_init,
-    .use_scsi = 1,
+    .if_default = IF_SCSI,
     .max_cpus = 1,
     .no_sdcard = 1
 };

vl.c +11 -9

index 5b357a3..6b1e546 100644
@@ -802,9 +802,9 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
 
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
-    int *use_scsi = opaque;
+    int *mach_if = opaque;
 
-    return drive_init(opts, *use_scsi) == NULL;
+    return drive_init(opts, *mach_if) == NULL;
 }
 
 static int drive_enable_snapshot(QemuOpts *opts, void *opaque)

@@ -815,14 +815,14 @@ static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
     return 0;
 }
 
-static void default_drive(int enable, int snapshot, int use_scsi,
+static void default_drive(int enable, int snapshot, int mach_if,
                           BlockInterfaceType type, int index,
                           const char *optstr)
 {
     QemuOpts *opts;
 
     if (type == IF_DEFAULT) {
-        type = use_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     if (!enable || drive_get_by_index(type, index)) {

@@ -833,7 +833,7 @@ static void default_drive(int enable, int snapshot, int use_scsi,
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
-    if (!drive_init(opts, use_scsi)) {
+    if (!drive_init(opts, mach_if)) {
         exit(1);
     }
 }

@@ -3547,14 +3547,16 @@ int main(int argc, char **argv, char **envp)
     /* open the virtual block devices */
     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->use_scsi, 1) != 0)
+    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
+                          &machine->mach_if, 1) != 0) {
         exit(1);
+    }
 
-    default_drive(default_cdrom, snapshot, machine->use_scsi,
+    default_drive(default_cdrom, snapshot, machine->mach_if,
                   IF_DEFAULT, 2, CDROM_OPTS);
-    default_drive(default_floppy, snapshot, machine->use_scsi,
+    default_drive(default_floppy, snapshot, machine->mach_if,
                   IF_FLOPPY, 0, FD_OPTS);
-    default_drive(default_sdcard, snapshot, machine->use_scsi,
+    default_drive(default_sdcard, snapshot, machine->mach_if,
                   IF_SD, 0, SD_OPTS);
 
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);

Signature · 1 line

1.7.1

Quoted 9 lines

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Quoted 5 lines

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

Kevin, could you review/ack this patch pls?

Quoted 27 lines

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +0 -0

index 99828ad..c9a49c8 100644
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
     return true;
 }
 

blockdev.c static bool do_check_io_limits(BlockIOLimit *io_limits) · line 278

Quoted 2 lines

blockdev.c +1 -1

-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, int mach_if)

Quoted 49 lines

blockdev.c +1 -1

 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +19 -0

index 5f27b64..8b126ad 100644
@@ -40,6 +40,22 @@ struct DriveInfo {
     int refcount;
 };
 
+/*
+ * Each qemu machine type defines a mach_if field for its default
+ * interface type. If its unspecified, we set it to IF_IDE.
+ */
+static inline int get_mach_if(int mach_if)
+{
+    assert(mach_if < IF_COUNT);
+    assert(mach_if >= IF_DEFAULT);
+
+    if ((mach_if == IF_NONE) || (mach_if == IF_DEFAULT)) {
+        return IF_IDE;
+    }
+
+    return mach_if;
+}
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

@@ -61,4 +77,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+
+
 #endif

hw/boards.h +0 -0

index a2e0a54..969fd67 100644

hw/boards.h typedef struct QEMUMachine { · line 23

Quoted 8 lines

hw/boards.h +1 -1

@@ -20,7 +20,7 @@ typedef struct QEMUMachine {
     const char *desc;
     QEMUMachineInitFunc *init;
     QEMUMachineResetFunc *reset;
-    int use_scsi;
+    int mach_if;

Am 22.10.2012 12:47, schrieb Michael S. Tsirkin:

Quoted 7 lines · Jason Baron

Quoted 6 lines

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

Quoted 3 lines

Quoted 3 lines

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Is this default mechanism necessary? Can't we make sure that each machine does define its preferred interface, and doesn't define it as IF_DEFAULT (which would be the same as an explicit IF_IDE anyway)?

Also, 'mach_if' isn't a very descriptive name. Something like 'default_drive_if' would be better.

Quoted 35 lines

Quoted 5 lines

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

Kevin, could you review/ack this patch pls?

Quoted 27 lines

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +0 -0

index 99828ad..c9a49c8 100644
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
     return true;
 }
 

Quoted 2 lines

blockdev.c static bool do_check_io_limits(BlockIOLimit *io_limits) · line 278

Quoted 2 lines

blockdev.c +1 -1

-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, int mach_if)

BlockInterfaceType, not int.

Quoted 49 lines

Quoted 49 lines

blockdev.c +1 -1

 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +19 -0

index 5f27b64..8b126ad 100644
@@ -40,6 +40,22 @@ struct DriveInfo {
     int refcount;
 };
 
+/*
+ * Each qemu machine type defines a mach_if field for its default
+ * interface type. If its unspecified, we set it to IF_IDE.
+ */
+static inline int get_mach_if(int mach_if)
+{
+    assert(mach_if < IF_COUNT);
+    assert(mach_if >= IF_DEFAULT);
+
+    if ((mach_if == IF_NONE) || (mach_if == IF_DEFAULT)) {
+        return IF_IDE;
+    }
+
+    return mach_if;
+}
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

@@ -61,4 +77,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+
+
 #endif

hw/boards.h +0 -0

index a2e0a54..969fd67 100644

Quoted 8 lines

hw/boards.h typedef struct QEMUMachine { · line 23

Quoted 8 lines

hw/boards.h +1 -1

@@ -20,7 +20,7 @@ typedef struct QEMUMachine {
     const char *desc;
     QEMUMachineInitFunc *init;
     QEMUMachineResetFunc *reset;
-    int use_scsi;
+    int mach_if;

Same here.

Kevin

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +0 -0

index 99828ad..c9a49c8 100644

blockdev.c +1 -1

 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +0 -0

index 5f27b64..8b126ad 100644

hw/boards.h +0 -0

     int max_cpus;
     unsigned int no_serial:1,
         no_parallel:1,

hw/device-hotplug.c +1 -1

index eec0fe3..33302f9 100644
@@ -39,7 +39,7 @@ DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->use_scsi);
+    dinfo = drive_init(opts, current_machine->mach_if);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;

hw/highbank.c +1 -1

index 11aa131..35cef06 100644
@@ -324,7 +324,7 @@ static QEMUMachine highbank_machine = {
     .name = "highbank",
     .desc = "Calxeda Highbank (ECX-1000)",
     .init = highbank_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/leon3.c +1 -1

index 7a9729d..cf9dcf8 100644
@@ -214,7 +214,7 @@ static QEMUMachine leon3_generic_machine = {
     .name     = "leon3_generic",
     .desc     = "Leon-3 generic",
     .init     = leon3_generic_hw_init,
-    .use_scsi = 0,
+    .mach_if = IF_DEFAULT,
 };
 
 static void leon3_machine_init(void)

hw/mips_jazz.c +2 -2

index db927f1..1c7a725 100644
@@ -325,14 +325,14 @@ static QEMUMachine mips_magnum_machine = {
     .name = "magnum",
     .desc = "MIPS Magnum",
     .init = mips_magnum_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine mips_pica61_machine = {
     .name = "pica61",
     .desc = "Acer Pica 61",
     .init = mips_pica61_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void mips_jazz_machine_init(void)

hw/pc_sysfw.c +1 -1

index b45f0ac..b8a03a6 100644
@@ -98,7 +98,7 @@ static void pc_fw_add_pflash_drv(void)
       return;
     }
 
-    drive_init(opts, machine->use_scsi);
+    drive_init(opts, machine->mach_if);
 }
 
 static void pc_system_flash_init(MemoryRegion *rom_memory,

hw/puv3.c +1 -1

index 43f7216..f68bb61 100644
@@ -120,7 +120,7 @@ static QEMUMachine puv3_machine = {
     .desc = "PKUnity Version-3 based on UniCore32",
     .init = puv3_init,
     .is_default = 1,
-    .use_scsi = 0,
+    .mach_if = IF_DEFAULT,
 };
 
 static void puv3_machine_init(void)

hw/realview.c +3 -3

index 19db4d0..7613f68 100644
@@ -382,14 +382,14 @@ static QEMUMachine realview_eb_machine = {
     .name = "realview-eb",
     .desc = "ARM RealView Emulation Baseboard (ARM926EJ-S)",
     .init = realview_eb_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine realview_eb_mpcore_machine = {
     .name = "realview-eb-mpcore",
     .desc = "ARM RealView Emulation Baseboard (ARM11MPCore)",
     .init = realview_eb_mpcore_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -403,7 +403,7 @@ static QEMUMachine realview_pbx_a9_machine = {
     .name = "realview-pbx-a9",
     .desc = "ARM RealView Platform Baseboard Explore for Cortex-A9",
     .init = realview_pbx_a9_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/spapr.c +1 -1

index 09b8e99..be8129e 100644
@@ -913,7 +913,7 @@ static QEMUMachine spapr_machine = {
     .reset = ppc_spapr_reset,
     .max_cpus = MAX_CPUS,
     .no_parallel = 1,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void spapr_machine_init(void)

hw/sun4m.c +12 -12

index a04b485..101d552 100644
@@ -1400,7 +1400,7 @@ static QEMUMachine ss5_machine = {
     .name = "SS-5",
     .desc = "Sun4m platform, SPARCstation 5",
     .init = ss5_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .is_default = 1,
 };
 

@@ -1408,7 +1408,7 @@ static QEMUMachine ss10_machine = {
     .name = "SS-10",
     .desc = "Sun4m platform, SPARCstation 10",
     .init = ss10_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1416,7 +1416,7 @@ static QEMUMachine ss600mp_machine = {
     .name = "SS-600MP",
     .desc = "Sun4m platform, SPARCserver 600MP",
     .init = ss600mp_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1424,7 +1424,7 @@ static QEMUMachine ss20_machine = {
     .name = "SS-20",
     .desc = "Sun4m platform, SPARCstation 20",
     .init = ss20_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -1432,35 +1432,35 @@ static QEMUMachine voyager_machine = {
     .name = "Voyager",
     .desc = "Sun4m platform, SPARCstation Voyager",
     .init = vger_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine ss_lx_machine = {
     .name = "LX",
     .desc = "Sun4m platform, SPARCstation LX",
     .init = ss_lx_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine ss4_machine = {
     .name = "SS-4",
     .desc = "Sun4m platform, SPARCstation 4",
     .init = ss4_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine scls_machine = {
     .name = "SPARCClassic",
     .desc = "Sun4m platform, SPARCClassic",
     .init = scls_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine sbook_machine = {
     .name = "SPARCbook",
     .desc = "Sun4m platform, SPARCbook",
     .init = sbook_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static const struct sun4d_hwdef sun4d_hwdefs[] = {

@@ -1677,7 +1677,7 @@ static QEMUMachine ss1000_machine = {
     .name = "SS-1000",
     .desc = "Sun4d platform, SPARCserver 1000",
     .init = ss1000_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 8,
 };
 

@@ -1685,7 +1685,7 @@ static QEMUMachine ss2000_machine = {
     .name = "SS-2000",
     .desc = "Sun4d platform, SPARCcenter 2000",
     .init = ss2000_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 20,
 };
 

@@ -1861,7 +1861,7 @@ static QEMUMachine ss2_machine = {
     .name = "SS-2",
     .desc = "Sun4c platform, SPARCstation 2",
     .init = ss2_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void sun4m_register_types(void)

hw/versatilepb.c +2 -2

index 7b1b025..af5120f 100644
@@ -374,14 +374,14 @@ static QEMUMachine versatilepb_machine = {
     .name = "versatilepb",
     .desc = "ARM Versatile/PB (ARM926EJ-S)",
     .init = vpb_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static QEMUMachine versatileab_machine = {
     .name = "versatileab",
     .desc = "ARM Versatile/AB (ARM926EJ-S)",
     .init = vab_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
 };
 
 static void versatile_machine_init(void)

hw/vexpress.c +2 -2

index 3596d1e..3c7c012 100644
@@ -495,7 +495,7 @@ static QEMUMachine vexpress_a9_machine = {
     .name = "vexpress-a9",
     .desc = "ARM Versatile Express for Cortex-A9",
     .init = vexpress_a9_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

@@ -503,7 +503,7 @@ static QEMUMachine vexpress_a15_machine = {
     .name = "vexpress-a15",
     .desc = "ARM Versatile Express for Cortex-A15",
     .init = vexpress_a15_init,
-    .use_scsi = 1,
+    .mach_if = IF_SCSI,
     .max_cpus = 4,
 };
 

hw/xilinx_zynq.c +0 -0

index fd46ba2..c70eb69 100644

vl.c +8 -6

                           BlockInterfaceType type, int index,
                           const char *optstr)
 {
     QemuOpts *opts;
 
     if (type == IF_DEFAULT) {
-        type = use_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     if (!enable || drive_get_by_index(type, index)) {

@@ -833,7 +833,7 @@ static void default_drive(int enable, int snapshot, int use_scsi,
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
-    if (!drive_init(opts, use_scsi)) {
+    if (!drive_init(opts, mach_if)) {
         exit(1);
     }
 }

@@ -3547,14 +3547,16 @@ int main(int argc, char **argv, char **envp)
     /* open the virtual block devices */
     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->use_scsi, 1) != 0)
+    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
+                          &machine->mach_if, 1) != 0) {
         exit(1);
+    }
 
-    default_drive(default_cdrom, snapshot, machine->use_scsi,
+    default_drive(default_cdrom, snapshot, machine->mach_if,
                   IF_DEFAULT, 2, CDROM_OPTS);
-    default_drive(default_floppy, snapshot, machine->use_scsi,
+    default_drive(default_floppy, snapshot, machine->mach_if,
                   IF_FLOPPY, 0, FD_OPTS);
-    default_drive(default_sdcard, snapshot, machine->use_scsi,
+    default_drive(default_sdcard, snapshot, machine->mach_if,
                   IF_SD, 0, SD_OPTS);
 
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);

Quoted 5 lines · Jason Baron

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

Even though q35's desire to default to IF_AHCI is driving this patch, generalizing the default interface type makes sense on its own. Even if we IF_AHCI should turn out to need more discussion.

Quoted 36 lines

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +0 -0

index 99828ad..c9a49c8 100644
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
     return true;
 }
 

blockdev.c static bool do_check_io_limits(BlockIOLimit *io_limits) · line 278

Quoted 2 lines

blockdev.c +1 -1

-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, int mach_if)

BlockInterfaceType mach_if

Suggest to rename mach_if to something more descriptive. Kevin's default_drive_if works for me.

Quoted 18 lines

blockdev.c +1 -1

 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +0 -0

index 5f27b64..8b126ad 100644

blockdev.h struct DriveInfo { · line 47

Quoted 8 lines

blockdev.h +5 -0

@@ -40,6 +40,22 @@ struct DriveInfo {
     int refcount;
 };
 
+/*
+ * Each qemu machine type defines a mach_if field for its default
+ * interface type. If its unspecified, we set it to IF_IDE.
+ */
+static inline int get_mach_if(int mach_if)

BlockInterfaceType mach_if, and return type.

blockdev.h struct DriveInfo { · line 58

Quoted 11 lines

blockdev.h +11 -0

+{
+    assert(mach_if < IF_COUNT);
+    assert(mach_if >= IF_DEFAULT);
+
+    if ((mach_if == IF_NONE) || (mach_if == IF_DEFAULT)) {
+        return IF_IDE;
+    }
+
+    return mach_if;
+}
+

I'm not sure we should map IF_NONE to IF_IDE.

get_mach_if() should return an interface type the board supports. In theory, we could have a board that doesn't define any controllers, but still lets the user define some with -device (say because the board sports a PCI bus). Then IF_NONE would be the only interface type that makes any sense, and therefore the only sensible value of get_mach_if().

If we drop the magic mapping of IF_NONE, only IF_DEFAULT is left, and that one's clearly marked "for use with drive_add() only". No real need for magic mapping then. Could drop get_mach_if() and use mach_if directly.

Quoted 48 lines · Markus Armbruster

Quoted 5 lines · Jason Baron

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

Even though q35's desire to default to IF_AHCI is driving this patch, generalizing the default interface type makes sense on its own. Even if we IF_AHCI should turn out to need more discussion.

Quoted 36 lines

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---
 blockdev.c          |    4 ++--
 blockdev.h          |   19 +++++++++++++++++++
 hw/boards.h         |    2 +-
 hw/device-hotplug.c |    2 +-
 hw/highbank.c       |    2 +-
 hw/leon3.c          |    2 +-
 hw/mips_jazz.c      |    4 ++--
 hw/pc_sysfw.c       |    2 +-
 hw/puv3.c           |    2 +-
 hw/realview.c       |    6 +++---
 hw/spapr.c          |    2 +-
 hw/sun4m.c          |   24 ++++++++++++------------
 hw/versatilepb.c    |    4 ++--
 hw/vexpress.c       |    4 ++--
 hw/xilinx_zynq.c    |    2 +-
 vl.c                |   20 +++++++++++---------
 16 files changed, 61 insertions(+), 40 deletions(-)

blockdev.c +0 -0

index 99828ad..c9a49c8 100644
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
     return true;
 }
 

Quoted 8 lines

blockdev.c static bool do_check_io_limits(BlockIOLimit *io_limits) · line 278

Quoted 2 lines

blockdev.c +1 -1

-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, int mach_if)

BlockInterfaceType mach_if

Suggest to rename mach_if to something more descriptive. Kevin's default_drive_if works for me.

ok.

Quoted 43 lines

Quoted 18 lines

blockdev.c +1 -1

 {
     const char *buf;
     const char *file = NULL;

@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             return NULL;
 	}
     } else {
-        type = default_to_scsi ? IF_SCSI : IF_IDE;
+        type = get_mach_if(mach_if);
     }
 
     max_devs = if_max_devs[type];

blockdev.h +0 -0

index 5f27b64..8b126ad 100644

blockdev.h struct DriveInfo { · line 47

Quoted 8 lines

blockdev.h +5 -0

@@ -40,6 +40,22 @@ struct DriveInfo {
     int refcount;
 };
 
+/*
+ * Each qemu machine type defines a mach_if field for its default
+ * interface type. If its unspecified, we set it to IF_IDE.
+ */
+static inline int get_mach_if(int mach_if)

BlockInterfaceType mach_if, and return type.

blockdev.h struct DriveInfo { · line 58

Quoted 11 lines

blockdev.h +11 -0

+{
+    assert(mach_if < IF_COUNT);
+    assert(mach_if >= IF_DEFAULT);
+
+    if ((mach_if == IF_NONE) || (mach_if == IF_DEFAULT)) {
+        return IF_IDE;
+    }
+
+    return mach_if;
+}
+

I'm not sure we should map IF_NONE to IF_IDE.

Quoted 6 lines

get_mach_if() should return an interface type the board supports. In theory, we could have a board that doesn't define any controllers, but still lets the user define some with -device (say because the board sports a PCI bus). Then IF_NONE would be the only interface type that makes any sense, and therefore the only sensible value of get_mach_if().

Ok, but no boards use IF_NONE in that sense currently. But planning for the future is good :)

Quoted 4 lines

If we drop the magic mapping of IF_NONE, only IF_DEFAULT is left, and that one's clearly marked "for use with drive_add() only". No real need for magic mapping then. Could drop get_mach_if() and use mach_if directly.

Meaning explicity set mach_if or default_drive_if to IF_IDE for all machine types that are currently either IF_NONE, IF_DEFAULT, or not explicitly defined?

From: Jason Baron <[email protected]>

The current QEMUMachine definition has a 'use_scsi' field to indicate if a machine type should use scsi by default. However, Q35 wants to use ahci by default. Thus, introdue a new field in the QEMUMachine defintion, mach_if.

This field should be initialized by the machine type to the default interface type which it wants to use (IF_SCSI, IF_AHCI, etc.). If no mach_if is defined, or it is set to 'IF_DEFAULT' or 'IF_NONE', we currently assume IF_IDE.

Please use 'static inline int get_mach_if(int mach_if)', when accesssing the new mach_if field.

hw/boards.h +1 -1

index a2e0a54..969fd67 100644
@@ -20,7 +20,7 @@ typedef struct QEMUMachine {
     const char *desc;
     QEMUMachineInitFunc *init;
     QEMUMachineResetFunc *reset;
-    int use_scsi;
+    int mach_if;
     int max_cpus;
     unsigned int no_serial:1,
         no_parallel:1,

hw/xilinx_zynq.c +0 -0

     .max_cpus = 1,
     .no_sdcard = 1
 };

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks. This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not result in the creating of an ahci disk. This is important, since we don't want to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

This patch also adds:

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +12 -1

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 
 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {
      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };
 
+int get_if_max_devs(BlockInterfaceType if_type)
+{
+    assert(if_type < IF_COUNT);
+    assert(if_type >= IF_DEFAULT);
+
+    return if_max_devs[if_type];
+}
+
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

@@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
     } else {
         /* no id supplied -> create one */
         dinfo->id = g_malloc0(32);
-        if (type == IF_IDE || type == IF_SCSI)
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         if (max_devs)
             snprintf(dinfo->id, 32, "%s%i%s%i",

@@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:

blockdev.h +2 -0

index 8b126ad..bbd1017 100644
@@ -21,6 +21,7 @@ typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 

@@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
     return mach_if;
 }
 
+int get_if_max_devs(BlockInterfaceType if_type);
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

hw/ide.h +6 -0

index 2db4079..0b7e000 100644
@@ -4,6 +4,7 @@
 #include "isa.h"
 #include "pci.h"
 #include "memory.h"
+#include "blockdev.h"
 
 #define MAX_IDE_DEVS	2
 

@@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 
 /* ide/core.c */
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
 void ide_drive_get(DriveInfo **hd, int max_bus);
+void ahci_drive_get(DriveInfo **hd, int max_bus);
+
+/* ide/ahci.c */
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
 
 #endif /* HW_IDE_H */

hw/ide/ahci.c +18 -0

index 68671bc..824b86f 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/sysbus.h>
+#include <blockdev.h>
 
 #include "monitor.h"
 #include "dma.h"

@@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+    DriveInfo *drive;
+
+    for (i = 0; i < dev->ahci.ports; i++) {
+        if (hd_table[i] == NULL) {
+            continue;
+        }
+        drive = hd_table[i];
+        assert(drive->type == IF_AHCI);
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i]);
+    }
+}

hw/ide/core.c +18 -5

index d683a8c..044da3c 100644
@@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 
-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)
 {
     int i;
+    int max_devs;
+
+    assert((type == IF_IDE) || type == IF_AHCI);
 
-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
         exit(1);
     }
-
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    max_devs = get_if_max_devs(type);
+    for (i = 0; i < max_bus * max_devs; i++) {
+        hd[i] = drive_get(type, i / max_devs, i % max_devs);
     }
 }
+
+void ide_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_IDE);
+}
+
+void ahci_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_AHCI);
+}

Signature · 1 line

1.7.1

Quoted 11 lines

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks. This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not result in the creating of an ahci disk. This is important, since we don't want to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

This patch also adds:

Quoted 8 lines

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

Kevin, could you review/ack this patch pls?

Quoted 18 lines

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +1 -0

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 57

Quoted 7 lines

blockdev.c +1 -0

 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {
      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 65

Quoted 7 lines

blockdev.c +7 -0

+int get_if_max_devs(BlockInterfaceType if_type)
+{
+    assert(if_type < IF_COUNT);
+    assert(if_type >= IF_DEFAULT);
+
+    return if_max_devs[if_type];
+}

Quoted 105 lines

blockdev.c +2 -1

 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

@@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
     } else {
         /* no id supplied -> create one */
         dinfo->id = g_malloc0(32);
-        if (type == IF_IDE || type == IF_SCSI)
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         if (max_devs)
             snprintf(dinfo->id, 32, "%s%i%s%i",

@@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:

blockdev.h +2 -0

index 8b126ad..bbd1017 100644
@@ -21,6 +21,7 @@ typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 

@@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
     return mach_if;
 }
 
+int get_if_max_devs(BlockInterfaceType if_type);
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

hw/ide.h +6 -0

index 2db4079..0b7e000 100644
@@ -4,6 +4,7 @@
 #include "isa.h"
 #include "pci.h"
 #include "memory.h"
+#include "blockdev.h"
 
 #define MAX_IDE_DEVS	2
 

@@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 
 /* ide/core.c */
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
 void ide_drive_get(DriveInfo **hd, int max_bus);
+void ahci_drive_get(DriveInfo **hd, int max_bus);
+
+/* ide/ahci.c */
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
 
 #endif /* HW_IDE_H */

hw/ide/ahci.c +18 -0

index 68671bc..824b86f 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/sysbus.h>
+#include <blockdev.h>
 
 #include "monitor.h"
 #include "dma.h"

@@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+    DriveInfo *drive;
+
+    for (i = 0; i < dev->ahci.ports; i++) {
+        if (hd_table[i] == NULL) {
+            continue;
+        }
+        drive = hd_table[i];
+        assert(drive->type == IF_AHCI);
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i]);
+    }
+}

hw/ide/core.c +0 -0

index d683a8c..044da3c 100644
@@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2344

Quoted 2 lines

hw/ide/core.c +1 -1

-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2349

Quoted 5 lines

hw/ide/core.c +3 -0

 {
     int i;
+    int max_devs;
+
+    assert((type == IF_IDE) || type == IF_AHCI);

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2352

Quoted 4 lines

hw/ide/core.c +1 -1

-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);

Quoted 12 lines

hw/ide/core.c +5 -3

         exit(1);
     }
-
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    max_devs = get_if_max_devs(type);
+    for (i = 0; i < max_bus * max_devs; i++) {
+        hd[i] = drive_get(type, i / max_devs, i % max_devs);
     }
 }
+
+void ide_drive_get(DriveInfo **hd, int max_bus)

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2369

Quoted 8 lines

hw/ide/core.c +8 -0

+{
+    ata_drive_get(hd, max_bus, IF_IDE);
+}
+
+void ahci_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_AHCI);
+}

Quoted 40 lines

Quoted 11 lines

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks. This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not result in the creating of an ahci disk. This is important, since we don't want to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

This patch also adds:

Quoted 8 lines

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

Kevin, could you review/ack this patch pls?

Quoted 18 lines

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +1 -0

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 

Quoted 7 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 57

Quoted 7 lines

blockdev.c +1 -0

 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {
      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };

What are the implications of this if we decided to add another AHCI controller which had a different number of ports? I suspect that a controller with less than 6 ports breaks when you add more drives than a single controller can handle, and one with more than 6 ports doesn't use up all of its ports before it adds another controller.

Markus?

Quoted 7 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 65

Quoted 7 lines

blockdev.c +7 -0

+int get_if_max_devs(BlockInterfaceType if_type)
+{
+    assert(if_type < IF_COUNT);
+    assert(if_type >= IF_DEFAULT);
+
+    return if_max_devs[if_type];
+}

if_max_devs has a specific obvious meaning within blockdev.c, but outside it's not as obvious. So this function could use a rename.

Quoted 106 lines

Quoted 106 lines

blockdev.c +2 -1

 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

@@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
     } else {
         /* no id supplied -> create one */
         dinfo->id = g_malloc0(32);
-        if (type == IF_IDE || type == IF_SCSI)
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         if (max_devs)
             snprintf(dinfo->id, 32, "%s%i%s%i",

@@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:

blockdev.h +2 -0

index 8b126ad..bbd1017 100644
@@ -21,6 +21,7 @@ typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 

@@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
     return mach_if;
 }
 
+int get_if_max_devs(BlockInterfaceType if_type);
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

hw/ide.h +6 -0

index 2db4079..0b7e000 100644
@@ -4,6 +4,7 @@
 #include "isa.h"
 #include "pci.h"
 #include "memory.h"
+#include "blockdev.h"
 
 #define MAX_IDE_DEVS	2
 

@@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 
 /* ide/core.c */
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
 void ide_drive_get(DriveInfo **hd, int max_bus);
+void ahci_drive_get(DriveInfo **hd, int max_bus);
+
+/* ide/ahci.c */
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);
 
 #endif /* HW_IDE_H */

hw/ide/ahci.c +18 -0

index 68671bc..824b86f 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/sysbus.h>
+#include <blockdev.h>
 
 #include "monitor.h"
 #include "dma.h"

@@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+    DriveInfo *drive;
+
+    for (i = 0; i < dev->ahci.ports; i++) {
+        if (hd_table[i] == NULL) {
+            continue;
+        }
+        drive = hd_table[i];
+        assert(drive->type == IF_AHCI);
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i]);
+    }
+}

hw/ide/core.c +0 -0

index d683a8c..044da3c 100644
@@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 

Quoted 2 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2344

Quoted 2 lines

hw/ide/core.c +1 -1

-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)

Could be static?

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2349

Quoted 5 lines

hw/ide/core.c +3 -0

 {
     int i;
+    int max_devs;
+
+    assert((type == IF_IDE) || type == IF_AHCI);

Interesting use of brackets.

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2352

Quoted 5 lines

hw/ide/core.c +1 -1

-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);

"Too many %s buses", type == IF_IDE ? "IDE" : "AHCI"

Quoted 15 lines

Quoted 15 lines

hw/ide/core.c +7 -3

         exit(1);
     }
-
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    max_devs = get_if_max_devs(type);
+    for (i = 0; i < max_bus * max_devs; i++) {
+        hd[i] = drive_get(type, i / max_devs, i % max_devs);
     }
 }
+
+void ide_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_IDE);

Quoted 6 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2369

Quoted 6 lines

hw/ide/core.c +6 -0

+}
+
+void ahci_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_AHCI);
+}

Kevin

Quoted 9 lines · Kevin Wolf

Quoted 9 lines

Quoted 9 lines

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks. This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not result in the creating of an ahci disk. This is important, since we don't want
to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

Quoted 31 lines

Quoted 31 lines

Quoted 10 lines

This patch also adds:

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

Kevin, could you review/ack this patch pls?

Quoted 18 lines

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +1 -0

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 

Quoted 2 lines

Quoted 2 lines

blockdev.c static const int if_max_devs[IF_COUNT] = {

Quoted 2 lines

blockdev.c +0 -0

 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const int if_max_devs[IF_COUNT] = {

Quoted 12 lines

Quoted 5 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 57

Quoted 5 lines

blockdev.c +1 -0

      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };

What are the implications of this if we decided to add another AHCI controller which had a different number of ports? I suspect that a controller with less than 6 ports breaks when you add more drives than a single controller can handle, and one with more than 6 ports doesn't use up all of its ports before it adds another controller.

Quoted 2 lines

Markus?

My plan was to make this field, machine dependent if/when we wanted a different size. I don't think it breaks anything to make this change at a later point. But please correct me, if I am wrong.

Quoted 10 lines

Quoted 7 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 65

Quoted 7 lines

blockdev.c +7 -0

+int get_if_max_devs(BlockInterfaceType if_type)
+{
+    assert(if_type < IF_COUNT);
+    assert(if_type >= IF_DEFAULT);
+
+    return if_max_devs[if_type];
+}

if_max_devs has a specific obvious meaning within blockdev.c, but outside it's not as obvious. So this function could use a rename.

ok.

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

blockdev.c +2 -1

 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

@@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
     } else {
         /* no id supplied -> create one */
         dinfo->id = g_malloc0(32);
-        if (type == IF_IDE || type == IF_SCSI)
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         if (max_devs)
             snprintf(dinfo->id, 32, "%s%i%s%i",

@@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:

blockdev.h +2 -0

index 8b126ad..bbd1017 100644
@@ -21,6 +21,7 @@ typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 

@@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
     return mach_if;
 }
 
+int get_if_max_devs(BlockInterfaceType if_type);
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

hw/ide.h +1 -0

index 2db4079..0b7e000 100644
@@ -4,6 +4,7 @@
 #include "isa.h"
 #include "pci.h"
 #include "memory.h"
+#include "blockdev.h"
 
 #define MAX_IDE_DEVS	2
 

@@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 

Quoted 7 lines

Quoted 7 lines

hw/ide.h int ide_get_geometry(BusState *bus, int unit, · line 43

Quoted 7 lines

hw/ide.h +5 -0

 /* ide/core.c */
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
 void ide_drive_get(DriveInfo **hd, int max_bus);
+void ahci_drive_get(DriveInfo **hd, int max_bus);
+
+/* ide/ahci.c */
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

hw/ide.h +0 -0

 #endif /* HW_IDE_H */

hw/ide/ahci.c +10 -0

index 68671bc..824b86f 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/sysbus.h>
+#include <blockdev.h>
 
 #include "monitor.h"
 #include "dma.h"

@@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+    DriveInfo *drive;
+
+    for (i = 0; i < dev->ahci.ports; i++) {
+        if (hd_table[i] == NULL) {

Quoted 8 lines

Quoted 8 lines

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1280

Quoted 8 lines

hw/ide/ahci.c +8 -0

+            continue;
+        }
+        drive = hd_table[i];
+        assert(drive->type == IF_AHCI);
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i]);
+    }
+}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

hw/ide/core.c +0 -0

index d683a8c..044da3c 100644
@@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 

Quoted 5 lines

Quoted 2 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2344

Quoted 2 lines

hw/ide/core.c +1 -1

-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)

Could be static?

Yes.

Quoted 7 lines

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2349

Quoted 5 lines

hw/ide/core.c +3 -0

 {
     int i;
+    int max_devs;
+
+    assert((type == IF_IDE) || type == IF_AHCI);

Interesting use of brackets.

oops.

Quoted 7 lines

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2352

Quoted 5 lines

hw/ide/core.c +1 -1

-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);

"Too many %s buses", type == IF_IDE ? "IDE" : "AHCI"

ok.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

hw/ide/core.c +5 -3

         exit(1);
     }
-
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    max_devs = get_if_max_devs(type);
+    for (i = 0; i < max_bus * max_devs; i++) {
+        hd[i] = drive_get(type, i / max_devs, i % max_devs);
     }
 }
+
+void ide_drive_get(DriveInfo **hd, int max_bus)

Quoted 8 lines

Quoted 8 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2369

Quoted 8 lines

hw/ide/core.c +8 -0

+{
+    ata_drive_get(hd, max_bus, IF_IDE);
+}
+
+void ahci_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_AHCI);
+}

Quoted 10 lines · Jason Baron

Quoted 9 lines · Kevin Wolf

Quoted 9 lines

Quoted 9 lines

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks. This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not result in the creating of an ahci disk. This is important, since we don't want
to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

This isn't the real argument for IF_AHCI. The real argument is that the (bus, unit) namespace for if=ide makes no sense for AHCI.

A board can have any number of IDE controllers. Each IDE controller provides one or two buses, and each bus takes up to two units. Together, we get a a board-specific number of buses, where each bus takes up to two units.

A board can have any number of AHCI controllers. Each AHCI controller provides a device-specific number of ports. Together, we get a board-specific number of buses, where each bus can takes a bus-specific number of units.

Plain q35 doesn't really need to differentiate between IDE and AHCI; it has only AHCI. What it needs is a "one bus with six units" namespace.

We could fix this by relaxing if=ide's rigid "two units per bus" for new machine types.

You fix it by introducing if=ahci. Probably simpler. But what's the plan for the next generation of controller? Yet another interface type?

Note that SCSI suffers from similar rigidity: seven units per bus. That's fine for SCSI-1, but not for some of the other variants.

"Differentiate" comes into play when a board sports both AHCI and IDE controllers. Which q35 doesn't, does it?

Then, the new if=ahci lets users select the kind of controller more easily than bus numbers would.

Drawback: existing command lines upgrade from pc to plain q35 gracefully only if they don't specify if=ide explicitly. Upgrading to some q35 variant with IDE controller on board is even worse: it works, but performance sucks. Some regard this as a feature.

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 10 lines

This patch also adds:

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

Kevin, could you review/ack this patch pls?

Quoted 18 lines

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +1 -0

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

blockdev.c static const int if_max_devs[IF_COUNT] = {

Quoted 2 lines

blockdev.c +0 -0

 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const 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. *
* All controllers used to imlement if=T drives need to support
* if_max_devs[T] units, for any T with if_max_devs[T] != 0.
* Otherwise, some index values map to "impossible" bus, unit
* values.
*
* For instance, if you change [IF_SCSI] to 255, -drive
* if=scsi,index=12 no longer means bus=1,unit=5, but
* bus=0,unit=12. With an lsi53c895a controller (7 units max),
* the drive can't be set up. Regression.

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 57

Quoted 5 lines

blockdev.c +1 -0

      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };

What are the implications of this if we decided to add another AHCI controller which had a different number of ports? I suspect that a controller with less than 6 ports breaks when you add more drives than a single controller can handle, and one with more than 6 ports doesn't use up all of its ports before it adds another controller.

Quoted 6 lines

Quoted 2 lines

Markus?

My plan was to make this field, machine dependent if/when we wanted a different size. I don't think it breaks anything to make this change at a later point. But please correct me, if I am wrong.

I'm afraid you are. See the comment immediately above, and commit 27d6bf40.

As far as I can see, the least bad solution is leaving if_max_devs[IF_AHCI] zero. Makes index=N an alias for unit=N,bus=0, and leaves rejecting invalid (bus,unit) to the board.

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 64

Quoted 7 lines

blockdev.c +7 -0

+int get_if_max_devs(BlockInterfaceType if_type)
+{
+    assert(if_type < IF_COUNT);
+    assert(if_type >= IF_DEFAULT);
+
+    return if_max_devs[if_type];
+}

if_max_devs has a specific obvious meaning within blockdev.c, but outside it's not as obvious. So this function could use a rename.

Quoted 1 line

ok.

No, get rid of it. if_max_devs[]'s purpose is mapping between index and (bus, unit), no more. See below.

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

blockdev.c +2 -1

 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.

@@ -518,7 +528,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
     } else {
         /* no id supplied -> create one */
         dinfo->id = g_malloc0(32);
-        if (type == IF_IDE || type == IF_SCSI)
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI)
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         if (max_devs)
             snprintf(dinfo->id, 32, "%s%i%s%i",

@@ -550,6 +560,7 @@ DriveInfo *drive_init(QemuOpts *opts, int mach_if)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:

blockdev.h +2 -0

index 8b126ad..bbd1017 100644
@@ -21,6 +21,7 @@ typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 

@@ -56,6 +57,7 @@ static inline int get_mach_if(int mach_if)
     return mach_if;
 }
 
+int get_if_max_devs(BlockInterfaceType if_type);
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);

hw/ide.h +1 -0

index 2db4079..0b7e000 100644
@@ -4,6 +4,7 @@
 #include "isa.h"
 #include "pci.h"
 #include "memory.h"
+#include "blockdev.h"
 
 #define MAX_IDE_DEVS	2
 

@@ -34,6 +35,11 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

hw/ide.h int ide_get_geometry(BusState *bus, int unit, · line 43

Quoted 7 lines

hw/ide.h +5 -0

 /* ide/core.c */
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type);
 void ide_drive_get(DriveInfo **hd, int max_bus);
+void ahci_drive_get(DriveInfo **hd, int max_bus);
+
+/* ide/ahci.c */
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table);

Shouldn't this go next to pci_ide_create_devs()'s declaration? Currently in hw/ide/pci.h.

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

hw/ide.h +0 -0

 #endif /* HW_IDE_H */

hw/ide/ahci.c +10 -0

index 68671bc..824b86f 100644
@@ -26,6 +26,7 @@
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/sysbus.h>
+#include <blockdev.h>
 
 #include "monitor.h"
 #include "dma.h"

@@ -1260,3 +1261,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)
+{
+    struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, pci_dev);
+    int i;
+    DriveInfo *drive;
+
+    for (i = 0; i < dev->ahci.ports; i++) {
+        if (hd_table[i] == NULL) {

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

hw/ide/ahci.c static void sysbus_ahci_register_types(void) · line 1280

Quoted 8 lines

hw/ide/ahci.c +8 -0

+            continue;
+        }
+        drive = hd_table[i];
+        assert(drive->type == IF_AHCI);
+        ide_create_drive(&dev->ahci.dev[i].port, 0,
+                         hd_table[i]);
+    }
+}

Shouldn't this go next to pci_ide_create_devs()'s definition? Currently in hw/ide/pci.c.

Consider adding a parameter for the number of elements in hd_table[], for robustness. As is, the caller has to ensure hd_table[] has at least dev->ahci.ports elements, which is not obvious from the function signature.

Quoted 49 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

hw/ide/core.c +0 -0

index d683a8c..044da3c 100644
@@ -2341,16 +2341,29 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 

Quoted 5 lines

Quoted 2 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2344

Quoted 2 lines

hw/ide/core.c +1 -1

-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ata_drive_get(DriveInfo **hd, int max_bus, BlockInterfaceType type)

Could be static?

Yes.

Quoted 7 lines

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2349

Quoted 5 lines

hw/ide/core.c +3 -0

 {
     int i;
+    int max_devs;
+
+    assert((type == IF_IDE) || type == IF_AHCI);

Interesting use of brackets.

oops.

Quoted 7 lines

Quoted 5 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2351

Quoted 5 lines

hw/ide/core.c +1 -1

-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);

"Too many %s buses", type == IF_IDE ? "IDE" : "AHCI"

ok.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

hw/ide/core.c +5 -3

         exit(1);
     }
-
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    max_devs = get_if_max_devs(type);
+    for (i = 0; i < max_bus * max_devs; i++) {
+        hd[i] = drive_get(type, i / max_devs, i % max_devs);
     }
 }
+
+void ide_drive_get(DriveInfo **hd, int max_bus)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

hw/ide/core.c const VMStateDescription vmstate_ide_bus = { · line 2368

Quoted 8 lines

hw/ide/core.c +8 -0

+{
+    ata_drive_get(hd, max_bus, IF_IDE);
+}
+
+void ahci_drive_get(DriveInfo **hd, int max_bus)
+{
+    ata_drive_get(hd, max_bus, IF_AHCI);
+}

For IDE, we always have two devices per bus. The boards number of buses varies in theory, and is always two in practice. Thus we have "#define MAX_IDE_DEVS 2" in ide.h, and "#define MAX_IDE_BUS 2" in all the <board>.c. The latter is passed to ide_drive_get() as parameter. Getting the well-known MAX_IDE_DEVS from get_if_max_devs() buys us nothing.

For AHCI, I figure we have a device-specific number of ports per device, and a board-specific number of devices (typically one). If there are multiple devices, they don't necessarily sport the same number of ports. Thus, a nested loop does not work.

Suggest

void ahci_drive_get(DriveInfo **hd, int bus, int max_ports)

which the board can call once for each device, with the device's true number of ports as argument. Look ma, no get_if_max_devs()!

Quoted 28 lines · Markus Armbruster

Quoted 12 lines · Jason Baron

Quoted 11 lines · Kevin Wolf

Quoted 11 lines

Quoted 11 lines

From: Jason Baron <[email protected]>

Introduce IF_AHCI so that q35 can differentiate between ide and ahci disks.
This allows q35 to specify its default disk type. It also allows q35 to differentiate between ahci and ide disks, such that -drive if=ide does not
result in the creating of an ahci disk. This is important, since we don't want
to have the meaning of if=ide changing once q35 is introduced. Thus, its important for this to be applied before we introduce q35.

This isn't the real argument for IF_AHCI. The real argument is that the (bus, unit) namespace for if=ide makes no sense for AHCI.

A board can have any number of IDE controllers. Each IDE controller provides one or two buses, and each bus takes up to two units. Together, we get a a board-specific number of buses, where each bus takes up to two units.

A board can have any number of AHCI controllers. Each AHCI controller provides a device-specific number of ports. Together, we get a board-specific number of buses, where each bus can takes a bus-specific number of units.

Quoted 8 lines

Plain q35 doesn't really need to differentiate between IDE and AHCI; it has only AHCI. What it needs is a "one bus with six units" namespace.

We could fix this by relaxing if=ide's rigid "two units per bus" for new machine types.

You fix it by introducing if=ahci. Probably simpler. But what's the plan for the next generation of controller? Yet another interface type?

Next generation might require a new interface type. But taking you're suggestions, if=ahci is now machine dependent, so it shouldn't be so bad (see incremental patch at end).

Quoted 1 line

Quoted 5 lines

Note that SCSI suffers from similar rigidity: seven units per bus. That's fine for SCSI-1, but not for some of the other variants.

"Differentiate" comes into play when a board sports both AHCI and IDE controllers. Which q35 doesn't, does it?

Yes, there is no ide controller by default. That said i've used the piix-ide controller to install windows xp, for example, on q35.

Quoted 8 lines

Then, the new if=ahci lets users select the kind of controller more easily than bus numbers would.

Drawback: existing command lines upgrade from pc to plain q35 gracefully only if they don't specify if=ide explicitly. Upgrading to some q35 variant with IDE controller on board is even worse: it works, but performance sucks. Some regard this as a feature.

well if=ide could give a warning on q35, suggesting using if=ahci, if we're not going to create an ide controller for if=ide (which I wasn't intending).

Quoted 69 lines

Quoted 31 lines

Quoted 31 lines

Quoted 31 lines

Quoted 10 lines

This patch also adds:

pci_ahci_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table)

Which provides a convient way of attaching ahci drives to an ahci controller.

Reviewed-by Paolo Bonzini <[email protected]>

Signed-off-by Jason Baron <[email protected]>

---

Kevin, could you review/ack this patch pls?

Quoted 18 lines

 blockdev.c    |   13 ++++++++++++-
 blockdev.h    |    2 ++
 hw/ide.h      |    6 ++++++
 hw/ide/ahci.c |   18 ++++++++++++++++++
 hw/ide/core.c |   23 ++++++++++++++++++-----
 5 files changed, 56 insertions(+), 6 deletions(-)

blockdev.c +1 -0

index c9a49c8..b684348 100644
@@ -33,6 +33,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

blockdev.c static const int if_max_devs[IF_COUNT] = {

Quoted 2 lines

blockdev.c +0 -0

 static const int if_max_devs[IF_COUNT] = {

@@ -52,8 +53,17 @@ static const 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. *
* All controllers used to imlement if=T drives need to support
* if_max_devs[T] units, for any T with if_max_devs[T] != 0.
* Otherwise, some index values map to "impossible" bus, unit
* values.
*
* For instance, if you change [IF_SCSI] to 255, -drive
* if=scsi,index=12 no longer means bus=1,unit=5, but
* bus=0,unit=12. With an lsi53c895a controller (7 units max),
* the drive can't be set up. Regression.

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

blockdev.c static const int if_max_devs[IF_COUNT] = { · line 57

Quoted 5 lines

blockdev.c +1 -0

      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 6,
 };

What are the implications of this if we decided to add another AHCI controller which had a different number of ports? I suspect that a controller with less than 6 ports breaks when you add more drives than a single controller can handle, and one with more than 6 ports doesn't use up all of its ports before it adds another controller.

Quoted 7 lines

Quoted 2 lines

Markus?

My plan was to make this field, machine dependent if/when we wanted a different
size. I don't think it breaks anything to make this change at a later point. But please correct me, if I am wrong.

I'm afraid you are. See the comment immediately above, and commit 27d6bf40.