[Qemu-devel] [PATCHv7 1/3] qdev: add bit property type

[Qemu-devel] [PATCHv7 1/3] qdev: add bit property type

[Michael S. Tsirkin]

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>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev-properties.c |   62 ++++++++++++++++++++++++++++++++++++++++++++-----
 hw/qdev.h            |   11 +++++++++
 2 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 217ddc0..9e123ae 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -9,6 +9,59 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
     return ptr;
 }
 
+static uint32_t qdev_get_prop_mask(Property *prop)
+{
+    assert(prop->info->type == PROP_TYPE_BIT);
+    return 0x1 << prop->bitnr;
+}
+
+static void bit_prop_set(DeviceState *dev, Property *props, bool val)
+{
+    uint32_t *p = qdev_get_prop_ptr(dev, props);
+    uint32_t mask = qdev_get_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_get_prop_ptr(dev, prop);
+    return snprintf(dest, len, (*p & qdev_get_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)
@@ -511,7 +564,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) {
@@ -524,8 +576,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)
@@ -585,14 +636,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..07b9603 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -64,6 +64,7 @@ struct Property {
     const char   *name;
     PropertyInfo *info;
     int          offset;
+    int          bitnr;
     void         *defval;
 };
 
@@ -82,6 +83,7 @@ enum PropertyType {
     PROP_TYPE_NETDEV,
     PROP_TYPE_VLAN,
     PROP_TYPE_PTR,
+    PROP_TYPE_BIT,
 };
 
 struct PropertyInfo {
@@ -173,6 +175,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 +205,14 @@ 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),                           \
+        .bitnr    = (_bit),                                      \
+        .offset    = offsetof(_state, _field)                    \
+            + 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)
-- 
1.6.6.rc1.43.gf55cc

Re: [Qemu-devel] [PATCHv7 1/3] qdev: add bit property type

[Anthony Liguori]

On 01/10/2010 05:52 AM, 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.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> Acked-by: Gerd Hoffmann<kraxel@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   hw/qdev-properties.c |   62 ++++++++++++++++++++++++++++++++++++++++++++-----
>   hw/qdev.h            |   11 +++++++++
>   2 files changed, 66 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 217ddc0..9e123ae 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -9,6 +9,59 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>       return ptr;
>   }
>
> +static uint32_t qdev_get_prop_mask(Property *prop)
> +{
> +    assert(prop->info->type == PROP_TYPE_BIT);
> +    return 0x1<<  prop->bitnr;
> +}
> +
> +static void bit_prop_set(DeviceState *dev, Property *props, bool val)
> +{
> +    uint32_t *p = qdev_get_prop_ptr(dev, props);
> +    uint32_t mask = qdev_get_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_get_prop_ptr(dev, prop);
> +    return snprintf(dest, len, (*p&  qdev_get_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)
> @@ -511,7 +564,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) {
> @@ -524,8 +576,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)
> @@ -585,14 +636,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..07b9603 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -64,6 +64,7 @@ struct Property {
>       const char   *name;
>       PropertyInfo *info;
>       int          offset;
> +    int          bitnr;
>       void         *defval;
>   };
>
> @@ -82,6 +83,7 @@ enum PropertyType {
>       PROP_TYPE_NETDEV,
>       PROP_TYPE_VLAN,
>       PROP_TYPE_PTR,
> +    PROP_TYPE_BIT,
>   };
>
>   struct PropertyInfo {
> @@ -173,6 +175,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 +205,14 @@ 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),                           \
> +        .bitnr    = (_bit),                                      \
> +        .offset    = offsetof(_state, _field)                    \
> +            + 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)
>    

[Qemu-devel] [PATCHv7 2/3] virtio: rename features -> guest_features

[Michael S. Tsirkin]

Rename features->guest_features. This is
what they are, avoid confusion with
host features which we also need to keep around.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390-virtio-bus.c |    2 +-
 hw/syborg_virtio.c   |    4 ++--
 hw/virtio-net.c      |   10 +++++-----
 hw/virtio-pci.c      |    4 ++--
 hw/virtio.c          |    8 ++++----
 hw/virtio.h          |    2 +-
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index dc154ed..6c0da11 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -251,7 +251,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
     if (vdev->set_features) {
         vdev->set_features(vdev, features);
     }
-    vdev->features = features;
+    vdev->guest_features = features;
 }
 
 VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
index a84206a..fe6fc23 100644
--- a/hw/syborg_virtio.c
+++ b/hw/syborg_virtio.c
@@ -90,7 +90,7 @@ static uint32_t syborg_virtio_readl(void *opaque, target_phys_addr_t offset)
         ret |= vdev->binding->get_features(s);
         break;
     case SYBORG_VIRTIO_GUEST_FEATURES:
