Patchwork [v2] hmp/qxl: info spice: add qxl info

login
register
mail settings
Submitter Alon Levy
Date May 24, 2012, 4:22 p.m.
Message ID <1337876572-2051-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/161159/
State New
Headers show

Comments

Alon Levy - May 24, 2012, 4:22 p.m.
For all devices print id, mode and guest_bug status.

Known problems: Prints devices from highest id to lowest.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
This one builds. Fixed qapi-schema to say additions are for 1.2

 hmp.c            |   11 +++++++++++
 hw/qxl.c         |   22 ++++++++++++++++++++++
 qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 ui/qemu-spice.h  |    5 +++++
 ui/spice-core.c  |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 130 insertions(+), 3 deletions(-)
Luiz Capitulino - May 25, 2012, 2:43 p.m.
On Thu, 24 May 2012 19:22:52 +0300
Alon Levy <alevy@redhat.com> wrote:

> For all devices print id, mode and guest_bug status.

Is qxl really tied to spice? In the meaning that it's impossible to use it
without spice? Wouldn't it be better to have 'info display' instead?

> Known problems: Prints devices from highest id to lowest.

That's because qapi lists usually inserts new items at the head. You could
add them at the tail instead. Requires more code, but works.

> 
> Signed-off-by: Alon Levy <alevy@redhat.com>
> ---
> This one builds. Fixed qapi-schema to say additions are for 1.2
> 
>  hmp.c            |   11 +++++++++++
>  hw/qxl.c         |   22 ++++++++++++++++++++++
>  qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>  ui/qemu-spice.h  |    5 +++++
>  ui/spice-core.c  |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 130 insertions(+), 3 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index bb0952e..5126921 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -331,6 +331,7 @@ void hmp_info_spice(Monitor *mon)
>  {
>      SpiceChannelList *chan;
>      SpiceInfo *info;
> +    QXLInfoList *qxl;
>  
>      info = qmp_query_spice(NULL);
>  
> @@ -353,6 +354,16 @@ void hmp_info_spice(Monitor *mon)
>      monitor_printf(mon, "  mouse-mode: %s\n",
>                     SpiceQueryMouseMode_lookup[info->mouse_mode]);
>  
> +    for (qxl = info->qxl; qxl; qxl = qxl->next) {
> +        if (qxl->value->guest_bug == -1 || qxl->value->mode == -1) {
> +            continue;
> +        }
> +        monitor_printf(mon, "qxl-%"PRId64":\n", qxl->value->id);
> +        monitor_printf(mon, "        mode: %s\n",
> +                SpiceQueryQXLMode_lookup[qxl->value->mode]);
> +        monitor_printf(mon, "   guest_bug: %"PRIu64"\n", qxl->value->guest_bug);
> +    }
> +
>      if (!info->has_channels || info->channels == NULL) {
>          monitor_printf(mon, "Channels: none\n");
>      } else {
> diff --git a/hw/qxl.c b/hw/qxl.c
> index b5e53ce..21c825c 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1711,6 +1711,28 @@ static DisplayChangeListener display_listener = {
>      .dpy_refresh = display_refresh,
>  };
>  
> +/* helpers for spice_info */
> +int qxl_get_guest_bug(DeviceState *dev)
> +{
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> +
> +    return qxl->guest_bug;
> +}
> +
> +int qxl_get_mode(DeviceState *dev)
> +{
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> +
> +    return qxl->mode;
> +}
> +
> +int qxl_get_id(DeviceState *dev)
> +{
> +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> +
> +    return qxl->id;
> +}
> +
>  static void qxl_init_ramsize(PCIQXLDevice *qxl, uint32_t ram_min_mb)
>  {
>      /* vga ram (bar 0) */
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2ca7195..af1fac2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -638,7 +638,7 @@
>  ##
>  # @SpiceQueryMouseMode
>  #
> -# An enumation of Spice mouse states.
> +# An enumeration of Spice mouse states.
>  #
>  # @client: Mouse cursor position is determined by the client.
>  #
> @@ -655,6 +655,44 @@
>    'data': [ 'client', 'server', 'unknown' ] }
>  
>  ##
> +# @SpiceQueryQXLMode
> +#
> +# An enumeration of QXL States.
> +#
> +# @undefined: guest driver in control but no primary device. Reached after a destroy primary IO
> +#             from native mode.
> +#
> +# @vga: no device driver in control. default mode, returns to it after any vga port access.
> +#
> +# @compat: No information is available about mouse mode used by
> +#           the spice server.
> +#
> +# @native: guest driver in control of device. Reached after a create primary IO.
> +#
> +# Note: hw/qxl.h has a qxl_mode enum, name chose to not confuse the two.
> +#
> +# Since: 1.2
> +##
> +{ 'enum': 'SpiceQueryQXLMode',
> +  'data': [ 'undefined', 'vga', 'compat', 'native' ] }
> +
> +##
> +# @QXLInfo
> +#
> +# Information about a QXL device.
> +#
> +# @id: qxl id, non negative integer, 0 for primary device.
> +#
> +# @guest_bug: Has a guest error been detected.
> +#
> +# @mode: Mode of device, based on guest activity.
> +#
> +# Since: 1.2
> +##
> +{ 'type': 'QXLInfo',
> +  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'} }
> +
> +##
>  # @SpiceInfo
>  #
>  # Information about the SPICE session.
> @@ -683,12 +721,17 @@
>  #
>  # @channels: a list of @SpiceChannel for each active spice channel
>  #
> +# @qxl: a list of @QXLInfo for each qxl device
> +#
> +#              Since: 1.2
> +#
>  # Since: 0.14.0
>  ##
>  { 'type': 'SpiceInfo',
>    'data': {'enabled': 'bool', '*host': 'str', '*port': 'int',
>             '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
> -           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']} }
> +           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel'],
> +           'qxl': ['QXLInfo']} }
>  
>  ##
>  # @query-spice
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index 3299da8..edcf3a5 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -47,6 +47,11 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
>  
>  CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
>  
> +/* implemented in hw/qxl.c */
> +int qxl_get_guest_bug(DeviceState *dev);
> +int qxl_get_mode(DeviceState *dev);
> +int qxl_get_id(DeviceState *dev);
> +
>  #else  /* CONFIG_SPICE */
>  #include "monitor.h"
>  
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 4fc48f8..25833e5 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -22,7 +22,6 @@
>  #include "sysemu.h"
>  
>  #include "qemu-common.h"
> -#include "qemu-spice.h"
>  #include "qemu-thread.h"
>  #include "qemu-timer.h"
>  #include "qemu-queue.h"
> @@ -37,6 +36,8 @@
>  #include "migration.h"
>  #include "monitor.h"
>  #include "hw/hw.h"
> +#include "hw/qdev.h"
> +#include "qemu-spice.h"
>  
>  /* core bits */
>  
> @@ -419,6 +420,50 @@ static SpiceChannelList *qmp_query_spice_channels(void)
>      return head;
>  }
>  
> +static int qdev_walk_qxl(DeviceState *dev, void *opaque)
> +{
> +    QXLInfoList **cur = opaque;
> +    QXLInfoList *qxl_info;
> +    int first = 0;
> +    const char *class_name = object_get_typename(OBJECT(dev));
> +
> +    if (strcmp(class_name, "qxl") != 0 &&
> +        strcmp(class_name, "qxl-vga") != 0) {
> +        return 0;
> +    }
> +    if ((*cur)->value == NULL) {
> +        first = 1;
> +        qxl_info = *cur;
> +    } else {
> +        qxl_info = g_malloc(sizeof(*qxl_info));
> +    }
> +    qxl_info->next = NULL;
> +    qxl_info->value = g_malloc(sizeof(*qxl_info->value));
> +    qxl_info->value->id = qxl_get_id(dev);
> +    qxl_info->value->guest_bug = qxl_get_guest_bug(dev);
> +    qxl_info->value->mode = qxl_get_mode(dev);
> +    if (!first) {
> +        (*cur)->next = qxl_info;
> +        *cur = qxl_info;
> +    }
> +    return 0;
> +}
> +
> +static int qbus_walk_all(BusState *bus, void *opaque)
> +{
> +    return 0;
> +}
> +
> +static QXLInfoList *qmp_query_qxl(void)
> +{
> +    QXLInfoList *root = g_malloc0(sizeof(*root));
> +    QXLInfoList *cur = root;
> +    BusState *default_bus = sysbus_get_default();
> +
> +    qbus_walk_children(default_bus, qdev_walk_qxl, qbus_walk_all, &cur);
> +    return root;
> +}
> +
>  SpiceInfo *qmp_query_spice(Error **errp)
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> @@ -461,6 +506,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
>          info->has_tls_port = true;
>          info->tls_port = tls_port;
>      }
> +    info->qxl = qmp_query_qxl();
>  
>  #if SPICE_SERVER_VERSION >= 0x000a03 /* 0.10.3 */
>      info->mouse_mode = spice_server_is_server_mouse(spice_server) ?
Alon Levy - May 28, 2012, 9:08 a.m.
On Fri, May 25, 2012 at 11:43:11AM -0300, Luiz Capitulino wrote:
> On Thu, 24 May 2012 19:22:52 +0300
> Alon Levy <alevy@redhat.com> wrote:
> 
> > For all devices print id, mode and guest_bug status.
> 
> Is qxl really tied to spice? In the meaning that it's impossible to use it
> without spice? Wouldn't it be better to have 'info display' instead?

Would a patch implementing 'info display' require me to implement it for
all displays?

> 
> > Known problems: Prints devices from highest id to lowest.
> 
> That's because qapi lists usually inserts new items at the head. You could
> add them at the tail instead. Requires more code, but works.

Right, I didn't think it was a real enough problem to fix.

> 
> > 
> > Signed-off-by: Alon Levy <alevy@redhat.com>
> > ---
> > This one builds. Fixed qapi-schema to say additions are for 1.2
> > 
> >  hmp.c            |   11 +++++++++++
> >  hw/qxl.c         |   22 ++++++++++++++++++++++
> >  qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++++++--
> >  ui/qemu-spice.h  |    5 +++++
> >  ui/spice-core.c  |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  5 files changed, 130 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index bb0952e..5126921 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -331,6 +331,7 @@ void hmp_info_spice(Monitor *mon)
> >  {
> >      SpiceChannelList *chan;
> >      SpiceInfo *info;
> > +    QXLInfoList *qxl;
> >  
> >      info = qmp_query_spice(NULL);
> >  
> > @@ -353,6 +354,16 @@ void hmp_info_spice(Monitor *mon)
> >      monitor_printf(mon, "  mouse-mode: %s\n",
> >                     SpiceQueryMouseMode_lookup[info->mouse_mode]);
> >  
> > +    for (qxl = info->qxl; qxl; qxl = qxl->next) {
> > +        if (qxl->value->guest_bug == -1 || qxl->value->mode == -1) {
> > +            continue;
> > +        }
> > +        monitor_printf(mon, "qxl-%"PRId64":\n", qxl->value->id);
> > +        monitor_printf(mon, "        mode: %s\n",
> > +                SpiceQueryQXLMode_lookup[qxl->value->mode]);
> > +        monitor_printf(mon, "   guest_bug: %"PRIu64"\n", qxl->value->guest_bug);
> > +    }
> > +
> >      if (!info->has_channels || info->channels == NULL) {
> >          monitor_printf(mon, "Channels: none\n");
> >      } else {
> > diff --git a/hw/qxl.c b/hw/qxl.c
> > index b5e53ce..21c825c 100644
> > --- a/hw/qxl.c
> > +++ b/hw/qxl.c
> > @@ -1711,6 +1711,28 @@ static DisplayChangeListener display_listener = {
> >      .dpy_refresh = display_refresh,
> >  };
> >  
> > +/* helpers for spice_info */
> > +int qxl_get_guest_bug(DeviceState *dev)
> > +{
> > +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> > +
> > +    return qxl->guest_bug;
> > +}
> > +
> > +int qxl_get_mode(DeviceState *dev)
> > +{
> > +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> > +
> > +    return qxl->mode;
> > +}
> > +
> > +int qxl_get_id(DeviceState *dev)
> > +{
> > +    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
> > +
> > +    return qxl->id;
> > +}
> > +
> >  static void qxl_init_ramsize(PCIQXLDevice *qxl, uint32_t ram_min_mb)
> >  {
> >      /* vga ram (bar 0) */
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 2ca7195..af1fac2 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -638,7 +638,7 @@
> >  ##
> >  # @SpiceQueryMouseMode
> >  #
> > -# An enumation of Spice mouse states.
> > +# An enumeration of Spice mouse states.
> >  #
> >  # @client: Mouse cursor position is determined by the client.
> >  #
> > @@ -655,6 +655,44 @@
> >    'data': [ 'client', 'server', 'unknown' ] }
> >  
> >  ##
> > +# @SpiceQueryQXLMode
> > +#
> > +# An enumeration of QXL States.
> > +#
> > +# @undefined: guest driver in control but no primary device. Reached after a destroy primary IO
> > +#             from native mode.
> > +#
> > +# @vga: no device driver in control. default mode, returns to it after any vga port access.
> > +#
> > +# @compat: No information is available about mouse mode used by
> > +#           the spice server.
> > +#
> > +# @native: guest driver in control of device. Reached after a create primary IO.
> > +#
> > +# Note: hw/qxl.h has a qxl_mode enum, name chose to not confuse the two.
> > +#
> > +# Since: 1.2
> > +##
> > +{ 'enum': 'SpiceQueryQXLMode',
> > +  'data': [ 'undefined', 'vga', 'compat', 'native' ] }
> > +
> > +##
> > +# @QXLInfo
> > +#
> > +# Information about a QXL device.
> > +#
> > +# @id: qxl id, non negative integer, 0 for primary device.
> > +#
> > +# @guest_bug: Has a guest error been detected.
> > +#
> > +# @mode: Mode of device, based on guest activity.
> > +#
> > +# Since: 1.2
> > +##
> > +{ 'type': 'QXLInfo',
> > +  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'} }
> > +
> > +##
> >  # @SpiceInfo
> >  #
> >  # Information about the SPICE session.
> > @@ -683,12 +721,17 @@
> >  #
> >  # @channels: a list of @SpiceChannel for each active spice channel
> >  #
> > +# @qxl: a list of @QXLInfo for each qxl device
> > +#
> > +#              Since: 1.2
> > +#
> >  # Since: 0.14.0
> >  ##
> >  { 'type': 'SpiceInfo',
> >    'data': {'enabled': 'bool', '*host': 'str', '*port': 'int',
> >             '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
> > -           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']} }
> > +           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel'],
> > +           'qxl': ['QXLInfo']} }
> >  
> >  ##
> >  # @query-spice
> > diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> > index 3299da8..edcf3a5 100644
> > --- a/ui/qemu-spice.h
> > +++ b/ui/qemu-spice.h
> > @@ -47,6 +47,11 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
> >  
> >  CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
> >  
> > +/* implemented in hw/qxl.c */
> > +int qxl_get_guest_bug(DeviceState *dev);
> > +int qxl_get_mode(DeviceState *dev);
> > +int qxl_get_id(DeviceState *dev);
> > +
> >  #else  /* CONFIG_SPICE */
> >  #include "monitor.h"
> >  
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 4fc48f8..25833e5 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -22,7 +22,6 @@
> >  #include "sysemu.h"
> >  
> >  #include "qemu-common.h"
> > -#include "qemu-spice.h"
> >  #include "qemu-thread.h"
> >  #include "qemu-timer.h"
> >  #include "qemu-queue.h"
> > @@ -37,6 +36,8 @@
> >  #include "migration.h"
> >  #include "monitor.h"
> >  #include "hw/hw.h"
> > +#include "hw/qdev.h"
> > +#include "qemu-spice.h"
> >  
> >  /* core bits */
> >  
> > @@ -419,6 +420,50 @@ static SpiceChannelList *qmp_query_spice_channels(void)
> >      return head;
> >  }
> >  
> > +static int qdev_walk_qxl(DeviceState *dev, void *opaque)
> > +{
> > +    QXLInfoList **cur = opaque;
> > +    QXLInfoList *qxl_info;
> > +    int first = 0;
> > +    const char *class_name = object_get_typename(OBJECT(dev));
> > +
> > +    if (strcmp(class_name, "qxl") != 0 &&
> > +        strcmp(class_name, "qxl-vga") != 0) {
> > +        return 0;
> > +    }
> > +    if ((*cur)->value == NULL) {
> > +        first = 1;
> > +        qxl_info = *cur;
> > +    } else {
> > +        qxl_info = g_malloc(sizeof(*qxl_info));
> > +    }
> > +    qxl_info->next = NULL;
> > +    qxl_info->value = g_malloc(sizeof(*qxl_info->value));
> > +    qxl_info->value->id = qxl_get_id(dev);
> > +    qxl_info->value->guest_bug = qxl_get_guest_bug(dev);
> > +    qxl_info->value->mode = qxl_get_mode(dev);
> > +    if (!first) {
> > +        (*cur)->next = qxl_info;
> > +        *cur = qxl_info;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int qbus_walk_all(BusState *bus, void *opaque)
> > +{
> > +    return 0;
> > +}
> > +
> > +static QXLInfoList *qmp_query_qxl(void)
> > +{
> > +    QXLInfoList *root = g_malloc0(sizeof(*root));
> > +    QXLInfoList *cur = root;
> > +    BusState *default_bus = sysbus_get_default();
> > +
> > +    qbus_walk_children(default_bus, qdev_walk_qxl, qbus_walk_all, &cur);
> > +    return root;
> > +}
> > +
> >  SpiceInfo *qmp_query_spice(Error **errp)
> >  {
> >      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> > @@ -461,6 +506,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
> >          info->has_tls_port = true;
> >          info->tls_port = tls_port;
> >      }
> > +    info->qxl = qmp_query_qxl();
> >  
> >  #if SPICE_SERVER_VERSION >= 0x000a03 /* 0.10.3 */
> >      info->mouse_mode = spice_server_is_server_mouse(spice_server) ?
>
Luiz Capitulino - May 28, 2012, 1:13 p.m.
On Mon, 28 May 2012 12:08:13 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Fri, May 25, 2012 at 11:43:11AM -0300, Luiz Capitulino wrote:
> > On Thu, 24 May 2012 19:22:52 +0300
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > For all devices print id, mode and guest_bug status.
> > 
> > Is qxl really tied to spice? In the meaning that it's impossible to use it
> > without spice? Wouldn't it be better to have 'info display' instead?
> 
> Would a patch implementing 'info display' require me to implement it for
> all displays?

Would be nice, even if we start with very basic information.
Alon Levy - May 28, 2012, 1:36 p.m.
On Mon, May 28, 2012 at 10:13:28AM -0300, Luiz Capitulino wrote:
> On Mon, 28 May 2012 12:08:13 +0300
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Fri, May 25, 2012 at 11:43:11AM -0300, Luiz Capitulino wrote:
> > > On Thu, 24 May 2012 19:22:52 +0300
> > > Alon Levy <alevy@redhat.com> wrote:
> > > 
> > > > For all devices print id, mode and guest_bug status.
> > > 
> > > Is qxl really tied to spice? In the meaning that it's impossible to use it
> > > without spice? Wouldn't it be better to have 'info display' instead?
> > 
> > Would a patch implementing 'info display' require me to implement it for
> > all displays?
> 
> Would be nice, even if we start with very basic information.

How would that work? I have QXLInfo that only makes sense when the
information is about a qxl device. Can't have opaque data in a QMP
response, so would this be a "info display qxl" "info display cirrus"
etc. or "info qxl"?
Luiz Capitulino - May 28, 2012, 7:06 p.m.
On Mon, 28 May 2012 16:36:16 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Mon, May 28, 2012 at 10:13:28AM -0300, Luiz Capitulino wrote:
> > On Mon, 28 May 2012 12:08:13 +0300
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Fri, May 25, 2012 at 11:43:11AM -0300, Luiz Capitulino wrote:
> > > > On Thu, 24 May 2012 19:22:52 +0300
> > > > Alon Levy <alevy@redhat.com> wrote:
> > > > 
> > > > > For all devices print id, mode and guest_bug status.
> > > > 
> > > > Is qxl really tied to spice? In the meaning that it's impossible to use it
> > > > without spice? Wouldn't it be better to have 'info display' instead?
> > > 
> > > Would a patch implementing 'info display' require me to implement it for
> > > all displays?
> > 
> > Would be nice, even if we start with very basic information.
> 
> How would that work? I have QXLInfo that only makes sense when the
> information is about a qxl device. Can't have opaque data in a QMP
> response, so would this be a "info display qxl" "info display cirrus"
> etc. or "info qxl"?

You could show what's available and/or being used currently.

To be honest, I'm not very familiar with any of those drivers and don't
know if/how management would use that information. I suggested 'info display'
because it seemed more natural to me (vs. having this info tied to 'info spice').
Gerd Hoffmann - May 29, 2012, 7:25 a.m.
Hi,

>> How would that work? I have QXLInfo that only makes sense when the
>> information is about a qxl device. Can't have opaque data in a QMP
>> response, so would this be a "info display qxl" "info display cirrus"
>> etc. or "info qxl"?
> 
> You could show what's available and/or being used currently.

I think (Alon, correct me if I'm wrong) the use case is being able to
figure whenever the guest drivers are installed and active.

There isn't much point in doing that for stdvga and cirrus IMHO.  It
might be useful for other paravirtual devices where we have to install
drivers (in windows guests, also older linux).  I'm not sure it is
possible given that ipxe and seabios support booting from
virtio-{blk,net,scsi}, so you'll see the device being used even if the
guest os lacks drivers ...

cheers,
  Gerd
Alon Levy - May 29, 2012, 7:57 a.m.
On Tue, May 29, 2012 at 09:25:40AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >> How would that work? I have QXLInfo that only makes sense when the
> >> information is about a qxl device. Can't have opaque data in a QMP
> >> response, so would this be a "info display qxl" "info display cirrus"
> >> etc. or "info qxl"?
> > 
> > You could show what's available and/or being used currently.
> 
> I think (Alon, correct me if I'm wrong) the use case is being able to
> figure whenever the guest drivers are installed and active.
> 
> There isn't much point in doing that for stdvga and cirrus IMHO.  It
> might be useful for other paravirtual devices where we have to install
> drivers (in windows guests, also older linux).  I'm not sure it is
> possible given that ipxe and seabios support booting from
> virtio-{blk,net,scsi}, so you'll see the device being used even if the
> guest os lacks drivers ...

I haven't really generalized it, but you make much sense, more then info
display - so maybe I could put this under "info paravirt", have a list
and a register function, with qxl the only registered callback for the
info command. This doesn't answer my other technical (and probably
having an obvious solution I'm missing) question about the type to use -
I could have a:

{ 'type': 'QXLInfo',
  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'}
  }

(like I do now)
And then aggregate that via lists with possibly zero members:

{ 'type': 'ParaVirtInfo',
  'data': {'qxl': ['QXLInfo'], 'virtserial': [VirtSerialInfo]} }

Luiz, is there a better solution?

> 
> cheers,
>   Gerd
>
Luiz Capitulino - May 29, 2012, 1:38 p.m.
On Tue, 29 May 2012 09:25:40 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> How would that work? I have QXLInfo that only makes sense when the
> >> information is about a qxl device. Can't have opaque data in a QMP
> >> response, so would this be a "info display qxl" "info display cirrus"
> >> etc. or "info qxl"?
> > 
> > You could show what's available and/or being used currently.
> 
> I think (Alon, correct me if I'm wrong) the use case is being able to
> figure whenever the guest drivers are installed and active.

Alon, can you confirm this? I'm still not clear on the use-case.

The two points I'm wondering are 1. If this is really tied to spice (and thus
this info should be part of query-spice) and now 2. if the use case above
really pertains to QMP.

I've talked to someone in the past about having a way to get information about
guest drivers statuses and the conclusion was that the guest-agent agent was
better suited for that.
Alon Levy - May 29, 2012, 2:51 p.m.
On Tue, May 29, 2012 at 10:38:20AM -0300, Luiz Capitulino wrote:
> On Tue, 29 May 2012 09:25:40 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > >> How would that work? I have QXLInfo that only makes sense when the
> > >> information is about a qxl device. Can't have opaque data in a QMP
> > >> response, so would this be a "info display qxl" "info display cirrus"
> > >> etc. or "info qxl"?
> > > 
> > > You could show what's available and/or being used currently.
> > 
> > I think (Alon, correct me if I'm wrong) the use case is being able to
> > figure whenever the guest drivers are installed and active.
> 
> Alon, can you confirm this? I'm still not clear on the use-case.
> 
> The two points I'm wondering are 1. If this is really tied to spice (and thus
> this info should be part of query-spice) and now 2. if the use case above
> really pertains to QMP.
> 
> I've talked to someone in the past about having a way to get information about
> guest drivers statuses and the conclusion was that the guest-agent agent was
> better suited for that.

The information I'm suggesting to expose is state of the guest as seen
from device pov:
 * guest_bug - this indicates that the qxl device has been instructed to
  do something illegal that it has ignored, and until a reset from the
  guest, (QXL_RESET io write), that would presumably indicate a restart
  of the guest driver, we would like to ignore the guest from now on.
  This information would be helpful for error report.
 * mode - this is another indication of presence or lack of a guest
  driver. This time more straight forward - if a certain IO is issued
  (QXL_CREATE_PRIMARY) then the driver is in native mode. If another IO
  (SET_MODE) we are in compat mode. If anything to change back to vga
  happens (vga io read/write), we are back in vga, and if a
  DESTROY_SURFACE happens we are in undefined mode. This status would be
  helpful for error reporting as well.

Since both pieces of information already exist in the qemu side, there
is no need to introduce an agent to retrieve them. They are easy to
retrive with existing management (libvirt/vdsm can do random monitor
commands iirc).

Alon
Luiz Capitulino - May 29, 2012, 4:44 p.m.
On Tue, 29 May 2012 17:51:50 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Tue, May 29, 2012 at 10:38:20AM -0300, Luiz Capitulino wrote:
> > On Tue, 29 May 2012 09:25:40 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > >   Hi,
> > > 
> > > >> How would that work? I have QXLInfo that only makes sense when the
> > > >> information is about a qxl device. Can't have opaque data in a QMP
> > > >> response, so would this be a "info display qxl" "info display cirrus"
> > > >> etc. or "info qxl"?
> > > > 
> > > > You could show what's available and/or being used currently.
> > > 
> > > I think (Alon, correct me if I'm wrong) the use case is being able to
> > > figure whenever the guest drivers are installed and active.
> > 
> > Alon, can you confirm this? I'm still not clear on the use-case.
> > 
> > The two points I'm wondering are 1. If this is really tied to spice (and thus
> > this info should be part of query-spice) and now 2. if the use case above
> > really pertains to QMP.
> > 
> > I've talked to someone in the past about having a way to get information about
> > guest drivers statuses and the conclusion was that the guest-agent agent was
> > better suited for that.
> 
> The information I'm suggesting to expose is state of the guest as seen
> from device pov:

Do you expect mngt apps to use that info or is it just for debugging? If it's
the latter, then maybe we could add query-qxl as gen=no and declare it unstable.

>  * guest_bug - this indicates that the qxl device has been instructed to
>   do something illegal that it has ignored, and until a reset from the
>   guest, (QXL_RESET io write), that would presumably indicate a restart
>   of the guest driver, we would like to ignore the guest from now on.
>   This information would be helpful for error report.
>
>  * mode - this is another indication of presence or lack of a guest
>   driver. This time more straight forward - if a certain IO is issued
>   (QXL_CREATE_PRIMARY) then the driver is in native mode. If another IO
>   (SET_MODE) we are in compat mode. If anything to change back to vga
>   happens (vga io read/write), we are back in vga, and if a
>   DESTROY_SURFACE happens we are in undefined mode. This status would be
>   helpful for error reporting as well.

What kind of error report?

> 
> Since both pieces of information already exist in the qemu side, there
> is no need to introduce an agent to retrieve them. They are easy to
> retrive with existing management (libvirt/vdsm can do random monitor
> commands iirc).
> 
> Alon
>
Alon Levy - May 30, 2012, 8:54 a.m.
On Tue, May 29, 2012 at 01:44:35PM -0300, Luiz Capitulino wrote:
> On Tue, 29 May 2012 17:51:50 +0300
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Tue, May 29, 2012 at 10:38:20AM -0300, Luiz Capitulino wrote:
> > > On Tue, 29 May 2012 09:25:40 +0200
> > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > 
> > > >   Hi,
> > > > 
> > > > >> How would that work? I have QXLInfo that only makes sense when the
> > > > >> information is about a qxl device. Can't have opaque data in a QMP
> > > > >> response, so would this be a "info display qxl" "info display cirrus"
> > > > >> etc. or "info qxl"?
> > > > > 
> > > > > You could show what's available and/or being used currently.
> > > > 
> > > > I think (Alon, correct me if I'm wrong) the use case is being able to
> > > > figure whenever the guest drivers are installed and active.
> > > 
> > > Alon, can you confirm this? I'm still not clear on the use-case.
> > > 
> > > The two points I'm wondering are 1. If this is really tied to spice (and thus
> > > this info should be part of query-spice) and now 2. if the use case above
> > > really pertains to QMP.
> > > 
> > > I've talked to someone in the past about having a way to get information about
> > > guest drivers statuses and the conclusion was that the guest-agent agent was
> > > better suited for that.
> > 
> > The information I'm suggesting to expose is state of the guest as seen
> > from device pov:
> 
> Do you expect mngt apps to use that info or is it just for debugging? If it's
> the latter, then maybe we could add query-qxl as gen=no and declare it unstable.

It's for debugging. I don't expect management to use it, but I do expect
it to be useful when someone comes with a problem to the list, as it's
easy to retrieve information.

Where do I add "gen=no", how do you want me to mark this unstable? The
point is to have this available, not to have this disabled. It's low
risk, it's only displayed when someone does "info spice" (or "info virt"
if someone would say that is preferable) or the equivalent QMP command.

> 
> >  * guest_bug - this indicates that the qxl device has been instructed to
> >   do something illegal that it has ignored, and until a reset from the
> >   guest, (QXL_RESET io write), that would presumably indicate a restart
> >   of the guest driver, we would like to ignore the guest from now on.
> >   This information would be helpful for error report.
> >
> >  * mode - this is another indication of presence or lack of a guest
> >   driver. This time more straight forward - if a certain IO is issued
> >   (QXL_CREATE_PRIMARY) then the driver is in native mode. If another IO
> >   (SET_MODE) we are in compat mode. If anything to change back to vga
> >   happens (vga io read/write), we are back in vga, and if a
> >   DESTROY_SURFACE happens we are in undefined mode. This status would be
> >   helpful for error reporting as well.
> 
> What kind of error report?

If someone reports an error involving spice/qxl this value would be a
quick indication of false alarm for some instances, i.e. showing vga
would probably mean there was no driver loaded in the guest.

> 
> > 
> > Since both pieces of information already exist in the qemu side, there
> > is no need to introduce an agent to retrieve them. They are easy to
> > retrive with existing management (libvirt/vdsm can do random monitor
> > commands iirc).
> > 
> > Alon
> > 
>
Luiz Capitulino - May 30, 2012, 1:16 p.m.
On Wed, 30 May 2012 11:54:11 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Tue, May 29, 2012 at 01:44:35PM -0300, Luiz Capitulino wrote:
> > On Tue, 29 May 2012 17:51:50 +0300
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Tue, May 29, 2012 at 10:38:20AM -0300, Luiz Capitulino wrote:
> > > > On Tue, 29 May 2012 09:25:40 +0200
> > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > 
> > > > >   Hi,
> > > > > 
> > > > > >> How would that work? I have QXLInfo that only makes sense when the
> > > > > >> information is about a qxl device. Can't have opaque data in a QMP
> > > > > >> response, so would this be a "info display qxl" "info display cirrus"
> > > > > >> etc. or "info qxl"?
> > > > > > 
> > > > > > You could show what's available and/or being used currently.
> > > > > 
> > > > > I think (Alon, correct me if I'm wrong) the use case is being able to
> > > > > figure whenever the guest drivers are installed and active.
> > > > 
> > > > Alon, can you confirm this? I'm still not clear on the use-case.
> > > > 
> > > > The two points I'm wondering are 1. If this is really tied to spice (and thus
> > > > this info should be part of query-spice) and now 2. if the use case above
> > > > really pertains to QMP.
> > > > 
> > > > I've talked to someone in the past about having a way to get information about
> > > > guest drivers statuses and the conclusion was that the guest-agent agent was
> > > > better suited for that.
> > > 
> > > The information I'm suggesting to expose is state of the guest as seen
> > > from device pov:
> > 
> > Do you expect mngt apps to use that info or is it just for debugging? If it's
> > the latter, then maybe we could add query-qxl as gen=no and declare it unstable.
> 
> It's for debugging. I don't expect management to use it, but I do expect
> it to be useful when someone comes with a problem to the list, as it's
> easy to retrieve information.
> 
> Where do I add "gen=no", how do you want me to mark this unstable? The
> point is to have this available, not to have this disabled. It's low
> risk, it's only displayed when someone does "info spice" (or "info virt"
> if someone would say that is preferable) or the equivalent QMP command.

It will be available. You can do the following:

 1. Add a query-qxl command, but that command should be in the experimental
    name space, this means that you should call it
    __org.qemu.experimental.query-qxl. Let's forget about gen=no for the
    moment.

    Also, don't forget to mention in the schema entry for the command that
    it's experimental and unstable.

  2. You have two options for HMP, either you add 'info qxl' or you
     extend 'info spice'. Your choice.

Patch

diff --git a/hmp.c b/hmp.c
index bb0952e..5126921 100644
--- a/hmp.c
+++ b/hmp.c
@@ -331,6 +331,7 @@  void hmp_info_spice(Monitor *mon)
 {
     SpiceChannelList *chan;
     SpiceInfo *info;
+    QXLInfoList *qxl;
 
     info = qmp_query_spice(NULL);
 
@@ -353,6 +354,16 @@  void hmp_info_spice(Monitor *mon)
     monitor_printf(mon, "  mouse-mode: %s\n",
                    SpiceQueryMouseMode_lookup[info->mouse_mode]);
 
+    for (qxl = info->qxl; qxl; qxl = qxl->next) {
+        if (qxl->value->guest_bug == -1 || qxl->value->mode == -1) {
+            continue;
+        }
+        monitor_printf(mon, "qxl-%"PRId64":\n", qxl->value->id);
+        monitor_printf(mon, "        mode: %s\n",
+                SpiceQueryQXLMode_lookup[qxl->value->mode]);
+        monitor_printf(mon, "   guest_bug: %"PRIu64"\n", qxl->value->guest_bug);
+    }
+
     if (!info->has_channels || info->channels == NULL) {
         monitor_printf(mon, "Channels: none\n");
     } else {
diff --git a/hw/qxl.c b/hw/qxl.c
index b5e53ce..21c825c 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1711,6 +1711,28 @@  static DisplayChangeListener display_listener = {
     .dpy_refresh = display_refresh,
 };
 
+/* helpers for spice_info */
+int qxl_get_guest_bug(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->guest_bug;
+}
+
+int qxl_get_mode(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->mode;
+}
+
+int qxl_get_id(DeviceState *dev)
+{
+    PCIQXLDevice *qxl = DO_UPCAST(PCIQXLDevice, pci.qdev, dev);
+
+    return qxl->id;
+}
+
 static void qxl_init_ramsize(PCIQXLDevice *qxl, uint32_t ram_min_mb)
 {
     /* vga ram (bar 0) */
diff --git a/qapi-schema.json b/qapi-schema.json
index 2ca7195..af1fac2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -638,7 +638,7 @@ 
 ##
 # @SpiceQueryMouseMode
 #
-# An enumation of Spice mouse states.
+# An enumeration of Spice mouse states.
 #
 # @client: Mouse cursor position is determined by the client.
 #
@@ -655,6 +655,44 @@ 
   'data': [ 'client', 'server', 'unknown' ] }
 
 ##
+# @SpiceQueryQXLMode
+#
+# An enumeration of QXL States.
+#
+# @undefined: guest driver in control but no primary device. Reached after a destroy primary IO
+#             from native mode.
+#
+# @vga: no device driver in control. default mode, returns to it after any vga port access.
+#
+# @compat: No information is available about mouse mode used by
+#           the spice server.
+#
+# @native: guest driver in control of device. Reached after a create primary IO.
+#
+# Note: hw/qxl.h has a qxl_mode enum, name chose to not confuse the two.
+#
+# Since: 1.2
+##
+{ 'enum': 'SpiceQueryQXLMode',
+  'data': [ 'undefined', 'vga', 'compat', 'native' ] }
+
+##
+# @QXLInfo
+#
+# Information about a QXL device.
+#
+# @id: qxl id, non negative integer, 0 for primary device.
+#
+# @guest_bug: Has a guest error been detected.
+#
+# @mode: Mode of device, based on guest activity.
+#
+# Since: 1.2
+##
+{ 'type': 'QXLInfo',
+  'data': {'id': 'int', 'guest_bug': 'int', 'mode': 'SpiceQueryQXLMode'} }
+
+##
 # @SpiceInfo
 #
 # Information about the SPICE session.
@@ -683,12 +721,17 @@ 
 #
 # @channels: a list of @SpiceChannel for each active spice channel
 #
+# @qxl: a list of @QXLInfo for each qxl device
+#
+#              Since: 1.2
+#
 # Since: 0.14.0
 ##
 { 'type': 'SpiceInfo',
   'data': {'enabled': 'bool', '*host': 'str', '*port': 'int',
            '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
-           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel']} }
+           'mouse-mode': 'SpiceQueryMouseMode', '*channels': ['SpiceChannel'],
+           'qxl': ['QXLInfo']} }
 
 ##
 # @query-spice
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 3299da8..edcf3a5 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -47,6 +47,11 @@  void do_info_spice(Monitor *mon, QObject **ret_data);
 
 CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
 
+/* implemented in hw/qxl.c */
+int qxl_get_guest_bug(DeviceState *dev);
+int qxl_get_mode(DeviceState *dev);
+int qxl_get_id(DeviceState *dev);
+
 #else  /* CONFIG_SPICE */
 #include "monitor.h"
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4fc48f8..25833e5 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -22,7 +22,6 @@ 
 #include "sysemu.h"
 
 #include "qemu-common.h"
-#include "qemu-spice.h"
 #include "qemu-thread.h"
 #include "qemu-timer.h"
 #include "qemu-queue.h"
@@ -37,6 +36,8 @@ 
 #include "migration.h"
 #include "monitor.h"
 #include "hw/hw.h"
+#include "hw/qdev.h"
+#include "qemu-spice.h"
 
 /* core bits */
 
@@ -419,6 +420,50 @@  static SpiceChannelList *qmp_query_spice_channels(void)
     return head;
 }
 
+static int qdev_walk_qxl(DeviceState *dev, void *opaque)
+{
+    QXLInfoList **cur = opaque;
+    QXLInfoList *qxl_info;
+    int first = 0;
+    const char *class_name = object_get_typename(OBJECT(dev));
+
+    if (strcmp(class_name, "qxl") != 0 &&
+        strcmp(class_name, "qxl-vga") != 0) {
+        return 0;
+    }
+    if ((*cur)->value == NULL) {
+        first = 1;
+        qxl_info = *cur;
+    } else {
+        qxl_info = g_malloc(sizeof(*qxl_info));
+    }
+    qxl_info->next = NULL;
+    qxl_info->value = g_malloc(sizeof(*qxl_info->value));
+    qxl_info->value->id = qxl_get_id(dev);
+    qxl_info->value->guest_bug = qxl_get_guest_bug(dev);
+    qxl_info->value->mode = qxl_get_mode(dev);
+    if (!first) {
+        (*cur)->next = qxl_info;
+        *cur = qxl_info;
+    }
+    return 0;
+}
+
+static int qbus_walk_all(BusState *bus, void *opaque)
+{
+    return 0;
+}
+
+static QXLInfoList *qmp_query_qxl(void)
+{
+    QXLInfoList *root = g_malloc0(sizeof(*root));
+    QXLInfoList *cur = root;
+    BusState *default_bus = sysbus_get_default();
+
+    qbus_walk_children(default_bus, qdev_walk_qxl, qbus_walk_all, &cur);
+    return root;
+}
+
 SpiceInfo *qmp_query_spice(Error **errp)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
@@ -461,6 +506,7 @@  SpiceInfo *qmp_query_spice(Error **errp)
         info->has_tls_port = true;
         info->tls_port = tls_port;
     }
+    info->qxl = qmp_query_qxl();
 
 #if SPICE_SERVER_VERSION >= 0x000a03 /* 0.10.3 */
     info->mouse_mode = spice_server_is_server_mouse(spice_server) ?