[Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Denis V. Lunev]

It is very convinient and useful for debug purpose to expose the set of
features negotiated with guest. The patch exports those features via
read-only bit properties.

This patchset is a proof-of-concept on a base of virtio network device.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Eric Blake <eblake@redhat.com>

Denis V. Lunev (2):
  qdev: add read-only bit/bit64 property
  virtio: show features acked by guest in 'info qtree' dump

 hw/core/qdev-properties.c    | 29 +++++++++++++++++++++++++++--
 hw/net/virtio-net.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h | 18 ++++++++++++++++++
 3 files changed, 89 insertions(+), 2 deletions(-)

-- 
2.5.0

[Qemu-devel] [RFC 1/2] qdev: add read-only bit/bit64 property

[Denis V. Lunev]

It would be very nice to expose VirtIO feature bits negotiated with guest
in 'info qtree'. Bits are exposed using QDEV framework using properties.
Though the usage of DEFINE_PROP_BIT is not welcome here. In this case
this bit could be set from the command line, which is badly wrong.

The patch adds read-only bit property, which will be used in the
subsequent patch for VirtIO net.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Eric Blake <eblake@redhat.com>
--
 hw/core/qdev-properties.c    | 29 +++++++++++++++++++++++++++--
 include/hw/qdev-properties.h | 18 ++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
---
 hw/core/qdev-properties.c    | 29 +++++++++++++++++++++++++++--
 include/hw/qdev-properties.h | 18 ++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 737d29c..d1827eb 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -73,7 +73,7 @@ static void set_enum(Object *obj, Visitor *v, const char *name, void *opaque,
 
 static uint32_t qdev_get_prop_mask(Property *prop)
 {
-    assert(prop->info == &qdev_prop_bit);
+    assert(prop->info == &qdev_prop_bit || prop->info == &qdev_prop_ro_bit);
     return 0x1 << prop->bitnr;
 }
 
@@ -127,11 +127,24 @@ PropertyInfo qdev_prop_bit = {
     .set   = prop_set_bit,
 };
 
+
+static void prop_noset_bit(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+}
+
+PropertyInfo qdev_prop_ro_bit = {
+    .name  = "bool",
+    .description = "on/off",
+    .get   = prop_get_bit,
+    .set   = prop_noset_bit,
+};
+
 /* Bit64 */
 
 static uint64_t qdev_get_prop_mask64(Property *prop)
 {
-    assert(prop->info == &qdev_prop_bit64);
+    assert(prop->info == &qdev_prop_bit64 || prop->info == &qdev_prop_ro_bit64);
     return 0x1ull << prop->bitnr;
 }
 
@@ -185,6 +198,18 @@ PropertyInfo qdev_prop_bit64 = {
     .set   = prop_set_bit64,
 };
 
+static void prop_noset_bit64(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+}
+
+PropertyInfo qdev_prop_ro_bit64 = {
+    .name  = "bool",
+    .description = "on/off",
+    .get   = prop_get_bit64,
+    .set   = prop_noset_bit64,
+};
+
 /* --- bool --- */
 
 static void get_bool(Object *obj, Visitor *v, const char *name, void *opaque,
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0586cac..8344ea5 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -6,7 +6,9 @@
 /*** qdev-properties.c ***/
 
 extern PropertyInfo qdev_prop_bit;
+extern PropertyInfo qdev_prop_ro_bit;
 extern PropertyInfo qdev_prop_bit64;
+extern PropertyInfo qdev_prop_ro_bit64;
 extern PropertyInfo qdev_prop_bool;
 extern PropertyInfo qdev_prop_uint8;
 extern PropertyInfo qdev_prop_uint16;
@@ -53,6 +55,14 @@ extern PropertyInfo qdev_prop_arraylen;
         .qtype     = QTYPE_QBOOL,                                \
         .defval    = (bool)_defval,                              \
         }
+#define DEFINE_PROP_RO_BIT(_name, _state, _field, _bit) {        \
+        .name      = (_name),                                    \
+        .info      = &(qdev_prop_ro_bit),                        \
+        .bitnr    = (_bit),                                      \
+        .offset    = offsetof(_state, _field)                    \
+            + type_check(uint32_t,typeof_field(_state, _field)), \
+        .qtype     = QTYPE_QBOOL,                                \
+        }
 #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) {       \
         .name      = (_name),                                           \
         .info      = &(qdev_prop_bit64),                                \