-        ret = vdev->features;
+        ret = vdev->guest_features;
         break;
     case SYBORG_VIRTIO_QUEUE_BASE:
         ret = virtio_queue_get_addr(vdev, vdev->queue_sel);
@@ -132,7 +132,7 @@ static void syborg_virtio_writel(void *opaque, target_phys_addr_t offset,
     case SYBORG_VIRTIO_GUEST_FEATURES:
         if (vdev->set_features)
             vdev->set_features(vdev, value);
-        vdev->features = value;
+        vdev->guest_features = value;
         break;
     case SYBORG_VIRTIO_QUEUE_BASE:
         if (value == 0)
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 2f201ff..ab20a33 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -768,11 +768,11 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         if (n->has_vnet_hdr) {
             tap_using_vnet_hdr(n->nic->nc.peer, 1);
             tap_set_offload(n->nic->nc.peer,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
-                            (n->vdev.features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
+                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_CSUM) & 1,
+                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO4) & 1,
+                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_TSO6) & 1,
+                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
+                    (n->vdev.guest_features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
         }
     }
 
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 62b46bd..4e1d5e1 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -181,7 +181,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 	}
         if (vdev->set_features)
             vdev->set_features(vdev, val);
-        vdev->features = val;
+        vdev->guest_features = val;
         break;
     case VIRTIO_PCI_QUEUE_PFN:
         pa = (target_phys_addr_t)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
@@ -239,7 +239,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
         ret |= vdev->binding->get_features(proxy);
         break;
     case VIRTIO_PCI_GUEST_FEATURES:
-        ret = vdev->features;
+        ret = vdev->guest_features;
         break;
     case VIRTIO_PCI_QUEUE_PFN:
         ret = virtio_queue_get_addr(vdev, vdev->queue_sel)
diff --git a/hw/virtio.c b/hw/virtio.c
index bb78ca8..f01548e 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -441,7 +441,7 @@ void virtio_reset(void *opaque)
     if (vdev->reset)
         vdev->reset(vdev);
 
-    vdev->features = 0;
+    vdev->guest_features = 0;
     vdev->queue_sel = 0;
     vdev->status = 0;
     vdev->isr = 0;
@@ -596,7 +596,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
     mb();
     /* Always notify when queue is empty (when feature acknowledge) */
     if ((vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT) &&
-        (!(vdev->features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) ||
+        (!(vdev->guest_features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) ||
          (vq->inuse || vring_avail_idx(vq) != vq->last_avail_idx)))
         return;
 
@@ -623,7 +623,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_8s(f, &vdev->status);
     qemu_put_8s(f, &vdev->isr);
     qemu_put_be16s(f, &vdev->queue_sel);
-    qemu_put_be32s(f, &vdev->features);
+    qemu_put_be32s(f, &vdev->guest_features);
     qemu_put_be32(f, vdev->config_len);
     qemu_put_buffer(f, vdev->config, vdev->config_len);
 
@@ -668,7 +668,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
                 features, supported_features);
         return -1;
     }
-    vdev->features = features;
+    vdev->guest_features = features;
     vdev->config_len = qemu_get_be32(f);
     qemu_get_buffer(f, vdev->config, vdev->config_len);
 
diff --git a/hw/virtio.h b/hw/virtio.h
index 35532a6..85ef171 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -100,7 +100,7 @@ struct VirtIODevice
     uint8_t status;
     uint8_t isr;
     uint16_t queue_sel;
-    uint32_t features;
+    uint32_t guest_features;
     size_t config_len;
     void *config;
     uint16_t config_vector;
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Michael S. Tsirkin]

Add feature bits as properties to virtio. This makes it possible to e.g. define
machine without indirect buffer support, which is required for 0.10
compatibility, or without hardware checksum support, which is required for 0.11
compatibility.  Since default values for optional features are now set by qdev,
get_features callback has been modified: it sets non-optional bits, and clears
bits not supported by host.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/s390-virtio-bus.c |   12 +++++++++---
 hw/s390-virtio-bus.h |    1 +
 hw/syborg_virtio.c   |   13 ++++++++-----
 hw/virtio-balloon.c  |    4 ++--
 hw/virtio-blk.c      |    6 +-----
 hw/virtio-blk.h      |    8 ++++++++
 hw/virtio-console.c  |    4 ++--
 hw/virtio-net.c      |   39 ++++++++++++++++-----------------------
 hw/virtio-net.h      |   20 ++++++++++++++++++++
 hw/virtio-pci.c      |   25 +++++++++++++++++--------
 hw/virtio.c          |    2 +-
 hw/virtio.h          |    7 ++++++-
 12 files changed, 91 insertions(+), 50 deletions(-)

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index 6c0da11..980e7eb 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -101,6 +101,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
     bus->dev_offs += dev_len;
 
     virtio_bind_device(vdev, &virtio_s390_bindings, dev);
