[PATCHv4,1/2] qdev: add bit property type

Submitted by Michael S. Tsirkin on Dec. 24, 2009, 12:43 p.m.

Details

Message ID 20091224124358.GB31553@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Dec. 24, 2009, 12:43 p.m.
This adds "bit" property type, which is a boolean stored in a 32 bit
integer field, with legal values on and off.  Will be used by virtio for
feature bits.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/qdev-properties.c |   70 +++++++++++++++++++++++++++++++++++++++++++++-----
 hw/qdev.h            |    9 ++++++
 2 files changed, 72 insertions(+), 7 deletions(-)

Comments

Gerd Hoffmann Jan. 4, 2010, 2:22 p.m.
On 12/24/09 13:43, Michael S. Tsirkin wrote:
> This adds "bit" property type, which is a boolean stored in a 32 bit
> integer field, with legal values on and off.  Will be used by virtio for
> feature bits.

> +static void *qdev_bit_prop_ptr(DeviceState *dev, Property *prop)
> +{
> +    void *ptr = dev;
> +    assert(prop->info->type == PROP_TYPE_BIT);
> +    ptr += prop->offset / 32;
> +    return ptr;
> +}

I don't like that idea.  IMHO prop->offset should be specified in bytes 
no matter what the property is.  Better add a new prop->bitnr field to 
specify which bit of the 32bit word should be toggled.  Then you don't 
need this function in the first place.

cheers,
   Gerd
Michael S. Tsirkin Jan. 4, 2010, 3:38 p.m.
On Mon, Jan 04, 2010 at 03:22:36PM +0100, Gerd Hoffmann wrote:
> On 12/24/09 13:43, Michael S. Tsirkin wrote:
>> This adds "bit" property type, which is a boolean stored in a 32 bit
>> integer field, with legal values on and off.  Will be used by virtio for
>> feature bits.
>
>> +static void *qdev_bit_prop_ptr(DeviceState *dev, Property *prop)
>> +{
>> +    void *ptr = dev;
>> +    assert(prop->info->type == PROP_TYPE_BIT);
>> +    ptr += prop->offset / 32;
>> +    return ptr;
>> +}
>
> I don't like that idea.  IMHO prop->offset should be specified in bytes  
> no matter what the property is.  Better add a new prop->bitnr field to  
> specify which bit of the 32bit word should be toggled.  Then you don't  
> need this function in the first place.
>
> cheers,
>   Gerd

OK, I reworked the patches in this way.

Patch hide | download patch | download mbox

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index fb07279..3782609 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -9,6 +9,67 @@  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
     return ptr;
 }
 
+static void *qdev_bit_prop_ptr(DeviceState *dev, Property *prop)
+{
+    void *ptr = dev;
+    assert(prop->info->type == PROP_TYPE_BIT);
+    ptr += prop->offset / 32;
+    return ptr;
+}
+
+static uint32_t qdev_bit_prop_mask(Property *prop)
+{
+    assert(prop->info->type == PROP_TYPE_BIT);
+    return 0x1 << (prop->offset % 32);
+}
+
+static void bit_prop_set(DeviceState *dev, Property *props, bool val)
+{
+    uint32_t *p = qdev_bit_prop_ptr(dev, props);
+    uint32_t mask = qdev_bit_prop_mask(props);
+    if (val)
+        *p |= ~mask;
+    else
+        *p &= ~mask;
+}
+
+static void qdev_prop_cpy(DeviceState *dev, Property *props, void *src)
+{
+    if (props->info->type == PROP_TYPE_BIT) {
+        bool *defval = src;
+        bit_prop_set(dev, props, *defval);
+    } else {
+        char *dst = qdev_get_prop_ptr(dev, props);
+        memcpy(dst, src, props->info->size);
+    }
+}
+
+/* Bit */
+static int parse_bit(DeviceState *dev, Property *prop, const char *str)
+{
+    if (!strncasecmp(str, "on", 2))
+        bit_prop_set(dev, prop, true);
+    else if (!strncasecmp(str, "off", 3))
+        bit_prop_set(dev, prop, false);
+    else
+        return -1;
+    return 0;
+}
+
+static int print_bit(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    uint8_t *p = qdev_bit_prop_ptr(dev, prop);
+    return snprintf(dest, len, (*p & qdev_bit_prop_mask(prop)) ? "on" : "off");
+}
+
+PropertyInfo qdev_prop_bit = {
+    .name  = "on/off",
+    .type  = PROP_TYPE_BIT,
+    .size  = sizeof(uint32_t),
+    .parse = parse_bit,
+    .print = print_bit,
+};
+
 /* --- 8bit integer --- */
 
 static int parse_uint8(DeviceState *dev, Property *prop, const char *str)
@@ -506,7 +567,6 @@  int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type)
 {
     Property *prop;
-    void *dst;
 
     prop = qdev_prop_find(dev, name);
     if (!prop) {
@@ -519,8 +579,7 @@  void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyT
                 __FUNCTION__, dev->info->name, name);
         abort();
     }
-    dst = qdev_get_prop_ptr(dev, prop);
-    memcpy(dst, src, prop->info->size);
+    qdev_prop_cpy(dev, prop, src);
 }
 
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value)
@@ -580,14 +639,11 @@  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 {
-    char *dst;
-
     if (!props)
         return;
     while (props->name) {
         if (props->defval) {
-            dst = qdev_get_prop_ptr(dev, props);
-            memcpy(dst, props->defval, props->info->size);
+            qdev_prop_cpy(dev, props, props->defval);
         }
         props++;
     }
diff --git a/hw/qdev.h b/hw/qdev.h
index bbcdba1..26853e0 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -82,6 +82,7 @@  enum PropertyType {
     PROP_TYPE_NETDEV,
     PROP_TYPE_VLAN,
     PROP_TYPE_PTR,
+    PROP_TYPE_BIT,
 };
 
 struct PropertyInfo {
@@ -173,6 +174,7 @@  void do_device_del(Monitor *mon, const QDict *qdict);
 
 /*** qdev-properties.c ***/
 
+extern PropertyInfo qdev_prop_bit;
 extern PropertyInfo qdev_prop_uint8;
 extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
@@ -202,6 +204,13 @@  extern PropertyInfo qdev_prop_pci_devfn;
             + type_check(_type,typeof_field(_state, _field)),           \
         .defval    = (_type[]) { _defval },                             \
         }
+#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \
+        .name      = (_name),                                             \
+        .info      = &(qdev_prop_bit),                                    \
+        .offset    = offsetof(_state, _field) * 32 + (_bit)                \
+            + type_check(uint32_t,typeof_field(_state, _field)),             \
+        .defval    = (bool[]) { (_defval) }, \
+        }
 
 #define DEFINE_PROP_UINT8(_n, _s, _f, _d)                       \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)