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

[Jason Baron <jbaron@redhat.com>]

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