+    dev->host_features = vdev->get_features(vdev, dev->host_features);
     s390_virtio_device_sync(dev);
 
     return 0;
@@ -222,9 +223,7 @@ static void s390_virtio_device_sync(VirtIOS390Device *dev)
     cur_offs += num_vq * VIRTIO_VQCONFIG_LEN;
 
     /* Sync feature bitmap */
-    if (dev->vdev->get_features) {
-        stl_phys(cur_offs, dev->vdev->get_features(dev->vdev));
-    }
+    stl_phys(cur_offs, dev->host_features);
 
     dev->feat_offs = cur_offs + dev->feat_len;
     cur_offs += dev->feat_len * 2;
@@ -310,10 +309,17 @@ static void virtio_s390_notify(void *opaque, uint16_t vector)
     kvm_s390_virtio_irq(s390_cpu_addr2state(0), 0, token);
 }
 
+static unsigned virtio_s390_get_features(void *opaque)
+{
+    VirtIOS390Device *dev = (VirtIOS390Device*)opaque;
+    return dev->host_features;
+}
+
 /**************** S390 Virtio Bus Device Descriptions *******************/
 
 static const VirtIOBindings virtio_s390_bindings = {
     .notify = virtio_s390_notify,
+    .get_features = virtio_s390_get_features,
 };
 
 static VirtIOS390DeviceInfo s390_virtio_net = {
diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
index ef36714..8ae2065 100644
--- a/hw/s390-virtio-bus.h
+++ b/hw/s390-virtio-bus.h
@@ -40,6 +40,7 @@ typedef struct VirtIOS390Device {
     VirtIODevice *vdev;
     DriveInfo *dinfo;
     NICConf nic;
+    uint32_t host_features;
 } VirtIOS390Device;
 
 typedef struct VirtIOS390Bus {
diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
index fe6fc23..65239a0 100644
--- a/hw/syborg_virtio.c
+++ b/hw/syborg_virtio.c
@@ -25,6 +25,7 @@
 #include "syborg.h"
 #include "sysbus.h"
 #include "virtio.h"
+#include "virtio-net.h"
 #include "sysemu.h"
 
 //#define DEBUG_SYBORG_VIRTIO
@@ -66,6 +67,7 @@ typedef struct {
     uint32_t int_enable;
     uint32_t id;
     NICConf nic;
+    uint32_t host_features;
 } SyborgVirtIOProxy;
 
 static uint32_t syborg_virtio_readl(void *opaque, target_phys_addr_t offset)
@@ -86,8 +88,7 @@ static uint32_t syborg_virtio_readl(void *opaque, target_phys_addr_t offset)
         ret = s->id;
         break;
     case SYBORG_VIRTIO_HOST_FEATURES:
-        ret = vdev->get_features(vdev);
-        ret |= vdev->binding->get_features(s);
+        ret = s->host_features;
         break;
     case SYBORG_VIRTIO_GUEST_FEATURES:
         ret = vdev->guest_features;
@@ -244,9 +245,8 @@ static void syborg_virtio_update_irq(void *opaque, uint16_t vector)
 
 static unsigned syborg_virtio_get_features(void *opaque)
 {
-    unsigned ret = 0;
-    ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
-    return ret;
+    SyborgVirtIOProxy *proxy = opaque;
+    return proxy->host_features;
 }
 
 static VirtIOBindings syborg_virtio_bindings = {
@@ -272,6 +272,8 @@ static int syborg_virtio_init(SyborgVirtIOProxy *proxy, VirtIODevice *vdev)
     qemu_register_reset(virtio_reset, vdev);
 
     virtio_bind_device(vdev, &syborg_virtio_bindings, proxy);
+    proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+    proxy->host_features = vdev->get_features(vdev, proxy->host_features);
     return 0;
 }
 
@@ -292,6 +294,7 @@ static SysBusDeviceInfo syborg_virtio_net_info = {
     .qdev.size  = sizeof(SyborgVirtIOProxy),
     .qdev.props = (Property[]) {
         DEFINE_NIC_PROPERTIES(SyborgVirtIOProxy, nic),
+        DEFINE_VIRTIO_NET_FEATURES(SyborgVirtIOProxy, host_features),
         DEFINE_PROP_END_OF_LIST(),
     }
 };
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index cfd3b41..e17880f 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -124,9 +124,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     dev->actual = config.actual;
 }
 
-static uint32_t virtio_balloon_get_features(VirtIODevice *vdev)
+static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
 {
-    return 0;
+    return f;
 }
 
 static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index a2f0639..cb1ae1f 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -432,19 +432,15 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
     memcpy(config, &blkcfg, s->config_size);
 }
 
