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

[Kevin Wolf <kwolf@redhat.com>]

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