@@ -62,6 +72,14 @@ extern PropertyInfo qdev_prop_arraylen;
         .qtype     = QTYPE_QBOOL,                                       \
         .defval    = (bool)_defval,                                     \
         }
+#define DEFINE_PROP_RO_BIT64(_name, _state, _field, _bit) {             \
+        .name      = (_name),                                           \
+        .info      = &(qdev_prop_ro_bit64),                             \
+        .bitnr    = (_bit),                                             \
+        .offset    = offsetof(_state, _field)                           \
+            + type_check(uint64_t, typeof_field(_state, _field)),       \
+        .qtype     = QTYPE_QBOOL,                                       \
+        }
 
 #define DEFINE_PROP_BOOL(_name, _state, _field, _defval) {       \
         .name      = (_name),                                    \
-- 
2.5.0

[Qemu-devel] [RFC 2/2] virtio: show features acked by guest in 'info qtree' dump

[Denis V. Lunev]

It is very convinient and useful for debug purpose to expose the set of
features negotiated with guest. The patch exports those features via
read-only bit properties.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hw/net/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5798f87..cb9ce9c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1904,6 +1904,50 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("ctrl_guest_offloads", VirtIONet, host_features,
                     VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true),
     DEFINE_PROP_BIT("mq", VirtIONet, host_features, VIRTIO_NET_F_MQ, false),