-static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
+static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
-    uint32_t features = 0;
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
     features |= (1 << VIRTIO_BLK_F_GEOMETRY);
 
     if (bdrv_enable_write_cache(s->bs))
         features |= (1 << VIRTIO_BLK_F_WCACHE);
-#ifdef __linux__
-    features |= (1 << VIRTIO_BLK_F_SCSI);
-#endif
     if (strcmp(s->serial_str, "0"))
         features |= 1 << VIRTIO_BLK_F_IDENTIFY;
     
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 23ad74c..c28f776 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -92,4 +92,12 @@ struct virtio_scsi_inhdr
     uint32_t residual;
 };
 
+#ifdef __linux__
+#define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
+        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
+        DEFINE_PROP_BIT("scsi", _state, _field, VIRTIO_BLK_F_SCSI, true)
+#else
+#define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
+        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
+#endif
 #endif
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 57f8f89..4f18ef2 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -51,9 +51,9 @@ static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
 }
 
-static uint32_t virtio_console_get_features(VirtIODevice *vdev)
+static uint32_t virtio_console_get_features(VirtIODevice *vdev, uint32_t f)
 {
-    return 0;
+    return f;
 }
 
 static int vcon_can_read(void *opaque)
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ab20a33..c2a389f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -147,34 +147,27 @@ static int peer_has_ufo(VirtIONet *n)
     return n->has_ufo;
 }
 
-static uint32_t virtio_net_get_features(VirtIODevice *vdev)
+static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 {
     VirtIONet *n = to_virtio_net(vdev);
-    uint32_t features = (1 << VIRTIO_NET_F_MAC) |
-                        (1 << VIRTIO_NET_F_MRG_RXBUF) |
-                        (1 << VIRTIO_NET_F_STATUS) |
-                        (1 << VIRTIO_NET_F_CTRL_VQ) |
-                        (1 << VIRTIO_NET_F_CTRL_RX) |
-                        (1 << VIRTIO_NET_F_CTRL_VLAN) |
-                        (1 << VIRTIO_NET_F_CTRL_RX_EXTRA);
 
     if (peer_has_vnet_hdr(n)) {
         tap_using_vnet_hdr(n->nic->nc.peer, 1);
+    } else {
+        features &= ~(0x1 << VIRTIO_NET_F_CSUM);
+        features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
+        features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO6);
+        features &= ~(0x1 << VIRTIO_NET_F_HOST_ECN);
+
+        features &= ~(0x1 << VIRTIO_NET_F_GUEST_CSUM);
+        features &= ~(0x1 << VIRTIO_NET_F_GUEST_TSO4);
+        features &= ~(0x1 << VIRTIO_NET_F_GUEST_TSO6);
+        features &= ~(0x1 << VIRTIO_NET_F_GUEST_ECN);
+    }
 
-        features |= (1 << VIRTIO_NET_F_CSUM);
-        features |= (1 << VIRTIO_NET_F_HOST_TSO4);
-        features |= (1 << VIRTIO_NET_F_HOST_TSO6);
-        features |= (1 << VIRTIO_NET_F_HOST_ECN);
-
-        features |= (1 << VIRTIO_NET_F_GUEST_CSUM);
-        features |= (1 << VIRTIO_NET_F_GUEST_TSO4);
-        features |= (1 << VIRTIO_NET_F_GUEST_TSO6);
-        features |= (1 << VIRTIO_NET_F_GUEST_ECN);
-
-        if (peer_has_ufo(n)) {
-            features |= (1 << VIRTIO_NET_F_GUEST_UFO);
-            features |= (1 << VIRTIO_NET_F_HOST_UFO);
-        }
+    if (!peer_has_vnet_hdr(n) || !peer_has_ufo(n)) {
+        features &= ~(0x1 << VIRTIO_NET_F_GUEST_UFO);
+        features &= ~(0x1 << VIRTIO_NET_F_HOST_UFO);
     }
 
     return features;
@@ -192,7 +185,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
     features |= (1 << VIRTIO_NET_F_HOST_TSO6);
     features |= (1 << VIRTIO_NET_F_HOST_ECN);
 
