[v3,01/26] blockdev: Introduce a default machine blockdev interface field, QEMUMachine->mach_if

From: Jason Baron <jbaron@redhat.com>

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 <pbonzini@redhat.com>
Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 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(-)

On Fri, Oct 19, 2012 at 04:43:26PM -0400, Jason Baron wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> 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 <pbonzini@redhat.com>
> Signed-off-by: Jason Baron <jbaron@redhat.com>

Kevin, could you review/ack this patch pls?


> ---
>  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(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 99828ad..c9a49c8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -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];
> diff --git a/blockdev.h b/blockdev.h
> index 5f27b64..8b126ad 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -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
> diff --git a/hw/boards.h b/hw/boards.h
> index a2e0a54..969fd67 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -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,
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index eec0fe3..33302f9 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -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;
> diff --git a/hw/highbank.c b/hw/highbank.c
> index 11aa131..35cef06 100644
> --- a/hw/highbank.c
> +++ b/hw/highbank.c
> @@ -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,
>  };
>  
> diff --git a/hw/leon3.c b/hw/leon3.c
> index 7a9729d..cf9dcf8 100644
> --- a/hw/leon3.c
> +++ b/hw/leon3.c
> @@ -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)
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index db927f1..1c7a725 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -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)
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..b8a03a6 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -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,
> diff --git a/hw/puv3.c b/hw/puv3.c
> index 43f7216..f68bb61 100644
> --- a/hw/puv3.c
> +++ b/hw/puv3.c
> @@ -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)
> diff --git a/hw/realview.c b/hw/realview.c
> index 19db4d0..7613f68 100644
> --- a/hw/realview.c
> +++ b/hw/realview.c
> @@ -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,
>  };
>  
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 09b8e99..be8129e 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -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)
> diff --git a/hw/sun4m.c b/hw/sun4m.c
> index a04b485..101d552 100644
> --- a/hw/sun4m.c
> +++ b/hw/sun4m.c
> @@ -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)
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 7b1b025..af5120f 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -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)
> diff --git a/hw/vexpress.c b/hw/vexpress.c
> index 3596d1e..3c7c012 100644
> --- a/hw/vexpress.c
> +++ b/hw/vexpress.c
> @@ -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,
>  };
>  
> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
> index fd46ba2..c70eb69 100644
> --- a/hw/xilinx_zynq.c
> +++ b/hw/xilinx_zynq.c
> @@ -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
>  };
> diff --git a/vl.c b/vl.c
> index 5b357a3..6b1e546 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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);
> -- 
> 1.7.1

Am 22.10.2012 12:47, schrieb Michael S. Tsirkin:
> On Fri, Oct 19, 2012 at 04:43:26PM -0400, Jason Baron wrote:
>> From: Jason Baron <jbaron@redhat.com>
>>
>> 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.

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.

>> Please use 'static inline int get_mach_if(int mach_if)', when accesssing the
>> new mach_if field.
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Jason Baron <jbaron@redhat.com>
> 
> Kevin, could you review/ack this patch pls?
> 
>> ---
>>  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(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 99828ad..c9a49c8 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -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)

BlockInterfaceType, not int.

>>  {
>>      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];
>> diff --git a/blockdev.h b/blockdev.h
>> index 5f27b64..8b126ad 100644
>> --- a/blockdev.h
>> +++ b/blockdev.h
>> @@ -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
>> diff --git a/hw/boards.h b/hw/boards.h
>> index a2e0a54..969fd67 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -20,7 +20,7 @@ typedef struct QEMUMachine {
>>      const char *desc;
>>      QEMUMachineInitFunc *init;
>>      QEMUMachineResetFunc *reset;
>> -    int use_scsi;
>> +    int mach_if;

Same here.

Kevin

On Mon, Oct 22, 2012 at 01:26:29PM +0200, Kevin Wolf wrote:
> Am 22.10.2012 12:47, schrieb Michael S. Tsirkin:
> > On Fri, Oct 19, 2012 at 04:43:26PM -0400, Jason Baron wrote:
> >> From: Jason Baron <jbaron@redhat.com>
> >>
> >> 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.
> 
> 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)?
> 

IF_NONE is currently defined as 0, so I wanted to make sure that any
machine types that didn't explicity define the 'mach_if' field would be
mapped to IF_IDE. If you don't like having 'IF_DEFAULT' there, I can
simply drop 2 'IF_DEFAULT' settings that I had. Would that be ok with
you?

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

