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

[Markus Armbruster]

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?

> @@ -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) {