-    return features & virtio_net_get_features(vdev);
+    return features;
 }
 
 static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 2085181..9130d75 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -153,4 +153,24 @@ struct virtio_net_ctrl_mac {
  #define VIRTIO_NET_CTRL_VLAN_ADD             0
  #define VIRTIO_NET_CTRL_VLAN_DEL             1
 
+#define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
+        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
+        DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
+        DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
+        DEFINE_PROP_BIT("mac", _state, _field, VIRTIO_NET_F_MAC, true), \
+        DEFINE_PROP_BIT("gso", _state, _field, VIRTIO_NET_F_GSO, true), \
+        DEFINE_PROP_BIT("guest_tso4", _state, _field, VIRTIO_NET_F_GUEST_TSO4, true), \
+        DEFINE_PROP_BIT("guest_tso6", _state, _field, VIRTIO_NET_F_GUEST_TSO6, true), \
+        DEFINE_PROP_BIT("guest_ecn", _state, _field, VIRTIO_NET_F_GUEST_ECN, true), \
+        DEFINE_PROP_BIT("guest_ufo", _state, _field, VIRTIO_NET_F_GUEST_UFO, true), \
+        DEFINE_PROP_BIT("host_tso4", _state, _field, VIRTIO_NET_F_HOST_TSO4, true), \
+        DEFINE_PROP_BIT("host_tso6", _state, _field, VIRTIO_NET_F_HOST_TSO6, true), \
+        DEFINE_PROP_BIT("host_ecn", _state, _field, VIRTIO_NET_F_HOST_ECN, true), \
+        DEFINE_PROP_BIT("host_ufo", _state, _field, VIRTIO_NET_F_HOST_UFO, true), \
+        DEFINE_PROP_BIT("mrg_rxbuf", _state, _field, VIRTIO_NET_F_MRG_RXBUF, true), \
+        DEFINE_PROP_BIT("status", _state, _field, VIRTIO_NET_F_STATUS, true), \
+        DEFINE_PROP_BIT("ctrl_vq", _state, _field, VIRTIO_NET_F_CTRL_VQ, true), \
+        DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
+        DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
+        DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true)
 #endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 4e1d5e1..6d0f9dd 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -16,6 +16,8 @@
 #include <inttypes.h>
 
 #include "virtio.h"
+#include "virtio-blk.h"
+#include "virtio-net.h"
 #include "pci.h"
 #include "sysemu.h"
 #include "msix.h"
@@ -92,6 +94,7 @@ typedef struct {
     uint32_t nvectors;
     DriveInfo *dinfo;
     NICConf nic;
+    uint32_t host_features;
 } VirtIOPCIProxy;
 
 /* virtio device */
@@ -175,7 +178,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 	/* Guest does not negotiate properly?  We have to assume nothing. */
 	if (val & (1 << VIRTIO_F_BAD_FEATURE)) {
 	    if (vdev->bad_features)
-		val = vdev->bad_features(vdev);
+		val = proxy->host_features & vdev->bad_features(vdev);
 	    else
 		val = 0;
 	}
@@ -235,8 +238,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
 
     switch (addr) {
     case VIRTIO_PCI_HOST_FEATURES:
-        ret = vdev->get_features(vdev);
-        ret |= vdev->binding->get_features(proxy);
+        ret = proxy->host_features;
         break;
     case VIRTIO_PCI_GUEST_FEATURES:
         ret = vdev->guest_features;
@@ -382,11 +384,8 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
 
 static unsigned virtio_pci_get_features(void *opaque)
 {
-    unsigned ret = 0;
-    ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
-    ret |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
-    ret |= (1 << VIRTIO_F_BAD_FEATURE);
-    return ret;
+    VirtIOPCIProxy *proxy = opaque;
+    return proxy->host_features;
 }
 
 static const VirtIOBindings virtio_pci_bindings = {
@@ -442,6 +441,9 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
                            virtio_map);
 
     virtio_bind_device(vdev, &virtio_pci_bindings, proxy);
+    proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
+    proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
+    proxy->host_features = vdev->get_features(vdev, proxy->host_features);
 }
 
 static int virtio_blk_init_pci(PCIDevice *pci_dev)
@@ -553,6 +555,7 @@ static PCIDeviceInfo virtio_info[] = {
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
             DEFINE_PROP_DRIVE("drive", VirtIOPCIProxy, dinfo),
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
+            DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_PROP_END_OF_LIST(),
         },
         .qdev.reset = virtio_pci_reset,
