[Qemu-devel] make apic hot-plugable

[Qemu-devel] make apic hot-plugable

[Liu Ping Fan]

The previous effort to make apic hot-plugable is thread:
  [PATCH V3] Introduce a new bus "ICC" to connect APIC
refer to :
  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg00413.html

But now, we are with qom. So remodeling the apic as a kid of CPUState (neglect the dependent apic, which is not usual any longer).

Any comments or suggesion?

[Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[Liu Ping Fan]

DeviceState can be created as kid of DeviceState/CPUState, not neccesary
attached to bus. This will be helpful to simulate the real hardware
submodule which sits inside package.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   28 ++++++++++++++++++++++++++++
 hw/qdev.h |    1 +
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index af54467..d2100a1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -26,6 +26,7 @@
    this API directly.  */
 
 #include "net.h"
+#include "qemu/cpu.h"
 #include "qdev.h"
 #include "sysemu.h"
 #include "error.h"
@@ -145,6 +146,33 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     return dev;
 }
 
+DeviceState *qdev_create_kid(Object *parent, const char *type)
+{
+    DeviceState *dev;
+    assert(parent);
+
+    if (object_class_by_name(type) == NULL) {
+        return NULL;
+    }
+
+    if (object_is_type_str(parent, TYPE_BUS)) {
+        return qdev_create(BUS(parent), type);
+    }
+
+    if (!object_is_type_str(parent, TYPE_DEVICE)
+        || !object_is_type_str(parent, TYPE_CPU)) {
+        return NULL;
+    }
+
+    dev = DEVICE(object_new(type));
+    if (!dev) {
+        return NULL;
+    }
+    object_property_add_child(OBJECT(parent), type, OBJECT(dev), NULL);
+
+    return dev;
+}
+
 /* Initialize a device.  Device properties should be set before calling
    this function.  IRQs and MMIO regions should be connected/mapped after
    calling this function.
diff --git a/hw/qdev.h b/hw/qdev.h
index f4683dc..aecc69e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -154,6 +154,7 @@ typedef struct GlobalProperty {
 
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
+DeviceState *qdev_create_kid(Object *parent, const char *type);
 bool qdev_exists(const char *name);
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[Andreas Färber]

Am 10.07.2012 08:16, schrieb Liu Ping Fan:
> DeviceState can be created as kid of DeviceState/CPUState, not neccesary
> attached to bus. This will be helpful to simulate the real hardware
> submodule which sits inside package.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |   28 ++++++++++++++++++++++++++++
>  hw/qdev.h |    1 +
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index af54467..d2100a1 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -26,6 +26,7 @@
>     this API directly.  */
>  
>  #include "net.h"
> +#include "qemu/cpu.h"
>  #include "qdev.h"
>  #include "sysemu.h"
>  #include "error.h"
> @@ -145,6 +146,33 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      return dev;
>  }
>  
> +DeviceState *qdev_create_kid(Object *parent, const char *type)
> +{
> +    DeviceState *dev;
> +    assert(parent);
> +
> +    if (object_class_by_name(type) == NULL) {
> +        return NULL;
> +    }
> +
> +    if (object_is_type_str(parent, TYPE_BUS)) {

This is only introduced in patch 2, no?

Andreas

> +        return qdev_create(BUS(parent), type);
> +    }
> +
> +    if (!object_is_type_str(parent, TYPE_DEVICE)
> +        || !object_is_type_str(parent, TYPE_CPU)) {
> +        return NULL;
> +    }
> +
> +    dev = DEVICE(object_new(type));
> +    if (!dev) {
> +        return NULL;
> +    }
> +    object_property_add_child(OBJECT(parent), type, OBJECT(dev), NULL);
> +
> +    return dev;
> +}
> +
>  /* Initialize a device.  Device properties should be set before calling
>     this function.  IRQs and MMIO regions should be connected/mapped after
>     calling this function.
> diff --git a/hw/qdev.h b/hw/qdev.h
> index f4683dc..aecc69e 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -154,6 +154,7 @@ typedef struct GlobalProperty {
>  
>  DeviceState *qdev_create(BusState *bus, const char *name);
>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> +DeviceState *qdev_create_kid(Object *parent, const char *type);
>  bool qdev_exists(const char *name);
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[Paolo Bonzini]

Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
> DeviceState can be created as kid of DeviceState/CPUState, not neccesary
> attached to bus. This will be helpful to simulate the real hardware
> submodule which sits inside package.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |   28 ++++++++++++++++++++++++++++
>  hw/qdev.h |    1 +
>  2 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index af54467..d2100a1 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -26,6 +26,7 @@
>     this API directly.  */
>  
>  #include "net.h"
> +#include "qemu/cpu.h"
>  #include "qdev.h"
>  #include "sysemu.h"
>  #include "error.h"
> @@ -145,6 +146,33 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      return dev;
>  }
>  
> +DeviceState *qdev_create_kid(Object *parent, const char *type)
> +{
> +    DeviceState *dev;
> +    assert(parent);
> +
> +    if (object_class_by_name(type) == NULL) {
> +        return NULL;
> +    }
> +
> +    if (object_is_type_str(parent, TYPE_BUS)) {
> +        return qdev_create(BUS(parent), type);
> +    }
> +
> +    if (!object_is_type_str(parent, TYPE_DEVICE)
> +        || !object_is_type_str(parent, TYPE_CPU)) {
> +        return NULL;
> +    }
> +
> +    dev = DEVICE(object_new(type));
> +    if (!dev) {
> +        return NULL;
> +    }
> +    object_property_add_child(OBJECT(parent), type, OBJECT(dev), NULL);

I don't like this.  The only additional functionality is "magic"
dispatching between qdev_create for buses and object_property_add_child
for devices.  This should be done with a method that is implemented in
both objects (e.g. an interface), not with type checks like this.

However, you're not even using the functionality, and designing APIs
without an effective need usually makes for bad APIs.

Instead, you can just move APIC creation in the CPU, and use
object_property_add_child there.

Paolo

> +    return dev;
> +}
> +
>  /* Initialize a device.  Device properties should be set before calling
>     this function.  IRQs and MMIO regions should be connected/mapped after
>     calling this function.
> diff --git a/hw/qdev.h b/hw/qdev.h
> index f4683dc..aecc69e 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -154,6 +154,7 @@ typedef struct GlobalProperty {
>  
>  DeviceState *qdev_create(BusState *bus, const char *name);
>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> +DeviceState *qdev_create_kid(Object *parent, const char *type);
>  bool qdev_exists(const char *name);
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts);
> 

Re: [Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[liu ping fan]

On Tue, Jul 10, 2012 at 4:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> DeviceState can be created as kid of DeviceState/CPUState, not neccesary
>> attached to bus. This will be helpful to simulate the real hardware
>> submodule which sits inside package.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/qdev.c |   28 ++++++++++++++++++++++++++++
>>  hw/qdev.h |    1 +
>>  2 files changed, 29 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index af54467..d2100a1 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -26,6 +26,7 @@
>>     this API directly.  */
>>
>>  #include "net.h"
>> +#include "qemu/cpu.h"
>>  #include "qdev.h"
>>  #include "sysemu.h"
>>  #include "error.h"
>> @@ -145,6 +146,33 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>>      return dev;
>>  }
>>
>> +DeviceState *qdev_create_kid(Object *parent, const char *type)
>> +{
>> +    DeviceState *dev;
>> +    assert(parent);
>> +
>> +    if (object_class_by_name(type) == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    if (object_is_type_str(parent, TYPE_BUS)) {
>> +        return qdev_create(BUS(parent), type);
>> +    }
>> +
>> +    if (!object_is_type_str(parent, TYPE_DEVICE)
>> +        || !object_is_type_str(parent, TYPE_CPU)) {
>> +        return NULL;
>> +    }
>> +
>> +    dev = DEVICE(object_new(type));
>> +    if (!dev) {
>> +        return NULL;
>> +    }
>> +    object_property_add_child(OBJECT(parent), type, OBJECT(dev), NULL);
>
> I don't like this.  The only additional functionality is "magic"
> dispatching between qdev_create for buses and object_property_add_child
> for devices.  This should be done with a method that is implemented in
> both objects (e.g. an interface), not with type checks like this.
>
> However, you're not even using the functionality, and designing APIs
> without an effective need usually makes for bad APIs.
>
> Instead, you can just move APIC creation in the CPU, and use
> object_property_add_child there.
>
OK, I will move the creation in the CPU. But I think as part of qom,
DeviceState can have a DeviceState child, so there is need for wrapper
for the function.  Maybe just make the qdev_create_kid(Object*) ->
qdev_create_kid(DeviceState*) ?

regards,
pingfan

> Paolo
>
>> +    return dev;
>> +}
>> +
>>  /* Initialize a device.  Device properties should be set before calling
>>     this function.  IRQs and MMIO regions should be connected/mapped after
>>     calling this function.
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index f4683dc..aecc69e 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -154,6 +154,7 @@ typedef struct GlobalProperty {
>>
>>  DeviceState *qdev_create(BusState *bus, const char *name);
>>  DeviceState *qdev_try_create(BusState *bus, const char *name);
>> +DeviceState *qdev_create_kid(Object *parent, const char *type);
>>  bool qdev_exists(const char *name);
>>  int qdev_device_help(QemuOpts *opts);
>>  DeviceState *qdev_device_add(QemuOpts *opts);
>>
>
>

Re: [Qemu-devel] [PATCH 1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

[Paolo Bonzini]

Il 11/07/2012 03:17, liu ping fan ha scritto:
> OK, I will move the creation in the CPU. But I think as part of qom,
> DeviceState can have a DeviceState child, so there is need for wrapper
> for the function.  Maybe just make the qdev_create_kid(Object*) ->
> qdev_create_kid(DeviceState*) ?

You can just use object_property_add_child whenever you would use
qdev_create_kid.

Paolo

[Qemu-devel] [PATCH 2/5] qom: introduce object_is_type_str(), so we can judge its type.

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/object.h |    2 ++
 qom/object.c          |    6 ++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8b17776..a66e996 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -479,6 +479,8 @@ void object_initialize(void *obj, const char *typename);
  */
 void object_finalize(void *obj);
 
+bool object_is_type_str(Object *obj, const char *typename);
+
 /**
  * object_dynamic_cast:
  * @obj: The object to cast.
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..6c27d90 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -425,6 +425,12 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
     return false;
 }
 
+bool object_is_type_str(Object *obj, const char *typename)
+{
+    TypeImpl *target_type = type_get_by_name(typename);
+    return !target_type || type_is_ancestor(obj->class->type, target_type);
+}
+
 static bool object_is_type(Object *obj, TypeImpl *target_type)
 {
     return !target_type || type_is_ancestor(obj->class->type, target_type);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 2/5] qom: introduce object_is_type_str(), so we can judge its type.

[Paolo Bonzini]

Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/qemu/object.h |    2 ++
>  qom/object.c          |    6 ++++++
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 8b17776..a66e996 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -479,6 +479,8 @@ void object_initialize(void *obj, const char *typename);
>   */
>  void object_finalize(void *obj);
>  
> +bool object_is_type_str(Object *obj, const char *typename);

Please call this object_is_instance_of, and just call
object_dynamic_cast internally so that interfaces are handled properly.

Paolo

> +
>  /**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
> diff --git a/qom/object.c b/qom/object.c
> index 00bb3b0..6c27d90 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -425,6 +425,12 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>      return false;
>  }
>  
> +bool object_is_type_str(Object *obj, const char *typename)
> +{
> +    TypeImpl *target_type = type_get_by_name(typename);
> +    return !target_type || type_is_ancestor(obj->class->type, target_type);
> +}
> +
>  static bool object_is_type(Object *obj, TypeImpl *target_type)
>  {
>      return !target_type || type_is_ancestor(obj->class->type, target_type);
> 

Re: [Qemu-devel] [PATCH 2/5] qom: introduce object_is_type_str(), so we can judge its type.

[liu ping fan]

On Tue, Jul 10, 2012 at 4:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  include/qemu/object.h |    2 ++
>>  qom/object.c          |    6 ++++++
>>  2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qemu/object.h b/include/qemu/object.h
>> index 8b17776..a66e996 100644
>> --- a/include/qemu/object.h
>> +++ b/include/qemu/object.h
>> @@ -479,6 +479,8 @@ void object_initialize(void *obj, const char *typename);
>>   */
>>  void object_finalize(void *obj);
>>
>> +bool object_is_type_str(Object *obj, const char *typename);
>
> Please call this object_is_instance_of, and just call
> object_dynamic_cast internally so that interfaces are handled properly.
>
OK, will update it in next version

regards,
pingfan

> Paolo
>
>> +
>>  /**
>>   * object_dynamic_cast:
>>   * @obj: The object to cast.
>> diff --git a/qom/object.c b/qom/object.c
>> index 00bb3b0..6c27d90 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -425,6 +425,12 @@ static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>>      return false;
>>  }
>>
>> +bool object_is_type_str(Object *obj, const char *typename)
>> +{
>> +    TypeImpl *target_type = type_get_by_name(typename);
>> +    return !target_type || type_is_ancestor(obj->class->type, target_type);
>> +}
>> +
>>  static bool object_is_type(Object *obj, TypeImpl *target_type)
>>  {
>>      return !target_type || type_is_ancestor(obj->class->type, target_type);
>>
>
>

[Qemu-devel] [PATCH 3/5] qdev: export the bus reset interface

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   17 ++++++++++++-----
 hw/qdev.h |    2 ++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index d2100a1..f7983e4 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -249,11 +249,9 @@ static int qdev_reset_one(DeviceState *dev, void *opaque)
 
 static int qbus_reset_one(BusState *bus, void *opaque)
 {
-    BusClass *bc = BUS_GET_CLASS(bus);
-    if (bc->reset) {
-        return bc->reset(bus);
-    }
-    return 0;
+    int ret;
+    ret = bus_reset(bus);
+    return ret;
 }
 
 void qdev_reset_all(DeviceState *dev)
@@ -766,6 +764,15 @@ void device_reset(DeviceState *dev)
     }
 }
 
+int bus_reset(BusState *bus)
+{
+    BusClass *bc = BUS_GET_CLASS(bus);
+    if (bc->reset) {
+        return bc->reset(bus);
+    }
+    return 0;
+}
+
 Object *qdev_get_machine(void)
 {
     static Object *dev;
diff --git a/hw/qdev.h b/hw/qdev.h
index aecc69e..5f88b4b 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -356,6 +356,8 @@ void qdev_machine_init(void);
  */
 void device_reset(DeviceState *dev);
 
+int bus_reset(BusState *bus);
+
 const VMStateDescription *qdev_get_vmsd(DeviceState *dev);
 
 const char *qdev_fw_name(DeviceState *dev);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 3/5] qdev: export the bus reset interface