ok, will update.

> >> Please use 'static inline int get_mach_if(int mach_if)', when accesssing the
> >> new mach_if field.
> >>
> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Jason Baron <jbaron@redhat.com>
> > 
> > Kevin, could you review/ack this patch pls?
> > 
> >> ---
> >>  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(-)
> >>
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 99828ad..c9a49c8 100644
> >> --- a/blockdev.c
> >> +++ b/blockdev.c
> >> @@ -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)
> 
> BlockInterfaceType, not int.
> 

ok.

> >>  {
> >>      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];
> >> diff --git a/blockdev.h b/blockdev.h
> >> index 5f27b64..8b126ad 100644
> >> --- a/blockdev.h
> >> +++ b/blockdev.h
> >> @@ -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
> >> diff --git a/hw/boards.h b/hw/boards.h
> >> index a2e0a54..969fd67 100644
> >> --- a/hw/boards.h
> >> +++ b/hw/boards.h
> >> @@ -20,7 +20,7 @@ typedef struct QEMUMachine {
> >>      const char *desc;
> >>      QEMUMachineInitFunc *init;
> >>      QEMUMachineResetFunc *reset;
> >> -    int use_scsi;
> >> +    int mach_if;
> 
> Same here.
> 
> Kevin

ok.

Thanks,

-Jason

Jason Baron <jbaron@redhat.com> writes:

> From: Jason Baron <jbaron@redhat.com>
>
> 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.

> 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 <pbonzini@redhat.com>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  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(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 99828ad..c9a49c8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -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)

BlockInterfaceType mach_if

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

>  {
>      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];
> diff --git a/blockdev.h b/blockdev.h
> index 5f27b64..8b126ad 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -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.

> +{
> +    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.

>  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

Unnecessary whitespace change, suggest to drop hunk.

> diff --git a/hw/boards.h b/hw/boards.h
> index a2e0a54..969fd67 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -20,7 +20,7 @@ typedef struct QEMUMachine {
>      const char *desc;
>      QEMUMachineInitFunc *init;
>      QEMUMachineResetFunc *reset;
> -    int use_scsi;
> +    int mach_if;

BlockInterfaceType mach_if

>      int max_cpus;
>      unsigned int no_serial:1,
>          no_parallel:1,
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index eec0fe3..33302f9 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -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;
> diff --git a/hw/highbank.c b/hw/highbank.c
> index 11aa131..35cef06 100644
> --- a/hw/highbank.c
> +++ b/hw/highbank.c
> @@ -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,
>  };
>  
> diff --git a/hw/leon3.c b/hw/leon3.c
> index 7a9729d..cf9dcf8 100644
> --- a/hw/leon3.c
> +++ b/hw/leon3.c
> @@ -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)
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index db927f1..1c7a725 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -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)
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..b8a03a6 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -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,
> diff --git a/hw/puv3.c b/hw/puv3.c
> index 43f7216..f68bb61 100644
> --- a/hw/puv3.c
> +++ b/hw/puv3.c
> @@ -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)
> diff --git a/hw/realview.c b/hw/realview.c
> index 19db4d0..7613f68 100644
> --- a/hw/realview.c
> +++ b/hw/realview.c
> @@ -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,
>  };
>  
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 09b8e99..be8129e 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -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)
> diff --git a/hw/sun4m.c b/hw/sun4m.c
> index a04b485..101d552 100644
> --- a/hw/sun4m.c
> +++ b/hw/sun4m.c
> @@ -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)
> diff --git a/hw/versatilepb.c b/hw/versatilepb.c
> index 7b1b025..af5120f 100644
> --- a/hw/versatilepb.c
> +++ b/hw/versatilepb.c
> @@ -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)
> diff --git a/hw/vexpress.c b/hw/vexpress.c
> index 3596d1e..3c7c012 100644
> --- a/hw/vexpress.c
> +++ b/hw/vexpress.c
> @@ -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,
>  };
>  
> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
> index fd46ba2..c70eb69 100644
> --- a/hw/xilinx_zynq.c
> +++ b/hw/xilinx_zynq.c
> @@ -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
>  };

Didn't check you covered all boards.

> diff --git a/vl.c b/vl.c
> index 5b357a3..6b1e546 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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;

BlockInterfaceType *mach_if

>  
> -    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 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);