@@ -564,6 +567,7 @@ static PCIDeviceInfo virtio_info[] = {
         .romfile    = "pxe-virtio.bin",
         .qdev.props = (Property[]) {
             DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
+            DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
             DEFINE_PROP_END_OF_LIST(),
         },
@@ -575,6 +579,7 @@ static PCIDeviceInfo virtio_info[] = {
         .exit      = virtio_exit_pci,
         .qdev.props = (Property[]) {
             DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
+            DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
             DEFINE_PROP_END_OF_LIST(),
         },
         .qdev.reset = virtio_pci_reset,
@@ -583,6 +588,10 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_balloon_init_pci,
         .exit      = virtio_exit_pci,
+        .qdev.props = (Property[]) {
+            DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+            DEFINE_PROP_END_OF_LIST(),
+        },
         .qdev.reset = virtio_pci_reset,
     },{
         /* end of list */
diff --git a/hw/virtio.c b/hw/virtio.c
index f01548e..3c609ce 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -650,7 +650,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
 {
     int num, i, ret;
     uint32_t features;
-    uint32_t supported_features = vdev->get_features(vdev) |
+    uint32_t supported_features =
         vdev->binding->get_features(vdev->binding_opaque);
 
     if (vdev->binding->load_config) {
diff --git a/hw/virtio.h b/hw/virtio.h
index 85ef171..3994cc9 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -105,7 +105,7 @@ struct VirtIODevice
     void *config;
     uint16_t config_vector;
     int nvectors;
-    uint32_t (*get_features)(VirtIODevice *vdev);
+    uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features);
     uint32_t (*bad_features)(VirtIODevice *vdev);
     void (*set_features)(VirtIODevice *vdev, uint32_t val);
     void (*get_config)(VirtIODevice *vdev, uint8_t *config);
@@ -176,4 +176,9 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev);
 
 void virtio_net_exit(VirtIODevice *vdev);
 
+#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
+	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
+			VIRTIO_RING_F_INDIRECT_DESC, true)
+
+
 #endif
-- 
1.6.6.rc1.43.gf55cc

[Qemu-devel] Re: [PATCHv7 3/3] virtio: add features as qdev properties

[Gerd Hoffmann]

   Hi,

> +#define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
> +        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
> +        DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
> +        DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
> +        DEFINE_PROP_BIT("mac", _state, _field, VIRTIO_NET_F_MAC, true), \

Didn't noticed in review, sorry.  This isn't going to work as all 
network cards already have a 'mac' property to set the mac address.  Try 
to create a virtio nic with a non-default mac address and watch qemu fail.

cheers,
   Gerd

[Qemu-devel] Re: [PATCHv7 3/3] virtio: add features as qdev properties

[Michael S. Tsirkin]

On Tue, Jan 12, 2010 at 06:05:54PM +0100, Gerd Hoffmann wrote:
>   Hi,
>
>> +#define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
>> +        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
>> +        DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
>> +        DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
>> +        DEFINE_PROP_BIT("mac", _state, _field, VIRTIO_NET_F_MAC, true), \
>
> Didn't noticed in review, sorry.  This isn't going to work as all  
> network cards already have a 'mac' property to set the mac address.  Try  
> to create a virtio nic with a non-default mac address and watch qemu 
> fail.
>
> cheers,
>   Gerd

Fixed, thanks!

Re: [Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Christoph Hellwig]

This patch causes 100% reproducible boot panics in a Linux guest using
virtio.

My qemu command line is:

/opt/qemu/bin/qemu-system-x86_64 \
        -m 1500 \
	-enable-kvm \
	-drive file=/dev/vg00/qemu-root,if=virtio,media=disk,cache=none,aio=threads \
	-kernel arch/x86/boot/bzImage \
	-append "root=/dev/vda console=tty0 console=ttyS0,38400n8" \
	-nographic

and the guest dmesg is:

[    2.578083] virtio-pci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI
10 (level, high) -> IRQ 10
[    2.583111] blk_queue_max_segment_size: set to minimum 4096
[    2.584651]  vda:
[    2.585075] BUG: unable to handle kernel NULL pointer dereference at (null)
[    2.587060] IP: [<c0228448>] create_empty_buffers+0x18/0xa0
[    2.588403] *pde = 00000000 
[    2.588403] Oops: 0002 [#1] SMP 
[    2.588403] last sysfs file: 
[    2.588403] Modules linked in:
[    2.588403] 
[    2.588403] Pid: 1, comm: swapper Not tainted 2.6.33-rc3-xfs #398 /Bochs
[    2.588403] EIP: 0060:[<c0228448>] EFLAGS: 00010296 CPU: 0
[    2.588403] EIP is at create_empty_buffers+0x18/0xa0
[    2.588403] EAX: 00000000 EBX: c1d97e80 ECX: 00000001 EDX: 00010000
[    2.588403] ESI: 00000000 EDI: 00000000 EBP: f7043b08 ESP: f7043afc
[    2.588403]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[    2.588403] Process swapper (pid: 1, ti=f7042000 task=f7040c70 task.ti=f7042000)
[    2.588403] Stack:
[    2.588403]  c1d97e80 00000000 f6c02848 f7043b78 c022a42d 00000007 f7043b2c c0194c92
[    2.588403] <0> c022d570 c1d97e80 c0900d52 f6c02988 f7043b3c c0194fc4 f6c02998 00010000
[    2.588403] <0> f7043b44 c019501b f7043b50 c0900d52 c1d97e80 f7043b6c c01d3eee 00000000
[    2.588403] Call Trace:
[    2.588403]  [<c022a42d>] ? block_read_full_page+0x2ed/0x340
[    2.588403]  [<c0194c92>] ? mark_held_locks+0x62/0x80
[    2.588403]  [<c022d570>] ? blkdev_get_block+0x0/0xd0
[    2.588403]  [<c0900d52>] ? _raw_spin_unlock_irq+0x22/0x30
[    2.588403]  [<c0194fc4>] ? trace_hardirqs_on_caller+0x134/0x180
[    2.588403]  [<c019501b>] ? trace_hardirqs_on+0xb/0x10
[    2.588403]  [<c0900d52>] ? _raw_spin_unlock_irq+0x22/0x30
[    2.588403]  [<c01d3eee>] ? add_to_page_cache_locked+0x7e/0xc0
[    2.588403]  [<c022e6ff>] ? blkdev_readpage+0xf/0x20
[    2.588403]  [<c01d4e13>] ? read_cache_page_async+0x73/0x140
[    2.588403]  [<c022e6f0>] ? blkdev_readpage+0x0/0x20
[    2.588403]  [<c01d4ef2>] ? read_cache_page+0x12/0x60
[    2.588403]  [<c0256645>] ? read_dev_sector+0x35/0x80
[    2.588403]  [<c0257661>] ? adfspart_check_ICS+0x21/0x190
[    2.588403]  [<c067a85a>] ? snprintf+0x1a/0x20
[    2.588403]  [<c025705f>] ? disk_name+0xaf/0xc0
[    2.588403]  [<c025721d>] ? rescan_partitions+0x1ad/0x4a0
[    2.588403]  [<c0257640>] ? adfspart_check_ICS+0x0/0x190
[    2.588403]  [<c066a5d4>] ? disk_get_part+0x74/0x90
[    2.588403]  [<c022ed4c>] ? __blkdev_get+0x17c/0x370
[    2.588403]  [<c0673a00>] ? kobject_put+0x20/0x50
[    2.588403]  [<c022ef4a>] ? blkdev_get+0xa/0x10
[    2.588403]  [<c025677f>] ? register_disk+0xef/0x110
[    2.588403]  [<c0669cc9>] ? add_disk+0xd9/0x130
[    2.588403]  [<c0669120>] ? exact_match+0x0/0x10
[    2.588403]  [<c06697a0>] ? exact_lock+0x0/0x20
[    2.588403]  [<c08eda09>] ? virtblk_probe+0x329/0x450
[    2.588403]  [<c06fbab0>] ? blk_done+0x0/0xc0
[    2.588403]  [<c07e1edc>] ? virtio_dev_probe+0xbc/0x100
[    2.588403]  [<c06ef339>] ? driver_probe_device+0x69/0x170
[    2.588403]  [<c0900ddd>] ? _raw_spin_unlock+0x1d/0x20
[    2.588403]  [<c06ef501>] ? __device_attach+0x41/0x50
[    2.588403]  [<c06ee8e3>] ? bus_for_each_drv+0x53/0x80
[    2.588403]  [<c06ef5ab>] ? device_attach+0x6b/0x70
[    2.588403]  [<c06ef4c0>] ? __device_attach+0x0/0x50
[    2.588403]  [<c06ee6c7>] ? bus_probe_device+0x27/0x40
[    2.588403]  [<c06ecec2>] ? device_add+0x4b2/0x600
[    2.588403]  [<c0192edd>] ? lockdep_init_map+0x3d/0x110
[    2.588403]  [<c06801a2>] ? __raw_spin_lock_init+0x32/0x60
[    2.588403]  [<c06ec5e7>] ? device_initialize+0x97/0xc0
[    2.588403]  [<c06ed022>] ? device_register+0x12/0x20
[    2.588403]  [<c07e1fc8>] ? register_virtio_device+0x68/0x90
[    2.588403]  [<c08f3c47>] ? virtio_pci_probe+0x14b/0x1b4
[    2.588403]  [<c0900ddd>] ? _raw_spin_unlock+0x1d/0x20
[    2.588403]  [<c068feae>] ? local_pci_probe+0xe/0x10
[    2.588403]  [<c06900e0>] ? pci_device_probe+0x60/0x80
[    2.588403]  [<c06ef339>] ? driver_probe_device+0x69/0x170
[    2.588403]  [<c06ef4b9>] ? __driver_attach+0x79/0x80
[    2.588403]  [<c06eeba3>] ? bus_for_each_dev+0x53/0x80
[    2.588403]  [<c06ef1e9>] ? driver_attach+0x19/0x20
[    2.588403]  [<c06ef440>] ? __driver_attach+0x0/0x80
[    2.588403]  [<c06ee517>] ? bus_add_driver+0x1f7/0x2c0
[    2.588403]  [<c0690020>] ? pci_device_remove+0x0/0x40
[    2.588403]  [<c06ef735>] ? driver_register+0x75/0x160
[    2.588403]  [<c0690324>] ? __pci_register_driver+0x54/0xc0
[    2.588403]  [<c0cd302d>] ? virtio_pci_init+0x34/0x47
[    2.588403]  [<c0cd2ff9>] ? virtio_pci_init+0x0/0x47
[    2.588403]  [<c0101033>] ? do_one_initcall+0x23/0x190
[    2.588403]  [<c01afc17>] ? init_irq_proc+0x67/0x80
[    2.588403]  [<c0ca5355>] ? kernel_init+0x130/0x189
[    2.588403]  [<c0ca5225>] ? kernel_init+0x0/0x189
[    2.588403]  [<c012f5ba>] ? kernel_thread_helper+0x6/0x1c
[    2.588403] Code: c0 31 f6 eb c1 8d b4 26 00 00 00 00 8d bc 27 00 00 00 00 55 89 e5 57 56 89 ce 53 b9 01 00 00 00 89 c3 e8 1c ff ff ff 89 c7 66 90 <09> 30 89 c2 8b 40 04 85 c0 75 f5 89 7a 04 8b 43 10 05 84 00 00

Re: [Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Michael S. Tsirkin]

On Tue, Jan 12, 2010 at 07:09:27PM +0100, Christoph Hellwig wrote:
> This patch causes 100% reproducible boot panics in a Linux guest using
> virtio.
> 
> My qemu command line is:
> 
> /opt/qemu/bin/qemu-system-x86_64 \
>         -m 1500 \
> 	-enable-kvm \
> 	-drive file=/dev/vg00/qemu-root,if=virtio,media=disk,cache=none,aio=threads \
> 	-kernel arch/x86/boot/bzImage \
> 	-append "root=/dev/vda console=tty0 console=ttyS0,38400n8" \
> 	-nographic
> 
> and the guest dmesg is:
> 
> [    2.578083] virtio-pci 0000:00:04.0: PCI INT A -> Link[LNKD] -> GSI
> 10 (level, high) -> IRQ 10
> [    2.583111] blk_queue_max_segment_size: set to minimum 4096
> [    2.584651]  vda:
> [    2.585075] BUG: unable to handle kernel NULL pointer dereference at (null)

Um, I see this too. Debugging.

-- 
MST

Re: [Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Michael S. Tsirkin]

On Tue, Jan 12, 2010 at 07:09:27PM +0100, Christoph Hellwig wrote:
> This patch causes 100% reproducible boot panics in a Linux guest using
> virtio.
> 
> My qemu command line is:
> 
> /opt/qemu/bin/qemu-system-x86_64 \
>         -m 1500 \
> 	-enable-kvm \
> 	-drive file=/dev/vg00/qemu-root,if=virtio,media=disk,cache=none,aio=threads \
> 	-kernel arch/x86/boot/bzImage \
> 	-append "root=/dev/vda console=tty0 console=ttyS0,38400n8" \
> 	-nographic
> 
> and the guest dmesg is:

So the issue is that wrong block size (0xffffffff) was passed
to guest.  Would it make sense to add some sanity checking in virtio-blk
to make it not crash but fail in probe? Which block size values
are sane?

-- 
MST

Re: [Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Christoph Hellwig]

On Tue, Jan 12, 2010 at 09:50:55PM +0200, Michael S. Tsirkin wrote:
> So the issue is that wrong block size (0xffffffff) was passed
> to guest.  Would it make sense to add some sanity checking in virtio-blk
> to make it not crash but fail in probe? Which block size values
> are sane?

Yes, I'll cook up a patch.  Basically powers of two up from 512 bytes
are theoretically sane.  In practice I doubt we'll ever see anything
other than 512 or 4095 bytes.

Re: [Qemu-devel] [PATCHv7 3/3] virtio: add features as qdev properties

[Michael S. Tsirkin]

On Tue, Jan 12, 2010 at 11:06:33PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 12, 2010 at 09:50:55PM +0200, Michael S. Tsirkin wrote:
> > So the issue is that wrong block size (0xffffffff) was passed
> > to guest.  Would it make sense to add some sanity checking in virtio-blk
> > to make it not crash but fail in probe? Which block size values
> > are sane?
> 
> Yes, I'll cook up a patch.  Basically powers of two up from 512 bytes
> are theoretically sane.  In practice I doubt we'll ever see anything
> other than 512 or 4095 bytes.

I also noticed that 0x8000 causes a crash. You can play with other
sizes and see what happens.

-- 
MST