[Qemu-devel] [RfC PATCH] qdev: helper macros for properties

[Qemu-devel] [RfC PATCH] qdev: helper macros for properties

[Gerd Hoffmann]

  Hi,

New revision of the helper macros for defining properties, going on top
of all other qdev patches posted today.

Some minor updates.  Now there are versions with and without defaults.
Problem of verifying the type of _state->_field still unsolved.  Dropped
the union idea because it doesn't help much here.

cheers,
  Gerd

---
 hw/pci.c       |    5 +++++
 hw/qdev-addr.h |    3 +++
 hw/qdev.h      |   29 +++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 03c7840..6ce3f8f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -55,6 +55,7 @@ static struct BusInfo pci_bus_info = {
     .size       = sizeof(PCIBus),
     .print_dev  = pcibus_dev_print,
     .props      = (Property[]) {
+#if 0
         {
             .name   = "addr",
             .info   = &qdev_prop_pci_devfn,
@@ -62,6 +63,10 @@ static struct BusInfo pci_bus_info = {
             .defval = (uint32_t[]) { -1 },
         },
         {/* end of list */}
+#else
+        DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
+        DEFINE_PROP_END_OF_LIST()
+#endif
     }
 };
 
diff --git a/hw/qdev-addr.h b/hw/qdev-addr.h
index f02bd7a..e135a96 100644
--- a/hw/qdev-addr.h
+++ b/hw/qdev-addr.h
@@ -1,2 +1,5 @@
+#define DEFINE_PROP_TADDR(_n, _s, _f)           \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_taddr)
+
 extern PropertyInfo qdev_prop_taddr;
 void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value);
diff --git a/hw/qdev.h b/hw/qdev.h
index f2b8b2e..7b2c062 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -152,6 +152,35 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_pci_devfn;
 
+#define DEFINE_PROP(_name, _state, _field, _prop) { \
+        .name      = (_name),                                    \
+        .info      = &(_prop),                                   \
+        .offset    = offsetof(_state, _field),                   \
+        }
+#define DEFINE_PROP_DEFAULT(_name, _state, _field, _defval, _prop, _type) { \
+        .name      = (_name),                                           \
+        .info      = &(_prop),                                          \
+        .offset    = offsetof(_state, _field),                          \
+        .defval    = (_type[]) { _defval },                             \
+        }
+
+#define DEFINE_PROP_UINT16(_n, _s, _f, _d)                      \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t)
+#define DEFINE_PROP_UINT32(_n, _s, _f, _d)                      \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t)
+#define DEFINE_PROP_HEX32(_n, _s, _f, _d)                       \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex32, uint32_t)
+#define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
+
+#define DEFINE_PROP_PTR(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr)
+#define DEFINE_PROP_MACADDR(_n, _s, _f)         \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr)
+
+#define DEFINE_PROP_END_OF_LIST()               \
+    {}
+
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
-- 
1.6.2.5

Re: [Qemu-devel] [RfC PATCH] qdev: helper macros for properties

[Paul Brook]

On Wednesday 15 July 2009, Gerd Hoffmann wrote:
>   Hi,
>
> New revision of the helper macros for defining properties, going on top
> of all other qdev patches posted today.

IMO this should have been part of the initial properties rework, not a 
followup change. Use of these macros should not be optional.

Paul

Re: [Qemu-devel] [RfC PATCH] qdev: helper macros for properties

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

Paul Brook wrote:
> On Wednesday 15 July 2009, Gerd Hoffmann wrote:
>   
>>   Hi,
>>
>> New revision of the helper macros for defining properties, going on top
>> of all other qdev patches posted today.
>>     
>
> IMO this should have been part of the initial properties rework, not a 
> followup change. Use of these macros should not be optional.
>   

For the common case, I'd rather something like the following.  Very GCC 
specific.

Regards,

Anthony Liguori

> Paul
>
>
>   


[-- Attachment #2: qdev-prop.patch --]
[-- Type: text/x-patch, Size: 2540 bytes --]

commit a7e2799c7c3c790c83e015b48ba34ac0975b93bb
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Thu Jul 16 22:16:02 2009 -0500

    Introduce macro for defining qdev properties.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/hw/escc.c b/hw/escc.c
index 9abd092..43230b9 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -952,21 +952,9 @@ static SysBusDeviceInfo escc_info = {
     .qdev.name  = "escc",
     .qdev.size  = sizeof(SerialState),
     .qdev.props = (Property[]) {
-        {
-            .name = "frequency",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, frequency),
-        },
-        {
-            .name = "it_shift",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, it_shift),
-        },
-        {
-            .name = "disabled",
-            .info = &qdev_prop_uint32,
-            .offset = offsetof(SerialState, disabled),
-        },
+        QDEV_PROP(SerialState, frequency),
+        QDEV_PROP(SerialState, it_shift),
+        QDEV_PROP(SerialState, disabled),
         {
             .name = "chrB",
             .info = &qdev_prop_ptr,
diff --git a/hw/qdev.h b/hw/qdev.h
index 11744fa..957ce22 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -143,6 +143,25 @@ void do_info_qtree(Monitor *mon);
 
 /*** qdev-properties.c ***/
 
+#define CHOOSE(a, b, c) __builtin_choose_expr(a, b, c)
+#define TYPES_COMPAT(a, b) __builtin_types_compatible_p(a, b)
+#define TYPEOF_FIELD(type, field) typeof(((type *)0)->field)
+#define BUILD_BUG() sizeof(char[-1])
+
+#define QDEV_PROP(type, field)                                     \
+    {                                                              \
+        .name = stringify(field),                                  \
+        .offset = offsetof(type, field),                           \
+        .info =                                                    \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), uint32_t),  \
+        &qdev_prop_uint32,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), int32_t),   \
+        &qdev_prop_uint32,                                         \
+        CHOOSE(TYPES_COMPAT(TYPEOF_FIELD(type, field), void *),    \
+        &qdev_prop_ptr,                                            \
+        NULL))),                                                   \
+    }
+
 extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_hex32;

Re: [Qemu-devel] [RfC PATCH] qdev: helper macros for properties

[Anthony Liguori]

Paul Brook wrote:
> On Wednesday 15 July 2009, Gerd Hoffmann wrote:
>   
>>   Hi,
>>
>> New revision of the helper macros for defining properties, going on top
>> of all other qdev patches posted today.
>>     
>
> IMO this should have been part of the initial properties rework, not a 
> followup change. Use of these macros should not be optional.
>   

I agree we want to use macros, but the property stuff is likely going to 
keep evolving as we figure out how it should be used.  For instance, 
post the property rework, there has to be a 1-1 mapping between 
properties and variables in state.  There were a number of places in the 
code where that wasn't always true.  A property was expanded to a 
different internal representation or in one case, there was no internal 
representation at all for a property as there was no internal state.

Conceptually, I think it's hard to figure out what the best policy is 
without doing a bunch of conversions.

Regards,

Anthony Liguori

> Paul
>
>
>