Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property
Marcel Apfelbaum writes: > On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote: >> John Snow writes: >> >> > Signed-off-by: John Snow [...] >> > @@ -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? > :) ) Okay %-) >- 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. Artifact of this hackery: two identical static functions: vl.c's machine_class_init() and pc.c's pc_generic_machine_class_init(). Trap for the unwary, and it caught John. Unless there are plans to get rid of pc_generic_machine_class_init() fairly soon, I'd recommend to give machine_class_init() external linkage with a suitable comment, and drop pc_generic_machine_class_init(). > I hope it helped, Sure did! I checked the CodeTransitions Wiki page. It covers this work, and points to http://wiki.qemu.org/Features/QOM/Machine for more information. Good.
Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property
On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote:
> John Snow writes:
>
> > Signed-off-by: John Snow
> > ---
> > 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;
> > GlobalPropert
Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property
John Snow writes:
> Signed-off-by: John Snow
> ---
> 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
> in
[Qemu-devel] [RFC v2 2/3] Add units-per-idebus property
Signed-off-by: John Snow
---
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];
+}
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
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;
@@ -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);
};
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,
BlockIn
