Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-21 Thread Peter Xu 
On Mon, Jun 19, 2017 at 11:09:58AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> >> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> >> > Peter Xu  writes:
> >> > 
> >> > > Let the old man "MigrationState" join the object family. Direct benefit
> >> > > is that we can start to use all the property features derived from
> >> > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> >> > > parameters (so will never need to set them up each time using HMP/QMP,
> >> > > this is really, really attractive for test writters), etc.
> >> > >
> >> > > I see no reason to disallow this happen yet. So let's start from this
> >> > > one, to see whether it would be anything good.
> >> > >
> >> > > No functional change at all.
> >> > >
> >> > > Signed-off-by: Peter Xu 
> >> > > ---
> >> > >  include/migration/migration.h | 19 ++
> >> > >  migration/migration.c | 61 
> >> > > ---
> >> > >  2 files changed, 59 insertions(+), 21 deletions(-)
> >> > >
> >> > > diff --git a/include/migration/migration.h 
> >> > > b/include/migration/migration.h
> >> > > index 79b5484..bd0186c 100644
> >> > > --- a/include/migration/migration.h
> >> > > +++ b/include/migration/migration.h
> >> > > @@ -21,6 +21,7 @@
> >> > >  #include "qapi-types.h"
> >> > >  #include "exec/cpu-common.h"
> >> > >  #include "qemu/coroutine_int.h"
> >> > > +#include "hw/qdev.h"
> >> > >  
> >> > >  #define QEMU_VM_FILE_MAGIC   0x5145564d
> >> > >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> >> > > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> >> > >  MIG_RP_MSG_MAX
> >> > >  };
> >> > >  
> >> > > +#define TYPE_MIGRATION "migration"
> >> > > +
> >> > >  /* State for the incoming migration */
> >> > >  struct MigrationIncomingState {
> >> > >  QEMUFile *from_src_file;
> >> > > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> >> > >  MigrationIncomingState *migration_incoming_get_current(void);
> >> > >  void migration_incoming_state_destroy(void);
> >> > >  
> >> > > +#define MIGRATION_CLASS(klass) \
> >> > > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> >> > > +#define MIGRATION_OBJ(obj) \
> >> > > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> >> > > +#define MIGRATION_GET_CLASS(obj) \
> >> > > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> >> > > +
> >> > > +typedef struct MigrationClass {
> >> > > +/*< private >*/
> >> > > +DeviceClass parent_class;
> >> > > +} MigrationClass;
> >> > > +
> >> > >  struct MigrationState
> >> > >  {
> >> > > +/*< private >*/
> >> > > +DeviceState parent_obj;
> >> > > +
> >> > > +/*< public >*/
> >> > 
> >> > Turning MigrationState into a QOM object so you can use QOM
> >> > infrastructure makes sense.  But why turn it into a device?  It doesn't
> >> > feel device-like to me.  Would ObjectClass and Object instead of
> >> > DeviceClass and DeviceState do?
> >> 
> >> Yeah. That's the main reason for the series to be (was) RFC.
> >> 
> >> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> >> thing (IIUC you got that already :). I am just curious about why that
> >> is not for QObject from the very beginning, then it'll be easier.
> >> 
> >> For now, IMHO QDev is okay as well for migration, as long as it's kept
> >> internally inside QEMU. But sure at least I should turn user_creatable
> >> to off. I'll investigate more to see how to make this a safer approach
> >> in next post.
> >
> > We could allow non-device QOM objects use the global property
> > system optionally.
> >
> > We could make qdev_prop_set_globals() work on any Object*, and
> > let QOM classes call it on post_init if they want to be
> > configurable by global properties too.
> >
> > Note that this would break qdev_prop_check_globals(), because it
> > expects globals to work only on TYPE_DEVICE.  We could address
> > that by introducing a new interface type to indicate the type
> > works with -global (something like
> > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> >
> > Such a system would probably allow us to replace
> > default_machine_opts, default_boot_order, default_display,
> > default_ram_size (and probably many other compat fields) on
> > MachineClass.  It could also be used to implement -machine and
> > -accel options by simply translating them to global properties
> > (like we already do for -cpu).
> >
> > (This sounds like reinventing a QemuOpts-like system on top of
> > global properties.  Maybe that's a bad thing, maybe that's a good
> > thing.  I'm not sure.)
> 
> Not sure I get the connection to QemuOpts.
> 
> Lifting the "adjust property default values on the command line" feature
> from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense.
> 
> But let's reconsider what exactly we're

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-19 Thread Markus Armbruster 
Eduardo Habkost  writes:

> On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
>> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
>> > Peter Xu  writes:
>> > 
>> > > Let the old man "MigrationState" join the object family. Direct benefit
>> > > is that we can start to use all the property features derived from
>> > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
>> > > parameters (so will never need to set them up each time using HMP/QMP,
>> > > this is really, really attractive for test writters), etc.
>> > >
>> > > I see no reason to disallow this happen yet. So let's start from this
>> > > one, to see whether it would be anything good.
>> > >
>> > > No functional change at all.
>> > >
>> > > Signed-off-by: Peter Xu 
>> > > ---
>> > >  include/migration/migration.h | 19 ++
>> > >  migration/migration.c | 61 
>> > > ---
>> > >  2 files changed, 59 insertions(+), 21 deletions(-)
>> > >
>> > > diff --git a/include/migration/migration.h 
>> > > b/include/migration/migration.h
>> > > index 79b5484..bd0186c 100644
>> > > --- a/include/migration/migration.h
>> > > +++ b/include/migration/migration.h
>> > > @@ -21,6 +21,7 @@
>> > >  #include "qapi-types.h"
>> > >  #include "exec/cpu-common.h"
>> > >  #include "qemu/coroutine_int.h"
>> > > +#include "hw/qdev.h"
>> > >  
>> > >  #define QEMU_VM_FILE_MAGIC   0x5145564d
>> > >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
>> > > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
>> > >  MIG_RP_MSG_MAX
>> > >  };
>> > >  
>> > > +#define TYPE_MIGRATION "migration"
>> > > +
>> > >  /* State for the incoming migration */
>> > >  struct MigrationIncomingState {
>> > >  QEMUFile *from_src_file;
>> > > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
>> > >  MigrationIncomingState *migration_incoming_get_current(void);
>> > >  void migration_incoming_state_destroy(void);
>> > >  
>> > > +#define MIGRATION_CLASS(klass) \
>> > > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
>> > > +#define MIGRATION_OBJ(obj) \
>> > > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
>> > > +#define MIGRATION_GET_CLASS(obj) \
>> > > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
>> > > +
>> > > +typedef struct MigrationClass {
>> > > +/*< private >*/
>> > > +DeviceClass parent_class;
>> > > +} MigrationClass;
>> > > +
>> > >  struct MigrationState
>> > >  {
>> > > +/*< private >*/
>> > > +DeviceState parent_obj;
>> > > +
>> > > +/*< public >*/
>> > 
>> > Turning MigrationState into a QOM object so you can use QOM
>> > infrastructure makes sense.  But why turn it into a device?  It doesn't
>> > feel device-like to me.  Would ObjectClass and Object instead of
>> > DeviceClass and DeviceState do?
>> 
>> Yeah. That's the main reason for the series to be (was) RFC.
>> 
>> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
>> thing (IIUC you got that already :). I am just curious about why that
>> is not for QObject from the very beginning, then it'll be easier.
>> 
>> For now, IMHO QDev is okay as well for migration, as long as it's kept
>> internally inside QEMU. But sure at least I should turn user_creatable
>> to off. I'll investigate more to see how to make this a safer approach
>> in next post.
>
> We could allow non-device QOM objects use the global property
> system optionally.
>
> We could make qdev_prop_set_globals() work on any Object*, and
> let QOM classes call it on post_init if they want to be
> configurable by global properties too.
>
> Note that this would break qdev_prop_check_globals(), because it
> expects globals to work only on TYPE_DEVICE.  We could address
> that by introducing a new interface type to indicate the type
> works with -global (something like
> INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
>
> Such a system would probably allow us to replace
> default_machine_opts, default_boot_order, default_display,
> default_ram_size (and probably many other compat fields) on
> MachineClass.  It could also be used to implement -machine and
> -accel options by simply translating them to global properties
> (like we already do for -cpu).
>
> (This sounds like reinventing a QemuOpts-like system on top of
> global properties.  Maybe that's a bad thing, maybe that's a good
> thing.  I'm not sure.)

Not sure I get the connection to QemuOpts.

Lifting the "adjust property default values on the command line" feature
from qdev / TYPE_DEVICE to QOM / TYPE_OBJECT may make sense.

But let's reconsider what exactly we're trying to accomplish for
migration.  The cover letter and commit message are too vague for me gue
figure it out.  Please describe how exactly you intend this series'
features to be used.  You mentioned -global and HW_COMPAT_* briefly;
make sure to cover both.  Concrete examples like "instead of

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-13 Thread Eduardo Habkost 
On Tue, Jun 13, 2017 at 11:52:15AM +0800, Peter Xu wrote:
> On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> > > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > > > Peter Xu  writes:
> > > > 
> > > > > Let the old man "MigrationState" join the object family. Direct 
> > > > > benefit
> > > > > is that we can start to use all the property features derived from
> > > > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > > > > parameters (so will never need to set them up each time using HMP/QMP,
> > > > > this is really, really attractive for test writters), etc.
> > > > >
> > > > > I see no reason to disallow this happen yet. So let's start from this
> > > > > one, to see whether it would be anything good.
> > > > >
> > > > > No functional change at all.
> > > > >
> > > > > Signed-off-by: Peter Xu 
> > > > > ---
> > > > >  include/migration/migration.h | 19 ++
> > > > >  migration/migration.c | 61 
> > > > > ---
> > > > >  2 files changed, 59 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/include/migration/migration.h 
> > > > > b/include/migration/migration.h
> > > > > index 79b5484..bd0186c 100644
> > > > > --- a/include/migration/migration.h
> > > > > +++ b/include/migration/migration.h
> > > > > @@ -21,6 +21,7 @@
> > > > >  #include "qapi-types.h"
> > > > >  #include "exec/cpu-common.h"
> > > > >  #include "qemu/coroutine_int.h"
> > > > > +#include "hw/qdev.h"
> > > > >  
> > > > >  #define QEMU_VM_FILE_MAGIC   0x5145564d
> > > > >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> > > > > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> > > > >  MIG_RP_MSG_MAX
> > > > >  };
> > > > >  
> > > > > +#define TYPE_MIGRATION "migration"
> > > > > +
> > > > >  /* State for the incoming migration */
> > > > >  struct MigrationIncomingState {
> > > > >  QEMUFile *from_src_file;
> > > > > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> > > > >  MigrationIncomingState *migration_incoming_get_current(void);
> > > > >  void migration_incoming_state_destroy(void);
> > > > >  
> > > > > +#define MIGRATION_CLASS(klass) \
> > > > > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> > > > > +#define MIGRATION_OBJ(obj) \
> > > > > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> > > > > +#define MIGRATION_GET_CLASS(obj) \
> > > > > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> > > > > +
> > > > > +typedef struct MigrationClass {
> > > > > +/*< private >*/
> > > > > +DeviceClass parent_class;
> > > > > +} MigrationClass;
> > > > > +
> > > > >  struct MigrationState
> > > > >  {
> > > > > +/*< private >*/
> > > > > +DeviceState parent_obj;
> > > > > +
> > > > > +/*< public >*/
> > > > 
> > > > Turning MigrationState into a QOM object so you can use QOM
> > > > infrastructure makes sense.  But why turn it into a device?  It doesn't
> > > > feel device-like to me.  Would ObjectClass and Object instead of
> > > > DeviceClass and DeviceState do?
> > > 
> > > Yeah. That's the main reason for the series to be (was) RFC.
> > > 
> > > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> > > thing (IIUC you got that already :). I am just curious about why that
> > > is not for QObject from the very beginning, then it'll be easier.
> > > 
> > > For now, IMHO QDev is okay as well for migration, as long as it's kept
> > > internally inside QEMU. But sure at least I should turn user_creatable
> > > to off. I'll investigate more to see how to make this a safer approach
> > > in next post.
> > 
> > We could allow non-device QOM objects use the global property
> > system optionally.
> > 
> > We could make qdev_prop_set_globals() work on any Object*, and
> > let QOM classes call it on post_init if they want to be
> > configurable by global properties too.
> > 
> > Note that this would break qdev_prop_check_globals(), because it
> > expects globals to work only on TYPE_DEVICE.  We could address
> > that by introducing a new interface type to indicate the type
> > works with -global (something like
> > INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> > 
> > Such a system would probably allow us to replace
> > default_machine_opts, default_boot_order, default_display,
> > default_ram_size (and probably many other compat fields) on
> > MachineClass.  It could also be used to implement -machine and
> > -accel options by simply translating them to global properties
> > (like we already do for -cpu).
> > 
> > (This sounds like reinventing a QemuOpts-like system on top of
> > global properties.  Maybe that's a bad thing, maybe that's a good
> > thing.  I'm not sure.)
> 
> Thanks Eduardo for the reviews and suggestions. 
> 
> It'll obviously benefit us a lot if all the objects can use the global
>

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-12 Thread Peter Xu 
On Mon, Jun 12, 2017 at 01:13:49PM -0300, Eduardo Habkost wrote:
> On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> > On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > > Peter Xu  writes:
> > > 
> > > > Let the old man "MigrationState" join the object family. Direct benefit
> > > > is that we can start to use all the property features derived from
> > > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > > > parameters (so will never need to set them up each time using HMP/QMP,
> > > > this is really, really attractive for test writters), etc.
> > > >
> > > > I see no reason to disallow this happen yet. So let's start from this
> > > > one, to see whether it would be anything good.
> > > >
> > > > No functional change at all.
> > > >
> > > > Signed-off-by: Peter Xu 
> > > > ---
> > > >  include/migration/migration.h | 19 ++
> > > >  migration/migration.c | 61 
> > > > ---
> > > >  2 files changed, 59 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/include/migration/migration.h 
> > > > b/include/migration/migration.h
> > > > index 79b5484..bd0186c 100644
> > > > --- a/include/migration/migration.h
> > > > +++ b/include/migration/migration.h
> > > > @@ -21,6 +21,7 @@
> > > >  #include "qapi-types.h"
> > > >  #include "exec/cpu-common.h"
> > > >  #include "qemu/coroutine_int.h"
> > > > +#include "hw/qdev.h"
> > > >  
> > > >  #define QEMU_VM_FILE_MAGIC   0x5145564d
> > > >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> > > > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> > > >  MIG_RP_MSG_MAX
> > > >  };
> > > >  
> > > > +#define TYPE_MIGRATION "migration"
> > > > +
> > > >  /* State for the incoming migration */
> > > >  struct MigrationIncomingState {
> > > >  QEMUFile *from_src_file;
> > > > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> > > >  MigrationIncomingState *migration_incoming_get_current(void);
> > > >  void migration_incoming_state_destroy(void);
> > > >  
> > > > +#define MIGRATION_CLASS(klass) \
> > > > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> > > > +#define MIGRATION_OBJ(obj) \
> > > > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> > > > +#define MIGRATION_GET_CLASS(obj) \
> > > > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> > > > +
> > > > +typedef struct MigrationClass {
> > > > +/*< private >*/
> > > > +DeviceClass parent_class;
> > > > +} MigrationClass;
> > > > +
> > > >  struct MigrationState
> > > >  {
> > > > +/*< private >*/
> > > > +DeviceState parent_obj;
> > > > +
> > > > +/*< public >*/
> > > 
> > > Turning MigrationState into a QOM object so you can use QOM
> > > infrastructure makes sense.  But why turn it into a device?  It doesn't
> > > feel device-like to me.  Would ObjectClass and Object instead of
> > > DeviceClass and DeviceState do?
> > 
> > Yeah. That's the main reason for the series to be (was) RFC.
> > 
> > I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> > thing (IIUC you got that already :). I am just curious about why that
> > is not for QObject from the very beginning, then it'll be easier.
> > 
> > For now, IMHO QDev is okay as well for migration, as long as it's kept
> > internally inside QEMU. But sure at least I should turn user_creatable
> > to off. I'll investigate more to see how to make this a safer approach
> > in next post.
> 
> We could allow non-device QOM objects use the global property
> system optionally.
> 
> We could make qdev_prop_set_globals() work on any Object*, and
> let QOM classes call it on post_init if they want to be
> configurable by global properties too.
> 
> Note that this would break qdev_prop_check_globals(), because it
> expects globals to work only on TYPE_DEVICE.  We could address
> that by introducing a new interface type to indicate the type
> works with -global (something like
> INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).
> 
> Such a system would probably allow us to replace
> default_machine_opts, default_boot_order, default_display,
> default_ram_size (and probably many other compat fields) on
> MachineClass.  It could also be used to implement -machine and
> -accel options by simply translating them to global properties
> (like we already do for -cpu).
> 
> (This sounds like reinventing a QemuOpts-like system on top of
> global properties.  Maybe that's a bad thing, maybe that's a good
> thing.  I'm not sure.)

Thanks Eduardo for the reviews and suggestions. 

It'll obviously benefit us a lot if all the objects can use the global
properties and the *_COMPAT_* legacy magic, while the tricky point is
that, QObject will then depend on QDev?

Generally speaking this sounds good to me.  Before I do anything else,
I believe I should wait to see how other QOM developers think about
it.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-12 Thread Eduardo Habkost 
On Mon, Jun 12, 2017 at 12:46:09PM +0800, Peter Xu wrote:
> On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> > Peter Xu  writes:
> > 
> > > Let the old man "MigrationState" join the object family. Direct benefit
> > > is that we can start to use all the property features derived from
> > > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > > parameters (so will never need to set them up each time using HMP/QMP,
> > > this is really, really attractive for test writters), etc.
> > >
> > > I see no reason to disallow this happen yet. So let's start from this
> > > one, to see whether it would be anything good.
> > >
> > > No functional change at all.
> > >
> > > Signed-off-by: Peter Xu 
> > > ---
> > >  include/migration/migration.h | 19 ++
> > >  migration/migration.c | 61 
> > > ---
> > >  2 files changed, 59 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > > index 79b5484..bd0186c 100644
> > > --- a/include/migration/migration.h
> > > +++ b/include/migration/migration.h
> > > @@ -21,6 +21,7 @@
> > >  #include "qapi-types.h"
> > >  #include "exec/cpu-common.h"
> > >  #include "qemu/coroutine_int.h"
> > > +#include "hw/qdev.h"
> > >  
> > >  #define QEMU_VM_FILE_MAGIC   0x5145564d
> > >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> > > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> > >  MIG_RP_MSG_MAX
> > >  };
> > >  
> > > +#define TYPE_MIGRATION "migration"
> > > +
> > >  /* State for the incoming migration */
> > >  struct MigrationIncomingState {
> > >  QEMUFile *from_src_file;
> > > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> > >  MigrationIncomingState *migration_incoming_get_current(void);
> > >  void migration_incoming_state_destroy(void);
> > >  
> > > +#define MIGRATION_CLASS(klass) \
> > > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> > > +#define MIGRATION_OBJ(obj) \
> > > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> > > +#define MIGRATION_GET_CLASS(obj) \
> > > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> > > +
> > > +typedef struct MigrationClass {
> > > +/*< private >*/
> > > +DeviceClass parent_class;
> > > +} MigrationClass;
> > > +
> > >  struct MigrationState
> > >  {
> > > +/*< private >*/
> > > +DeviceState parent_obj;
> > > +
> > > +/*< public >*/
> > 
> > Turning MigrationState into a QOM object so you can use QOM
> > infrastructure makes sense.  But why turn it into a device?  It doesn't
> > feel device-like to me.  Would ObjectClass and Object instead of
> > DeviceClass and DeviceState do?
> 
> Yeah. That's the main reason for the series to be (was) RFC.
> 
> I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
> thing (IIUC you got that already :). I am just curious about why that
> is not for QObject from the very beginning, then it'll be easier.
> 
> For now, IMHO QDev is okay as well for migration, as long as it's kept
> internally inside QEMU. But sure at least I should turn user_creatable
> to off. I'll investigate more to see how to make this a safer approach
> in next post.

We could allow non-device QOM objects use the global property
system optionally.

We could make qdev_prop_set_globals() work on any Object*, and
let QOM classes call it on post_init if they want to be
configurable by global properties too.

Note that this would break qdev_prop_check_globals(), because it
expects globals to work only on TYPE_DEVICE.  We could address
that by introducing a new interface type to indicate the type
works with -global (something like
INTERFACE_CONFIGURABLE_BY_GLOBAL_PROPERTIES, but shorter?).

Such a system would probably allow us to replace
default_machine_opts, default_boot_order, default_display,
default_ram_size (and probably many other compat fields) on
MachineClass.  It could also be used to implement -machine and
-accel options by simply translating them to global properties
(like we already do for -cpu).

(This sounds like reinventing a QemuOpts-like system on top of
global properties.  Maybe that's a bad thing, maybe that's a good
thing.  I'm not sure.)

> [...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-11 Thread Peter Xu 
On Fri, Jun 09, 2017 at 03:39:24PM +0200, Markus Armbruster wrote:
> Peter Xu  writes:
> 
> > Let the old man "MigrationState" join the object family. Direct benefit
> > is that we can start to use all the property features derived from
> > current QDev, like: HW_COMPAT_* bits, command line setup for migration
> > parameters (so will never need to set them up each time using HMP/QMP,
> > this is really, really attractive for test writters), etc.
> >
> > I see no reason to disallow this happen yet. So let's start from this
> > one, to see whether it would be anything good.
> >
> > No functional change at all.
> >
> > Signed-off-by: Peter Xu 
> > ---
> >  include/migration/migration.h | 19 ++
> >  migration/migration.c | 61 
> > ---
> >  2 files changed, 59 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 79b5484..bd0186c 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -21,6 +21,7 @@
> >  #include "qapi-types.h"
> >  #include "exec/cpu-common.h"
> >  #include "qemu/coroutine_int.h"
> > +#include "hw/qdev.h"
> >  
> >  #define QEMU_VM_FILE_MAGIC   0x5145564d
> >  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> > @@ -49,6 +50,8 @@ enum mig_rp_message_type {
> >  MIG_RP_MSG_MAX
> >  };
> >  
> > +#define TYPE_MIGRATION "migration"
> > +
> >  /* State for the incoming migration */
> >  struct MigrationIncomingState {
> >  QEMUFile *from_src_file;
> > @@ -91,8 +94,24 @@ struct MigrationIncomingState {
> >  MigrationIncomingState *migration_incoming_get_current(void);
> >  void migration_incoming_state_destroy(void);
> >  
> > +#define MIGRATION_CLASS(klass) \
> > +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> > +#define MIGRATION_OBJ(obj) \
> > +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> > +#define MIGRATION_GET_CLASS(obj) \
> > +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> > +
> > +typedef struct MigrationClass {
> > +/*< private >*/
> > +DeviceClass parent_class;
> > +} MigrationClass;
> > +
> >  struct MigrationState
> >  {
> > +/*< private >*/
> > +DeviceState parent_obj;
> > +
> > +/*< public >*/
> 
> Turning MigrationState into a QOM object so you can use QOM
> infrastructure makes sense.  But why turn it into a device?  It doesn't
> feel device-like to me.  Would ObjectClass and Object instead of
> DeviceClass and DeviceState do?

Yeah. That's the main reason for the series to be (was) RFC.

I was trying to use the HW_COMPAT_* bits and -global, and that's QDev
thing (IIUC you got that already :). I am just curious about why that
is not for QObject from the very beginning, then it'll be easier.

For now, IMHO QDev is okay as well for migration, as long as it's kept
internally inside QEMU. But sure at least I should turn user_creatable
to off. I'll investigate more to see how to make this a safer approach
in next post.

> 
> >  size_t bytes_xfer;
> >  size_t xfer_limit;
> >  QemuThread thread;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 48c94c9..98b77e2 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -93,29 +93,13 @@ static bool deferred_incoming;
> >  /* For outgoing */
> >  MigrationState *migrate_get_current(void)
> >  {
> > -static bool once;
> > -static MigrationState current_migration = {
> > -.state = MIGRATION_STATUS_NONE,
> > -.xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
> > -.mbps = -1,
> > -.parameters = {
> > -.compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> > -.compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> > -.decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> > -.cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
> > -.cpu_throttle_increment = 
> > DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> > -.max_bandwidth = MAX_THROTTLE,
> > -.downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
> > -.x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
> > -},
> > -};
> > +static MigrationState *current_migration;
> 
> You add an indirection...
> 
> >  
> > -if (!once) {
> > -current_migration.parameters.tls_creds = g_strdup("");
> > -current_migration.parameters.tls_hostname = g_strdup("");
> > -once = true;
> > +if (!current_migration) {
> > +current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> 
> ... possibly just so you can use object_new().  Have you considered
> object_initialize()?

Yes, but even with object_initialize() I still need that indirection?

Or, I can put the init into main() in some way to avoid the
indirection.

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-09 Thread Markus Armbruster 
Peter Xu  writes:

> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
>
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
>
> No functional change at all.
>
> Signed-off-by: Peter Xu 
> ---
>  include/migration/migration.h | 19 ++
>  migration/migration.c | 61 
> ---
>  2 files changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 79b5484..bd0186c 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -21,6 +21,7 @@
>  #include "qapi-types.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/coroutine_int.h"
> +#include "hw/qdev.h"
>  
>  #define QEMU_VM_FILE_MAGIC   0x5145564d
>  #define QEMU_VM_FILE_VERSION_COMPAT  0x0002
> @@ -49,6 +50,8 @@ enum mig_rp_message_type {
>  MIG_RP_MSG_MAX
>  };
>  
> +#define TYPE_MIGRATION "migration"
> +
>  /* State for the incoming migration */
>  struct MigrationIncomingState {
>  QEMUFile *from_src_file;
> @@ -91,8 +94,24 @@ struct MigrationIncomingState {
>  MigrationIncomingState *migration_incoming_get_current(void);
>  void migration_incoming_state_destroy(void);
>  
> +#define MIGRATION_CLASS(klass) \
> +OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
> +#define MIGRATION_OBJ(obj) \
> +OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
> +#define MIGRATION_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
> +
> +typedef struct MigrationClass {
> +/*< private >*/
> +DeviceClass parent_class;
> +} MigrationClass;
> +
>  struct MigrationState
>  {
> +/*< private >*/
> +DeviceState parent_obj;
> +
> +/*< public >*/

Turning MigrationState into a QOM object so you can use QOM
infrastructure makes sense.  But why turn it into a device?  It doesn't
feel device-like to me.  Would ObjectClass and Object instead of
DeviceClass and DeviceState do?

>  size_t bytes_xfer;
>  size_t xfer_limit;
>  QemuThread thread;
> diff --git a/migration/migration.c b/migration/migration.c
> index 48c94c9..98b77e2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -93,29 +93,13 @@ static bool deferred_incoming;
>  /* For outgoing */
>  MigrationState *migrate_get_current(void)
>  {
> -static bool once;
> -static MigrationState current_migration = {
> -.state = MIGRATION_STATUS_NONE,
> -.xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
> -.mbps = -1,
> -.parameters = {
> -.compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> -.compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> -.decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> -.cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
> -.cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> -.max_bandwidth = MAX_THROTTLE,
> -.downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
> -.x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
> -},
> -};
> +static MigrationState *current_migration;

You add an indirection...

>  
> -if (!once) {
> -current_migration.parameters.tls_creds = g_strdup("");
> -current_migration.parameters.tls_hostname = g_strdup("");
> -once = true;
> +if (!current_migration) {
> +current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));

... possibly just so you can use object_new().  Have you considered
object_initialize()?

>  }
> -return _migration;
> +
> +return current_migration;
>  }
>  
>  MigrationIncomingState *migration_incoming_get_current(void)
> @@ -2123,3 +2107,38 @@ void migrate_fd_connect(MigrationState *s)
>  s->migration_thread_running = true;
>  }
>  
> +static void migration_instance_init(Object *obj)
> +{
> +MigrationState *ms = MIGRATION_OBJ(obj);
> +
> +ms->state = MIGRATION_STATUS_NONE;
> +ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
> +ms->mbps = -1;
> +ms->parameters = (MigrationParameters) {
> +.compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
> +.compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
> +.decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> +.cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
> +.cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
> +.max_bandwidth = MAX_THROTTLE,
> +.downtime_limit =

Re: [Qemu-devel] [PATCH v2 2/6] migration: let MigrationState be a qdev

2017-06-09 Thread Juan Quintela 
Peter Xu  wrote:
> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QDev, like: HW_COMPAT_* bits, command line setup for migration
> parameters (so will never need to set them up each time using HMP/QMP,
> this is really, really attractive for test writters), etc.
>
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
>
> No functional change at all.
>
> Signed-off-by: Peter Xu 

Reviewed-by: Juan Quintela