[Paolo Bonzini]

Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev.c |   17 ++++++++++++-----
>  hw/qdev.h |    2 ++
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d2100a1..f7983e4 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -249,11 +249,9 @@ static int qdev_reset_one(DeviceState *dev, void *opaque)
>  
>  static int qbus_reset_one(BusState *bus, void *opaque)
>  {
> -    BusClass *bc = BUS_GET_CLASS(bus);
> -    if (bc->reset) {
> -        return bc->reset(bus);
> -    }
> -    return 0;
> +    int ret;
> +    ret = bus_reset(bus);
> +    return ret;
>  }
>  
>  void qdev_reset_all(DeviceState *dev)
> @@ -766,6 +764,15 @@ void device_reset(DeviceState *dev)
>      }
>  }
>  
> +int bus_reset(BusState *bus)
> +{
> +    BusClass *bc = BUS_GET_CLASS(bus);
> +    if (bc->reset) {
> +        return bc->reset(bus);
> +    }
> +    return 0;
> +}

Two comments:

1) Is this correct? Resetting a bus should reset all the children before
resetting the bus itself.

2) Does it make sense to export it, since we're going towards removing
unnecessary buses?

Paolo

> +
>  Object *qdev_get_machine(void)
>  {
>      static Object *dev;
> diff --git a/hw/qdev.h b/hw/qdev.h
> index aecc69e..5f88b4b 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -356,6 +356,8 @@ void qdev_machine_init(void);
>   */
>  void device_reset(DeviceState *dev);
>  
> +int bus_reset(BusState *bus);
> +
>  const VMStateDescription *qdev_get_vmsd(DeviceState *dev);
>  
>  const char *qdev_fw_name(DeviceState *dev);
> 

[Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[Liu Ping Fan]

This will give the embeded logic module, such as apic has the
opportunity to reset.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 qom/cpu.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index 5b36046..6aea8e6 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -20,10 +20,26 @@
 
 #include "qemu/cpu.h"
 #include "qemu-common.h"
+#include "hw/qdev.h"
+
+static int cpu_reset_kid(Object *child, void *opaque)
+{
+    if (object_is_type_str(child, TYPE_DEVICE)) {
+        device_reset(DEVICE(child));
+    } else if (object_is_type_str(child, TYPE_BUS)) {
+        bus_reset(BUS(child));
+    } else {
+        printf("cpu's child must be DEVICE or BUS");
+        abort();
+    }
+    return 0;
+}
 
 void cpu_reset(CPUState *cpu)
 {
     CPUClass *klass = CPU_GET_CLASS(cpu);
+    Object *obj = OBJECT(cpu);
+    object_child_foreach(obj, cpu_reset_kid, NULL);
 
     if (klass->reset != NULL) {
         (*klass->reset)(cpu);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[Paolo Bonzini]

Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
> This will give the embeded logic module, such as apic has the
> opportunity to reset.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  qom/cpu.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5b36046..6aea8e6 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -20,10 +20,26 @@
>  
>  #include "qemu/cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev.h"
> +
> +static int cpu_reset_kid(Object *child, void *opaque)
> +{
> +    if (object_is_type_str(child, TYPE_DEVICE)) {
> +        device_reset(DEVICE(child));
> +    } else if (object_is_type_str(child, TYPE_BUS)) {
> +        bus_reset(BUS(child));
> +    } else {
> +        printf("cpu's child must be DEVICE or BUS");
> +        abort();
> +    }
> +    return 0;
> +}
>  
>  void cpu_reset(CPUState *cpu)
>  {
>      CPUClass *klass = CPU_GET_CLASS(cpu);
> +    Object *obj = OBJECT(cpu);
> +    object_child_foreach(obj, cpu_reset_kid, NULL);

Ok, now I see what you want to do.  Next time, please add meaningful
commit messages to all patches in the series, even those that only add
infrastructure.

It really looks like time is ripe to make CPUs children of Device, so
you can just use qdev_reset_all to reset the CPU.

Paolo

>      if (klass->reset != NULL) {
>          (*klass->reset)(cpu);
> 

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[Andreas Färber]

Am 10.07.2012 10:41, schrieb Paolo Bonzini:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> This will give the embeded logic module, such as apic has the
>> opportunity to reset.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  qom/cpu.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 5b36046..6aea8e6 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -20,10 +20,26 @@
>>  
>>  #include "qemu/cpu.h"
>>  #include "qemu-common.h"
>> +#include "hw/qdev.h"
>> +
>> +static int cpu_reset_kid(Object *child, void *opaque)
>> +{
>> +    if (object_is_type_str(child, TYPE_DEVICE)) {
>> +        device_reset(DEVICE(child));
>> +    } else if (object_is_type_str(child, TYPE_BUS)) {
>> +        bus_reset(BUS(child));
>> +    } else {
>> +        printf("cpu's child must be DEVICE or BUS");
>> +        abort();
>> +    }
>> +    return 0;
>> +}
>>  
>>  void cpu_reset(CPUState *cpu)
>>  {
>>      CPUClass *klass = CPU_GET_CLASS(cpu);
>> +    Object *obj = OBJECT(cpu);
>> +    object_child_foreach(obj, cpu_reset_kid, NULL);
> 
> Ok, now I see what you want to do.  Next time, please add meaningful
> commit messages to all patches in the series, even those that only add
> infrastructure.
> 
> It really looks like time is ripe to make CPUs children of Device, so
> you can just use qdev_reset_all to reset the CPU.

While we agree on that goal, the way there has proven controversial,
please review and comment on the two approaches around.

One thing we definitely need to do is to split up qdev.h.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[liu ping fan]

On Tue, Jul 10, 2012 at 6:12 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.07.2012 10:41, schrieb Paolo Bonzini:
>> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>>> This will give the embeded logic module, such as apic has the
>>> opportunity to reset.
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  qom/cpu.c |   16 ++++++++++++++++
>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qom/cpu.c b/qom/cpu.c
>>> index 5b36046..6aea8e6 100644
>>> --- a/qom/cpu.c
>>> +++ b/qom/cpu.c
>>> @@ -20,10 +20,26 @@
>>>
>>>  #include "qemu/cpu.h"
>>>  #include "qemu-common.h"
>>> +#include "hw/qdev.h"
>>> +
>>> +static int cpu_reset_kid(Object *child, void *opaque)
>>> +{
>>> +    if (object_is_type_str(child, TYPE_DEVICE)) {
>>> +        device_reset(DEVICE(child));
>>> +    } else if (object_is_type_str(child, TYPE_BUS)) {
>>> +        bus_reset(BUS(child));
>>> +    } else {
>>> +        printf("cpu's child must be DEVICE or BUS");
>>> +        abort();
>>> +    }
>>> +    return 0;
>>> +}
>>>
>>>  void cpu_reset(CPUState *cpu)
>>>  {
>>>      CPUClass *klass = CPU_GET_CLASS(cpu);
>>> +    Object *obj = OBJECT(cpu);
>>> +    object_child_foreach(obj, cpu_reset_kid, NULL);
>>
>> Ok, now I see what you want to do.  Next time, please add meaningful
>> commit messages to all patches in the series, even those that only add
>> infrastructure.
>>
>> It really looks like time is ripe to make CPUs children of Device, so
>> you can just use qdev_reset_all to reset the CPU.
>
> While we agree on that goal, the way there has proven controversial,
> please review and comment on the two approaches around.
>
> One thing we definitely need to do is to split up qdev.h.
>
Could you remember the main topic about these?  I miss these discussion.

Thanks,
pingfan

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[Igor Mammedov]

On 07/11/2012 03:17 AM, liu ping fan wrote:
> On Tue, Jul 10, 2012 at 6:12 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 10.07.2012 10:41, schrieb Paolo Bonzini:
>>> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>>>> This will give the embeded logic module, such as apic has the
>>>> opportunity to reset.
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>   qom/cpu.c |   16 ++++++++++++++++
>>>>   1 files changed, 16 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qom/cpu.c b/qom/cpu.c
>>>> index 5b36046..6aea8e6 100644
>>>> --- a/qom/cpu.c
>>>> +++ b/qom/cpu.c
>>>> @@ -20,10 +20,26 @@
>>>>
>>>>   #include "qemu/cpu.h"
>>>>   #include "qemu-common.h"
>>>> +#include "hw/qdev.h"
>>>> +
>>>> +static int cpu_reset_kid(Object *child, void *opaque)
>>>> +{
>>>> +    if (object_is_type_str(child, TYPE_DEVICE)) {
>>>> +        device_reset(DEVICE(child));
>>>> +    } else if (object_is_type_str(child, TYPE_BUS)) {
>>>> +        bus_reset(BUS(child));
>>>> +    } else {
>>>> +        printf("cpu's child must be DEVICE or BUS");
>>>> +        abort();
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>>
>>>>   void cpu_reset(CPUState *cpu)
>>>>   {
>>>>       CPUClass *klass = CPU_GET_CLASS(cpu);
>>>> +    Object *obj = OBJECT(cpu);
>>>> +    object_child_foreach(obj, cpu_reset_kid, NULL);
>>>
>>> Ok, now I see what you want to do.  Next time, please add meaningful
>>> commit messages to all patches in the series, even those that only add
>>> infrastructure.
>>>
>>> It really looks like time is ripe to make CPUs children of Device, so
>>> you can just use qdev_reset_all to reset the CPU.
>>
>> While we agree on that goal, the way there has proven controversial,
>> please review and comment on the two approaches around.
>>
>> One thing we definitely need to do is to split up qdev.h.
>>
> Could you remember the main topic about these?  I miss these discussion.
http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03912.html
the rest was on irc

> Thanks,
> pingfan
>
>> Andreas
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>
>>
>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[liu ping fan]

On Tue, Jul 10, 2012 at 4:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> This will give the embeded logic module, such as apic has the
>> opportunity to reset.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  qom/cpu.c |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 5b36046..6aea8e6 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -20,10 +20,26 @@
>>
>>  #include "qemu/cpu.h"
>>  #include "qemu-common.h"
>> +#include "hw/qdev.h"
>> +
>> +static int cpu_reset_kid(Object *child, void *opaque)
>> +{
>> +    if (object_is_type_str(child, TYPE_DEVICE)) {
>> +        device_reset(DEVICE(child));
>> +    } else if (object_is_type_str(child, TYPE_BUS)) {
>> +        bus_reset(BUS(child));
>> +    } else {
>> +        printf("cpu's child must be DEVICE or BUS");
>> +        abort();
>> +    }
>> +    return 0;
>> +}
>>
>>  void cpu_reset(CPUState *cpu)
>>  {
>>      CPUClass *klass = CPU_GET_CLASS(cpu);
>> +    Object *obj = OBJECT(cpu);
>> +    object_child_foreach(obj, cpu_reset_kid, NULL);
>
> Ok, now I see what you want to do.  Next time, please add meaningful
> commit messages to all patches in the series, even those that only add
> infrastructure.
>
OK, I will.

> It really looks like time is ripe to make CPUs children of Device, so
> you can just use qdev_reset_all to reset the CPU.
>
Change CPUState as child of Device? It is cool, but I think it will a
huge work with argue.
> Paolo
>
>>      if (klass->reset != NULL) {
>>          (*klass->reset)(cpu);
>>
>
>

Re: [Qemu-devel] [PATCH 4/5] qom-cpu: during cpu reset, it will reset its child

[Paolo Bonzini]

Il 11/07/2012 03:17, liu ping fan ha scritto:
>> > It really looks like time is ripe to make CPUs children of Device, so
>> > you can just use qdev_reset_all to reset the CPU.
>> >
> Change CPUState as child of Device? It is cool, but I think it will a
> huge work with argue.

Luckily other people (Igor, Andreas, Anthony) are already working on that.

Paolo

[Qemu-devel] [PATCH 5/5] apic: create apic as a child of cpu, not system_bus any longer

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/pc.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index c7e9ab3..8df58c9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -886,17 +886,17 @@ DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
+static DeviceState *apic_init(void *cpu, void *env, uint8_t apic_id)
 {
     DeviceState *dev;
     static int apic_mapped;
 
     if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-apic");
+        dev = qdev_create_kid(OBJECT(cpu), "kvm-apic");
     } else if (xen_enabled()) {
-        dev = qdev_create(NULL, "xen-apic");
+        dev = qdev_create_kid(OBJECT(cpu), "xen-apic");
     } else {
-        dev = qdev_create(NULL, "apic");
+        dev = qdev_create_kid(OBJECT(cpu), "apic");
     }
 
     qdev_prop_set_uint8(dev, "id", apic_id);
@@ -945,7 +945,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
     }
     env = &cpu->env;
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = apic_init(env, env->cpuid_apic_id);
+        env->apic_state = apic_init(cpu, env, env->cpuid_apic_id);
     }
     qemu_register_reset(pc_cpu_reset, cpu);
     pc_cpu_reset(cpu);
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 5/5] apic: create apic as a child of cpu, not system_bus any longer

[Paolo Bonzini]

Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/pc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index c7e9ab3..8df58c9 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -886,17 +886,17 @@ DeviceState *cpu_get_current_apic(void)
>      }
>  }
>  
> -static DeviceState *apic_init(void *env, uint8_t apic_id)
> +static DeviceState *apic_init(void *cpu, void *env, uint8_t apic_id)
>  {
>      DeviceState *dev;
>      static int apic_mapped;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        dev = qdev_create(NULL, "kvm-apic");
> +        dev = qdev_create_kid(OBJECT(cpu), "kvm-apic");
>      } else if (xen_enabled()) {
> -        dev = qdev_create(NULL, "xen-apic");
> +        dev = qdev_create_kid(OBJECT(cpu), "xen-apic");
>      } else {
> -        dev = qdev_create(NULL, "apic");
> +        dev = qdev_create_kid(OBJECT(cpu), "apic");
>      }

Does it make sense instead to do this in the realize method of the CPU?

Paolo

>  
>      qdev_prop_set_uint8(dev, "id", apic_id);
> @@ -945,7 +945,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>      }
>      env = &cpu->env;
>      if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> +        env->apic_state = apic_init(cpu, env, env->cpuid_apic_id);
>      }
>      qemu_register_reset(pc_cpu_reset, cpu);
>      pc_cpu_reset(cpu);
> 

Re: [Qemu-devel] [PATCH 5/5] apic: create apic as a child of cpu, not system_bus any longer

[Andreas Färber]

Am 10.07.2012 10:45, schrieb Paolo Bonzini:
> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/pc.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index c7e9ab3..8df58c9 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -886,17 +886,17 @@ DeviceState *cpu_get_current_apic(void)
>>      }
>>  }
>>  
>> -static DeviceState *apic_init(void *env, uint8_t apic_id)
>> +static DeviceState *apic_init(void *cpu, void *env, uint8_t apic_id)
>>  {
>>      DeviceState *dev;
>>      static int apic_mapped;
>>  
>>      if (kvm_irqchip_in_kernel()) {
>> -        dev = qdev_create(NULL, "kvm-apic");
>> +        dev = qdev_create_kid(OBJECT(cpu), "kvm-apic");
>>      } else if (xen_enabled()) {
>> -        dev = qdev_create(NULL, "xen-apic");
>> +        dev = qdev_create_kid(OBJECT(cpu), "xen-apic");
>>      } else {
>> -        dev = qdev_create(NULL, "apic");
>> +        dev = qdev_create_kid(OBJECT(cpu), "apic");
>>      }
> 
> Does it make sense instead to do this in the realize method of the CPU?

Igor was working on patches to do that. We ran into other design issues
on that road, yesterday I made a proposal how we might proceed with his
approach:

http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00992.html

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 5/5] apic: create apic as a child of cpu, not system_bus any longer

[Igor Mammedov]

On 07/10/2012 12:41 PM, Andreas Färber wrote:
> Am 10.07.2012 10:45, schrieb Paolo Bonzini:
>> Il 10/07/2012 08:16, Liu Ping Fan ha scritto:
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>   hw/pc.c |   10 +++++-----
>>>   1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index c7e9ab3..8df58c9 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -886,17 +886,17 @@ DeviceState *cpu_get_current_apic(void)
>>>       }
>>>   }
>>>
>>> -static DeviceState *apic_init(void *env, uint8_t apic_id)
>>> +static DeviceState *apic_init(void *cpu, void *env, uint8_t apic_id)
>>>   {
>>>       DeviceState *dev;
>>>       static int apic_mapped;
>>>
>>>       if (kvm_irqchip_in_kernel()) {
>>> -        dev = qdev_create(NULL, "kvm-apic");
>>> +        dev = qdev_create_kid(OBJECT(cpu), "kvm-apic");
>>>       } else if (xen_enabled()) {
>>> -        dev = qdev_create(NULL, "xen-apic");
>>> +        dev = qdev_create_kid(OBJECT(cpu), "xen-apic");
>>>       } else {
>>> -        dev = qdev_create(NULL, "apic");
>>> +        dev = qdev_create_kid(OBJECT(cpu), "apic");
>>>       }
>>
>> Does it make sense instead to do this in the realize method of the CPU?
>
> Igor was working on patches to do that. We ran into other design issues
> on that road, yesterday I made a proposal how we might proceed with his
> approach:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg00992.html
>
> Andreas


I was trying to put APIC by-value inside of X86_CPU and do in-place initialization
in x86_cpu_initfn(), but that has it's own drawbacks:
   1. currently all apic flavors have common instance sizeof(APICCommonState) that
      could be used as a way to reserve space in CPUX86State. But they could diverge
      and create problems later when nobody remembers that sizeof(whatever apic) must
      be equal to sizeof(APICCommonState)
   2. it won't solve OOM anyway because of a initfn could fail due to OOM in any
      object_property_add() call

So if not-inplace apic creation in realize() is acceptable alternative then I'd like to
respin apic patches with this change.

-- 
-----
  Igor