Patchwork [1/5] qdev: introduce qdev_create_kid(Object *parent, const char *type)

login
register
mail settings
Submitter pingfan liu
Date July 10, 2012, 6:16 a.m.
Message ID <1341900967-4344-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/170053/
State New
Headers show

Comments

pingfan liu - July 10, 2012, 6:16 a.m.
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(-)
Andreas Färber - July 10, 2012, 8:12 a.m.
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);
>
Paolo Bonzini - July 10, 2012, 8:45 a.m.
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);
>
pingfan liu - July 11, 2012, 1:17 a.m.
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);
>>
>
>
Paolo Bonzini - July 11, 2012, 7:01 a.m.
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

Patch

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);