+
+    DEFINE_PROP_RO_BIT64("csum_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CSUM),
+    DEFINE_PROP_RO_BIT64("guest_csum_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_CSUM),
+    DEFINE_PROP_RO_BIT64("gso_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GSO),
+    DEFINE_PROP_RO_BIT64("guest_tso4_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_TSO4),
+    DEFINE_PROP_RO_BIT64("guest_tso6_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_TSO6),
+    DEFINE_PROP_RO_BIT64("guest_ecn_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_ECN),
+    DEFINE_PROP_RO_BIT64("guest_ufo_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_UFO),
+    DEFINE_PROP_RO_BIT64("guest_announce_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_GUEST_ANNOUNCE),
+    DEFINE_PROP_RO_BIT64("host_tso4_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_HOST_TSO4),
+    DEFINE_PROP_RO_BIT64("host_tso6_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_HOST_TSO6),
+    DEFINE_PROP_RO_BIT64("host_ecn_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_HOST_ECN),
+    DEFINE_PROP_RO_BIT64("host_ufo_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_HOST_UFO),
+    DEFINE_PROP_RO_BIT64("mrg_rxbuf_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_MRG_RXBUF),
+    DEFINE_PROP_RO_BIT64("status_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_STATUS),
+    DEFINE_PROP_RO_BIT64("ctrl_vq_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CTRL_VQ),
+    DEFINE_PROP_RO_BIT64("ctrl_rx_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CTRL_RX),
+    DEFINE_PROP_RO_BIT64("ctrl_vlan_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CTRL_VLAN),
+    DEFINE_PROP_RO_BIT64("ctrl_rx_extra_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CTRL_RX_EXTRA),
+    DEFINE_PROP_RO_BIT64("ctrl_mac_addr_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_CTRL_MAC_ADDR),
+    DEFINE_PROP_RO_BIT64("ctrl_guest_offloads_acked", VirtIODevice,
+                         guest_features, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS),
+    DEFINE_PROP_RO_BIT64("mq_acked", VirtIODevice, guest_features,
+                         VIRTIO_NET_F_MQ),
+
     DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf),
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
-- 
2.5.0

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Cornelia Huck]

On Wed, 11 May 2016 12:52:02 +0300
"Denis V. Lunev" <den@openvz.org> wrote:

> It is very convinient and useful for debug purpose to expose the set of
> features negotiated with guest. The patch exports those features via
> read-only bit properties.

I agree that it would be very helpful to be able to access the
negotiated features via the monitor, but I disagree with your approach,
especially the need to expose every single feature bit via a property.

What about a command like 'info virtio' instead? This would allow us to
dump not only guest features, but the host features initially offered
alongside them and things like the status byte (so you can actually
check whether feature negotiation was successful). Maybe similar to
'info block'.

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Denis V. Lunev]

On 05/11/2016 12:52 PM, Denis V. Lunev wrote:
> It is very convinient and useful for debug purpose to expose the set of
> features negotiated with guest. The patch exports those features via
> read-only bit properties.
>
> This patchset is a proof-of-concept on a base of virtio network device.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
>
> Denis V. Lunev (2):
>    qdev: add read-only bit/bit64 property
>    virtio: show features acked by guest in 'info qtree' dump
>
>   hw/core/qdev-properties.c    | 29 +++++++++++++++++++++++++++--
>   hw/net/virtio-net.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/qdev-properties.h | 18 ++++++++++++++++++
>   3 files changed, 89 insertions(+), 2 deletions(-)
>
alternatively, we can define new property type, which
will set bit in one place and shows the value of two bits.

Any suggestion would be welcome.

Den

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Denis V. Lunev]

On 05/11/2016 01:34 PM, Cornelia Huck wrote:
> On Wed, 11 May 2016 12:52:02 +0300
> "Denis V. Lunev" <den@openvz.org> wrote:
>
>> It is very convinient and useful for debug purpose to expose the set of
>> features negotiated with guest. The patch exports those features via
>> read-only bit properties.
> I agree that it would be very helpful to be able to access the
> negotiated features via the monitor, but I disagree with your approach,
> especially the need to expose every single feature bit via a property.
>
> What about a command like 'info virtio' instead? This would allow us to
> dump not only guest features, but the host features initially offered
> alongside them and things like the status byte (so you can actually
> check whether feature negotiation was successful). Maybe similar to
> 'info block'.
>
Hmmm. Do you propose to print bits as HEX? Because if we are going
have then splitted out we have to provide individual bit descriptions.
Thus the amount of code could be similar.

Actually I can add generic dump code into VirtIODevice and will use
host_features/guest_features fields but we should have a callback
defined for each device which will provide bit names.

Den

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Cornelia Huck]

On Wed, 11 May 2016 14:06:57 +0300
"Denis V. Lunev" <den@openvz.org> wrote:

> On 05/11/2016 01:34 PM, Cornelia Huck wrote:
> > On Wed, 11 May 2016 12:52:02 +0300
> > "Denis V. Lunev" <den@openvz.org> wrote:
> >
> >> It is very convinient and useful for debug purpose to expose the set of
> >> features negotiated with guest. The patch exports those features via
> >> read-only bit properties.
> > I agree that it would be very helpful to be able to access the
> > negotiated features via the monitor, but I disagree with your approach,
> > especially the need to expose every single feature bit via a property.
> >
> > What about a command like 'info virtio' instead? This would allow us to
> > dump not only guest features, but the host features initially offered
> > alongside them and things like the status byte (so you can actually
> > check whether feature negotiation was successful). Maybe similar to
> > 'info block'.
> >
> Hmmm. Do you propose to print bits as HEX? 

Just print a 1 if the bit is set. This would make it easy to see if
e.g. the guest only negotiated a small subset of the offered features.

> Because if we are going
> have then splitted out we have to provide individual bit descriptions.
> Thus the amount of code could be similar.
> 
> Actually I can add generic dump code into VirtIODevice and will use
> host_features/guest_features fields but we should have a callback
> defined for each device which will provide bit names.

What about the individual drivers providing an array of strings for the
feature names? Then you could easily print a list of feature bit names.

For the virtio status (which I would find as useful as the feature
bits), it's even easier as the status bits are common.

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Denis V. Lunev]

On 05/11/2016 03:02 PM, Cornelia Huck wrote:
> On Wed, 11 May 2016 14:06:57 +0300
> "Denis V. Lunev" <den@openvz.org> wrote:
>
>> On 05/11/2016 01:34 PM, Cornelia Huck wrote:
>>> On Wed, 11 May 2016 12:52:02 +0300
>>> "Denis V. Lunev" <den@openvz.org> wrote:
>>>
>>>> It is very convinient and useful for debug purpose to expose the set of
>>>> features negotiated with guest. The patch exports those features via
>>>> read-only bit properties.
>>> I agree that it would be very helpful to be able to access the
>>> negotiated features via the monitor, but I disagree with your approach,
>>> especially the need to expose every single feature bit via a property.
>>>
>>> What about a command like 'info virtio' instead? This would allow us to
>>> dump not only guest features, but the host features initially offered
>>> alongside them and things like the status byte (so you can actually
>>> check whether feature negotiation was successful). Maybe similar to
>>> 'info block'.
>>>
>> Hmmm. Do you propose to print bits as HEX?
> Just print a 1 if the bit is set. This would make it easy to see if
> e.g. the guest only negotiated a small subset of the offered features.
>
>> Because if we are going
>> have then splitted out we have to provide individual bit descriptions.
>> Thus the amount of code could be similar.
>>
>> Actually I can add generic dump code into VirtIODevice and will use
>> host_features/guest_features fields but we should have a callback
>> defined for each device which will provide bit names.
> What about the individual drivers providing an array of strings for the
> feature names? Then you could easily print a list of feature bit names.
>
> For the virtio status (which I would find as useful as the feature
> bits), it's even easier as the status bits are common.
>
OK. We could try this approach.

Do you know any reasonable way to enumerate all VirtIO devices?
There is no need for this in the older code. We will need to enumerate
all VirtIO devices within 'info virtio' command.

Den

Re: [Qemu-devel] [RFC for 2.7 0/2] virtio: show guest features in 'info qtree'

[Cornelia Huck]

On Wed, 11 May 2016 16:49:45 +0300
"Denis V. Lunev" <den@openvz.org> wrote:

> Do you know any reasonable way to enumerate all VirtIO devices?
> There is no need for this in the older code. We will need to enumerate
> all VirtIO devices within 'info virtio' command.

Hum. No nice way, I think, as there's no global list.

An alternative would be to require a qom path and check whether the
specified device has a virtio-bus as child.

Re: [Qemu-devel] [RFC 2/2] virtio: show features acked by guest in 'info qtree' dump

[Michael S. Tsirkin]

On Wed, May 11, 2016 at 12:52:04PM +0300, Denis V. Lunev wrote:
> It is very convinient and useful for debug purpose to expose the set of
> features negotiated with guest. The patch exports those features via
> read-only bit properties.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Eric Blake <eblake@redhat.com>

Fine but can we avoid the code duplication, using some macro?
Same name and it are nearly always used with both
host and guest features.

> ---
>  hw/net/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5798f87..cb9ce9c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1904,6 +1904,50 @@ static Property virtio_net_properties[] = {
>      DEFINE_PROP_BIT("ctrl_guest_offloads", VirtIONet, host_features,
>                      VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true),
>      DEFINE_PROP_BIT("mq", VirtIONet, host_features, VIRTIO_NET_F_MQ, false),
> +
> +    DEFINE_PROP_RO_BIT64("csum_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CSUM),
> +    DEFINE_PROP_RO_BIT64("guest_csum_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_CSUM),
> +    DEFINE_PROP_RO_BIT64("gso_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GSO),
> +    DEFINE_PROP_RO_BIT64("guest_tso4_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_TSO4),
> +    DEFINE_PROP_RO_BIT64("guest_tso6_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_TSO6),
> +    DEFINE_PROP_RO_BIT64("guest_ecn_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_ECN),
> +    DEFINE_PROP_RO_BIT64("guest_ufo_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_UFO),
> +    DEFINE_PROP_RO_BIT64("guest_announce_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_GUEST_ANNOUNCE),
> +    DEFINE_PROP_RO_BIT64("host_tso4_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_HOST_TSO4),
> +    DEFINE_PROP_RO_BIT64("host_tso6_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_HOST_TSO6),
> +    DEFINE_PROP_RO_BIT64("host_ecn_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_HOST_ECN),
> +    DEFINE_PROP_RO_BIT64("host_ufo_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_HOST_UFO),
> +    DEFINE_PROP_RO_BIT64("mrg_rxbuf_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_MRG_RXBUF),
> +    DEFINE_PROP_RO_BIT64("status_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_STATUS),
> +    DEFINE_PROP_RO_BIT64("ctrl_vq_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CTRL_VQ),
> +    DEFINE_PROP_RO_BIT64("ctrl_rx_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CTRL_RX),
> +    DEFINE_PROP_RO_BIT64("ctrl_vlan_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CTRL_VLAN),
> +    DEFINE_PROP_RO_BIT64("ctrl_rx_extra_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CTRL_RX_EXTRA),
> +    DEFINE_PROP_RO_BIT64("ctrl_mac_addr_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_CTRL_MAC_ADDR),
> +    DEFINE_PROP_RO_BIT64("ctrl_guest_offloads_acked", VirtIODevice,
> +                         guest_features, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS),
> +    DEFINE_PROP_RO_BIT64("mq_acked", VirtIODevice, guest_features,
> +                         VIRTIO_NET_F_MQ),
> +
>      DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf),
>      DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
>                         TX_TIMER_INTERVAL),
> -- 
> 2.5.0