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

[Marcel Apfelbaum]

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

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

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

I hope it helped,
Marcel

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