On Wed, Oct 24, 2012 at 03:12:36PM +0200, Markus Armbruster wrote:
> Jason Baron <jbaron@redhat.com> writes:
> 
> > From: Jason Baron <jbaron@redhat.com>
> >
> > 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.
> 
> > 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 <pbonzini@redhat.com>
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  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(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 99828ad..c9a49c8 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -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)
> 
> BlockInterfaceType mach_if
> 
> Suggest to rename mach_if to something more descriptive.  Kevin's
> default_drive_if works for me.
> 

ok.

> 
> >  {
> >      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];
> > diff --git a/blockdev.h b/blockdev.h
> > index 5f27b64..8b126ad 100644
> > --- a/blockdev.h
> > +++ b/blockdev.h
> > @@ -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.
> 
> > +{
> > +    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().
> 

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

> 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?

Thanks,

-Jason

Jason Baron <jbaron@redhat.com> writes:

> From: Jason Baron <jbaron@redhat.com>
>
> 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.
[...]
> diff --git a/hw/boards.h b/hw/boards.h
> index a2e0a54..969fd67 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -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,
[...]
> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
> index fd46ba2..c70eb69 100644
> --- a/hw/xilinx_zynq.c
> +++ b/hw/xilinx_zynq.c
> @@ -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,

I doubt this compiles, and if it does, the compiler is mean to you :)

>      .max_cpus = 1,
>      .no_sdcard = 1
>  };
[...]

Jason Baron <jbaron@redhat.com> writes:

> On Wed, Oct 24, 2012 at 03:12:36PM +0200, Markus Armbruster wrote:
>> Jason Baron <jbaron@redhat.com> writes:
>> 
>> > From: Jason Baron <jbaron@redhat.com>
>> >
>> > 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.
>> 
>> > 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 <pbonzini@redhat.com>
>> > Signed-off-by: Jason Baron <jbaron@redhat.com>
>> > ---
>> >  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(-)
>> >
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 99828ad..c9a49c8 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -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)
>> 
>> BlockInterfaceType mach_if
>> 
>> Suggest to rename mach_if to something more descriptive.  Kevin's
>> default_drive_if works for me.
>> 
>
> ok.
>
>> 
>> >  {
>> >      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];
>> > diff --git a/blockdev.h b/blockdev.h
>> > index 5f27b64..8b126ad 100644
>> > --- a/blockdev.h
>> > +++ b/blockdev.h
>> > @@ -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.
>> 
>> > +{
>> > +    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().
>> 
>
> Ok, but no boards use IF_NONE in that sense currently. But planning for
> the future is good :)
>
>> 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?

Yes.

I count 97 machine types.  24 have .use_scsi = 1.  Two have explicit
.use_scsi = 0, and the remaining 71 have it implicitly.  I very much
doubt these 73 all sport IDE controllers.  Last time I checked[*], 41
machine types supported CD-ROM drives.  Makes me estimate we have ~55
machine types that have neither onboard SCSI nor IDE.

What happens when .use_scsi = 0 and the board doesn't provide IDE?
-drive without if= defines a block backend, but no frontend.  Just like
if=none, except for the (misleading) default ID.  Likewise, when
.use_scsi = 1 and the board doesn't provide SCSI.

You convert the 24 .use_scsi = 1 to .mach_if = IF_SCSI.  Fair enough.
But I think it should be changed to a more suitable value for machine
types that don't actually provide SCSI.  Suspect highbank is an example.
Can be done as followup patch; your choice.

You convert the two explicit .use_scsi = 0 (leon3_generic and puv3.c) to
IF_DEFAULT, effectively IF_IDE.  In both cases, the machine doesn't
actually provide an IDE controller as far as I can tell.  Again,
changing it to a more suitable value would make sense.  Followup patch,
or drop the .use_scsi = 0 in a preparatory patch, or convert to "no
initializer" to fold this case into the next one, or whatever else works
for you.

You leave the 71 without a .use_scsi initializer alone, which results in
IF_NONE, effectively IF_IDE.  Fair enough.  Again, it should be changed
to a more suitable value for the machine types that don't provide IDE.
If you drop the magic mapping, IF_NONE means IF_NONE, which may be a
more suitable value for the ones that don't provide IDE.  So you'd have
to add .mach_if = IF_IDE only to the minority that does provide IDE, and
leave the remaining ~55 ones alone.

Not so bad, isn't it?

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