| Message ID | 1337876572-2051-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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) ?
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) ? >
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.
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"?
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').
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
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 >
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.
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
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 >
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 > > >
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.
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) ?
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(-)