[PATCH] Make some structure static
Not used outside C module.
Signed-off-by: Frediano Ziglio
---
hw/vfio/pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa387f043..a1522a011a 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2558,7 +2558,7 @@ static bool vfio_display_migration_needed(void *opaque)
(vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
}
-const VMStateDescription vmstate_vfio_display = {
+static const VMStateDescription vmstate_vfio_display = {
.name = "VFIOPCIDevice/VFIODisplay",
.version_id = 1,
.minimum_version_id = 1,
@@ -2570,7 +2570,7 @@ const VMStateDescription vmstate_vfio_display = {
}
};
-const VMStateDescription vmstate_vfio_pci_config = {
+static const VMStateDescription vmstate_vfio_pci_config = {
.name = "VFIOPCIDevice",
.version_id = 1,
.minimum_version_id = 1,
--
2.34.1
[PATCH] Fix typo in comment (uin32_t -> uint32_t)
Signed-off-by: Frediano Ziglio
---
hw/vfio/pci.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 6e64a2654e..4bb7d7d257 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -181,7 +181,7 @@ struct VFIOPCIDevice {
Notifier irqchip_change_notifier;
};
-/* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw */
+/* Use uint32_t for vendor & device so PCI_ANY_ID expands and cannot
match hw */
static inline bool vfio_pci_is(VFIOPCIDevice *vdev, uint32_t vendor,
uint32_t device)
{
return (vendor == PCI_ANY_ID || vendor == vdev->vendor_id) &&
--
2.34.1
Re: qemu-system-i386
Hi, did you compile on yourself? Downloaded from some website? Regards, Frediano Il giorno dom 29 ott 2023 alle ore 20:13 Joachim Roden ha scritto:
Re: [RFC v2 2/2] spice: Add an option to forward the dmabuf directly to the encoder (v2)
Il giorno lun 30 gen 2023 alle ore 02:24 Kasireddy, Vivek
ha scritto:
>
> Hi Frediano, Gerd,
>
> >
> > Il giorno mar 24 gen 2023 alle ore 06:41 Kasireddy, Vivek
> > ha scritto:
> > >
> > > + Frediano
> > >
> > > Hi Gerd,
> > >
> > > >
> > > > Hi,
> > > >
> > > > > Here is the flow of things from the Qemu side:
> > > > > - Call gl_scanout (to update the fd) and gl_draw_async just like
> > > > > in the local display case.
> > > >
> > > > Ok.
> > > >
> > > > > - Additionally, create an update with the cmd set to QXL_CMD_DRAW
> > > > > to trigger the creation of a new drawable (associated with the fd)
> > > > > by the Spice server.
> > > > > - Wait (or block) until the Encoder is done encoding the content.
> > > > > - Unblock the pipeline once the async completion cookie is received.
> > > >
> > > > Care to explain? For qemu it should make a difference what spice-server
> > > > does with the dma-bufs passed (local display / encode video + send to
> > > > remote).
> > > [Kasireddy, Vivek] I agree that Qemu shouldn't care what the spice-server
> > > does
> > > with the dmabuf fds but somehow a drawable has to be created in the remote
> > client
> > > case. This is needed as most of the core functions in the server
> > > (associated with
> > > display-channel, video-stream, encoder, etc) operate on drawables.
> > > Therefore, I
> > > figured since Qemu already tells the server to create a drawable in the
> > > non-gl
> > case
> > > (by creating an update that includes a QXL_CMD_DRAW cmd), the same thing
> > > can be done in the gl + remote client case as well.
> > >
> >
> > This is a hack. It's combining an invalid command in order to cause
> > some side effects on spice server but it also needs a change in spice
> > server to detect and handle this hack. QXL_CMD_DRAW is mainly bound to
> > 2D commands and should come with valid bitmap data.
> >
> > > Alternatively, we could make the server create a drawable as a response to
> > gl_scanout,
> > > when it detects a remote client. IIUC, I think this can be done but seems
> > > rather
> > messy
> > > given that currently, the server only creates a drawable (inside
> > red_process_display)
> > > in the case of QXL_CMD_DRAW sent by Qemu/applications:
> > > switch (ext_cmd.cmd.type) {
> > > case QXL_CMD_DRAW: {
> > > auto red_drawable = red_drawable_new(worker->qxl,
> > >mem_slots,
> > > ext_cmd.group_id,
> > > ext_cmd.cmd.data,
> > > ext_cmd.flags); //
> > > returns with 1 ref
> > >
> > > if (red_drawable) {
> > > display_channel_process_draw(worker->display_channel,
> > std::move(red_drawable),
> > >
> > > worker->process_display_generation);
> > > }
> > >
> >
> > Yes, it sounds a bit long but surely better than hacking Qemu, spice
> > server and defining a new hacky ABI that will be hard to maintain and
> > understand.
> >
> > > The other option I can think of is to just not deal with drawables at all
> > > and
> > somehow
> > > directly share the dmabuf fd with the Encoder. This solution also seems
> > > very
> > messy
> > > and invasive to me as we'd not be able to leverage the existing APIs (in
> > > display-
> > channel,
> > > video-stream, etc) that create and manage streams efficiently.
> > >
> >
> > Yes, that currently seems pretty long as a change but possibly the
> > most clean in future, it's up to some refactory. The Encoder does not
> > either need technically a RedDrawable, Drawable but frame data encoded
> > in a format it can manage (either raw memory data or dmabuf at the
> > moment).
> [Kasireddy, Vivek] Ok, I'll work on refactoring Spice server code to pass
> the dmabuf fd directly to the Encoder without having to creating drawables.
>
> Thanks,
> Vivek
>
Hi Vivek,
thanks for the effort. I'll try to help although I'll be pretty
busy for a while.
I'd try (consider them only as suggestions) to edit
display_channel_gl_scanout, dcc_gl_scanout_item_new,
display_channel_gl_draw, dcc_gl_draw_item_new to handle requests from
Qemu. Specifically in dcc_gl_scanout_item_new and dcc_gl_draw_item_new
you probably want to remove the check and error for Unix sockets. As
code you wrote in Qemu you will need to create a new surface (if not
already there or if size changed) handling a new scanout. Also
probably better to create a VideoStream specifically for GL surfaces.
How to have some code shortcut to deliver the GL surface directly to
the stream? No much suggestions.
It would probably be nice to add an additional method in VideoEncoder
(video-encoder.h) to accept a GL surface instead of a SpiceBitmap.
Frediano
> >
> > > >
> > > > > #ifdef HAVE_SPICE_GL
> > > > > +} else if (spice_dmabuf_encode) {
> > > > > +if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
> > > > > +
Re: [QEMU][PATCH v4 07/10] hw/xen/xen-hvm-common: Use g_new and error_setg_errno
Il giorno mer 25 gen 2023 alle ore 22:07 Stefano Stabellini ha scritto: > > On Wed, 25 Jan 2023, Vikram Garhwal wrote: > > Replace g_malloc with g_new and perror with error_setg_errno. > > error_setg_errno -> error_report ? Also in the title > > Signed-off-by: Vikram Garhwal Frediano
Re: [RFC v2 2/2] spice: Add an option to forward the dmabuf directly to the encoder (v2)
Il giorno mar 24 gen 2023 alle ore 06:41 Kasireddy, Vivek
ha scritto:
>
> + Frediano
>
> Hi Gerd,
>
> >
> > Hi,
> >
> > > Here is the flow of things from the Qemu side:
> > > - Call gl_scanout (to update the fd) and gl_draw_async just like
> > > in the local display case.
> >
> > Ok.
> >
> > > - Additionally, create an update with the cmd set to QXL_CMD_DRAW
> > > to trigger the creation of a new drawable (associated with the fd)
> > > by the Spice server.
> > > - Wait (or block) until the Encoder is done encoding the content.
> > > - Unblock the pipeline once the async completion cookie is received.
> >
> > Care to explain? For qemu it should make a difference what spice-server
> > does with the dma-bufs passed (local display / encode video + send to
> > remote).
> [Kasireddy, Vivek] I agree that Qemu shouldn't care what the spice-server does
> with the dmabuf fds but somehow a drawable has to be created in the remote
> client
> case. This is needed as most of the core functions in the server (associated
> with
> display-channel, video-stream, encoder, etc) operate on drawables. Therefore,
> I
> figured since Qemu already tells the server to create a drawable in the
> non-gl case
> (by creating an update that includes a QXL_CMD_DRAW cmd), the same thing
> can be done in the gl + remote client case as well.
>
This is a hack. It's combining an invalid command in order to cause
some side effects on spice server but it also needs a change in spice
server to detect and handle this hack. QXL_CMD_DRAW is mainly bound to
2D commands and should come with valid bitmap data.
> Alternatively, we could make the server create a drawable as a response to
> gl_scanout,
> when it detects a remote client. IIUC, I think this can be done but seems
> rather messy
> given that currently, the server only creates a drawable (inside
> red_process_display)
> in the case of QXL_CMD_DRAW sent by Qemu/applications:
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_DRAW: {
> auto red_drawable = red_drawable_new(worker->qxl,
> >mem_slots,
> ext_cmd.group_id,
> ext_cmd.cmd.data,
> ext_cmd.flags); // returns
> with 1 ref
>
> if (red_drawable) {
> display_channel_process_draw(worker->display_channel,
> std::move(red_drawable),
>
> worker->process_display_generation);
> }
>
Yes, it sounds a bit long but surely better than hacking Qemu, spice
server and defining a new hacky ABI that will be hard to maintain and
understand.
> The other option I can think of is to just not deal with drawables at all and
> somehow
> directly share the dmabuf fd with the Encoder. This solution also seems very
> messy
> and invasive to me as we'd not be able to leverage the existing APIs (in
> display-channel,
> video-stream, etc) that create and manage streams efficiently.
>
Yes, that currently seems pretty long as a change but possibly the
most clean in future, it's up to some refactory. The Encoder does not
either need technically a RedDrawable, Drawable but frame data encoded
in a format it can manage (either raw memory data or dmabuf at the
moment).
> >
> > > #ifdef HAVE_SPICE_GL
> > > +} else if (spice_dmabuf_encode) {
> > > +if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
> > > +error_report("dmabuf-encode=on currently only works and
> > > tested"
> > > + "with gstreamer:h264");
> > > +exit(1);
> > > +}
> >
> > IMHO we should not hard-code todays spice-server capabilities like this.
> > For starters this isn't true for spice-server versions which don't (yet)
> > have your patches. Also the capability might depend on hardware
> > support. IMHO we need some feature negotiation between qemu and spice
> > here.
> [Kasireddy, Vivek] Ok, I can get rid of this chunk in v3. However, given the
> numerous
> features supported by the Spice server, I suspect implementing feature
> negotiation
> might get really challenging. Is there any other way around this that you can
> think of?
>
> Thanks,
> Vivek
>
> >
> > take care,
> > Gerd
>
Frediano
Re: [RFC v2 1/2] spice: Add an option for users to provide a preferred codec
Il giorno lun 23 gen 2023 alle ore 08:37 Vivek Kasireddy
ha scritto:
>
> Giving users an option to choose a particular codec will enable
> them to make an appropriate decision based on their hardware and
> use-case.
>
> Cc: Gerd Hoffmann
> Cc: Marc-André Lureau
> Cc: Dongwon Kim
> Signed-off-by: Vivek Kasireddy
> ---
> qemu-options.hx | 5 +
> ui/spice-core.c | 14 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3aa3a2f5a3..aab8df0922 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2142,6 +2142,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> "
> [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
> " [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
> " [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
> +" [,preferred-codec=:\n"
> " [,gl=[on|off]][,rendernode=]\n"
> " enable spice\n"
> " at least one of {port, tls-port} is mandatory\n",
> @@ -2237,6 +2238,10 @@ SRST
> ``seamless-migration=[on|off]``
> Enable/disable spice seamless migration. Default is off.
>
> +``preferred-codec=:``
> +Provide the preferred codec the Spice server should use.
> +Default would be spice:mjpeg.
> +
> ``gl=[on|off]``
> Enable/disable OpenGL context. Default is off.
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 72f8f1681c..6e00211e3a 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -469,6 +469,9 @@ static QemuOptsList qemu_spice_opts = {
> },{
> .name = "streaming-video",
> .type = QEMU_OPT_STRING,
> +},{
> +.name = "preferred-codec",
> +.type = QEMU_OPT_STRING,
> },{
> .name = "agent-mouse",
> .type = QEMU_OPT_BOOL,
> @@ -644,6 +647,7 @@ static void qemu_spice_init(void)
> char *x509_key_file = NULL,
> *x509_cert_file = NULL,
> *x509_cacert_file = NULL;
> +const char *preferred_codec = NULL;
> int port, tls_port, addr_flags;
> spice_image_compression_t compression;
> spice_wan_compression_t wan_compr;
> @@ -795,6 +799,16 @@ static void qemu_spice_init(void)
> spice_server_set_streaming_video(spice_server,
> SPICE_STREAM_VIDEO_OFF);
> }
>
> +preferred_codec = qemu_opt_get(opts, "preferred-codec");
> +if (preferred_codec) {
> +if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
> +error_report("Preferred codec name is not valid");
> +exit(1);
> +}
> +} else {
> +spice_server_set_video_codecs(spice_server, "spice:mjpeg");
Why overriding the compiled in Spice default?
Apart from that the commit seems good to me.
> +}
> +
> spice_server_set_agent_mouse
> (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
> spice_server_set_playback_compression
Frediano
Re: [RFC PATCH 13/21] contrib/gitdm: Add more entries to the Red Hat domain
Il giorno lun 5 ott 2020 alle ore 09:39 Philippe Mathieu-Daudé < f4...@amsat.org> ha scritto: > On Mon, Oct 5, 2020 at 10:05 AM Frediano Ziglio > wrote: > > > > Hi, > > can I disagree? If the contribution is personal I use my personal > address, if the contribution is from the job in the company I'm using the > company address. > > Certainly! Can I add your personal address to the "individual > contributor" list instead? > > Sure > > > Regards, > >Frediano > > > > > > Il giorno dom 4 ott 2020 alle ore 19:05 Philippe Mathieu-Daudé < > f4...@amsat.org> ha scritto: > >> > >> Cc: Frediano Ziglio > >> Cc: Frediano Ziglio > >> Cc: Nir Soffer > >> Cc: Nir Soffer > >> Signed-off-by: Philippe Mathieu-Daudé > >> --- > >> To the developers Cc'ed: If you agree with your entry, please > >> reply with a Reviewed-by/Acked-by tag. If you disagree or doesn't > >> care, please either reply with Nack-by or ignore this patch. > >> I'll repost in 2 weeks as formal patch (not RFC) with only the > >> entries acked by their author. > >> --- > >> contrib/gitdm/group-map-redhat | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/contrib/gitdm/group-map-redhat > b/contrib/gitdm/group-map-redhat > >> index d15db2d35e..0419e82795 100644 > >> --- a/contrib/gitdm/group-map-redhat > >> +++ b/contrib/gitdm/group-map-redhat > >> @@ -6,3 +6,5 @@ da...@gibson.dropbear.id.au > >> laur...@vivier.eu > >> p...@fedoraproject.org > >> arm...@pond.sub.org > >> +fredd...@gmail.com > >> +nir...@gmail.com > >> -- > >> 2.26.2 > >> >
Re: [RFC PATCH 13/21] contrib/gitdm: Add more entries to the Red Hat domain
Hi, can I disagree? If the contribution is personal I use my personal address, if the contribution is from the job in the company I'm using the company address. Regards, Frediano Il giorno dom 4 ott 2020 alle ore 19:05 Philippe Mathieu-Daudé < f4...@amsat.org> ha scritto: > Cc: Frediano Ziglio > Cc: Frediano Ziglio > Cc: Nir Soffer > Cc: Nir Soffer > Signed-off-by: Philippe Mathieu-Daudé > --- > To the developers Cc'ed: If you agree with your entry, please > reply with a Reviewed-by/Acked-by tag. If you disagree or doesn't > care, please either reply with Nack-by or ignore this patch. > I'll repost in 2 weeks as formal patch (not RFC) with only the > entries acked by their author. > --- > contrib/gitdm/group-map-redhat | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/contrib/gitdm/group-map-redhat > b/contrib/gitdm/group-map-redhat > index d15db2d35e..0419e82795 100644 > --- a/contrib/gitdm/group-map-redhat > +++ b/contrib/gitdm/group-map-redhat > @@ -6,3 +6,5 @@ da...@gibson.dropbear.id.au > laur...@vivier.eu > p...@fedoraproject.org > arm...@pond.sub.org > +fredd...@gmail.com > +nir...@gmail.com > -- > 2.26.2 > >
Re: [PATCH v2 5/6] spice: get monitors physical dimension
>
> From: Marc-André Lureau
>
> Note that for consistency, we use the same logic as MonitorsConfig to
> figure out the associated monitor. However, I can't find traces of the
> discussion/patches about the "new spice-server" behaviour: it still uses
> the multiple-configurations path in git master.
>
This part is now obsolete.
> Signed-off-by: Marc-André Lureau
> ---
> include/ui/console.h | 3 +++
> ui/spice-display.c | 9 +
> 2 files changed, 12 insertions(+)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 353d2e49a1..e7303d8b98 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -119,6 +119,9 @@ typedef struct DisplaySurface {
> } DisplaySurface;
>
> typedef struct QemuUIInfo {
> +/* physical dimension */
> +uint16_t width_mm;
> +uint16_t height_mm;
> /* geometry */
> int xoff;
> int yoff;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index b304c13149..a10f72bc5c 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -679,7 +679,16 @@ static int interface_client_monitors_config(QXLInstance
> *sin,
> info.width = mc->monitors[head].width;
> info.height = mc->monitors[head].height;
> }
> +#if SPICE_SERVER_VERSION >= 0x000e04 /* release 0.14.4 */
> +if (mc->flags & VD_AGENT_CONFIG_MONITORS_FLAG_PHYSICAL_SIZE) {
> +VDAgentMonitorMM *mm = (void *)>monitors[mc->num_of_monitors];
>
> +if (mc->num_of_monitors > head) {
This check is the same of above. Won't be better to move al these block
inside the above if block and remove here?
> +info.width_mm = mm[head].width;
> +info.height_mm = mm[head].height;
> +}
> +}
> +#endif
> trace_qemu_spice_ui_info(ssd->qxl.id, info.width, info.height);
> dpy_set_ui_info(ssd->dcl.con, );
> return 1;
Frediano
Re: [PATCH] spice: remove obsolete callback
>
> From: Marc-André Lureau
>
> The "attach_worker" callbacks aren't doing anything in QEMU, but they
> were mandatory until Spice server commit
> 6aa1a17c69dc3cc02f338a78b3266e4c00ea1c1a ("spice-qxl: Remove QXLWorker
> definition").
>
> Furthermore, the old spelling is deprecated since commit
> 974692bda1e77af92b71ed43b022439448492cb9 ("spice-qxl: Fix typo in
> callback name and remove obsolete parameter")
>
> Compile that code out if Spice server version is recent enough.
>
> Fix compiler deprecation warnings with Spice > 0.14.3 (not released
> yet). We may want to wait until newer version is actually released to
> apply the patch.
>
You could use
#if SPICE_SERVER_VERSION <= 0x000e03 /* release 0.14.3 */
> Signed-off-by: Marc-André Lureau
> ---
> hw/display/qxl.c | 4
> ui/spice-display.c | 4
> 2 files changed, 8 insertions(+)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 11871340e7..a02072dee0 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -518,12 +518,14 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct
> QXLCommandExt *ext)
>
> /* spice display interface callbacks */
>
> +#if SPICE_SERVER_VERSION < 0x000e04 /* release 0.14.4 */
> static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
> {
> PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>
> trace_qxl_interface_attach_worker(qxl->id);
> }
> +#endif
>
> static void interface_set_compression_level(QXLInstance *sin, int level)
> {
> @@ -1156,7 +1158,9 @@ static const QXLInterface qxl_interface = {
> .base.major_version = SPICE_INTERFACE_QXL_MAJOR,
> .base.minor_version = SPICE_INTERFACE_QXL_MINOR,
>
> +#if SPICE_SERVER_VERSION < 0x000e04 /* release 0.14.4 */
> .attache_worker = interface_attach_worker,
> +#endif
For 0.14.4 you can provide .attached_worker instead, even if version used would
be downgraded will work. Same function could be used (with a cast) for
attache_worker.
It depends on the usefulness of the trace call.
> .set_compression_level = interface_set_compression_level,
> #if SPICE_NEEDS_SET_MM_TIME
> .set_mm_time = interface_set_mm_time,
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 19632fdf6c..811936ff7f 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -500,10 +500,12 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay
> *ssd)
>
> /* spice display interface callbacks */
>
> +#if SPICE_SERVER_VERSION < 0x000e04 /* release 0.14.4 */
> static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker)
> {
> /* nothing to do */
> }
> +#endif
>
> static void interface_set_compression_level(QXLInstance *sin, int level)
> {
> @@ -709,7 +711,9 @@ static const QXLInterface dpy_interface = {
> .base.major_version = SPICE_INTERFACE_QXL_MAJOR,
> .base.minor_version = SPICE_INTERFACE_QXL_MINOR,
>
> +#if SPICE_SERVER_VERSION < 0x000e04 /* release 0.14.4 */
> .attache_worker = interface_attach_worker,
> +#endif
> .set_compression_level = interface_set_compression_level,
> #if SPICE_NEEDS_SET_MM_TIME
> .set_mm_time = interface_set_mm_time,
Frediano
[PATCH] ui: Add more mouse buttons to SPICE
From: Frediano Ziglio
Add support for SIDE and EXTRA buttons.
The constants for buttons in both SPICE and QEMU are defined as
LEFT
MIDDLE
RIGHT
UP
DOWN
SIDE
EXTRA
(same order).
"button_mask" contains for each bit the state of a button. Qemu currently
uses bits 0, 1, 2 respectively as LEFT, RIGHT, MIDDLE; also add bits 4
and 5 as UP and DOWN (using wheel movements). SPICE protocol uses
a bitmask based on the order above where LEFT is bit 0, MIDDLE is
bit 1 and so on till EXTRA being bit 6. To avoid clash with Qemu usage
SPICE bitmask from SIDE are move a bit more resulting respectively
in 0x40 and 0x80 values.
Signed-off-by: Frediano Ziglio
---
ui/spice-input.c | 2 ++
1 file changed, 2 insertions(+)
See also https://gitlab.freedesktop.org/spice/spice/-/merge_requests/140
diff --git a/ui/spice-input.c b/ui/spice-input.c
index cd4bb0043f..d5bba231c9 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -123,6 +123,8 @@ static void spice_update_buttons(QemuSpicePointer *pointer,
[INPUT_BUTTON_RIGHT] = 0x02,
[INPUT_BUTTON_WHEEL_UP]= 0x10,
[INPUT_BUTTON_WHEEL_DOWN] = 0x20,
+[INPUT_BUTTON_SIDE]= 0x40,
+[INPUT_BUTTON_EXTRA] = 0x80,
};
if (wheel < 0) {
--
2.25.4
[PATCH] ui: Add more mouse buttons to SPICE
From: Frediano Ziglio
Add support for SIDE and EXTRA buttons.
Signed-off-by: Frediano Ziglio
---
ui/spice-input.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ui/spice-input.c b/ui/spice-input.c
index cd4bb0043f..d5bba231c9 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -123,6 +123,8 @@ static void spice_update_buttons(QemuSpicePointer *pointer,
[INPUT_BUTTON_RIGHT] = 0x02,
[INPUT_BUTTON_WHEEL_UP]= 0x10,
[INPUT_BUTTON_WHEEL_DOWN] = 0x20,
+[INPUT_BUTTON_SIDE]= 0x40,
+[INPUT_BUTTON_EXTRA] = 0x80,
};
if (wheel < 0) {
--
2.25.4
[PATCH] ui: Add more mouse buttons to SPICE
Add support for SIDE and EXTRA buttons.
Signed-off-by: Frediano Ziglio
---
ui/spice-input.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ui/spice-input.c b/ui/spice-input.c
index cd4bb0043f..d5bba231c9 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -123,6 +123,8 @@ static void spice_update_buttons(QemuSpicePointer *pointer,
[INPUT_BUTTON_RIGHT] = 0x02,
[INPUT_BUTTON_WHEEL_UP]= 0x10,
[INPUT_BUTTON_WHEEL_DOWN] = 0x20,
+[INPUT_BUTTON_SIDE]= 0x40,
+[INPUT_BUTTON_EXTRA] = 0x80,
};
if (wheel < 0) {
--
2.25.4
Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms
>
> On 10/23/19 8:42 AM, Laurent Vivier wrote:
> > Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> >> Signed-off-by: Frediano Ziglio
> >> ---
> >> util/qemu-timer.c | 6 +-
> >> 1 file changed, 1 insertion(+), 5 deletions(-)
> >>
> >> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> >> index d428fec567..094a20a05a 100644
> >> --- a/util/qemu-timer.c
> >> +++ b/util/qemu-timer.c
> >> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
> >> ms = DIV_ROUND_UP(ns, SCALE_MS);
> >>
> >> /* To avoid overflow problems, limit this to 2^31, i.e. approx 25
> >> days */
> >> -if (ms > (int64_t) INT32_MAX) {
> >> -ms = INT32_MAX;
> >> -}
> >> -
> >> -return (int) ms;
> >> +return (int) MIN(ms, (int64_t) INT32_MAX);
> >> }
>
> Why so many casts? It should also work as:
>
> return MIN(ms, INT32_MAX);
>
This was former version. Laurent pointed out that MIN macro
is using ternary operator which is expected to find the same time
on second and third part so the cast inside the MIN macro.
The cast before MIN was kept from previous code.
Frediano
[PATCH 2/3] event_notifier: avoid dandling file descriptor in event_notifier_cleanup
If rfd is equal to wfd the file descriptor is closed but
rfd will still have the closed value.
The EventNotifier structure should not be used again after calling
event_notifier_cleanup or should be initialized again but make
sure to not have dandling file descriptors around.
Signed-off-by: Frediano Ziglio
---
util/event_notifier-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
index 73c4046b58..00d93204f9 100644
--- a/util/event_notifier-posix.c
+++ b/util/event_notifier-posix.c
@@ -80,8 +80,8 @@ void event_notifier_cleanup(EventNotifier *e)
{
if (e->rfd != e->wfd) {
close(e->rfd);
-e->rfd = -1;
}
+e->rfd = -1;
close(e->wfd);
e->wfd = -1;
}
--
2.21.0
[PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms
Signed-off-by: Frediano Ziglio
---
util/qemu-timer.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index d428fec567..094a20a05a 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
ms = DIV_ROUND_UP(ns, SCALE_MS);
/* To avoid overflow problems, limit this to 2^31, i.e. approx 25 days */
-if (ms > (int64_t) INT32_MAX) {
-ms = INT32_MAX;
-}
-
-return (int) ms;
+return (int) MIN(ms, (int64_t) INT32_MAX);
}
--
2.21.0
[PATCH 1/3] util/async: avoid useless cast
event_notifier_dummy_cb is already compatible with EventNotifierHandler. Signed-off-by: Frediano Ziglio --- util/async.c | 1 - 1 file changed, 1 deletion(-) diff --git a/util/async.c b/util/async.c index ca83e32c7f..b1fa5319e5 100644 --- a/util/async.c +++ b/util/async.c @@ -429,7 +429,6 @@ AioContext *aio_context_new(Error **errp) aio_set_event_notifier(ctx, >notifier, false, - (EventNotifierHandler *) event_notifier_dummy_cb, event_notifier_poll); #ifdef CONFIG_LINUX_AIO -- 2.21.0
Re: [Qemu-devel] [RFC] spice-core: allow setting properties from QMP
>
> Hello Eric,
>
> > A new command may be okay, however,
>
> thanks, I've fix the typos and updated the patch to use an Enum, which
> indeed makes more sense.
>
> I've also updated "spice-query" command to provide the current value
> of the "video-codec" property,
> but it made me wonder if I should improve this QMP interface with a
> json list, or keep the current string-based list
> ("enc1:codec1;enc2:codec2").
>
> I CC the spice-devel list to get their point of view
>
> The current behavior is:
>
> --> { "execute": "set-spice", "arguments": { "property":
> "video-codecs", "value": "spice:mjpeg;gstreamer:h264" } }
> <-- {"return":{},"id":"libvirt-23"}
It looks complicated from the user. Why not just
--> { "execute": "set-spice", "arguments": { "video-codecs":
"spice:mjpeg;gstreamer:h264" } }
>
> --> { "execute": "query-spice" }
> <-- { "video-codecs": "spice:mjpeg;gstreamer:h264;" }
>
>
> I put the new version of the RFC patch below
>
> best regards,
>
> Kevin
>
> ---
>
> This patch allows setting spice properties from the QMP interface.
>
> At the moment, only the 'video-codecs' property is supported.
>
> Signed-off-by: Kevin Pouget
> ---
> qapi/ui.json| 42 --
> ui/spice-core.c | 21 +
> 2 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 59e412139a..5f67096bcb 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -211,12 +211,16 @@
> #
> # @channels: a list of @SpiceChannel for each active spice channel
> #
> +# @video-codecs: The list of encoders:codecs currently allowed for
> +#video streaming (since: ...)
> +#
> # Since: 0.14.0
> ##
> { 'struct': 'SpiceInfo',
>'data': {'enabled': 'bool', 'migrated': 'bool', '*host': 'str',
> '*port': 'int',
> '*tls-port': 'int', '*auth': 'str', '*compiled-version': 'str',
> - 'mouse-mode': 'SpiceQueryMouseMode', '*channels':
> ['SpiceChannel']},
> + 'mouse-mode': 'SpiceQueryMouseMode', '*channels':
> ['SpiceChannel'],
> + 'video-codecs': 'str'},
>'if': 'defined(CONFIG_SPICE)' }
>
> ##
> @@ -257,7 +261,8 @@
> #"tls": false
> # },
> # [ ... more channels follow ... ]
> -# ]
> +# ],
> +# "video-codecs": "spice:mjpeg;gstreamer:h264;"
> # }
> #}
> #
> @@ -265,6 +270,39 @@
> { 'command': 'query-spice', 'returns': 'SpiceInfo',
>'if': 'defined(CONFIG_SPICE)' }
>
> +##
> +# @SpiceProperty:
> +#
> +# An enumeration of Spice properties that can be set at runtime.
> +#
> +# @video-codecs: the ;-separated list of video-codecs allowed for
> +# spice-server video streaming.
> +#
> +# Since: ...
> +##
> +{ 'enum': 'SpiceProperty',
> + 'data': [ 'video-codecs'],
> + 'if': 'defined(CONFIG_SPICE)' }
> +
> +##
> +# @set-spice:
> +#
> +# Set Spice properties.
> +# @property: the SPICE property to modify
> +# @value: the new value to affect to this property
> +#
> +# Since: ...
> +#
> +# Example:
> +#
> +# -> { "execute": "set-spice", "arguments": { "property": "video-codecs",
> +# "value": "spice:mjpeg;" } }
> +# <- { "returns": {} }
> +##
> +{ 'command': 'set-spice',
> + 'data': {'property': 'SpiceProperty', 'value': 'str'},
> + 'if': 'defined(CONFIG_SPICE)' }
> +
> ##
> # @SPICE_CONNECTED:
> #
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 63e8694df8..1660f49f15 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -506,6 +506,25 @@ static QemuOptsList qemu_spice_opts = {
> },
> };
>
> +void qmp_set_spice(SpiceProperty property, const char *value, Error **errp)
> +{
> +int invalid_codecs;
> +
> +switch(property) {
> +case SPICE_PROPERTY_VIDEO_CODECS:
> +invalid_codecs = spice_server_set_video_codecs(spice_server, value);
> +
> +if (invalid_codecs) {
> +error_setg(errp, "Found %d invalid video-codecs while
> setting spice"
> + " property 'video-codec=%s'", invalid_codecs, value);
> +}
> +break;
> +default:
> +/* only reached in case of version mismatched */
> +error_setg(errp, "Property #%d not supported.", property);
> +}
> +}
> +
> SpiceInfo *qmp_query_spice(Error **errp)
> {
> QemuOpts *opts = QTAILQ_FIRST(_spice_opts.head);
> @@ -555,6 +574,8 @@ SpiceInfo *qmp_query_spice(Error **errp)
> SPICE_QUERY_MOUSE_MODE_SERVER :
> SPICE_QUERY_MOUSE_MODE_CLIENT;
>
> +info->video_codecs = spice_server_get_video_codecs(spice_server);
> +
> /* for compatibility with the original command */
> info->has_channels = true;
> info->channels = qmp_query_spice_channels();
> --
> 2.21.0
>
>
[Qemu-devel] [PATCH 2/2] audio: Do not check for audio_calloc failure
audio_calloc uses g_malloc0 which never returns in case of
memory failure.
Signed-off-by: Frediano Ziglio
---
audio/audio.c | 48 ++--
1 file changed, 6 insertions(+), 42 deletions(-)
diff --git a/audio/audio.c b/audio/audio.c
index 472721a7a9..909c817103 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -811,12 +811,7 @@ static int audio_attach_capture (HWVoiceOut *hw)
SWVoiceOut *sw;
HWVoiceOut *hw_cap = >hw;
-sc = audio_calloc(__func__, 1, sizeof(*sc));
-if (!sc) {
-dolog ("Could not allocate soft capture voice (%zu bytes)\n",
- sizeof (*sc));
-return -1;
-}
+sc = g_malloc0(sizeof(*sc));
sc->cap = cap;
sw = >sw;
@@ -1960,15 +1955,10 @@ CaptureVoiceOut *AUD_add_capture (
if (audio_validate_settings (as)) {
dolog ("Invalid settings were passed when trying to add capture\n");
audio_print_settings (as);
-goto err0;
+return NULL;
}
-cb = audio_calloc(__func__, 1, sizeof(*cb));
-if (!cb) {
-dolog ("Could not allocate capture callback information, size %zu\n",
- sizeof (*cb));
-goto err0;
-}
+cb = g_malloc0(sizeof(*cb));
cb->ops = *ops;
cb->opaque = cb_opaque;
@@ -1981,12 +1971,7 @@ CaptureVoiceOut *AUD_add_capture (
HWVoiceOut *hw;
CaptureVoiceOut *cap;
-cap = audio_calloc(__func__, 1, sizeof(*cap));
-if (!cap) {
-dolog ("Could not allocate capture voice, size %zu\n",
- sizeof (*cap));
-goto err1;
-}
+cap = g_malloc0(sizeof(*cap));
hw = >hw;
QLIST_INIT (>sw_head);
@@ -1994,23 +1979,11 @@ CaptureVoiceOut *AUD_add_capture (
/* XXX find a more elegant way */
hw->samples = 4096 * 4;
-hw->mix_buf = audio_calloc(__func__, hw->samples,
- sizeof(struct st_sample));
-if (!hw->mix_buf) {
-dolog ("Could not allocate capture mix buffer (%d samples)\n",
- hw->samples);
-goto err2;
-}
+hw->mix_buf = g_new0(struct st_sample, hw->samples);
audio_pcm_init_info (>info, as);
-cap->buf = audio_calloc(__func__, hw->samples, 1 << hw->info.shift);
-if (!cap->buf) {
-dolog ("Could not allocate capture buffer "
- "(%d samples, each %d bytes)\n",
- hw->samples, 1 << hw->info.shift);
-goto err3;
-}
+cap->buf = g_malloc0_n(hw->samples, 1 << hw->info.shift);
hw->clip = mixeng_clip
[hw->info.nchannels == 2]
@@ -2025,15 +1998,6 @@ CaptureVoiceOut *AUD_add_capture (
audio_attach_capture (hw);
}
return cap;
-
-err3:
-g_free (cap->hw.mix_buf);
-err2:
-g_free (cap);
-err1:
-g_free (cb);
-err0:
-return NULL;
}
}
--
2.20.1
[Qemu-devel] [PATCH 1/2] audio: Use g_strdup_printf instead of manual building a string
Instead of using lot of low level function and manually allocate
the temporary string in audio_process_options use more high
level GLib function. The function is not used in hot path but to
read some initial setting.
Signed-off-by: Frediano Ziglio
---
audio/audio.c | 25 +
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/audio/audio.c b/audio/audio.c
index d163ffbc88..472721a7a9 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -454,9 +454,7 @@ static void audio_print_options (const char *prefix,
static void audio_process_options (const char *prefix,
struct audio_option *opt)
{
-char *optname;
-const char qemu_prefix[] = "QEMU_";
-size_t preflen, optlen;
+gchar *prefix_upper;
if (audio_bug(__func__, !prefix)) {
dolog ("prefix = NULL\n");
@@ -468,10 +466,10 @@ static void audio_process_options (const char *prefix,
return;
}
-preflen = strlen (prefix);
+prefix_upper = g_utf8_strup(prefix, -1);
for (; opt->name; opt++) {
-size_t len, i;
+char *optname;
int def;
if (!opt->valp) {
@@ -480,21 +478,7 @@ static void audio_process_options (const char *prefix,
continue;
}
-len = strlen (opt->name);
-/* len of opt->name + len of prefix + size of qemu_prefix
- * (includes trailing zero) + zero + underscore (on behalf of
- * sizeof) */
-optlen = len + preflen + sizeof (qemu_prefix) + 1;
-optname = g_malloc (optlen);
-
-pstrcpy (optname, optlen, qemu_prefix);
-
-/* copy while upper-casing, including trailing zero */
-for (i = 0; i <= preflen; ++i) {
-optname[i + sizeof (qemu_prefix) - 1] = qemu_toupper(prefix[i]);
-}
-pstrcat (optname, optlen, "_");
-pstrcat (optname, optlen, opt->name);
+optname = g_strdup_printf("QEMU_%s_%s", prefix_upper, opt->name);
def = 1;
switch (opt->tag) {
@@ -532,6 +516,7 @@ static void audio_process_options (const char *prefix,
*opt->overriddenp = !def;
g_free (optname);
}
+g_free(prefix_upper);
}
static void audio_print_settings (struct audsettings *as)
--
2.20.1
[Qemu-devel] [PATCH] spice: Remove unused include
The definitions in the header are not used. Also this fixes porting SPICE to Windows where the header is not available. Signed-off-by: Frediano Ziglio --- ui/spice-core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/spice-core.c b/ui/spice-core.c index 525e0929b9..fb87944a24 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -18,7 +18,6 @@ #include "qemu/osdep.h" #include -#include #include "sysemu/sysemu.h" #include "ui/qemu-spice.h" -- 2.20.1
Re: [Qemu-devel] [Spice-devel] Always get Invalid password while trying to connect to spice server
> > On martedì 25 dicembre 2018 09:04:31 CET, Uri Lublin wrote: > > Hi, > > Hi and thanks for your answer. > > > It's hard to tell without more details. > > I'll try to provide all the details, let me know if you need anything else. > > > How do you set the password ? > > I set the password using the virt-manager interface: in the "Spice server" > section I just check the "password" flag and I set a password. It used to > work. I don't use virt-manager directly from the virtualization server > because it doesn't have any graphical interface: I connect to it using > virt-manager from my desktop PC (more details follow). > > > Do you use secure connections ? > > I connect to the remote libvirt server using virt-manager from my desktop. > The libvirt URI is qemu+ssh://root@ip:22/system so I use ssh to connect. > > > Maybe you turned on a firewall and a rule is missing. > > There is a firewall, but it didn't change. SSH port is open (and I can > connect to the libvirt server using virt-manager). I also opened a broad > range of spice ports (5900-5930) and that works too because if I uncheck > the "password" field it connects to the spice server without any issue. > > I also tried to connect directly to the spice server using virt-viewer > instead of virt-manager: > > remote-viewer spice://ip:5906 > > 5906 is the spice port. I can check which VM gets assigned to which port > using the virt-manager interface, in the "Spice server" section. > > remote-viewer triggers the same error: wrong password. > > > What is your qemu-kvm command line ? > > LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/bin QEMU_AUDIO_DRV=spice > /usr/bin/qemu-system-x86_64 -name guest=guild-devel,debug-threads=on -S > -object > secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-17-guild-devel/master-key.aes > -machine pc-q35-3.0,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu > EPYC-IBPB,x2apic=on,tsc-deadline=on,hypervisor=on,tsc_adjust=on,cmp_legacy=on,perfctr_core=on,virt-ssbd=on,monitor=off > -drive > file=/usr/share/ovmf/x64/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on > -drive > file=/var/lib/libvirt/qemu/nvram/guild-devel_VARS.fd,if=pflash,format=raw,unit=1 > -m 4096 -realtime mlock=off -smp 16,sockets=16,cores=1,threads=1 -uuid > fd44b44b-2e22-4d2f-ae19-433934443576 -no-user-config -nodefaults -chardev > socket,id=charmonitor,fd=32,server,nowait -mon > chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew > -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global > ICH9-LPC.disable_s3=1 -global ICH9-LPC.disable_s4=1 -boot strict=on -device > pcie-root-port,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x2 > -device pcie-root-port,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 > -device pcie-root-port,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 > -device pcie-root-port,port=0x13,chassis=4,id=pci.4,bus=pcie.0,addr=0x2.0x3 > -device pcie-root-port,port=0x14,chassis=5,id=pci.5,bus=pcie.0,addr=0x2.0x4 > -device pcie-root-port,port=0x15,chassis=6,id=pci.6,bus=pcie.0,addr=0x2.0x5 > -device pcie-root-port,port=0x16,chassis=7,id=pci.7,bus=pcie.0,addr=0x2.0x6 > -device qemu-xhci,p2=15,p3=15,id=usb,bus=pci.2,addr=0x0 -device > virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 -drive > file=/var/lib/libvirt/images/Fedora-Workstation-Live-x86_64-29-1.2.iso,format=raw,if=none,id=drive-sata0-0-0,media=cdrom,readonly=on > -device ide-cd,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0,bootindex=2 > -drive > file=/var/lib/libvirt/images/guild-devel/guild-devel.qcow2,format=qcow2,if=none,id=drive-virtio-disk0,cache=writeback,aio=threads > -device > virtio-blk-pci,scsi=off,bus=pci.4,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,write-cache=on,werror=stop,rerror=stop > -netdev tap,fd=35,id=hostnet0,vhost=on,vhostfd=36 -device > virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:b6:70:81,bus=pci.1,addr=0x0 > -chardev pty,id=charserial0 -device > isa-serial,chardev=charserial0,id=serial0 -chardev > socket,id=charchannel0,fd=37,server,nowait -device > virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 > -chardev spicevmc,id=charchannel1,name=vdagent -device > virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0 > -device usb-tablet,id=input0,bus=usb.0,port=1 -spice > port=5905,addr=0.0.0.0,seamless-migration=on -k en-us -device > virtio-vga,id=video0,virgl=on,max_outputs=1,bus=pcie.0,addr=0x1 -device > ich9-intel-hda,id=sound0,bus=pcie.0,addr=0x1b -device > hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev > spicevmc,id=charredir0,name=usbredir -device > usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 -chardev > spicevmc,id=charredir1,name=usbredir -device > usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 -device > virtio-balloon-pci,id=balloon0,bus=pci.5,addr=0x0 -object >
Re: [Qemu-devel] [PATCH for-4.0 v3] configure: bump spice-server required version to 0.12.5
> > On Thu, Nov 29, 2018 at 03:32:28AM -0500, Frediano Ziglio wrote: > > > On Thu, Nov 29, 2018 at 12:09 PM Gerd Hoffmann wrote: > > > > > > > > On Wed, Nov 28, 2018 at 07:59:32PM +0400, Marc-André Lureau wrote: > > > > > Looking at chardev/spice.c code, I realize compilation was broken for > > > > > a while with spice-server < 0.12.3. Let's bump required version > > > > > to 0.12.5, released May 19 2014, instead of adding more #ifdef. > > > > > > > > Oh, you did the 0.12.5 patch already. Scratch my other reply then. > > > > > > > > > - if $pkg_config --atleast-version=0.12.0 spice-server && \ > > > > > + if $pkg_config --atleast-version=0.12.5 spice-server && \ > > > > > $pkg_config --atleast-version=0.12.3 spice-protocol && \ > > > > > > > > I think we should adjust spice-protocol too to whatever 0.12.5 requires > > > > to build. > > > > > > > > > > Why not leave that responsibility to pkg-config, and only require in > > > qemu what is required there? > > > > > > > > > > That is remove explicit requirement in configure script? > > I can see that spice-core.h (spice-server, one of the mail include) is > > including spice-protocol headers. > > Looking at configure both are required so would make sense to check > > only spice-server, unless packaging has some bugs if you have spice-server > > (devel) installed you also have spice-protocol. > > seems the spice-protocol dep is there due to qemu itself needing it: > > commit 358689fe299c306f1d81bea57a5067d0abb56699 > Author: Michal Privoznik > Date: Fri Mar 1 08:43:18 2013 +0100 > > configure: Require at least spice-protocol-0.12.3 > > As of 5a49d3e9 we assume SPICE_PORT_EVENT_BREAK to be defined. > However, it is defined not in 0.12.2 what we require now, but in > 0.12.3. Therefore in order to prevent build failure we must > adjust our minimal requirements. > > Signed-off-by: Stefan Hajnoczi > > That makes sense. So, when spice-server 0.12.5 requires spice-protocol > 0.12.8+ anyway I think we can savely drop the spice-protocol check. > > cheers, > Gerd > My idea was more to revert (kind of) 7e3efdac75caca0b283f8e76ad24c924b4718e7b: commit 7e3efdac75caca0b283f8e76ad24c924b4718e7b Author: Alon Levy Date: Wed Mar 7 16:19:03 2012 +0200 spice: require spice-protocol >= 0.8.1 Requiring spice-server >= 0.8.2 is not enough since spice-server.pc doesn't require spice-protocol (any version). Until that is fixed upstream an explicit requirement in qemu fixes compilation broken since commit 2e1a98c9c1b90ca093278c6b43244dc46604d7b7 Author: Alon Levy Date: Fri Feb 24 23:19:30 2012 +0200 qxl: introduce QXLCookie Reported-by: Peter Maydell Signed-off-by: Alon Levy Signed-off-by: Gerd Hoffmann diff --git a/configure b/configure index 0111774cb0..62aa7609e1 100755 --- a/configure +++ b/configure @@ -2592,6 +2592,7 @@ EOF spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null) spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null) if $pkg_config --atleast-version=0.8.2 spice-server >/dev/null 2>&1 && \ + $pkg_config --atleast-version=0.8.1 spice-protocol > /dev/null 2>&1 && \ compile_prog "$spice_cflags" "$spice_libs" ; then spice="yes" libs_softmmu="$libs_softmmu $spice_libs" The rationale does not apply any more, spice-server depends on spice-protocol. Frediano
Re: [Qemu-devel] [PATCH for-4.0 v3] configure: bump spice-server required version to 0.12.5
> On Thu, Nov 29, 2018 at 12:09 PM Gerd Hoffmann wrote: > > > > On Wed, Nov 28, 2018 at 07:59:32PM +0400, Marc-André Lureau wrote: > > > Looking at chardev/spice.c code, I realize compilation was broken for > > > a while with spice-server < 0.12.3. Let's bump required version > > > to 0.12.5, released May 19 2014, instead of adding more #ifdef. > > > > Oh, you did the 0.12.5 patch already. Scratch my other reply then. > > > > > - if $pkg_config --atleast-version=0.12.0 spice-server && \ > > > + if $pkg_config --atleast-version=0.12.5 spice-server && \ > > > $pkg_config --atleast-version=0.12.3 spice-protocol && \ > > > > I think we should adjust spice-protocol too to whatever 0.12.5 requires > > to build. > > > > Why not leave that responsibility to pkg-config, and only require in > qemu what is required there? > > That is remove explicit requirement in configure script? I can see that spice-core.h (spice-server, one of the mail include) is including spice-protocol headers. Looking at configure both are required so would make sense to check only spice-server, unless packaging has some bugs if you have spice-server (devel) installed you also have spice-protocol. Frediano
Re: [Qemu-devel] [PATCH for-4.0 v3] configure: bump spice-server required version to 0.12.5
> > On Wed, Nov 28, 2018 at 07:59:32PM +0400, Marc-André Lureau wrote: > > Looking at chardev/spice.c code, I realize compilation was broken for > > a while with spice-server < 0.12.3. Let's bump required version > > to 0.12.5, released May 19 2014, instead of adding more #ifdef. > > Oh, you did the 0.12.5 patch already. Scratch my other reply then. > > > - if $pkg_config --atleast-version=0.12.0 spice-server && \ > > + if $pkg_config --atleast-version=0.12.5 spice-server && \ > > $pkg_config --atleast-version=0.12.3 spice-protocol && \ > > I think we should adjust spice-protocol too to whatever 0.12.5 requires > to build. > > cheers, > Gerd > Was 0.12.8 Frediano
Re: [Qemu-devel] [PATCH for-4.0 v3] configure: bump spice-server required version to 0.12.5
>
> Looking at chardev/spice.c code, I realize compilation was broken for
> a while with spice-server < 0.12.3. Let's bump required version
> to 0.12.5, released May 19 2014, instead of adding more #ifdef.
>
> (this patch combines changes from an early version and some of
> Frediano "[PATCH 2/2] spice: Bump required spice-server version to
> 0.12.6")
>
Minor, I didn't do much, you can remove this last paragraph
> According to repology, all the distros that are build target platforms
> for QEMU include it:
>
> RHEL-7: 0.14.0
> Debian (Stretch): 0.12.8
> Debian (Jessie): 0.12.5
> FreeBSD (ports): 0.14.0
> OpenSUSE Leap 15: 0.14.0
> Ubuntu (Xenial): 0.12.6
>
> Note that a previous version of this patch was bumping version to
> 0.12.6. Unfortunately, Debian Jessie (oldstable) is stuck with spice
> server 0.12.5, and QEMU should keep building until after 2y of current
> stable (Stretch), which will be around June 17th 2019. Qemu 4.1
> should thus be free of bumping to spice-server 0.12.6 during 4.1
> development cycle.
>
I would not refer to previous patch version, kind of starting with
the "Debian Jessie "...
> Signed-off-by: Marc-André Lureau
> ---
> include/ui/qemu-spice.h | 6 --
> chardev/spice.c | 10 --
> hw/display/qxl.c| 2 --
> ui/spice-core.c | 8
> configure | 4 ++--
> 5 files changed, 2 insertions(+), 28 deletions(-)
>
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index c6d50eb87a..8c23dfe717 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -46,13 +46,7 @@ int qemu_spice_migrate_info(const char *hostname, int
> port, int tls_port,
> #else
> #define SPICE_NEEDS_SET_MM_TIME 0
> #endif
> -
Really minor, an empty line would not be bad.
> -#if SPICE_SERVER_VERSION >= 0x000c02
> void qemu_spice_register_ports(void);
> -#else
> -static inline Chardev *qemu_chr_open_spice_port(const char *name)
> -{ return NULL; }
> -#endif
>
> #else /* CONFIG_SPICE */
>
> diff --git a/chardev/spice.c b/chardev/spice.c
> index e66e3ad568..173c257949 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -77,7 +77,6 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t
> *buf, int len)
> return bytes;
> }
>
> -#if SPICE_SERVER_VERSION >= 0x000c02
> static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t event)
> {
> SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
> @@ -95,7 +94,6 @@ static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t
> event)
> trace_spice_vmc_event(chr_event);
> qemu_chr_be_event(chr, chr_event);
> }
> -#endif
>
> static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
> {
> @@ -119,9 +117,7 @@ static SpiceCharDeviceInterface vmc_interface = {
> .state = vmc_state,
> .write = vmc_write,
> .read = vmc_read,
> -#if SPICE_SERVER_VERSION >= 0x000c02
> .event = vmc_event,
> -#endif
> #if SPICE_SERVER_VERSION >= 0x000c06
> .flags = SPICE_CHAR_DEVICE_NOTIFY_WRITABLE,
> #endif
> @@ -223,9 +219,7 @@ static void char_spice_finalize(Object *obj)
> }
>
> g_free((char *)s->sin.subtype);
> -#if SPICE_SERVER_VERSION >= 0x000c02
> g_free((char *)s->sin.portname);
> -#endif
> }
>
> static void spice_vmc_set_fe_open(struct Chardev *chr, int fe_open)
> @@ -240,7 +234,6 @@ static void spice_vmc_set_fe_open(struct Chardev *chr,
> int fe_open)
>
> static void spice_port_set_fe_open(struct Chardev *chr, int fe_open)
> {
> -#if SPICE_SERVER_VERSION >= 0x000c02
> SpiceChardev *s = SPICE_CHARDEV(chr);
>
> if (fe_open) {
> @@ -248,7 +241,6 @@ static void spice_port_set_fe_open(struct Chardev *chr,
> int fe_open)
> } else {
> spice_server_port_event(>sin, SPICE_PORT_EVENT_CLOSED);
> }
> -#endif
> }
>
> static void spice_chr_accept_input(struct Chardev *chr)
> @@ -298,7 +290,6 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
> chr_open(chr, type);
> }
>
> -#if SPICE_SERVER_VERSION >= 0x000c02
> static void qemu_chr_open_spice_port(Chardev *chr,
> ChardevBackend *backend,
> bool *be_opened,
> @@ -331,7 +322,6 @@ void qemu_spice_register_ports(void)
> vmc_register_interface(s);
> }
> }
> -#endif
>
> static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend
> *backend,
> Error **errp)
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 9087db5dee..8e9a65e75b 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1189,9 +1189,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d)
> return;
> }
> trace_qxl_enter_vga_mode(d->id);
> -#if SPICE_SERVER_VERSION >= 0x000c03 /* release 0.12.3 */
> spice_qxl_driver_unload(>ssd.qxl);
> -#endif
>
Re: [Qemu-devel] [Spice-devel] [PATCH 2/2] spice: Bump required spice-server version to 0.12.6
>
> On Wed, Nov 28, 2018 at 12:23 PM Frediano Ziglio wrote:
> >
> > Version 0.12.6 was released on 12th June 2015.
> >
> > Signed-off-by: Frediano Ziglio
> > ---
> > chardev/spice.c | 12
> > configure | 4 ++--
> > hw/display/qxl.c| 27 ---
> > hw/display/qxl.h| 2 --
> > include/ui/qemu-spice.h | 12
> > ui/spice-core.c | 8
> > ui/spice-display.c | 10 --
> > 7 files changed, 2 insertions(+), 73 deletions(-)
> >
> > diff --git a/chardev/spice.c b/chardev/spice.c
> > index e66e3ad568..e0b44474f5 100644
> > --- a/chardev/spice.c
> > +++ b/chardev/spice.c
> > @@ -77,7 +77,6 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t
> > *buf, int len)
> > return bytes;
> > }
> >
> > -#if SPICE_SERVER_VERSION >= 0x000c02
> > static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t event)
> > {
> > SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
> > @@ -95,7 +94,6 @@ static void vmc_event(SpiceCharDeviceInstance *sin,
> > uint8_t event)
> > trace_spice_vmc_event(chr_event);
> > qemu_chr_be_event(chr, chr_event);
> > }
> > -#endif
> >
> > static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
> > {
> > @@ -119,12 +117,8 @@ static SpiceCharDeviceInterface vmc_interface = {
> > .state = vmc_state,
> > .write = vmc_write,
> > .read = vmc_read,
> > -#if SPICE_SERVER_VERSION >= 0x000c02
> > .event = vmc_event,
> > -#endif
> > -#if SPICE_SERVER_VERSION >= 0x000c06
> > .flags = SPICE_CHAR_DEVICE_NOTIFY_WRITABLE,
> > -#endif
> > };
> >
> >
> > @@ -223,9 +217,7 @@ static void char_spice_finalize(Object *obj)
> > }
> >
> > g_free((char *)s->sin.subtype);
> > -#if SPICE_SERVER_VERSION >= 0x000c02
> > g_free((char *)s->sin.portname);
> > -#endif
> > }
> >
> > static void spice_vmc_set_fe_open(struct Chardev *chr, int fe_open)
> > @@ -240,7 +232,6 @@ static void spice_vmc_set_fe_open(struct Chardev *chr,
> > int fe_open)
> >
> > static void spice_port_set_fe_open(struct Chardev *chr, int fe_open)
> > {
> > -#if SPICE_SERVER_VERSION >= 0x000c02
> > SpiceChardev *s = SPICE_CHARDEV(chr);
> >
> > if (fe_open) {
> > @@ -248,7 +239,6 @@ static void spice_port_set_fe_open(struct Chardev *chr,
> > int fe_open)
> > } else {
> > spice_server_port_event(>sin, SPICE_PORT_EVENT_CLOSED);
> > }
> > -#endif
> > }
> >
> > static void spice_chr_accept_input(struct Chardev *chr)
> > @@ -298,7 +288,6 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
> > chr_open(chr, type);
> > }
> >
> > -#if SPICE_SERVER_VERSION >= 0x000c02
> > static void qemu_chr_open_spice_port(Chardev *chr,
> > ChardevBackend *backend,
> > bool *be_opened,
> > @@ -331,7 +320,6 @@ void qemu_spice_register_ports(void)
> > vmc_register_interface(s);
> > }
> > }
> > -#endif
> >
> > static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend
> > *backend,
> > Error **errp)
> > diff --git a/configure b/configure
> > index 0a3c6a72c3..b8f1fd053d 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4563,8 +4563,8 @@ int main(void) { spice_server_new(); return 0; }
> > EOF
> >spice_cflags=$($pkg_config --cflags spice-protocol spice-server
> >2>/dev/null)
> >spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
> > - if $pkg_config --atleast-version=0.12.0 spice-server && \
> > - $pkg_config --atleast-version=0.12.3 spice-protocol && \
> > + if $pkg_config --atleast-version=0.12.6 spice-server && \
> > + $pkg_config --atleast-version=0.12.10 spice-protocol && \
>
> Why do you bump spice-protocol?
>
Why not? spice-server 0.12.6 requires spice-protocol 0.12.10.
Frediano
Re: [Qemu-devel] [Spice-devel] [PATCH] spice: Use new SpiceImageCompression definition
> > Hi > On Wed, Nov 28, 2018 at 9:48 AM Gerd Hoffmann wrote: > > > > On Tue, Nov 27, 2018 at 01:35:02PM +0100, Christophe Fergeau wrote: > > > hey, > > > > > > On Mon, Nov 26, 2018 at 03:30:36PM +, Frediano Ziglio wrote: > > > > Definitions were updated by spice-server in patch de66161 included > > > > in 0.12.6 released on 12th June 2015. > > > > > > QEMU's configure only checks for spice-server 0.12.0: > > > > > I don't know how far back QEMU wants to support spice-server. > > > Apart from this, the patch looks good to me. > > > > 0.12.6 is more than three years old, so this or something newer should > > be available in most distros meanwhile. Raising the bar to that version > > looks ok to me (separate patch please, and please also drop #ifdefs we > > don't need any more then). > > See https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg00486.html > "bump spice-server required version to 0.12.6" > Opss.. https://lists.freedesktop.org/archives/spice-devel/2018-November/046279.html slightly different, can you merge them? > > thanks, > > Gerd > > Frediano
[Qemu-devel] [PATCH 2/2] spice: Bump required spice-server version to 0.12.6
Version 0.12.6 was released on 12th June 2015.
Signed-off-by: Frediano Ziglio
---
chardev/spice.c | 12
configure | 4 ++--
hw/display/qxl.c| 27 ---
hw/display/qxl.h| 2 --
include/ui/qemu-spice.h | 12
ui/spice-core.c | 8
ui/spice-display.c | 10 --
7 files changed, 2 insertions(+), 73 deletions(-)
diff --git a/chardev/spice.c b/chardev/spice.c
index e66e3ad568..e0b44474f5 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -77,7 +77,6 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t
*buf, int len)
return bytes;
}
-#if SPICE_SERVER_VERSION >= 0x000c02
static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t event)
{
SpiceChardev *scd = container_of(sin, SpiceChardev, sin);
@@ -95,7 +94,6 @@ static void vmc_event(SpiceCharDeviceInstance *sin, uint8_t
event)
trace_spice_vmc_event(chr_event);
qemu_chr_be_event(chr, chr_event);
}
-#endif
static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
{
@@ -119,12 +117,8 @@ static SpiceCharDeviceInterface vmc_interface = {
.state = vmc_state,
.write = vmc_write,
.read = vmc_read,
-#if SPICE_SERVER_VERSION >= 0x000c02
.event = vmc_event,
-#endif
-#if SPICE_SERVER_VERSION >= 0x000c06
.flags = SPICE_CHAR_DEVICE_NOTIFY_WRITABLE,
-#endif
};
@@ -223,9 +217,7 @@ static void char_spice_finalize(Object *obj)
}
g_free((char *)s->sin.subtype);
-#if SPICE_SERVER_VERSION >= 0x000c02
g_free((char *)s->sin.portname);
-#endif
}
static void spice_vmc_set_fe_open(struct Chardev *chr, int fe_open)
@@ -240,7 +232,6 @@ static void spice_vmc_set_fe_open(struct Chardev *chr, int
fe_open)
static void spice_port_set_fe_open(struct Chardev *chr, int fe_open)
{
-#if SPICE_SERVER_VERSION >= 0x000c02
SpiceChardev *s = SPICE_CHARDEV(chr);
if (fe_open) {
@@ -248,7 +239,6 @@ static void spice_port_set_fe_open(struct Chardev *chr, int
fe_open)
} else {
spice_server_port_event(>sin, SPICE_PORT_EVENT_CLOSED);
}
-#endif
}
static void spice_chr_accept_input(struct Chardev *chr)
@@ -298,7 +288,6 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
chr_open(chr, type);
}
-#if SPICE_SERVER_VERSION >= 0x000c02
static void qemu_chr_open_spice_port(Chardev *chr,
ChardevBackend *backend,
bool *be_opened,
@@ -331,7 +320,6 @@ void qemu_spice_register_ports(void)
vmc_register_interface(s);
}
}
-#endif
static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
diff --git a/configure b/configure
index 0a3c6a72c3..b8f1fd053d 100755
--- a/configure
+++ b/configure
@@ -4563,8 +4563,8 @@ int main(void) { spice_server_new(); return 0; }
EOF
spice_cflags=$($pkg_config --cflags spice-protocol spice-server 2>/dev/null)
spice_libs=$($pkg_config --libs spice-protocol spice-server 2>/dev/null)
- if $pkg_config --atleast-version=0.12.0 spice-server && \
- $pkg_config --atleast-version=0.12.3 spice-protocol && \
+ if $pkg_config --atleast-version=0.12.6 spice-server && \
+ $pkg_config --atleast-version=0.12.10 spice-protocol && \
compile_prog "$spice_cflags" "$spice_libs" ; then
spice="yes"
libs_softmmu="$libs_softmmu $spice_libs"
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 9087db5dee..9387061ecb 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -276,11 +276,9 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
-#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
if (qxl->max_outputs) {
spice_qxl_set_max_monitors(>ssd.qxl, qxl->max_outputs);
}
-#endif
qxl->guest_monitors_config = qxl->ram->monitors_config;
spice_qxl_monitors_config_async(>ssd.qxl,
qxl->ram->monitors_config,
@@ -544,22 +542,6 @@ static void interface_set_compression_level(QXLInstance
*sin, int level)
qxl_rom_set_dirty(qxl);
}
-#if SPICE_NEEDS_SET_MM_TIME
-static void interface_set_mm_time(QXLInstance *sin, uint32_t mm_time)
-{
-PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
-
-if (!qemu_spice_display_is_running(>ssd)) {
-return;
-}
-
-trace_qxl_interface_set_mm_time(qxl->id, mm_time);
-qxl->shadow_rom.mm_clock = cpu_to_le32(mm_time);
-qxl->rom->mm_clock = cpu_to_le32(mm_time);
-qxl_rom_set_dirty(qxl);
-}
-#endif
-
static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
{
[Qemu-devel] [PATCH 1/2] spice: Use new SpiceImageCompression definition
Definitions were updated by spice-server in patch de66161 included
in 0.12.6 released on 12th June 2015.
Signed-off-by: Frediano Ziglio
---
ui/spice-core.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ebaae24643..9d45d895ed 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -331,12 +331,12 @@ static const char *stream_video_names[] = {
stream_video_names, ARRAY_SIZE(stream_video_names))
static const char *compression_names[] = {
-[ SPICE_IMAGE_COMPRESS_OFF ] = "off",
-[ SPICE_IMAGE_COMPRESS_AUTO_GLZ ] = "auto_glz",
-[ SPICE_IMAGE_COMPRESS_AUTO_LZ ] = "auto_lz",
-[ SPICE_IMAGE_COMPRESS_QUIC ] = "quic",
-[ SPICE_IMAGE_COMPRESS_GLZ ] = "glz",
-[ SPICE_IMAGE_COMPRESS_LZ ] = "lz",
+[SPICE_IMAGE_COMPRESSION_OFF] = "off",
+[SPICE_IMAGE_COMPRESSION_AUTO_GLZ] = "auto_glz",
+[SPICE_IMAGE_COMPRESSION_AUTO_LZ] = "auto_lz",
+[SPICE_IMAGE_COMPRESSION_QUIC] = "quic",
+[SPICE_IMAGE_COMPRESSION_GLZ] = "glz",
+[SPICE_IMAGE_COMPRESSION_LZ] = "lz",
};
#define parse_compression(_name)\
parse_name(_name, "image compression", \
@@ -643,7 +643,7 @@ void qemu_spice_init(void)
*x509_cert_file = NULL,
*x509_cacert_file = NULL;
int port, tls_port, addr_flags;
-spice_image_compression_t compression;
+SpiceImageCompression compression;
spice_wan_compression_t wan_compr;
bool seamless_migration;
@@ -754,7 +754,7 @@ void qemu_spice_init(void)
#endif
}
-compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
+compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
str = qemu_opt_get(opts, "image-compression");
if (str) {
compression = parse_compression(str);
--
2.17.2
[Qemu-devel] [PATCH] spice: Use new SpiceImageCompression definition
Definitions were updated by spice-server in patch de66161 included
in 0.12.6 released on 12th June 2015.
Signed-off-by: Frediano Ziglio
---
ui/spice-core.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ebaae24643..2e6a255a35 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -331,12 +331,12 @@ static const char *stream_video_names[] = {
stream_video_names, ARRAY_SIZE(stream_video_names))
static const char *compression_names[] = {
-[ SPICE_IMAGE_COMPRESS_OFF ] = "off",
-[ SPICE_IMAGE_COMPRESS_AUTO_GLZ ] = "auto_glz",
-[ SPICE_IMAGE_COMPRESS_AUTO_LZ ] = "auto_lz",
-[ SPICE_IMAGE_COMPRESS_QUIC ] = "quic",
-[ SPICE_IMAGE_COMPRESS_GLZ ] = "glz",
-[ SPICE_IMAGE_COMPRESS_LZ ] = "lz",
+[ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
+[ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto_glz",
+[ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto_lz",
+[ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
+[ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
+[ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
};
#define parse_compression(_name)\
parse_name(_name, "image compression", \
@@ -643,7 +643,7 @@ void qemu_spice_init(void)
*x509_cert_file = NULL,
*x509_cacert_file = NULL;
int port, tls_port, addr_flags;
-spice_image_compression_t compression;
+SpiceImageCompression compression;
spice_wan_compression_t wan_compr;
bool seamless_migration;
@@ -754,7 +754,7 @@ void qemu_spice_init(void)
#endif
}
-compression = SPICE_IMAGE_COMPRESS_AUTO_GLZ;
+compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
str = qemu_opt_get(opts, "image-compression");
if (str) {
compression = parse_compression(str);
--
2.17.2
Re: [Qemu-devel] [RFC PATCH spice v3 1/3] QXL interface: add a function to identify monitors in the guest
> On Thu, Nov 08, 2018 at 11:05:10AM +0100, Lukáš Hrázký wrote:
> > Hello,
> >
> > On Thu, 2018-11-08 at 07:49 +0100, Gerd Hoffmann wrote:
> > > Hi,
> > >
> > > > + * The device_display_id_{start,count} denotes the sequence of device
> > > > display
> > > > + * IDs that map to the zero-based sequence of monitor IDs provided by
> > > > monitors
> > > > + * config on this interface. For example with device_display_id_start
> > > > = 2 and
> > > > + * device_display_id_count = 3 you get the following mapping:
> > > > + * monitor_id -> device_display_id
> > > > + * 0 -> 2
> > > > + * 1 -> 3
> > > > + * 2 -> 4
> > > > + *
> > > > + * Note this example is unsupported in practice. The only supported
> > > > cases are
> > > > + * either a single device display ID (count = 1) or multiple device
> > > > display IDs
> > > > + * in a sequence starting from 0.
> > >
> > > This is confusing. The usage of this api in the qemu counterpart looks
> > > sane though.
> >
> > Not sure what you find confusing in particular... The example? Using an
> > example and then saying it's not supported?
>
> Yes, that for example. Also wondering why device_display_id doesn't
> start at zero.
>
You introduced this ID so you should remember.
It starts from 0, just this is not the first registration, the other displays
were registered to other QXL instances.
This was introduced for virtio-gpu to be able to register the different
outputs to different QXL instances so to have
first output/display:
spice_qxl_set_device_info(instance1, path1, 0, 1);
second:
spice_qxl_set_device_info(instance2, path1, 1, 1);
third:
spice_qxl_set_device_info(instance3, path1, 2, 1);
So as you can see device_display_id start from 0 but does not mean
is always 0 for every call.
> cheers,
> Gerd
>
>
>
Frediano
Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
>
> On Mon, 2018-11-05 at 14:08 +0100, Gerd Hoffmann wrote:
> > On Mon, Nov 05, 2018 at 01:18:57PM +0100, Lukáš Hrázký wrote:
> > > On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote:
> > > > > 2. Have a single function as follows:
> > > > >
> > > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > > >const char *device_address,
> > > > >uint32_t device_display_id_start,
> > > > >uint32_t device_display_id_count);
> > > >
> > > > How about:
> > > >
> > > > void spice_qxl_set_device_info(QXLInstance *instance,
> > > >const char *device_address,
> > > >uint32_t device_display_id);
> > > >
> > > > I don't think we need start+count:
> > > >
> > > > * For single-head devices device_display_id will be zero.
> > > > * For one-channel-per-head multihead devices (i.e. virtio-gpu)
> > > >device_display_id will enumerate the heads (so everybody can figure
> > > >which channel is which head).
> > > > * For one-channel-per-device multihead devices (i.e. qxl/linux)
> > > >device_display_id will be zero too. Number of heads is set via
> > > >spice_qxl_set_max_monitors().
> > >
> > > That requires nontrivial and unexpected logic for the one-channel-per-
> > > device multihead devices case. The API should be doing what it says and
> > > the dumber the better, this seems too smart to me...
> >
> > Well, the device_display_id_count argument is redundant with
> > spice_qxl_set_max_monitors(). That isn't a great API either.
> >
> > I can see that it simplifies the logic in spice-server if we have a
> > single function call instead of two. So we could deprecate
> > spice_qxl_set_max_monitors() in favour of your
> > spice_qxl_set_device_info() variant.
> >
> > spice_qxl_set_max_monitors() would then basically do this:
> >
> > spice_qxl_set_max_monitors(qxl, max)
> > {
> > spice_qxl_set_device_info(qxl, NULL, 0, max);
> > }
>
> I can't actually do this, it does the wrong thing for the one-channel-
> per-head (virtio-gpu) case. For that case it would send all
> device_display_ids to 0 on all interfaces, but they need to be
> different numbers.
>
This function is only called for QXL cards devices so no virtio-gpu,
no cirrus, no svga or anything else.
> For backwards compatibility when spice_qxl_set_device_info() is not
> called I think we need to fallback to the old behaviour, as we are
> missing the necessary information.
>
> I'll send v3 with a single function using start + count and deprecate
> the set_max_monitors one.
>
> Cheers,
> Lukas
>
> > cheers,
> > Gerd
> >
>
Frediano
Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> On Mon, 2018-11-05 at 07:52 +0100, Gerd Hoffmann wrote: > > > 2. Have a single function as follows: > > > > > > void spice_qxl_set_device_info(QXLInstance *instance, > > >const char *device_address, > > >uint32_t device_display_id_start, > > >uint32_t device_display_id_count); > > > > How about: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id); > > > > I don't think we need start+count: > > > > * For single-head devices device_display_id will be zero. > > * For one-channel-per-head multihead devices (i.e. virtio-gpu) > >device_display_id will enumerate the heads (so everybody can figure > >which channel is which head). > > * For one-channel-per-device multihead devices (i.e. qxl/linux) > >device_display_id will be zero too. Number of heads is set via > >spice_qxl_set_max_monitors(). > > That requires nontrivial and unexpected logic for the one-channel-per- > device multihead devices case. The API should be doing what it says and > the dumber the better, this seems too smart to me... > > That said, I don't find it significantly worse than the other options > (none of which seems great), so I'd just like we reached some consesus > and be done with it... > > Cheers, > Lukas > > > cheers, > > Gerd > > Personally I prefer the single function API, either with or without count. About the not trivial for compatibility you'll have to support Qemu versions that either: - does not call new APIs or spice_qxl_set_max_monitors. A QXL card will have the maximum possible number of monitors in the guest; - does not call new APIs but call spice_qxl_set_max_monitors. A QXL card will have a maximum of monitor specified by spice_qxl_set_max_monitors in the guest; - call new APIs (the choice in this case to call spice_qxl_set_max_monitors or not is yours). Frediano
Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id
> Hi,
>
> > > vga->con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> > > +qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> > > +if (qxl->id != 0) {
> > > +error_setg(errp, "primary qxl-vga device must be console 0 "
> > > + "(first display device on the command line)");
> > > +return;
> > > +}
> >
> > In the comment this seems no more required so why testing it?
>
> I'd expect it'll confuse guests.
>
> If you are sure this is not the case feel free to send a patch dropping
> this check.
>
Had a look more deep into Qemu code. Both qxl_realize_primary and
qxl_realize_secondary calls qxl_realize_common which calls
qemu_spice_add_display_interface to register the interface to spice.
Both qxl_realize_primary and qxl_realize_secondary set qxl->id
(first with 0 second from 1 as code below was doing).
There are 2 fields "id": PCIQXLDevice->id and PCIQXLDevice->ssd.qxl.id,
having PCIQXLDevice->ssd.qxl.id ==
qemu_console_get_index(PCIQXLDevice->vga->con)
== channel_id (PCIQXLDevice->ssd.qxl.id is set in
qemu_spice_add_display_interface).
Looks like PCIQXLDevice->id, beside to distinguish if primary or not is
only used for tracing. Would not be better to remove PCIQXLDevice->id
at all after adding the have_vga flag?
> > > static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> > > {
> > > -static int device_id = 1;
> > > PCIQXLDevice *qxl = PCI_QXL(dev);
> > >
> > > -qxl->id = device_id++;
> > > qxl_init_ramsize(qxl);
> > > memory_region_init_ram(>vga.vram, OBJECT(dev), "qxl.vgavram",
> > > qxl->vga.vram_size, _fatal);
> > > qxl->vga.vram_ptr = memory_region_get_ram_ptr(>vga.vram);
> > > qxl->vga.con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> > > +qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
> > >
> >
> > As these IDs must be contiguous this means that there must be the
> > requirement
> > that if there is a qxl interface only qxl interfaces are used and no other
> > console which seems to me wrong.
>
> Hmm. Didn't know this is a requirement. But as far I know qxl->id is
> also expected to be the channel_id. So with qxl and non-qxl mixed we
> are in trouble no matter what:
>
> * with the patch we break the contiguous id requirement.
PCIQXLDevice->id? No reason actually to be contiguous.
PCIQXLDevice->ssd.qxl.id is already qemu_console_get_index so potentially
not contiguous.
> * without the patch we break the qxl->id == channel_id requirement.
Yes, can be broken although I suspect for other reasons this is true.
>
> So, what now?
>
> cheers,
> Gerd
>
I would personally add the "have_vga" field and use a single id field,
this will make sure to have the same consistent value.
Frediano
Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
> > > 2. Have a single function as follows: > > > > void spice_qxl_set_device_info(QXLInstance *instance, > >const char *device_address, > >uint32_t device_display_id_start, > >uint32_t device_display_id_count); > > How about: > > void spice_qxl_set_device_info(QXLInstance *instance, >const char *device_address, >uint32_t device_display_id); > > I don't think we need start+count: > > * For single-head devices device_display_id will be zero. > * For one-channel-per-head multihead devices (i.e. virtio-gpu) >device_display_id will enumerate the heads (so everybody can figure >which channel is which head). > * For one-channel-per-device multihead devices (i.e. qxl/linux) >device_display_id will be zero too. Number of heads is set via >spice_qxl_set_max_monitors(). > > cheers, > Gerd > What about "Console VNC" case? Passing a dummy (like -1) value for device_display_id ? Kind of "I don't know which output is." Frediano
Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id
>
> Hi,
>
> > > vga->con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> > > +qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> > > +if (qxl->id != 0) {
> > > +error_setg(errp, "primary qxl-vga device must be console 0 "
> > > + "(first display device on the command line)");
> > > +return;
> > > +}
> >
> > In the comment this seems no more required so why testing it?
>
> I'd expect it'll confuse guests.
>
> If you are sure this is not the case feel free to send a patch dropping
> this check.
>
> > > static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> > > {
> > > -static int device_id = 1;
> > > PCIQXLDevice *qxl = PCI_QXL(dev);
> > >
> > > -qxl->id = device_id++;
> > > qxl_init_ramsize(qxl);
> > > memory_region_init_ram(>vga.vram, OBJECT(dev), "qxl.vgavram",
> > > qxl->vga.vram_size, _fatal);
> > > qxl->vga.vram_ptr = memory_region_get_ram_ptr(>vga.vram);
> > > qxl->vga.con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> > > +qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
> > >
> >
> > As these IDs must be contiguous this means that there must be the
> > requirement
> > that if there is a qxl interface only qxl interfaces are used and no other
> > console which seems to me wrong.
>
> Hmm. Didn't know this is a requirement. But as far I know qxl->id is
> also expected to be the channel_id. So with qxl and non-qxl mixed we
> are in trouble no matter what:
>
> * with the patch we break the contiguous id requirement.
yes
> * without the patch we break the qxl->id == channel_id requirement.
no, why? qxl->id is not Qemu index.
>
> So, what now?
>
> cheers,
> Gerd
>
Frediano
Re: [Qemu-devel] [PULL 1/2] qxl: store channel id in qxl->id
>
> See qemu_spice_add_display_interface(), the console index is also used
> as channel id. So put that into the qxl->id field too.
>
> In typical use cases (one primary qxl-vga device, optionally one or more
> secondary qxl devices, no non-qxl display devices) this doesn't change
> anything.
>
> With this in place the qxl->id can not be used any more to figure
> whenever a given device is primary (with vga compat mode) or secondary.
> So add a bool to track this.
>
> Cc: spice-de...@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann
> Message-id: 20181012114540.27829-1-kra...@redhat.com
> ---
> hw/display/qxl.h | 1 +
> hw/display/qxl.c | 19 ---
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index dd9c0522b7..6f9d1f21fa 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -34,6 +34,7 @@ typedef struct PCIQXLDevice {
> PortioList vga_port_list;
> SimpleSpiceDisplay ssd;
> intid;
> +bool have_vga;
> uint32_t debug;
> uint32_t guestdebug;
> uint32_t cmdlog;
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index f608abc769..9087db5dee 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -848,7 +848,7 @@ static int interface_get_cursor_command(QXLInstance *sin,
> struct QXLCommandExt *
> qxl->guest_primary.commands++;
> qxl_track_command(qxl, ext);
> qxl_log_command(qxl, "csr", ext);
> -if (qxl->id == 0) {
> +if (qxl->have_vga) {
> qxl_render_cursor(qxl, ext);
> }
> trace_qxl_ring_cursor_get(qxl->id, qxl_mode_to_string(qxl->mode));
> @@ -1255,7 +1255,7 @@ static void qxl_soft_reset(PCIQXLDevice *d)
> d->current_async = QXL_UNDEFINED_IO;
> qemu_mutex_unlock(>async_lock);
>
> -if (d->id == 0) {
> +if (d->have_vga) {
> qxl_enter_vga_mode(d);
> } else {
> d->mode = QXL_MODE_UNDEFINED;
> @@ -2139,7 +2139,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
> **errp)
>
> memory_region_init_io(>io_bar, OBJECT(qxl), _io_ops, qxl,
>"qxl-ioports", io_size);
> -if (qxl->id == 0) {
> +if (qxl->have_vga) {
> vga_dirty_log_start(>vga);
> }
> memory_region_set_flush_coalesced(>io_bar);
> @@ -2171,7 +2171,7 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
> **errp)
>
> /* print pci bar details */
> dprint(qxl, 1, "ram/%s: %" PRId64 " MB [region 0]\n",
> - qxl->id == 0 ? "pri" : "sec", qxl->vga.vram_size / MiB);
> + qxl->have_vga ? "pri" : "sec", qxl->vga.vram_size / MiB);
> dprint(qxl, 1, "vram/32: %" PRIx64 " MB [region 1]\n",
> qxl->vram32_size / MiB);
> dprint(qxl, 1, "vram/64: %" PRIx64 " MB %s\n",
> @@ -2199,7 +2199,6 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
> VGACommonState *vga = >vga;
> Error *local_err = NULL;
>
> -qxl->id = 0;
> qxl_init_ramsize(qxl);
> vga->vbe_size = qxl->vgamem_size;
> vga->vram_size_mb = qxl->vga.vram_size / MiB;
> @@ -2210,8 +2209,15 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
> vga, "vga");
> portio_list_set_flush_coalesced(>vga_port_list);
> portio_list_add(>vga_port_list, pci_address_space_io(dev), 0x3b0);
> +qxl->have_vga = true;
>
> vga->con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> +qxl->id = qemu_console_get_index(vga->con); /* == channel_id */
> +if (qxl->id != 0) {
> +error_setg(errp, "primary qxl-vga device must be console 0 "
> + "(first display device on the command line)");
> +return;
> +}
In the comment this seems no more required so why testing it?
>
> qxl_realize_common(qxl, _err);
> if (local_err) {
> @@ -2226,15 +2232,14 @@ static void qxl_realize_primary(PCIDevice *dev, Error
> **errp)
>
> static void qxl_realize_secondary(PCIDevice *dev, Error **errp)
> {
> -static int device_id = 1;
> PCIQXLDevice *qxl = PCI_QXL(dev);
>
> -qxl->id = device_id++;
> qxl_init_ramsize(qxl);
> memory_region_init_ram(>vga.vram, OBJECT(dev), "qxl.vgavram",
> qxl->vga.vram_size, _fatal);
> qxl->vga.vram_ptr = memory_region_get_ram_ptr(>vga.vram);
> qxl->vga.con = graphic_console_init(DEVICE(dev), 0, _ops, qxl);
> +qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */
>
As these IDs must be contiguous this means that there must be the requirement
that if there is a qxl interface only qxl interfaces are used and no other
console which seems to me wrong.
> qxl_realize_common(qxl, errp);
> }
Frediano
Re: [Qemu-devel] [RFC PATCH qemu v2 2/2] spice: set device address and device display ID in QXL interface
>
> Calls new SPICE QXL interface functions to set:
>
> * The hardware address of the graphics device represented by the QXL
> interface (e.g. a PCI path):
> spice_qxl_set_device_address(...)
>
> * The device display IDs (the IDs of the device's monitors that belong
> to this QXL interface):
> spice_qxl_monitor_set_device_display_id(...)
>
> Signed-off-by: Lukáš Hrázký
> ---
> hw/display/qxl.c | 8
> ui/spice-core.c| 45 -
> ui/spice-display.c | 5 +
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index f608abc769..63144f42b4 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2184,6 +2184,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl,
> Error **errp)
> SPICE_INTERFACE_QXL_MAJOR, SPICE_INTERFACE_QXL_MINOR);
> return;
> }
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> +// register all possible device display IDs to SPICE server
> +for (int i = 0; i < qxl->max_outputs; ++i) {
> +spice_qxl_monitor_set_device_display_id(>ssd.qxl, i, i);
> +}
> +#endif
> +
I think would be better if this were the default in spice-server.
> qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
>
> qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index a4fbbc3898..2e01beea03 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -35,6 +35,8 @@
> #include "qemu/option.h"
> #include "migration/misc.h"
> #include "hw/hw.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> #include "ui/spice-display.h"
>
> /* core bits */
> @@ -871,6 +873,26 @@ bool qemu_spice_have_display_interface(QemuConsole *con)
> return false;
> }
>
> +/*
> + * Recursively (in reverse order) appends addresses of PCI devices as it
> moves
> + * up in the PCI hierarchy.
> + *
> + * @returns true on success, false when the buffer wasn't large enough
> + */
> +static bool append_pci_address_recursive(char *buf, size_t buf_size, const
> PCIDevice *pci)
I won't use the "_recursive" in the name, looks like more an implementation
detail.
> +{
> +PCIBus *bus = pci_get_bus(pci);
> +if (!pci_bus_is_root(bus)) {
> +append_pci_address_recursive(buf, buf_size, bus->parent_dev);
> +}
> +
> +size_t len = strlen(buf);
> +size_t written = snprintf(buf + len, buf_size - len, "/%02x.%x",
> +PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
> +
> +return written > 0 && written < buf_size - len;
written is always > 0, unless there's a bug in snprintf. Note that Qemu uses
MingW snprintf on Windows so even on Windows this returns always > 0 (note
that snprintf returns an int which is signed while size_t is unsigned and
some implementation could return < 0, but not in Qemu).
> +}
> +
> int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
> {
> if (g_slist_find(spice_consoles, con)) {
> @@ -878,7 +900,28 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin,
> QemuConsole *con)
> }
> qxlin->id = qemu_console_get_index(con);
> spice_consoles = g_slist_append(spice_consoles, con);
> -return qemu_spice_add_interface(>base);
> +int res = qemu_spice_add_interface(>base);
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> +DeviceState *dev = DEVICE(object_property_get_link(OBJECT(con),
> "device", _abort));
> +PCIDevice *pci = (PCIDevice *) object_dynamic_cast(OBJECT(dev),
> TYPE_PCI_DEVICE);
> +
> +if (pci == NULL) {
> +warn_report("Setting PCI path of a display device to SPICE: Not a
> PCI device.");
> +return res;
> +}
> +
> +char device_path[128] = "pci/"; // hardcoded PCI domain
> +if (!append_pci_address_recursive(device_path, sizeof(device_path),
> pci)) {
> +warn_report("Setting PCI path of a display device to SPICE: "
> +"Too many PCI devices in the chain.");
> +return res;
> +}
> +
> +spice_qxl_set_device_address(qxlin, device_path);
> +#endif
> +
> +return res;
> }
>
> static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 2f8adb6b9f..f620750803 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1129,6 +1129,11 @@ static void qemu_spice_display_init_one(QemuConsole
> *con)
>
> ssd->qxl.base.sif = _interface.base;
> qemu_spice_add_display_interface(>qxl, con);
> +
> +#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
Surely OT: won't be better to have something like
#if SPICE_SERVER_VERSION >= SPICE_SERVER_VERSION_NUMBER(0,14,2)
> +spice_qxl_monitor_set_device_display_id(>qxl, 0,
> qemu_console_get_head(con));
> +#endif
> +
> qemu_spice_create_host_memslot(ssd);
>
> register_displaychangelistener(>dcl);
Re: [Qemu-devel] [RFC PATCH spice v2 1/2] QXL interface: add functions to identify monitors in the guest
>
> Adds two functions to let QEMU provide information to identify graphics
> devices and their monitors in the guest:
>
> * device address - The path identifying the device on the system (e.g. PCI
> path):
> spice_qxl_set_device_address(...)
>
> * device display ID - The index of the monitor on the graphics device:
> spice_qxl_monitor_set_device_display_id(...)
This seems to indicate that this device is bound in some way to the previous
information but having 2 APIs make more fragile, potentially one could
call a function and not the other or in different order or mismatch them.
>
> Signed-off-by: Lukáš Hrázký
> ---
> server/red-qxl.c | 89
> server/spice-qxl.h | 5 +++
> server/spice-server.syms | 6 +++
> 3 files changed, 100 insertions(+)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0b2043e1 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
> #include "red-qxl.h"
>
>
> +#define MAX_PATH_LEN 256
Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
confusing, maybe MAX_DEVICE_PATH_LEN would be better.
> +#define MAX_MONITORS_COUNT 16
> +
> struct QXLState {
> QXLWorker qxl_worker;
> QXLInstance *qxl;
> @@ -53,6 +56,9 @@ struct QXLState {
> unsigned int max_monitors;
> RedsState *reds;
> RedWorker *worker;
> +char device_address[MAX_PATH_LEN];
> +uint32_t device_display_ids[MAX_MONITORS_COUNT];
> +size_t monitors_count; // length of ^^^
size_t look a bit too much for a number of item in a small array.
>
> pthread_mutex_t scanout_mutex;
> SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
> red_qxl_async_complete(qxl, cookie);
> }
>
> +/**
> + * spice_qxl_set_device_address:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs to
> + *
> + * Sets the hardware address (e.g. PCI) of the graphics device represented
> by
> + * this QXL interface instance.
> + *
> + * The supported format is:
> + * "pci//./.../."
> + *
> + * The "pci" identifies the rest of the string as a PCI adress. It is the
> only
typo: adress
> + * supported address at the moment, other identifiers can be introduced
> later.
> + * is the PCI domain, followed by . of any PCI
> bridges
> + * in the chain leading to the device. The last . is the
> + * graphics device.
Maybe better to specify also the encoding, like decimal/hexadecimal and number
of digits.
> + */
I would prefer documentation in the header so people don't have to open
the source to get it, also considering the the header is public.
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_address(QXLInstance *instance, const char
> *device_address)
> +{
> +g_return_if_fail(device_address != NULL);
> +
> +size_t dp_len = strnlen(device_address, MAX_PATH_LEN);
> +if (dp_len >= MAX_PATH_LEN) {
> +spice_error("PCI device path too long: %lu > %u", dp_len,
> MAX_PATH_LEN);
> +return;
> +}
> +
> +strncpy(instance->st->device_address, device_address, MAX_PATH_LEN);
> +
> +g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> device_address);
> +}
> +
> +/**
> + * spice_qxl_monitor_set_device_display_id:
> + * @instance the QXL instance to set the device display ID to
> + * @monitor_id the SPICE monitor ID to set the device display ID to
> + * @device_display_id the actual ID of the display (output) on the graphics
> device
> + *
> + * Sets the device display ID for a given monitor ID in a QXL instance. The
> + * monitor IDs are expected and required to be a consecutive sequence
> starting
> + * at 0. The function requires the calls to be made in the sequence to
> prevent
> + * holes.
> + *
> + * The requirement for the monitor ID to be a sequence starting from 0 comes
> + * from the mechanism of generating a single display_id from channel_id and
> + * monitor_id on the client:
> + *
> + * display_id = channel_id + monitor_id
> + *
No, the monitor_id sequence is defined as a sequence from 0, has nothing
to do with display_id. Also this comment seems to indicate that this new
interface is bad designed having the same limit, which is not.
> + * This is unambiguous either if there is only a single channel with
> multiple
> + * monitors ("legacy" QXL on linux case) or multiple channels with only a
> + * single monitor. Also both channel_id and monitor_id need to be a sequence
> + * starting from 0, otherwise there is still a possibility of collisions.
They are both defined (channel_id and monitor_id) as sequences starting from 0.
I don't see why need to be specified here.
> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> + uint32_t monitor_id,
> +
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > Hi, > > > > When using qemu_console_get_head() it doesn't work correctly, it would > > > use the qxl card's data. It would work if spice-server would filter the > > > list to only include the entries for the given display channel before > > > calling the ->client_monitors_config() callback. But it doesn't, we get > > > the complete set. > > > > That's why I said is a bug, IMHO it should. > > Hmm, this should be easily detectable on qemu side I think. If > num_of_monitors (for a non-qxl card) is > 1, then we have an old, > non-filtering spice-server. If num_of_monitors == 1, then it is a new, > filtering spice-server. Or a single-head channel configuration, in > which case it doesn't matter whenever it filters or not. > > So this should be fixable without causing too much compatibility pain. > > cheers, > Gerd > Sorry, why fixing in Qemu is the bug is in spice-server? I really don't follow your reasoning. For me Qemu should not change at all and spice-server should do the filtering. Is this not fixing everything? Frediano
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote: > > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote: > > > > > Ok. We probably should fix interface_client_monitors_config() to use > > > > > the channel_id instead of qemu_console_get_head() then. > > > > > > > > It's not that simple. This would break the QXL with multiple monitors > > > > per channel case. > > > > > > It is that simple. > > > > > > qxl doesn't use that code path and has its own version of the callback > > > (in qxl.c). Fixing it there is a bit more tricky. > > > > Ok.. and what's actually the problem using qemu_console_get_head()? It > > just doesn't feel right to me using channel_id as an index into this > > array. It is not the right index strictly speaking. > > Assume you have one qxl and one virtio-gpu device (one head each), for > example: > >00:02.0 qxl channel 0 >00:03.0 virtio-gpu channel 1 > > So the client will assign display 0 to qxl and display 1 to virtio-gpu. > In interface_client_monitors_config() we have to pick the correct array > entry. > > When using the channel_id it works correctly. > > When using qemu_console_get_head() it doesn't work correctly, it would > use the qxl card's data. It would work if spice-server would filter the > list to only include the entries for the given display channel before > calling the ->client_monitors_config() callback. But it doesn't, we get > the complete set. > That's why I said is a bug, IMHO it should. > > > > I think we should fix spice server to actually do the filtering and > > > > only send monitors_config that belongs to the device to the QXL > > > > interface. As Frediano mentioned, it looks more like a bug. > > > > > > Only problem is changing the callback semantics changes the library api. > > > We could add a second version of the callback which gets called with a > > > filtered list (if present). Not sure this is worth the effort though. > > > > That's right. But if we don't actually break any currently supported > > use cases, it might be fine? The only thing we would be breaking is > > the virtio-gpu, I think? Is that already something we don't want to > > break? > > It would break any multihead configuration which uses multiple display > channels. Yes, virtio-gpu is one case. Breaking that would not be very > nice, but maybe acceptable. The other case is multiple qxl devices > (i.e. windows guest style multihead). Breaking that is out of > question. > Windows is not a problem as it ignores the data passed to client_monitors_config. > cheers, > Gerd > Frediano
Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest
> > > > So, if I remember correctly, Gerd recommended returning this value from > > > the function. But I think it needs more explanation here. What exactly > > > is a "monitor_id" supposed to represent? It is not used in your follow- > > > up qemu patch so it's hard to tell. > > > > It's supposed to be the actual monitor_id that we use in SPICE to > > identify the monitors. I've just spent quite some time looking up where > > the monitor_id actually comes from, and it's actually set all the way > > down in the driver (either xf86-video-qxl or the KMS driver in the > > kernel) and passed through the monitors_config functions to SPICE > > server. > > How does all this monitors_config work currently in case multiple > display channels are present? > > There is the QXLInterface->client_monitors_config() callback. > > There is the spice_qxl_monitors_config_async() function. > > Both are linked to a display channel. The server/client messages go > through the main channel though ... > > So what happens if a message from the client arrives? Is that > broadcasted as-is to all display channels? The current qemu code > (interface_client_monitors_config in spice-display.c, which is used with > virtio-gpu) simply uses the head as index into the array, so it looks > like spice-server doesn't do any filtering here (like only filling in > the monitors which belong to the display channel). > Yes, it is a broadcast but is a bug that is working only because the message is handled only by Linux driver and in Linux configuration is expected that there is only a QXL card supporting multiple monitor (in this case the broadcast is equivalent to send only the monitor for the given QXL card). > spice_qxl_monitors_config_async() is never called with virtio-gpu, > except when opengl is enabled. In that case qemu simply sets > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0] > (see qemu_spice_gl_monitor_config in spice-display.c). Which would be > ok in case spice-server merges the configs from all channels before > sending it off to the client. Not sure this actually happens ... > Yes, server send all the monitors together (or at least it should). So client sends and receives all the monitor sizes at once. > > Interstingly enough, that seems to be the ID we want to have in the > > device_display_id attribute. I expect (still need to look that up, I'm > > out of time right now) that for virtio-gpu the ID is a bit different, > > Keep in mind that multiple display devices don't really work right now, > and possibly we need to fix not only spice but qemu too. > What does not work at the moment? On Windows is normal to have multiple QXL cards and is working for instance. We use QXL card and vGPU and is not working that bad (beside the problem we are fixing here). > > And yeah, I didn't use the id in the QEMU patches, as I didn't know > > how, I expect Gerd to have some grand plans for it :) > > IIRC the latest plan was to just keep things as is, plan with one > channel per monitor for all future things, and just not support > one-qxl-device-multihead in combination with multiple display channels. > > Is that correct? > Well, I think depends form the definition of "things". As we are changing the code we are not "just keep things as is". But yes, for the future we plan to support only one monitor per channel so monitor_id == 0 (and so will be display_id == channel_id). I suppose by "things" in "just keep things as is" you mean the SPICE (client <-> server) protocol. > I don't think we need the monitors_id in qemu then, qemu can simply use > the channel_id (except for the legacy qxl case). > Yes, and the channel_id is provided by Qemu to spice-server (as id of the QXL interface) so there's no reason to ask to spice-server to provide back channel_id. > cheers, > Gerd > Frediano
Re: [Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field
Thanks. How is your hand? Frediano - Original Message - > > On Sun, Dec 31, 2017 at 05:17:43AM -0500, Frediano Ziglio wrote: > > ping > > > > > > > > ping > > > > > > > > > > > ping the series > > > > > > Was on sick leave for a few weeks, back now, will process asap but will > probably take a while nevertheless due to the big backlog I have now. > > cheers, > Gerd > > >
Re: [Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field
ping
>
> ping
>
> >
> > ping the series
> >
> > >
> > > This fields points to an old interface that is no more
> > > used in the current code.
> > >
> > > Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> > > ---
> > > hw/display/qxl.c | 1 -
> > > include/ui/spice-display.h | 1 -
> > > ui/spice-display.c | 2 --
> > > 3 files changed, 4 deletions(-)
> > >
> > > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > > index 99365c3e8f..b9fa067f6e 100644
> > > --- a/hw/display/qxl.c
> > > +++ b/hw/display/qxl.c
> > > @@ -518,7 +518,6 @@ static void interface_attach_worker(QXLInstance *sin,
> > > QXLWorker *qxl_worker)
> > > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> > >
> > > trace_qxl_interface_attach_worker(qxl->id);
> > > -qxl->ssd.worker = qxl_worker;
> > > }
> > >
> > > static void interface_set_compression_level(QXLInstance *sin, int level)
> > > diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> > > index aaf2019889..6b5c73b21c 100644
> > > --- a/include/ui/spice-display.h
> > > +++ b/include/ui/spice-display.h
> > > @@ -86,7 +86,6 @@ struct SimpleSpiceDisplay {
> > > DisplayChangeListener dcl;
> > > void *buf;
> > > int bufsize;
> > > -QXLWorker *worker;
> > > QXLInstance qxl;
> > > uint32_t unique;
> > > pixman_image_t *surface;
> > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > index ad1ceafb3f..85a72fe76a 100644
> > > --- a/ui/spice-display.c
> > > +++ b/ui/spice-display.c
> > > @@ -519,7 +519,6 @@ static void interface_attach_worker(QXLInstance *sin,
> > > QXLWorker *qxl_worker)
> > > SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay,
> > > qxl);
> > >
> > > dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
> > > -ssd->worker = qxl_worker;
> > > }
> > >
> > > static void interface_set_compression_level(QXLInstance *sin, int level)
> > > @@ -1028,7 +1027,6 @@ static void qemu_spice_display_init_one(QemuConsole
> > > *con)
> > >
> > > ssd->qxl.base.sif = _interface.base;
> > > qemu_spice_add_display_interface(>qxl, con);
> > > -assert(ssd->worker);
> > > qemu_spice_create_host_memslot(ssd);
> > >
> > > register_displaychangelistener(>dcl);
> >
> >
>
>
Re: [Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field
ping
>
> ping the series
>
> >
> > This fields points to an old interface that is no more
> > used in the current code.
> >
> > Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> > ---
> > hw/display/qxl.c | 1 -
> > include/ui/spice-display.h | 1 -
> > ui/spice-display.c | 2 --
> > 3 files changed, 4 deletions(-)
> >
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index 99365c3e8f..b9fa067f6e 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -518,7 +518,6 @@ static void interface_attach_worker(QXLInstance *sin,
> > QXLWorker *qxl_worker)
> > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> >
> > trace_qxl_interface_attach_worker(qxl->id);
> > -qxl->ssd.worker = qxl_worker;
> > }
> >
> > static void interface_set_compression_level(QXLInstance *sin, int level)
> > diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> > index aaf2019889..6b5c73b21c 100644
> > --- a/include/ui/spice-display.h
> > +++ b/include/ui/spice-display.h
> > @@ -86,7 +86,6 @@ struct SimpleSpiceDisplay {
> > DisplayChangeListener dcl;
> > void *buf;
> > int bufsize;
> > -QXLWorker *worker;
> > QXLInstance qxl;
> > uint32_t unique;
> > pixman_image_t *surface;
> > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > index ad1ceafb3f..85a72fe76a 100644
> > --- a/ui/spice-display.c
> > +++ b/ui/spice-display.c
> > @@ -519,7 +519,6 @@ static void interface_attach_worker(QXLInstance *sin,
> > QXLWorker *qxl_worker)
> > SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> >
> > dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
> > -ssd->worker = qxl_worker;
> > }
> >
> > static void interface_set_compression_level(QXLInstance *sin, int level)
> > @@ -1028,7 +1027,6 @@ static void qemu_spice_display_init_one(QemuConsole
> > *con)
> >
> > ssd->qxl.base.sif = _interface.base;
> > qemu_spice_add_display_interface(>qxl, con);
> > -assert(ssd->worker);
> > qemu_spice_create_host_memslot(ssd);
> >
> > register_displaychangelistener(>dcl);
>
>
Re: [Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field
ping the series
>
> This fields points to an old interface that is no more
> used in the current code.
>
> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> ---
> hw/display/qxl.c | 1 -
> include/ui/spice-display.h | 1 -
> ui/spice-display.c | 2 --
> 3 files changed, 4 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 99365c3e8f..b9fa067f6e 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -518,7 +518,6 @@ static void interface_attach_worker(QXLInstance *sin,
> QXLWorker *qxl_worker)
> PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>
> trace_qxl_interface_attach_worker(qxl->id);
> -qxl->ssd.worker = qxl_worker;
> }
>
> static void interface_set_compression_level(QXLInstance *sin, int level)
> diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
> index aaf2019889..6b5c73b21c 100644
> --- a/include/ui/spice-display.h
> +++ b/include/ui/spice-display.h
> @@ -86,7 +86,6 @@ struct SimpleSpiceDisplay {
> DisplayChangeListener dcl;
> void *buf;
> int bufsize;
> -QXLWorker *worker;
> QXLInstance qxl;
> uint32_t unique;
> pixman_image_t *surface;
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index ad1ceafb3f..85a72fe76a 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -519,7 +519,6 @@ static void interface_attach_worker(QXLInstance *sin,
> QXLWorker *qxl_worker)
> SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
>
> dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
> -ssd->worker = qxl_worker;
> }
>
> static void interface_set_compression_level(QXLInstance *sin, int level)
> @@ -1028,7 +1027,6 @@ static void qemu_spice_display_init_one(QemuConsole
> *con)
>
> ssd->qxl.base.sif = _interface.base;
> qemu_spice_add_display_interface(>qxl, con);
> -assert(ssd->worker);
> qemu_spice_create_host_memslot(ssd);
>
> register_displaychangelistener(>dcl);
[Qemu-devel] [PATCH 3/4] spice: remove only written event_mask field
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
ui/spice-core.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 85b9ea2127..6d579faaae 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -89,7 +89,6 @@ static void timer_remove(SpiceTimer *timer)
struct SpiceWatch {
int fd;
-int event_mask;
SpiceWatchFunc func;
void *opaque;
};
@@ -111,11 +110,10 @@ static void watch_update_mask(SpiceWatch *watch, int
event_mask)
IOHandler *on_read = NULL;
IOHandler *on_write = NULL;
-watch->event_mask = event_mask;
-if (watch->event_mask & SPICE_WATCH_EVENT_READ) {
+if (event_mask & SPICE_WATCH_EVENT_READ) {
on_read = watch_read;
}
-if (watch->event_mask & SPICE_WATCH_EVENT_WRITE) {
+if (event_mask & SPICE_WATCH_EVENT_WRITE) {
on_write = watch_write;
}
qemu_set_fd_handler(watch->fd, on_read, on_write, watch);
--
2.14.3
[Qemu-devel] [PATCH 4/4] spice: remove unused timer list
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
ui/spice-core.c | 4
1 file changed, 4 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6d579faaae..2baf0c7120 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -55,9 +55,7 @@ static QemuThread me;
struct SpiceTimer {
QEMUTimer *timer;
-QTAILQ_ENTRY(SpiceTimer) next;
};
-static QTAILQ_HEAD(, SpiceTimer) timers = QTAILQ_HEAD_INITIALIZER(timers);
static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
{
@@ -65,7 +63,6 @@ static SpiceTimer *timer_add(SpiceTimerFunc func, void
*opaque)
timer = g_malloc0(sizeof(*timer));
timer->timer = timer_new_ms(QEMU_CLOCK_REALTIME, func, opaque);
-QTAILQ_INSERT_TAIL(, timer, next);
return timer;
}
@@ -83,7 +80,6 @@ static void timer_remove(SpiceTimer *timer)
{
timer_del(timer->timer);
timer_free(timer->timer);
-QTAILQ_REMOVE(, timer, next);
g_free(timer);
}
--
2.14.3
[Qemu-devel] [PATCH 2/4] spice: remove unused watch list
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
ui/spice-core.c | 4
1 file changed, 4 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ea04dc69b5..85b9ea2127 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -92,9 +92,7 @@ struct SpiceWatch {
int event_mask;
SpiceWatchFunc func;
void *opaque;
-QTAILQ_ENTRY(SpiceWatch) next;
};
-static QTAILQ_HEAD(, SpiceWatch) watches = QTAILQ_HEAD_INITIALIZER(watches);
static void watch_read(void *opaque)
{
@@ -131,7 +129,6 @@ static SpiceWatch *watch_add(int fd, int event_mask,
SpiceWatchFunc func, void *
watch->fd = fd;
watch->func = func;
watch->opaque = opaque;
-QTAILQ_INSERT_TAIL(, watch, next);
watch_update_mask(watch, event_mask);
return watch;
@@ -140,7 +137,6 @@ static SpiceWatch *watch_add(int fd, int event_mask,
SpiceWatchFunc func, void *
static void watch_remove(SpiceWatch *watch)
{
qemu_set_fd_handler(watch->fd, NULL, NULL, NULL);
-QTAILQ_REMOVE(, watch, next);
g_free(watch);
}
--
2.14.3
[Qemu-devel] [PATCH 1/4] spice: remove QXLWorker interface field
This fields points to an old interface that is no more
used in the current code.
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
hw/display/qxl.c | 1 -
include/ui/spice-display.h | 1 -
ui/spice-display.c | 2 --
3 files changed, 4 deletions(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 99365c3e8f..b9fa067f6e 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -518,7 +518,6 @@ static void interface_attach_worker(QXLInstance *sin,
QXLWorker *qxl_worker)
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
trace_qxl_interface_attach_worker(qxl->id);
-qxl->ssd.worker = qxl_worker;
}
static void interface_set_compression_level(QXLInstance *sin, int level)
diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index aaf2019889..6b5c73b21c 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -86,7 +86,6 @@ struct SimpleSpiceDisplay {
DisplayChangeListener dcl;
void *buf;
int bufsize;
-QXLWorker *worker;
QXLInstance qxl;
uint32_t unique;
pixman_image_t *surface;
diff --git a/ui/spice-display.c b/ui/spice-display.c
index ad1ceafb3f..85a72fe76a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -519,7 +519,6 @@ static void interface_attach_worker(QXLInstance *sin,
QXLWorker *qxl_worker)
SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
-ssd->worker = qxl_worker;
}
static void interface_set_compression_level(QXLInstance *sin, int level)
@@ -1028,7 +1027,6 @@ static void qemu_spice_display_init_one(QemuConsole *con)
ssd->qxl.base.sif = _interface.base;
qemu_spice_add_display_interface(>qxl, con);
-assert(ssd->worker);
qemu_spice_create_host_memslot(ssd);
register_displaychangelistener(>dcl);
--
2.14.3
[Qemu-devel] [Bug 1635339] Re: qxl_pre_save assertion failure on vm "save"
wddm dod 0.17 version released which fixes the issue guest side. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1635339 Title: qxl_pre_save assertion failure on vm "save" Status in QEMU: Confirmed Bug description: When I try and save my Windows 10 VM, I see an assertion failure, and the machine is shut down. I see the following in the log: main_channel_handle_parsed: agent start qemu-system-x86_64: /build/qemu-Zwynhi/qemu-2.5+dfsg/hw/display/qxl.c:2101: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed. 2016-10-20 11:52:42.713+: shutting down Please let me know what other information would be relevant! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1635339/+subscriptions
[Qemu-devel] [Bug 1635339] Re: qxl_pre_save assertion failure on vm "save"
Similar issue, seems not caused by save/restore/migration but is still detecting offset problems with resource deallocation. See https://lists.freedesktop.org/archives/spice-devel/2017-April/037248.html. Still working on some updates for the driver. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1635339 Title: qxl_pre_save assertion failure on vm "save" Status in QEMU: Confirmed Bug description: When I try and save my Windows 10 VM, I see an assertion failure, and the machine is shut down. I see the following in the log: main_channel_handle_parsed: agent start qemu-system-x86_64: /build/qemu-Zwynhi/qemu-2.5+dfsg/hw/display/qxl.c:2101: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed. 2016-10-20 11:52:42.713+: shutting down Please let me know what other information would be relevant! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1635339/+subscriptions
[Qemu-devel] [Bug 1635339] Re: qxl_pre_save assertion failure on vm "save"
Next version of the driver will solve the problem (already fixed in master). -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1635339 Title: qxl_pre_save assertion failure on vm "save" Status in QEMU: Confirmed Bug description: When I try and save my Windows 10 VM, I see an assertion failure, and the machine is shut down. I see the following in the log: main_channel_handle_parsed: agent start qemu-system-x86_64: /build/qemu-Zwynhi/qemu-2.5+dfsg/hw/display/qxl.c:2101: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed. 2016-10-20 11:52:42.713+: shutting down Please let me know what other information would be relevant! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1635339/+subscriptions
[Qemu-devel] [Bug 1635339] Re: qxl_pre_save assertion failure on vm "save"
Is this problem limited to commands or also to data attached to the commands? To me looks like a limitation Qemu should remove on the long run. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1635339 Title: qxl_pre_save assertion failure on vm "save" Status in QEMU: Confirmed Bug description: When I try and save my Windows 10 VM, I see an assertion failure, and the machine is shut down. I see the following in the log: main_channel_handle_parsed: agent start qemu-system-x86_64: /build/qemu-Zwynhi/qemu-2.5+dfsg/hw/display/qxl.c:2101: qxl_pre_save: Assertion `d->last_release_offset < d->vga.vram_size' failed. 2016-10-20 11:52:42.713+: shutting down Please let me know what other information would be relevant! To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1635339/+subscriptions
Re: [Qemu-devel] [PATCH] ui/egl-helpers: fix egl 1.5 display init
>
> On 03/17/2017 05:53 AM, Marc-André Lureau wrote:
> > On Fri, Mar 17, 2017 at 12:12 PM Gerd Hoffmann <kra...@redhat.com> wrote:
> >
> >> Unfortunaly switching to getPlatformDisplayEXT isn't as easy as
> >> implemented by 0ea1523fb6703aa0dcd65e66b59e96fec028e60a. See the
> >> longish comment for the complete story.
> >>
> >> Cc: Frediano Ziglio <fzig...@redhat.com>
> >> Suggested-by: Hans de Goede <hdego...@redhat.com>
> >> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> >> ---
> >
> >
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
>
> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>
> >> ui/egl-helpers.c | 53
> >> +++--
> >> 1 file changed, 47 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> >> index 584dd1b..48686cf 100644
> >> --- a/ui/egl-helpers.c
> >> +++ b/ui/egl-helpers.c
> >> @@ -192,6 +192,51 @@ EGLSurface qemu_egl_init_surface_x11(EGLContext ectx,
> >> Window win)
> >>
> >> /* --
> >> */
> >>
> >> +/*
> >> + * Taken from glamor_egl.h from the Xorg xserver, which is MIT licensed
> >> + *
> >> + * Create an EGLDisplay from a native display type. This is a little
> >> quirky
> >> + * for a few reasons.
> >> + *
> >> + * 1: GetPlatformDisplayEXT and GetPlatformDisplay are the API you want
> >> to
> >> + * use, but have different function signatures in the third argument;
> >> this
> >> + * happens not to matter for us, at the moment, but it means epoxy won't
> >> alias
> >> + * them together.
> >> + *
> >> + * 2: epoxy 1.3 and earlier don't understand EGL client extensions, which
> >> + * means you can't call "eglGetPlatformDisplayEXT" directly, as the
> >> resolver
> >> + * will crash.
> >> + *
> >> + * 3: You can't tell whether you have EGL 1.5 at this point, because
> >> + * eglQueryString(EGL_VERSION) is a property of the display, which we
> >> don't
> >> + * have yet. So you have to query for extensions no matter what.
> >> Fortunately
> >> + * epoxy_has_egl_extension _does_ let you query for client extensions, so
> >> + * we don't have to write our own extension string parsing.
> >> + *
> >> + * 4. There is no EGL_KHR_platform_base to complement the EXT one, thus
> >> one
> >> + * needs to know EGL 1.5 is supported in order to use the
> >> eglGetPlatformDisplay
> >> + * function pointer.
> >> + * We can workaround this (circular dependency) by probing for the EGL
> >> 1.5
> >> + * platform extensions (EGL_KHR_platform_gbm and friends) yet it doesn't
> >> seem
> >> + * like mesa will be able to adverise these (even though it can do EGL
> >> 1.5).
> >> + */
> >> +static EGLDisplay qemu_egl_get_display(void *native)
> >> +{
> >> +#ifdef EGL_PLATFORM_GBM_MESA
> >> +/* In practise any EGL 1.5 implementation would support the EXT
> >> extension */
> >> +if (epoxy_has_egl_extension(NULL, "EGL_EXT_platform_base")) {
Should not check EGL_MESA_platform_gbm instead?
What happens if package is compiled against Mesa but in the system Nvidia
OpenGL is installed? In this case EGL_PLATFORM_GBM_MESA is defines,
possibly EGL_EXT_platform_base is supported but EGL_PLATFORM_GBM_MESA
is not as Nvidia does not support it (so EGL_PLATFORM_GBM_MESA won't work).
Maybe Xorg code support also Nvidia case (that does not have gbm) ?
> >> +PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplayEXT =
> >> +(void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
> >> +if (getPlatformDisplayEXT) {
> >> +return getPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA, native,
> >> NULL);
> >> +}
> >> +}
> >> +#endif
> >> +
> >> +/* fallback */
> >> +return eglGetDisplay(native);
> >> +}
> >> +
> >> int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
> >> {
> >> static const EGLint conf_att_gl[] = {
> >> @@ -222,12 +267,8 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool
> >> gles, bool debug)
> >> setenv("LIBGL_DEBUG", "verbose", true);
> >> }
> >>
> >> -egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
> >> -#ifdef EGL_MESA_platform_gbm
> >> -qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA,
> >> dpy, NULL);
> >> -#else
> >> -qemu_egl_display = eglGetDisplay(dpy);
> >> -#endif
> >> +egl_dbg("qemu_egl_get_display (dpy %p) ...\n", dpy);
> >> +qemu_egl_display = qemu_egl_get_display(dpy);
> >> if (qemu_egl_display == EGL_NO_DISPLAY) {
> >> error_report("egl: eglGetDisplay failed");
> >> return -1;
Re: [Qemu-devel] [PATCH] egl-helpers: Support newer MESA versions
>
>
>
> Hi,
>
> On 20-02-17 10:50, Frediano Ziglio wrote:
> > According to
> > https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_platform_gbm.txt
> > if MESA_platform_gbm is supported display should be initialized
> > from a GBM handle using eglGetPlatformDisplayEXT.
> >
> > Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
> > ---
> > This should fix
> > http://www.spinics.net/linux/fedora/libvir/msg142837.html
> >
> > Tested on Fedora rawhide.
> > ---
> > ui/egl-helpers.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > index cd24568..964c5a5 100644
> > --- a/ui/egl-helpers.c
> > +++ b/ui/egl-helpers.c
> > @@ -219,7 +219,11 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool
> > gles, bool debug)
> > }
> >
> > egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
> > +#ifdef EGL_MESA_platform_gbm
> > +qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA,
> > dpy, NULL);
> > +#else
> > qemu_egl_display = eglGetDisplay(dpy);
> > +#endif
> > if (qemu_egl_display == EGL_NO_DISPLAY) {
> > error_report("egl: eglGetDisplay failed");
> > return -1;
> >
>
> That fix is incomplete, you need some magic to work properly on older libGL
> versions.
>
> Attached is a (compile tested only) proper patch. I do not have a qemu git
> clone
> handy atm, so this is not a git format-patch patch, it is against the Fedora
> Rawhide
> srpm. I've a test build with these patches here:
>
> https://fedorapeople.org/~jwrdegoede/qemu-glvnd/
>
> I was planning on doing a git clone qemu and send a proper patch after I got
> some
> testing feedback. Feel free to use this as a base for a v2 of your patch.
>
> Regards,
>
> Hans
>
Wouldn't be easier to call the "old" eglGetDisplay if eglGetPlatformDisplayEXT
returns EGL_NO_DISPLAY ? Kind of
egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
-qemu_egl_display = eglGetDisplay(dpy);
+#ifdef EGL_MESA_platform_gbm
+qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA, dpy,
NULL);
+#else
+qemu_egl_display = EGL_NO_DISPLAY;
+#endif
+if (qemu_egl_display == EGL_NO_DISPLAY)
+qemu_egl_display = eglGetDisplay(dpy);
if (qemu_egl_display == EGL_NO_DISPLAY) {
error_report("egl: eglGetDisplay failed");
return -1;
Your patch should not even compile on older system which does not define
EGL_PLATFORM_GBM_MESA.
I tested my patch and works on both rawhide and FC25 (with very recent Mesa
version)
Frediano
[Qemu-devel] [PATCH] egl-helpers: Support newer MESA versions
According to
https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_platform_gbm.txt
if MESA_platform_gbm is supported display should be initialized
from a GBM handle using eglGetPlatformDisplayEXT.
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
This should fix
http://www.spinics.net/linux/fedora/libvir/msg142837.html
Tested on Fedora rawhide.
---
ui/egl-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index cd24568..964c5a5 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -219,7 +219,11 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles,
bool debug)
}
egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
+#ifdef EGL_MESA_platform_gbm
+qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA, dpy,
NULL);
+#else
qemu_egl_display = eglGetDisplay(dpy);
+#endif
if (qemu_egl_display == EGL_NO_DISPLAY) {
error_report("egl: eglGetDisplay failed");
return -1;
--
2.9.3
[Qemu-devel] [PATCH v2] egl-helpers: Change file licensing to LGPLv2
The relicense permits sharing the code with Spice which is LGPL. All people listed below have agreed to the relicense: - Arei Gonglei; - Cole Robinson; - Gerd Hoffmann; - Peter Maydell. Signed-off-by: Frediano Ziglio <fzig...@redhat.com> Acked-by: Cole Robinson <crobi...@redhat.com> --- ui/egl-helpers.c | 16 1 file changed, 16 insertions(+) Changes since v1: - extended commit message. diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index 79cee05..cd24568 100644 --- a/ui/egl-helpers.c +++ b/ui/egl-helpers.c @@ -1,3 +1,19 @@ +/* + * Copyright (C) 2015-2016 Gerd Hoffmann <kra...@redhat.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + */ #include "qemu/osdep.h" #include #include -- 2.9.3
[Qemu-devel] [PATCH v2] usb: Fix typo in documentation
simliar -> similar Signed-off-by: Frediano Ziglio <fzig...@redhat.com> --- docs/usb-storage.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Changes since v1: - extend commit message diff --git a/docs/usb-storage.txt b/docs/usb-storage.txt index fbc1f2e..551af6f 100644 --- a/docs/usb-storage.txt +++ b/docs/usb-storage.txt @@ -34,7 +34,7 @@ with tree logical units: Number three emulates the classic bulk-only transport protocol too. It's called "usb-bot". It shares most code with "usb-storage", and the guest will not be able to see the difference. The qemu command -line interface is simliar to usb-uas though, i.e. no automatic scsi +line interface is similar to usb-uas though, i.e. no automatic scsi disk creation. It also features support for up to 16 LUNs. The LUN numbers must be continuous, i.e. for three devices you must use 0+1+2. The 0+1+5 numbering from the "usb-uas" example isn't going to work -- 2.9.3
[Qemu-devel] [PATCH] egl-helpers: Change file licensing to LGPL2
Signed-off-by: Frediano Ziglio <fzig...@redhat.com> --- ui/egl-helpers.c | 16 1 file changed, 16 insertions(+) diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index 79cee05..cd24568 100644 --- a/ui/egl-helpers.c +++ b/ui/egl-helpers.c @@ -1,3 +1,19 @@ +/* + * Copyright (C) 2015-2016 Gerd Hoffmann <kra...@redhat.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + */ #include "qemu/osdep.h" #include #include -- 2.9.3
[Qemu-devel] [PATCH] usb: Fix typo in documentation
Signed-off-by: Frediano Ziglio <fzig...@redhat.com> --- docs/usb-storage.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usb-storage.txt b/docs/usb-storage.txt index fbc1f2e..551af6f 100644 --- a/docs/usb-storage.txt +++ b/docs/usb-storage.txt @@ -34,7 +34,7 @@ with tree logical units: Number three emulates the classic bulk-only transport protocol too. It's called "usb-bot". It shares most code with "usb-storage", and the guest will not be able to see the difference. The qemu command -line interface is simliar to usb-uas though, i.e. no automatic scsi +line interface is similar to usb-uas though, i.e. no automatic scsi disk creation. It also features support for up to 16 LUNs. The LUN numbers must be continuous, i.e. for three devices you must use 0+1+2. The 0+1+5 numbering from the "usb-uas" example isn't going to work -- 2.9.3
[Qemu-devel] ui/egl-helpers.c license and reuse
Hi, I work in the SPICE team (www.spice-space.org). I'd like to reuse some code from this file for some test for our project. The license of the 2 projects are a bit different (GPL2 against LGPL2) so in theory I couldn't do it. The code is mainly doing EGL initialization. Could I obtain a dispensation to reuse to code as LGPL2 ? Alternatively where can I get similar code with compatible LGPL2 license ? Regards, Frediano
[Qemu-devel] [Bug 1606899] Re: virtio-vga does not let guest poweroff properly
Removed the parameters, now the command line is
/usr/bin/qemu-system-x86_64 -machine accel=kvm -name rawhide -machine
pc-i440fx-2.3,accel=kvm,usb=off -cpu Haswell-noTSX -m 2048 -realtime
mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid
64216421-aec4-4ce4-aa52-aed9e4e31a1c -no-user-config -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/rawhide.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc
base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=discard -no-hpet
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1
-boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7
-device ich9-usb-
uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6
-device ich9-usb-
uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 -device ich9
-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 -device
virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive
file=/home/rawhide.qcow2,if=none,id=drive-virtio-disk0,format=qcow2
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio-
disk0,id=virtio-disk0,bootindex=1 -drive if=none,id=drive-
ide0-0-0,readonly=on -device ide-cd,bus=ide.0,unit=0,drive=drive-
ide0-0-0,id=ide0-0-0 -netdev user,id=hostnet0 -device virtio-net-
pci,netdev=hostnet0,id=net0,mac=52:54:00:fc:11:43,bus=pci.0,addr=0x3
-chardev pty,id=charserial0 -device isa-
serial,chardev=charserial0,id=serial0 -chardev
socket,id=charchannel0,path=/var/lib/libvirt/qemu/channel/target/rawhide.org.qemu.guest_agent.0,server,nowait
-device virtserialport,bus=virtio-
serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
-chardev spicevmc,id=charchannel1,name=vdagent -device
virtserialport,bus=virtio-
serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0
-device usb-tablet,id=input0 -spice ipv4,addr=0.0.0.0,port=5900,disable-
ticketing,image-compression=lz,seamless-migration=on,streaming-
video=filter -device virtio-vga,bus=pci.0,addr=0x2 -chardev
spicevmc,id=charredir0,name=usbredir -device usb-
redir,chardev=charredir0,id=redir0 -chardev
spicevmc,id=charredir1,name=usbredir -device usb-
redir,chardev=charredir1,id=redir1 -device virtio-balloon-
pci,id=balloon0,bus=pci.0,addr=0x8 -msg timestamp=on
Shutdown stops again.
(gdb) thread apply all bt full
Thread 5 (Thread 0x7fab8f1ff700 (LWP 3152)):
#0 0x7fac23b7d32d in poll () at ../sysdeps/unix/syscall-template.S:84
#1 0x7fac27913a46 in g_main_context_iterate (priority=,
n_fds=2, fds=0x5643798d6f00, timeout=, context=0x5643785d7760)
at gmain.c:4135
poll_func = 0x7fac27922330
max_priority = 2147483647
timeout = 2147483647
some_ready =
nfds = 2
allocated_nfds = 4
fds = 0x5643798d6f00
#2 0x7fac27913a46 in g_main_context_iterate (context=0x5643785d7760,
block=block@entry=1, dispatch=dispatch@entry=1, self=) at
gmain.c:3835
max_priority = 2147483647
timeout = 2147483647
some_ready =
nfds = 2
allocated_nfds = 4
fds = 0x5643798d6f00
#3 0x7fac27913dd2 in g_main_loop_run (loop=0x564378645560) at gmain.c:4034
__func__ = "g_main_loop_run"
#4 0x7fac25820e70 in red_worker_main (arg=) at
red-worker.c:1570
worker =
__FUNCTION__ = "red_worker_main"
loop = 0x564378645560
#5 0x7fac23e4f5ca in start_thread (arg=0x7fab8f1ff700) at
pthread_create.c:333
__res =
pd = 0x7fab8f1ff700
now =
unwind_buf =
{cancel_jmp_buf = {{jmp_buf = {140374817371904,
1634185351380305518, 140727220814255, 4096, 140374817371904, 140374817372608,
-1586721908543748498, -1588208112698816914}, mask_was_saved = 0}}, priv = {pad
= {0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
not_first_call =
pagesize_m1 =
sp =
freesize =
#6 0x7fac23b88ead in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Thread 4 (Thread 0x7fac10d2c700 (LWP 3150)):
#0 0x7fac23e54bd0 in pthread_cond_wait@@GLIBC_2.3.2 () at
../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1 0x564376e71d09 in qemu_cond_wait (cond=,
mutex=)
at /usr/src/debug/qemu-2.6.0/util/qemu-thread-posix.c:123
err =
__func__ = "qemu_cond_wait"
#2 0x564376b762df in qemu_kvm_cpu_thread_fn (arg=) at
/usr/src/debug/qemu-2.6.0/cpus.c:1030
cpu =
r =
#3 0x7fac23e4f5ca in start_thread (arg=0x7fac10d2c700) at
pthread_create.c:333
__res =
pd = 0x7fac10d2c700
now =
unwind_buf =
{cancel_jmp_buf = {{jmp_buf = {140376993351424,
1634185351380305518, 140727220813631, 4096, 140376993351424, 140376993352128,
-1588104572869835154, -1588208112698816914}, mask_was_saved = 0}}, priv = {pad
= {0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
not_first_call =
pagesize_m1 =
[Qemu-devel] [Bug 1606899] [NEW] virtio-vga does not let guest poweroff properly
Public bug reported: I have a VM running rawhide (Fedora development) and I can't poweroff the machine when I enable virtio-vga. Reboot works correctly. Using QXL works also. The machine arrive to print the "Powering off" message (from Linux kernel) but then hangs. The command line is /usr/bin/qemu-system-x86_64 -machine accel=kvm -name rawhide -machine pc-i440fx-2.3,accel=kvm,usb=off -cpu Haswell-noTSX -m 2048 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid 64216421-aec4-4ce4-aa52-aed9e4e31a1c -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/rawhide.monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=discard -no-hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device ich9-usb- uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 -device ich9-usb- uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 -device ich9 -usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/home/rawhide.qcow2,if=none,id=drive-virtio-disk0,format=qcow2 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio- disk0,id=virtio-disk0,bootindex=1 -drive if=none,id=drive- ide0-0-0,readonly=on -device ide-cd,bus=ide.0,unit=0,drive=drive- ide0-0-0,id=ide0-0-0 -netdev user,id=hostnet0 -device virtio-net- pci,netdev=hostnet0,id=net0,mac=52:54:00:fc:11:43,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa- serial,chardev=charserial0,id=serial0 -chardev socket,id=charchannel0,path=/var/lib/libvirt/qemu/channel/target/rawhide.org.qemu.guest_agent.0,server,nowait -device virtserialport,bus=virtio- serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0 -chardev spicevmc,id=charchannel1,name=vdagent -device virtserialport,bus=virtio- serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0 -device usb-tablet,id=input0 -spice ipv4,addr=0.0.0.0,port=5900,disable- ticketing,image-compression=lz,seamless-migration=on,streaming- video=filter -device virtio-vga,bus=pci.0,addr=0x2 -device intel- hda,id=sound0,bus=pci.0,addr=0x4 -device hda- duplex,id=sound0-codec0,bus=sound0.0,cad=0 -chardev spicevmc,id=charredir0,name=usbredir -device usb- redir,chardev=charredir0,id=redir0 -chardev spicevmc,id=charredir1,name=usbredir -device usb- redir,chardev=charredir1,id=redir1 -device virtio-balloon- pci,id=balloon0,bus=pci.0,addr=0x8 -msg timestamp=on I though was due to Virgl but disabling it does not change. I'm using Qemu 2.6.0 from Fedora 24. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1606899 Title: virtio-vga does not let guest poweroff properly Status in QEMU: New Bug description: I have a VM running rawhide (Fedora development) and I can't poweroff the machine when I enable virtio-vga. Reboot works correctly. Using QXL works also. The machine arrive to print the "Powering off" message (from Linux kernel) but then hangs. The command line is /usr/bin/qemu-system-x86_64 -machine accel=kvm -name rawhide -machine pc-i440fx-2.3,accel=kvm,usb=off -cpu Haswell-noTSX -m 2048 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid 64216421-aec4-4ce4-aa52-aed9e4e31a1c -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/rawhide.monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=discard -no- hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb- ehci1,id=usb,bus=pci.0,addr=0x6.0x7 -device ich9-usb- uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x6 -device ich9-usb- uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x6.0x1 -device ich9 -usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x6.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/home/rawhide.qcow2,if=none,id=drive-virtio-disk0,format=qcow2 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x7,drive=drive-virtio- disk0,id=virtio-disk0,bootindex=1 -drive if=none,id=drive- ide0-0-0,readonly=on -device ide-cd,bus=ide.0,unit=0,drive=drive- ide0-0-0,id=ide0-0-0 -netdev user,id=hostnet0 -device virtio-net- pci,netdev=hostnet0,id=net0,mac=52:54:00:fc:11:43,bus=pci.0,addr=0x3 -chardev pty,id=charserial0 -device isa- serial,chardev=charserial0,id=serial0 -chardev socket,id=charchannel0,path=/var/lib/libvirt/qemu/channel/target/rawhide.org.qemu.guest_agent.0,server,nowait -device virtserialport,bus=virtio-
Re: [Qemu-devel] [Spice-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
> > On Tue, Jul 19, 2016 at 09:41:22AM -0400, Frediano Ziglio wrote: > > > I don't think we have strong reasons to support software encoding, video > > > encoding is really expensive, and that mmap/copy is not going to be > > > marginal, so even less these 2 syscalls. > > > > > > > Using HW encoding is not easy at it seems: > > - you have to have client supporting server HW encoders; > > - you have to install additional software often closed source, accepting > > patents; > > - you have to have right permission on the system. > > What are you doing if these option are not respected? Do not allow > > connections? Showing blank screen? > > With a good (local) connection I can easily play using software MJPEG, why > > we should avoid such configurations? > > What we should be aiming/optimizing for is the hardware-accelerated > case. We will need a fallback when this is not usable, but the various > copies/encoding/... are going to be very expensive by themselves. Are > these changes (passing texture rather than dmabuf) making a significant > difference with software encoding? > > Christophe > Got some experimental results passing DRM primes to gstreamer (https://www.youtube.com/watch?v=NFDvMHfXUHA). With VAAPI it's working the full frame processing decreased by a 50%. No, they are not expensive and we could just (in case of fallback) import in a new GL context to have them extracted correctly. I'm actually trying to make it works again with software encoders (no VAAPI and having to extract raw data). It's working as with kernel 4.6 i915 allows mmap but as the texture are in a different format I get quite some garbage (they should be extracted with GL which know these problems). Frediano
Re: [Qemu-devel] [Spice-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
> Hi > > - Original Message - > > > > > > Hi > > > > > > - Original Message - > > > > Forgot to add RFC to the subject > > > > > > > > > > What's the rationale? if you share the texture id, you must share the GL > > > context too, right? Why not use a lower level dmabuf fd that can be > > > imported > > > by the server gl context (which is also what the protocol require > > > anyway)? > > > > > > > Yes, the display and context are shared using spice_qxl_gl_init. > > Importing again into a gl context would mean that you have to export the > > DRM prime and import again in a separate (not shared) context. > > It's also doable, just add 2 system call and wrapping/unwrapping. > > Would be good to pass the EGLDisplay then so spice-server don't have to > > initialize again possibly using another physical card. > > > > We have 4 cases: > > - client not connected; > > - local client; > > - remote client, software encoding; > > - remote client, hardware encoding. > > > > Before optimizing those syscalls and changing API etc, I would like to know > if they are expensive (it's not my feeling) > > Also, it is possible virglrenderer could be optimized to avoid exporting the > prime fd for each scanout, if the backing image is always the same. > > Sharing a GL context brings new issues. If spice server could use its own > context, we have some context isolation (gl is still bad at MT iirc). > > > Client not connected > > Passing the texture is a no-operation, passing DRM prime require to > > extract the handle and close every frame. > > > > Local client > > In this case there is no overhear, DRM prime is always extracted and > > passed to the client > > > > Remote client, software encoding > > Due to different problems (DRM prime not mmap-able or data not portably > > extractable) we'll need to import the DRM prime into a different EGL > > context (not shared with the original one), create another texture, > > extract data and free all texture/DRM prime. > > I don't think we have strong reasons to support software encoding, video > encoding is really expensive, and that mmap/copy is not going to be > marginal, so even less these 2 syscalls. > Using HW encoding is not easy at it seems: - you have to have client supporting server HW encoders; - you have to install additional software often closed source, accepting patents; - you have to have right permission on the system. What are you doing if these option are not respected? Do not allow connections? Showing blank screen? With a good (local) connection I can easily play using software MJPEG, why we should avoid such configurations? > > > > Remote client, hardware encoding > > It's not clear if it's better to pass the DRM prime or the texture, > > some API pass the texture. I got confirmation that gst_dmabuf_allocator_new > > could try to use mmap in some cases so we should check this somehow > > to make sure it does not. > > > > We definitely don't want any mmap/copy to take place for hw encoding. > Sure but that's hard to avoid fall backs with all different setups. > > Taking into account that DRM prime came with "free" reference counting > > creating the DRM prime from texture basically increase a counter which is > > used by our implementation to make sure texture is still existing so > > possibly passing texture instead of DRM prime just save a system call > > in the normal case. I don't know what happens to the DRM object handle when > > the texture is destroyed (in Qemu) with glDeleteTextures if bindings keep > > texture "alive" or are all reset. > > > > > > Could be that keeping qemu_spice_gl_scanout and > > spice_qxl_gl_scanout_texture > > as current implementation and adding a spice_qxl_gl_init/spice_qxl_gl_setup > > passing just QXLInstance and EGLDisplay is a better solution. > > > > Does is sound reasonable? > > I wouldn't rush with API changes before we have a better idea how hw encoding > can be done without mmap and wether its really worth it (I would rather see > spice spawning a seperate gl context and process for the encoding than > sharing it) > I'm not rushing, this was the idea of RFC. Spawing a process helps just to solve library licenses. My list of patches for spice-server used the passed context just to create a new context which is shared with the provided one, as I said using different gl context and importing the DRM prime is a good option. Passing the EGLDisplay from Qemu helps solving: - double EGL initialization; - multiple cards issues; - -chroot/-runas Qemu options, where you loose access and you are not able to initialize EGL/VAAPI again. I can see that Qemu searching for the card is different from VAAPI. In case of multiple cards and Qemu run as a daemon (not having Xwayland/X) you can end up using two physical cards. I'll try VAAPI DRM prime passing, I hope this week. Frediano
Re: [Qemu-devel] [Spice-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
>
> Hi
>
> - Original Message -
> > Forgot to add RFC to the subject
> >
>
> What's the rationale? if you share the texture id, you must share the GL
> context too, right? Why not use a lower level dmabuf fd that can be imported
> by the server gl context (which is also what the protocol require anyway)?
>
Yes, the display and context are shared using spice_qxl_gl_init.
Importing again into a gl context would mean that you have to export the
DRM prime and import again in a separate (not shared) context.
It's also doable, just add 2 system call and wrapping/unwrapping.
Would be good to pass the EGLDisplay then so spice-server don't have to
initialize again possibly using another physical card.
We have 4 cases:
- client not connected;
- local client;
- remote client, software encoding;
- remote client, hardware encoding.
Client not connected
Passing the texture is a no-operation, passing DRM prime require to
extract the handle and close every frame.
Local client
In this case there is no overhear, DRM prime is always extracted and
passed to the client
Remote client, software encoding
Due to different problems (DRM prime not mmap-able or data not portably
extractable) we'll need to import the DRM prime into a different EGL
context (not shared with the original one), create another texture,
extract data and free all texture/DRM prime.
Remote client, hardware encoding
It's not clear if it's better to pass the DRM prime or the texture,
some API pass the texture. I got confirmation that gst_dmabuf_allocator_new
could try to use mmap in some cases so we should check this somehow
to make sure it does not.
Taking into account that DRM prime came with "free" reference counting
creating the DRM prime from texture basically increase a counter which is
used by our implementation to make sure texture is still existing so
possibly passing texture instead of DRM prime just save a system call
in the normal case. I don't know what happens to the DRM object handle when
the texture is destroyed (in Qemu) with glDeleteTextures if bindings keep
texture "alive" or are all reset.
Could be that keeping qemu_spice_gl_scanout and spice_qxl_gl_scanout_texture
as current implementation and adding a spice_qxl_gl_init/spice_qxl_gl_setup
passing just QXLInstance and EGLDisplay is a better solution.
Does is sound reasonable?
Frediano
> >
> > >
> > > ---
> > > ui/spice-core.c| 5 -
> > > ui/spice-display.c | 29 -
> > > 2 files changed, 8 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > > index da05054..f7647f7 100644
> > > --- a/ui/spice-core.c
> > > +++ b/ui/spice-core.c
> > > @@ -828,11 +828,6 @@ void qemu_spice_init(void)
> > >
> > > #ifdef HAVE_SPICE_GL
> > > if (qemu_opt_get_bool(opts, "gl", 0)) {
> > > -if ((port != 0) || (tls_port != 0)) {
> > > -error_report("SPICE GL support is local-only for now and "
> > > - "incompatible with -spice port/tls-port");
> > > -exit(1);
> > > -}
> > > if (egl_rendernode_init() != 0) {
> > > error_report("Failed to initialize EGL render node for SPICE
> > > GL");
> > > exit(1);
> > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > index 2a77a54..72137bd 100644
> > > --- a/ui/spice-display.c
> > > +++ b/ui/spice-display.c
> > > @@ -852,6 +852,10 @@ static void qemu_spice_gl_block_timer(void *opaque)
> > > static QEMUGLContext qemu_spice_gl_create_context(DisplayChangeListener
> > > *dcl,
> > >QEMUGLParams *params)
> > > {
> > > +SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay,
> > > dcl);
> > > +
> > > +spice_qxl_gl_init(>qxl, qemu_egl_display, qemu_egl_rn_ctx);
> > > +
> > > eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
> > > qemu_egl_rn_ctx);
> > > return qemu_egl_create_context(dcl, params);
> > > @@ -864,28 +868,11 @@ static void
> > > qemu_spice_gl_scanout(DisplayChangeListener
> > > *dcl,
> > >uint32_t w, uint32_t h)
> > > {
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay,
> > > dcl);
> > > -EGLint stride = 0, fourcc = 0;
> > > -int fd = -1;
> > > -
> > > -if (tex_id) {
> > > -fd = egl_get_fd_for_texture(tex_id, , );
> > > -if (fd < 0) {
> > > -fprintf(stderr, "%s: failed to get fd for texture\n",
> > > __func__);
> > > -return;
> > > -}
> > > -dprint(1, "%s: %dx%d (stride %d, fourcc 0x%x)\n", __func__,
> > > - w, h, stride, fourcc);
> > > -} else {
> > > -dprint(1, "%s: no texture (no framebuffer)\n", __func__);
> > > -}
> > > -
> > > -assert(!tex_id || fd >= 0);
> > >
> > > -/* note: spice server will close the fd */
> > > -spice_qxl_gl_scanout(>qxl, fd,
Re: [Qemu-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
Forgot to add RFC to the subject
Frediano
>
> ---
> ui/spice-core.c| 5 -
> ui/spice-display.c | 29 -
> 2 files changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index da05054..f7647f7 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -828,11 +828,6 @@ void qemu_spice_init(void)
>
> #ifdef HAVE_SPICE_GL
> if (qemu_opt_get_bool(opts, "gl", 0)) {
> -if ((port != 0) || (tls_port != 0)) {
> -error_report("SPICE GL support is local-only for now and "
> - "incompatible with -spice port/tls-port");
> -exit(1);
> -}
> if (egl_rendernode_init() != 0) {
> error_report("Failed to initialize EGL render node for SPICE
> GL");
> exit(1);
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 2a77a54..72137bd 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -852,6 +852,10 @@ static void qemu_spice_gl_block_timer(void *opaque)
> static QEMUGLContext qemu_spice_gl_create_context(DisplayChangeListener
> *dcl,
>QEMUGLParams *params)
> {
> +SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> +
> +spice_qxl_gl_init(>qxl, qemu_egl_display, qemu_egl_rn_ctx);
> +
> eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
> qemu_egl_rn_ctx);
> return qemu_egl_create_context(dcl, params);
> @@ -864,28 +868,11 @@ static void qemu_spice_gl_scanout(DisplayChangeListener
> *dcl,
>uint32_t w, uint32_t h)
> {
> SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> -EGLint stride = 0, fourcc = 0;
> -int fd = -1;
> -
> -if (tex_id) {
> -fd = egl_get_fd_for_texture(tex_id, , );
> -if (fd < 0) {
> -fprintf(stderr, "%s: failed to get fd for texture\n", __func__);
> -return;
> -}
> -dprint(1, "%s: %dx%d (stride %d, fourcc 0x%x)\n", __func__,
> - w, h, stride, fourcc);
> -} else {
> -dprint(1, "%s: no texture (no framebuffer)\n", __func__);
> -}
> -
> -assert(!tex_id || fd >= 0);
>
> -/* note: spice server will close the fd */
> -spice_qxl_gl_scanout(>qxl, fd,
> - surface_width(ssd->ds),
> - surface_height(ssd->ds),
> - stride, fourcc, y_0_top);
> +spice_qxl_gl_scanout_texture(>qxl, tex_id,
> + surface_width(ssd->ds),
> + surface_height(ssd->ds),
> + y_0_top);
>
> qemu_spice_gl_monitor_config(ssd, x, y, w, h);
> }
[Qemu-devel] [PATCH Qemu] Change spice-server protocol for GL texture passing
---
ui/spice-core.c| 5 -
ui/spice-display.c | 29 -
2 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index da05054..f7647f7 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -828,11 +828,6 @@ void qemu_spice_init(void)
#ifdef HAVE_SPICE_GL
if (qemu_opt_get_bool(opts, "gl", 0)) {
-if ((port != 0) || (tls_port != 0)) {
-error_report("SPICE GL support is local-only for now and "
- "incompatible with -spice port/tls-port");
-exit(1);
-}
if (egl_rendernode_init() != 0) {
error_report("Failed to initialize EGL render node for SPICE GL");
exit(1);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 2a77a54..72137bd 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -852,6 +852,10 @@ static void qemu_spice_gl_block_timer(void *opaque)
static QEMUGLContext qemu_spice_gl_create_context(DisplayChangeListener *dcl,
QEMUGLParams *params)
{
+SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
+
+spice_qxl_gl_init(>qxl, qemu_egl_display, qemu_egl_rn_ctx);
+
eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE,
qemu_egl_rn_ctx);
return qemu_egl_create_context(dcl, params);
@@ -864,28 +868,11 @@ static void qemu_spice_gl_scanout(DisplayChangeListener
*dcl,
uint32_t w, uint32_t h)
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
-EGLint stride = 0, fourcc = 0;
-int fd = -1;
-
-if (tex_id) {
-fd = egl_get_fd_for_texture(tex_id, , );
-if (fd < 0) {
-fprintf(stderr, "%s: failed to get fd for texture\n", __func__);
-return;
-}
-dprint(1, "%s: %dx%d (stride %d, fourcc 0x%x)\n", __func__,
- w, h, stride, fourcc);
-} else {
-dprint(1, "%s: no texture (no framebuffer)\n", __func__);
-}
-
-assert(!tex_id || fd >= 0);
-/* note: spice server will close the fd */
-spice_qxl_gl_scanout(>qxl, fd,
- surface_width(ssd->ds),
- surface_height(ssd->ds),
- stride, fourcc, y_0_top);
+spice_qxl_gl_scanout_texture(>qxl, tex_id,
+ surface_width(ssd->ds),
+ surface_height(ssd->ds),
+ y_0_top);
qemu_spice_gl_monitor_config(ssd, x, y, w, h);
}
--
2.7.4
[Qemu-devel] [PATCH] vnc: send cursor when a new client is connecting
If you have hardware cursor and you are reconnecting the VNC client
you need to send the cursor. Failing to do so make the cursor invisible
till is changed.
Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
---
ui/vnc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ui/vnc.c b/ui/vnc.c
index ce4c669..825e65b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2046,6 +2046,9 @@ static void set_encodings(VncState *vs, int32_t
*encodings, size_t n_encodings)
break;
case VNC_ENCODING_RICH_CURSOR:
vs->features |= VNC_FEATURE_RICH_CURSOR_MASK;
+if (vs->vd->cursor) {
+vnc_cursor_define(vs);
+}
break;
case VNC_ENCODING_EXT_KEY_EVENT:
send_ext_key_event_ack(vs);
--
2.5.0
[Qemu-devel] [PATCH] qxl: Fix new function name for spice-server library
The new spice-server function to limit the number of monitors (0.12.6)
changed while development from spice_qxl_set_monitors_config_limit to
spice_qxl_max_monitors (accepted upstream).
By mistake I post patch with former name.
This patch fix the function name.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
I tested again doing a clean build, unfortunately I did some mistake
and my tests worked.
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 4e5ff69..2288238 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -273,8 +273,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
} else {
#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
if (qxl-max_outputs) {
-spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
-qxl-max_outputs);
+spice_qxl_set_max_monitors(qxl-ssd.qxl, qxl-max_outputs);
}
#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
--
2.1.0
Re: [Qemu-devel] [PATCH v3] qxl: allow to specify head limit to qxl driver
Hi, This patch rely on some change in spice-protocol which are not still accepted. See http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html. What is the upstream status here? accepted meanwhile? cheers, Gerd Accepted and merged, see patch at http://cgit.freedesktop.org/spice/spice/commit/?id=6d4e58f70ddf9c178e2cc20dd5a3ade8b95fd142 Frediano
Re: [Qemu-devel] [PATCH v3] qxl: allow to specify head limit to qxl driver
Ping
- Original Message -
From: Frediano Ziglio fzig...@redhat.com
To: kra...@redhat.com, spice-de...@lists.freedesktop.org, berra...@redhat.com
Cc: qemu-devel@nongnu.org, Frediano Ziglio fzig...@redhat.com
Sent: Monday, July 6, 2015 7:56:38 AM
Subject: [Qemu-devel] [PATCH v3] qxl: allow to specify head limit to qxl
driver
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 26 +-
hw/display/qxl.h | 3 +++
2 files changed, 24 insertions(+), 5 deletions(-)
Changes from v2:
- removed the RPC after spice-server upstream
- rebased
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index f87a5ee..4e5ff69 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -271,6 +271,12 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+if (qxl-max_outputs) {
+spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
+qxl-max_outputs);
+}
+#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
spice_qxl_monitors_config_async(qxl-ssd.qxl,
qxl-ram-monitors_config,
@@ -991,6 +997,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
QXLRom *rom = memory_region_get_ram_ptr(qxl-rom_bar);
int i;
+unsigned max_outputs = ARRAY_SIZE(rom-client_monitors_config.heads);
if (qxl-revision 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl-id,
@@ -1013,17 +1020,23 @@ static int
interface_client_monitors_config(QXLInstance *sin,
if (!monitors_config) {
return 1;
}
+
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+/* limit number of outputs based on setting limit */
+if (qxl-max_outputs qxl-max_outputs = max_outputs) {
+max_outputs = qxl-max_outputs;
+}
+#endif
+
memset(rom-client_monitors_config, 0,
sizeof(rom-client_monitors_config));
rom-client_monitors_config.count = monitors_config-num_of_monitors;
/* monitors_config-flags ignored */
-if (rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads)) {
+if (rom-client_monitors_config.count = max_outputs) {
trace_qxl_client_monitors_config_capped(qxl-id,
monitors_config-num_of_monitors,
-
ARRAY_SIZE(rom-client_monitors_config.heads));
-rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads);
+max_outputs);
+rom-client_monitors_config.count = max_outputs;
}
for (i = 0 ; i rom-client_monitors_config.count ; ++i) {
VDAgentMonConfig *monitor = monitors_config-monitors[i];
@@ -2274,6 +2287,9 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32(vram64_size_mb, PCIQXLDevice, vram_size_mb,
-1),
DEFINE_PROP_UINT32(vgamem_mb, PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32(surfaces, PCIQXLDevice, ssd.num_surfaces, 1024),
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+DEFINE_PROP_UINT16(max_outputs, PCIQXLDevice, max_outputs, 0),
+#endif
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..2ddf065 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,9 @@ typedef struct PCIQXLDevice {
QXLModes *modes;
uint32_t rom_size;
MemoryRegion rom_bar;
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+uint16_t max_outputs;
+#endif
/* vram pci bar */
uint32_t vram_size;
--
2.1.0
[Qemu-devel] [PATCH v3] qxl: allow to specify head limit to qxl driver
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 26 +-
hw/display/qxl.h | 3 +++
2 files changed, 24 insertions(+), 5 deletions(-)
Changes from v2:
- removed the RPC after spice-server upstream
- rebased
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index f87a5ee..4e5ff69 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -271,6 +271,12 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+if (qxl-max_outputs) {
+spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
+qxl-max_outputs);
+}
+#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
spice_qxl_monitors_config_async(qxl-ssd.qxl,
qxl-ram-monitors_config,
@@ -991,6 +997,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
QXLRom *rom = memory_region_get_ram_ptr(qxl-rom_bar);
int i;
+unsigned max_outputs = ARRAY_SIZE(rom-client_monitors_config.heads);
if (qxl-revision 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl-id,
@@ -1013,17 +1020,23 @@ static int interface_client_monitors_config(QXLInstance
*sin,
if (!monitors_config) {
return 1;
}
+
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+/* limit number of outputs based on setting limit */
+if (qxl-max_outputs qxl-max_outputs = max_outputs) {
+max_outputs = qxl-max_outputs;
+}
+#endif
+
memset(rom-client_monitors_config, 0,
sizeof(rom-client_monitors_config));
rom-client_monitors_config.count = monitors_config-num_of_monitors;
/* monitors_config-flags ignored */
-if (rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads)) {
+if (rom-client_monitors_config.count = max_outputs) {
trace_qxl_client_monitors_config_capped(qxl-id,
monitors_config-num_of_monitors,
-ARRAY_SIZE(rom-client_monitors_config.heads));
-rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads);
+max_outputs);
+rom-client_monitors_config.count = max_outputs;
}
for (i = 0 ; i rom-client_monitors_config.count ; ++i) {
VDAgentMonConfig *monitor = monitors_config-monitors[i];
@@ -2274,6 +2287,9 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32(vram64_size_mb, PCIQXLDevice, vram_size_mb, -1),
DEFINE_PROP_UINT32(vgamem_mb, PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32(surfaces, PCIQXLDevice, ssd.num_surfaces, 1024),
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+DEFINE_PROP_UINT16(max_outputs, PCIQXLDevice, max_outputs, 0),
+#endif
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..2ddf065 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,9 @@ typedef struct PCIQXLDevice {
QXLModes *modes;
uint32_t rom_size;
MemoryRegion rom_bar;
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+uint16_t max_outputs;
+#endif
/* vram pci bar */
uint32_t vram_size;
--
2.1.0
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
For the same reason there is the v = l test.
The v = l test state that the value can be out of range so it not always a
constant in the range.
Adding the v 0 check for every invalid value. As these are executed only for
logging should not be a performance penalty.
I also hope the compiler is able to optimize
if (v 0 || v = l)
with
if ((unsigned) v = l)
Frediano
11.06.2015 16:17, Frediano Ziglio wrote:
In qxl_v2n check that value is not negative.
Why do you think it is necessary?
Thanks,
/mjt
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl-logger.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index d944d3f..faed869 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -93,7 +93,7 @@ static const char *const spice_cursor_type[] = {
static const char *qxl_v2n(const char *const n[], size_t l, int v)
{
-if (v = l || !n[v]) {
+if (v 0 || v = l || !n[v]) {
return ???;
}
return n[v];
Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] Check value for invalid negative values
On Do, 2015-06-18 at 05:58 -0400, Frediano Ziglio wrote: For the same reason there is the v = l test. The v = l test state that the value can be out of range so it not always a constant in the range. Adding the v 0 check for every invalid value. As these are executed only for logging should not be a performance penalty. I also hope the compiler is able to optimize if (v 0 || v = l) with if ((unsigned) v = l) Just make v explicitly unsigned? cheers, Gerd Do you mean in the prototype? Well, this could have side effect due to different conversions so is not a so trivial patch. Explicitly casting to unsigned would do but is IMHO less easy to read that an explicit check. Frediano
Re: [Qemu-devel] [PATCH v2] RFC: qxl: allow to specify head limit to qxl driver
On Do, 2015-06-11 at 10:38 +0100, Frediano Ziglio wrote: libvirt has this as a video card parameter (actually set to 1 but not used). This parameter will allow to limit setting a use can do (which could be confusing). Hmm. So heads is '1' by default but ignored today. When this starts to be actually applied that will break existing multihead setups I suspect. Yes, unfortunately libvirt always set the default to 1 even if not used by driver. However the Qemu default (with no parameter) is still unlimited so it's a libvirt issue more then Qemu. This patch rely on some change in spice-protocol which are not still accepted. See http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html. Spice-server changes, right? Needs #ifdefs so qemu continues to build with old spice-server versions. The spice-server changes need to be upstream first. Yes, I'll do. Also: when we pass on the limit to spice-server anyway spice-server can take care to apply the limit both ways and not call the -client_monitors_config() callback with more than $limit monitors. Has the advantage to reduce the test matrix: Limit either works or doesn't. There will be no spice-server/qemu version combination where the limit is applied one way only. Gerd Is always Qemu that decide to apply the limitation, default (even for spice-server) is still unlimited. As Qemu compiled with option on won't run with older version there will never this problem. However I would agree that the patch for Qemu keeping all limits on one place would be smaller. Frediano
[Qemu-devel] [RFC PATCH v2] qxl: allows to specify head limit to qxl driver
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 26 +-
hw/display/qxl.h | 3 +++
2 files changed, 24 insertions(+), 5 deletions(-)
Change from v1:
- check spice-server version.
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b220e2d..d6e9369 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -272,6 +272,12 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+if (qxl-max_outputs) {
+spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
+qxl-max_outputs);
+}
+#endif
qxl-guest_monitors_config = qxl-ram-monitors_config;
spice_qxl_monitors_config_async(qxl-ssd.qxl,
qxl-ram-monitors_config,
@@ -992,6 +998,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
QXLRom *rom = memory_region_get_ram_ptr(qxl-rom_bar);
int i;
+unsigned max_outputs = ARRAY_SIZE(rom-client_monitors_config.heads);
if (qxl-revision 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl-id,
@@ -1014,17 +1021,23 @@ static int interface_client_monitors_config(QXLInstance
*sin,
if (!monitors_config) {
return 1;
}
+
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+/* limit number of outputs based on setting limit */
+if (qxl-max_outputs qxl-max_outputs = max_outputs) {
+max_outputs = qxl-max_outputs;
+}
+#endif
+
memset(rom-client_monitors_config, 0,
sizeof(rom-client_monitors_config));
rom-client_monitors_config.count = monitors_config-num_of_monitors;
/* monitors_config-flags ignored */
-if (rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads)) {
+if (rom-client_monitors_config.count = max_outputs) {
trace_qxl_client_monitors_config_capped(qxl-id,
monitors_config-num_of_monitors,
-ARRAY_SIZE(rom-client_monitors_config.heads));
-rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads);
+max_outputs);
+rom-client_monitors_config.count = max_outputs;
}
for (i = 0 ; i rom-client_monitors_config.count ; ++i) {
VDAgentMonConfig *monitor = monitors_config-monitors[i];
@@ -2278,6 +2291,9 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32(vram64_size_mb, PCIQXLDevice, vram_size_mb, -1),
DEFINE_PROP_UINT32(vgamem_mb, PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32(surfaces, PCIQXLDevice, ssd.num_surfaces, 1024),
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+DEFINE_PROP_UINT16(max_outputs, PCIQXLDevice, max_outputs, 0),
+#endif
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..2ddf065 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,9 @@ typedef struct PCIQXLDevice {
QXLModes *modes;
uint32_t rom_size;
MemoryRegion rom_bar;
+#if SPICE_SERVER_VERSION = 0x000c06 /* release 0.12.6 */
+uint16_t max_outputs;
+#endif
/* vram pci bar */
uint32_t vram_size;
--
2.1.0
[Qemu-devel] [PATCH 1/2] Constify some variable
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl-logger.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index c900c2c..d944d3f 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -22,7 +22,7 @@
#include qemu/timer.h
#include qxl.h
-static const char *qxl_type[] = {
+static const char *const qxl_type[] = {
[ QXL_CMD_NOP ] = nop,
[ QXL_CMD_DRAW ]= draw,
[ QXL_CMD_UPDATE ] = update,
@@ -31,7 +31,7 @@ static const char *qxl_type[] = {
[ QXL_CMD_SURFACE ] = surface,
};
-static const char *qxl_draw_type[] = {
+static const char *const qxl_draw_type[] = {
[ QXL_DRAW_NOP ] = nop,
[ QXL_DRAW_FILL] = fill,
[ QXL_DRAW_OPAQUE ] = opaque,
@@ -48,7 +48,7 @@ static const char *qxl_draw_type[] = {
[ QXL_DRAW_ALPHA_BLEND ] = alpha-blend,
};
-static const char *qxl_draw_effect[] = {
+static const char *const qxl_draw_effect[] = {
[ QXL_EFFECT_BLEND] = blend,
[ QXL_EFFECT_OPAQUE ] = opaque,
[ QXL_EFFECT_REVERT_ON_DUP] = revert-on-dup,
@@ -59,12 +59,12 @@ static const char *qxl_draw_effect[] = {
[ QXL_EFFECT_OPAQUE_BRUSH ] = opaque-brush,
};
-static const char *qxl_surface_cmd[] = {
+static const char *const qxl_surface_cmd[] = {
[ QXL_SURFACE_CMD_CREATE ] = create,
[ QXL_SURFACE_CMD_DESTROY ] = destroy,
};
-static const char *spice_surface_fmt[] = {
+static const char *const spice_surface_fmt[] = {
[ SPICE_SURFACE_FMT_INVALID ] = invalid,
[ SPICE_SURFACE_FMT_1_A ] = alpha/1,
[ SPICE_SURFACE_FMT_8_A ] = alpha/8,
@@ -74,14 +74,14 @@ static const char *spice_surface_fmt[] = {
[ SPICE_SURFACE_FMT_32_ARGB ] = ARGB/32,
};
-static const char *qxl_cursor_cmd[] = {
+static const char *const qxl_cursor_cmd[] = {
[ QXL_CURSOR_SET ] = set,
[ QXL_CURSOR_MOVE ] = move,
[ QXL_CURSOR_HIDE ] = hide,
[ QXL_CURSOR_TRAIL ] = trail,
};
-static const char *spice_cursor_type[] = {
+static const char *const spice_cursor_type[] = {
[ SPICE_CURSOR_TYPE_ALPHA ] = alpha,
[ SPICE_CURSOR_TYPE_MONO] = mono,
[ SPICE_CURSOR_TYPE_COLOR4 ] = color4,
@@ -91,7 +91,7 @@ static const char *spice_cursor_type[] = {
[ SPICE_CURSOR_TYPE_COLOR32 ] = color32,
};
-static const char *qxl_v2n(const char *n[], size_t l, int v)
+static const char *qxl_v2n(const char *const n[], size_t l, int v)
{
if (v = l || !n[v]) {
return ???;
--
2.1.0
[Qemu-devel] [PATCH 2/2] Check value for invalid negative values
In qxl_v2n check that value is not negative.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl-logger.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index d944d3f..faed869 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -93,7 +93,7 @@ static const char *const spice_cursor_type[] = {
static const char *qxl_v2n(const char *const n[], size_t l, int v)
{
-if (v = l || !n[v]) {
+if (v 0 || v = l || !n[v]) {
return ???;
}
return n[v];
--
2.1.0
[Qemu-devel] [PATCH v2] RFC: qxl: allow to specify head limit to qxl driver
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020221.html.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 20 +++-
hw/display/qxl.h | 1 +
2 files changed, 16 insertions(+), 5 deletions(-)
This version change the way limit is set. It came from Gerd Hoffmann
suggestion.
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b220e2d..678fde5 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -272,6 +272,10 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice
*qxl, int replay)
QXL_COOKIE_TYPE_POST_LOAD_MONITORS_CONFIG,
0));
} else {
+if (qxl-max_outputs) {
+spice_qxl_set_monitors_config_limit(qxl-ssd.qxl,
+qxl-max_outputs);
+}
qxl-guest_monitors_config = qxl-ram-monitors_config;
spice_qxl_monitors_config_async(qxl-ssd.qxl,
qxl-ram-monitors_config,
@@ -992,6 +996,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
QXLRom *rom = memory_region_get_ram_ptr(qxl-rom_bar);
int i;
+unsigned max_outputs = ARRAY_SIZE(rom-client_monitors_config.heads);
if (qxl-revision 4) {
trace_qxl_client_monitors_config_unsupported_by_device(qxl-id,
@@ -1014,17 +1019,21 @@ static int interface_client_monitors_config(QXLInstance
*sin,
if (!monitors_config) {
return 1;
}
+
+/* limit number of outputs based on setting limit */
+if (qxl-max_outputs qxl-max_outputs = max_outputs) {
+max_outputs = qxl-max_outputs;
+}
+
memset(rom-client_monitors_config, 0,
sizeof(rom-client_monitors_config));
rom-client_monitors_config.count = monitors_config-num_of_monitors;
/* monitors_config-flags ignored */
-if (rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads)) {
+if (rom-client_monitors_config.count = max_outputs) {
trace_qxl_client_monitors_config_capped(qxl-id,
monitors_config-num_of_monitors,
-ARRAY_SIZE(rom-client_monitors_config.heads));
-rom-client_monitors_config.count =
-ARRAY_SIZE(rom-client_monitors_config.heads);
+max_outputs);
+rom-client_monitors_config.count = max_outputs;
}
for (i = 0 ; i rom-client_monitors_config.count ; ++i) {
VDAgentMonConfig *monitor = monitors_config-monitors[i];
@@ -2278,6 +2287,7 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32(vram64_size_mb, PCIQXLDevice, vram_size_mb, -1),
DEFINE_PROP_UINT32(vgamem_mb, PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32(surfaces, PCIQXLDevice, ssd.num_surfaces, 1024),
+DEFINE_PROP_UINT16(max_outputs, PCIQXLDevice, max_outputs, 0),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..d045f59 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,7 @@ typedef struct PCIQXLDevice {
QXLModes *modes;
uint32_t rom_size;
MemoryRegion rom_bar;
+uint16_t max_outputs;
/* vram pci bar */
uint32_t vram_size;
--
2.1.0
Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver
2015-06-09 10:12 GMT+01:00 Gerd Hoffmann kra...@redhat.com:
On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
There is a hard limit of 16 monitors in the qxl device ...
Ok, didn't see it.
Main question and stop over are parameter name.
max_outputs please, to be consistent with upcoming virtio-vga.
I'll do.
+if (d-max_heads d-max_heads = 64) {
+rom-flags = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
+rom-max_heads = cpu_to_le16(d-max_heads);
+}
rom does already carry information about the heads. How about simply
applying the limit when filling rom-client_monitors_config in
interface_client_monitors_config()?
No device changes, no driver changes, alot less trouble.
cheers,
Gerd
So what you are proposing is client should limit heads based on
spice-server limit and also spice-server should limit monitor received
from client. I'll try it. So this would require only Qemu + libvirt
change. This is slightly change anyway as guest will see more heads
without monitor connected instead of limiting heads. But anyway is a
way to enforce heads.
Frediano
Re: [Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver
2015-06-09 10:43 GMT+01:00 Gerd Hoffmann kra...@redhat.com:
On Di, 2015-06-09 at 10:26 +0100, Frediano Ziglio wrote:
2015-06-09 10:12 GMT+01:00 Gerd Hoffmann kra...@redhat.com:
On Di, 2015-06-09 at 09:49 +0100, Frediano Ziglio wrote:
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
There is a hard limit of 16 monitors in the qxl device ...
Ok, didn't see it.
The rom-client_monitors_config heads array has 16 entries, and I
suspect the same limit could be in more places ...
In my header is 64 entries. Anyway, it has an hard limit.
+if (d-max_heads d-max_heads = 64) {
+rom-flags = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
+rom-max_heads = cpu_to_le16(d-max_heads);
+}
rom does already carry information about the heads. How about simply
applying the limit when filling rom-client_monitors_config in
interface_client_monitors_config()?
No device changes, no driver changes, alot less trouble.
cheers,
Gerd
So what you are proposing is client should limit heads based on
spice-server limit and also spice-server should limit monitor received
from client.
No. You'll add a new property like you did. In
interface_client_monitors_config(), where qemu already checks it
wouldn't overflow the 16 entries in the array it has, you additionally
check for the configured limit.
Which will filter the monitors_config sent from spice client to the
guest to not carry more than $limit entries. So, if you configured two
heads, and spice client says fyi, I have three heads here ..., qemu
will forward only two entries guests to the guest. Therefore the guest
will use two heads them only, leaving the third unused.
In theory spice-server could do the filtering too, but doing it in qemu
is easier as you can easily pass in the limit as device property.
cheers,
Gerd
I did change in interface_client_monitors_config and also in
spice-server adding a new spice function to cap the monitor numbers
from guest to client. This is required as client base the
configuration it send based on the number of head available on the
guest. So faking the guest in spice-server in both directions make
client and guest happy. Yes, this does not require driver changes. I
added a function to spice-server so at least a bump of minor ABI
version is needed. I don't know if there are a function to set some
configuration that does not require a new ABI.
Frediano
[Qemu-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver
This patch allow to limit number of heads using qxl driver. By default
qxl driver is not limited on any kind on head use so can decide to use
as much heads.
libvirt has this as a video card parameter (actually set to 1 but not
used). This parameter will allow to limit setting a use can do (which
could be confusing).
This patch rely on some change in spice-protocol which are not still
accepted. See
http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html.
Main question and stop over are parameter name. Consider that this
parameter is actually more a hint to drivers. I'm looking anyway to
a way to enforce this in spice-server.
Signed-off-by: Frediano Ziglio fzig...@redhat.com
---
hw/display/qxl.c | 6 ++
hw/display/qxl.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b220e2d..e9ccd30 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -327,6 +327,11 @@ static void init_qxl_rom(PCIQXLDevice *d)
rom-log_level = cpu_to_le32(d-guestdebug);
rom-modes_offset = cpu_to_le32(sizeof(QXLRom));
+if (d-max_heads d-max_heads = 64) {
+rom-flags = cpu_to_le64(QXL_ROM_FLAG_MAX_HEADS);
+rom-max_heads = cpu_to_le16(d-max_heads);
+}
+
rom-slot_gen_bits = MEMSLOT_GENERATION_BITS;
rom-slot_id_bits = MEMSLOT_SLOT_BITS;
rom-slots_start = 1;
@@ -2278,6 +2283,7 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32(vram64_size_mb, PCIQXLDevice, vram_size_mb, -1),
DEFINE_PROP_UINT32(vgamem_mb, PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32(surfaces, PCIQXLDevice, ssd.num_surfaces, 1024),
+DEFINE_PROP_UINT16(max_heads, PCIQXLDevice, max_heads, 0),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index deddd54..d785761 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -99,6 +99,7 @@ typedef struct PCIQXLDevice {
QXLModes *modes;
uint32_t rom_size;
MemoryRegion rom_bar;
+uint16_t max_heads;
/* vram pci bar */
uint32_t vram_size;
--
2.1.0
Re: [Qemu-devel] [RFC PATCH v3] tests: rtl8139: test timers and interrupt
2015-02-06 16:54 GMT+00:00 Stefan Hajnoczi stefa...@redhat.com: On Thu, Jan 08, 2015 at 06:38:23PM +, Frediano Ziglio wrote: Test behaviour of timers and interrupts related to timeouts. Signed-off-by: Frediano Ziglio fredd...@gmail.com --- tests/Makefile | 2 +- tests/rtl8139-test.c | 181 +++ 2 files changed, 182 insertions(+), 1 deletion(-) This patch was derived from a test I did while implementing timer in rtl8139 code. Now that there is support for integrated testing I converted it. The test was tested on a real NIC. As if it's the first test I wrote I don't know if syntax and details are fine. For instance should I remove nop test? Should I split my test? Changed from v2: - style (variable declaration, Perl script not able to spot it) Changed from v1: - style Thanks, applied to my net tree: https://github.com/stefanha/qemu/commits/net Hi, all the comments seems to refer to my patch for testing the card but from https://github.com/stefanha/qemu/commits/net looks like you applied Paolo patch. Is it expected? Regards, Frediano
Re: [Qemu-devel] [RFC PATCH v3] tests: rtl8139: test timers and interrupt
2015-01-20 13:36 GMT+00:00 Paolo Bonzini pbonz...@redhat.com:
On 08/01/2015 19:38, Frediano Ziglio wrote:
Test behaviour of timers and interrupts related to timeouts.
Signed-off-by: Frediano Ziglio fredd...@gmail.com
---
tests/Makefile | 2 +-
tests/rtl8139-test.c | 181
+++
2 files changed, 182 insertions(+), 1 deletion(-)
This patch was derived from a test I did while implementing timer in
rtl8139 code. Now that there is support for integrated testing I converted
it. The test was tested on a real NIC.
As if it's the first test I wrote I don't know if syntax and details are
fine. For instance should I remove nop test? Should I split my test?
Respectively, no and if you want.
Reviewed-by: Paolo Bonzini pbonz...@redhat.com
As the last person who touched the rtl8139 timer code, I'm glad I didn't
break anything. :)
Paolo
Hi,
sorry I didn't get everything. Was my patch/test fine or should I
change something? I saw you posted a patch for timer. Was your patch
tested with my code and you find some problems or was my patch that
had some problems?
Regards,
Frediano
Changed from v2:
- style (variable declaration, Perl script not able to spot it)
Changed from v1:
- style
diff --git a/tests/Makefile b/tests/Makefile
index e4ddb6a..8858407 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -320,7 +320,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o
$(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
tests/e1000-test$(EXESUF): tests/e1000-test.o
-tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o
+tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o $(libqos-pc-obj-y)
tests/pcnet-test$(EXESUF): tests/pcnet-test.o
tests/eepro100-test$(EXESUF): tests/eepro100-test.o
tests/vmxnet3-test$(EXESUF): tests/vmxnet3-test.o
diff --git a/tests/rtl8139-test.c b/tests/rtl8139-test.c
index f6a1be3..4e0bf02 100644
--- a/tests/rtl8139-test.c
+++ b/tests/rtl8139-test.c
@@ -10,19 +10,200 @@
#include glib.h
#include string.h
#include libqtest.h
+#include libqos/pci-pc.h
#include qemu/osdep.h
+#include qemu-common.h
/* Tests only initialization so far. TODO: Replace with functional tests */
static void nop(void)
{
}
+#define CLK 3300
+#define NS_PER_SEC 10ULL
+
+static QPCIBus *pcibus;
+static QPCIDevice *dev;
+static void *dev_base;
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+QPCIDevice **pdev = (QPCIDevice **) data;
+
+*pdev = dev;
+}
+
+static QPCIDevice *get_device(void)
+{
+QPCIDevice *dev;
+
+pcibus = qpci_init_pc();
+qpci_device_foreach(pcibus, 0x10ec, 0x8139, save_fn, dev);
+g_assert(dev != NULL);
+
+return dev;
+}
+
+#define PORT(name, len, val) \
+static unsigned __attribute__((unused)) in_##name(void) \
+{ \
+unsigned res = qpci_io_read##len(dev, dev_base+(val)); \
+g_test_message(*%s - %x\n, #name, res); \
+return res; \
+} \
+static void out_##name(unsigned v) \
+{ \
+g_test_message(%x - *%s\n, v, #name); \
+qpci_io_write##len(dev, dev_base+(val), v); \
+}
+
+PORT(Timer, l, 0x48)
+PORT(IntrMask, w, 0x3c)
+PORT(IntrStatus, w, 0x3E)
+PORT(TimerInt, l, 0x54)
+
+#define fatal(...) do { g_test_message(__VA_ARGS__); g_assert(0); } while
(0)
+
+static void test_timer(void)
+{
+const unsigned from = 0.95 * CLK;
+const unsigned to = 1.6 * CLK;
+unsigned prev, curr, next;
+unsigned cnt, diff;
+
+out_IntrMask(0);
+
+in_IntrStatus();
+in_Timer();
+in_Timer();
+
+/* Test 1. test counter continue and continue */
+out_TimerInt(0); /* disable timer */
+out_IntrStatus(0x4000);
+out_Timer(12345); /* reset timer to 0 */
+curr = in_Timer();
+if (curr 0.1 * CLK) {
+fatal(time too big %u\n, curr);
+}
+for (cnt = 0; ; ) {
+clock_step(1 * NS_PER_SEC);
+prev = curr;
+curr = in_Timer();
+
+/* test skip is in a specific range */
+diff = (curr-prev) 0xu;
+if (diff from || diff to) {
+fatal(Invalid diff %u (%u-%u)\n, diff, from, to);
+}
+if (curr prev ++cnt == 3) {
+break;
+}
+}
+
+/* Test 2. Check we didn't get an interrupt with TimerInt == 0 */
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test 3. Setting TimerInt to 1 and Timer to 0 get interrupt */
+out_TimerInt(1);
+out_Timer(0);
+clock_step(40);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test 3. Check acknowledge */
+out_IntrStatus(0x4000);
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test. Status set after
Re: [Qemu-devel] [PATCH 2/2] qemu-common.h: optimise muldiv64 for x86_64 architecture
2015-01-09 12:22 GMT+00:00 Peter Maydell peter.mayd...@linaro.org:
On 9 January 2015 at 11:25, Frediano Ziglio fredd...@gmail.com wrote:
As this platform can do multiply/divide using 128 bit precision use
these instructions to implement it.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f3033ae..880659d 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,11 +370,23 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
-#ifdef CONFIG_INT128
+#if defined(CONFIG_INT128) !defined(__x86_64__)
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
return (__int128)a * b / c;
}
+#elif defined(__x86_64__)
+/* Optimised x64 version. This assume that a*b/c fits in 64 bit */
This assumption isn't necessarily true, and this implementation
will dump core on overflow. For instance:
Inputs: a = 8000 b = 8000 c = 1
fn muldiv64 result 0
fn muldiv64_with_int128 result 0
fn muldiv64_with_uint128 result 0
Floating point exception (core dumped)
-- PMM
Yes, I know, it was meant to be a precondition, not a math rule. Doing
some grep I'm not really sure this is valid in all cases however I
would ask if the truncation is handled correctly. Surely in some cases
the call to this function is not needed like in muldiv64(1, tks,
usb_bit_time); or in muldiv64((int16_t) s-rtc.comp, 1000, 0x8000);
Frediano
Re: [Qemu-devel] [PATCH 2/2] qemu-common.h: optimise muldiv64 for x86_64 architecture
2015-01-09 15:52 GMT+00:00 Peter Maydell peter.mayd...@linaro.org: On 9 January 2015 at 15:41, Frediano Ziglio fredd...@gmail.com wrote: 2015-01-09 12:22 GMT+00:00 Peter Maydell peter.mayd...@linaro.org: +/* Optimised x64 version. This assume that a*b/c fits in 64 bit */ This assumption isn't necessarily true, and this implementation will dump core on overflow. Yes, I know, it was meant to be a precondition, not a math rule. Doing some grep I'm not really sure this is valid in all cases however I would ask if the truncation is handled correctly. Surely in some cases the call to this function is not needed like in muldiv64(1, tks, usb_bit_time); or in muldiv64((int16_t) s-rtc.comp, 1000, 0x8000); Sure, there are some cases where we know the calculation won't overflow, but there are also cases where we can't be so sure, and if the parameters can be controlled by the guest then we absolutely can't allow the guest to cause QEMU to SIGFPE. Incidentally, in the examples you quote gcc is generally able (because the function is inline) to do a better job because of some of the constants involved: for instance with muldiv64((int16_t) s-rtc.comp, 1000, 0x8000); it generates a completely inline insn sequence with one mul and no div insns. So I think at this point I come back to an earlier question: do you have profiling results showing this function to be a performance problem on a real workload? If not, it just doesn't seem to me to be worth the risk and the maintenance burden to add a native x86 assembly version. -- PMM I agree (after some digging) we are not sure we won't get that overflow. Agree to drop the second patch. However I would retain the first. Compiler can use it to optimize much easier. For instance if compiler understand that the multiplication fits into a 64 bit can decide to avoid the 128 bit operation easily, not so easy with all these shift, multiply, division and union structure. I didn't do any profiling. Frediano
[Qemu-devel] [PATCH] x86_64: optimise muldiv64 for x86_64 architecture
As this platform can do multiply/divide using 128 bit precision use
these instruction to implement it.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..5366220 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,7 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifndef __x86_64__
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +393,18 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b,
uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#else
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
+#endif
/* Round number down to multiple */
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
--
1.9.1
Re: [Qemu-devel] [PATCH] x86_64: optimise muldiv64 for x86_64 architecture
2015-01-09 11:24 GMT+00:00 Paolo Bonzini pbonz...@redhat.com:
On 09/01/2015 12:04, Frediano Ziglio wrote:
2015-01-09 10:35 GMT+00:00 Paolo Bonzini pbonz...@redhat.com:
On 09/01/2015 11:27, Frediano Ziglio wrote:
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..5366220 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,7 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifndef __x86_64__
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +393,18 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t
b, uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#else
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
+#endif
Good idea. However, if you have __int128, you can just do
return (__int128)a * b / c
and the compiler should generate the right code. Conveniently, there is
already CONFIG_INT128 that you can use.
Well, it works but in our case b = c, that is a * b / c is always
2^64.
This is not necessarily the case. Quick grep:
hw/timer/hpet.c:return (muldiv64(value, HPET_CLK_PERIOD, FS_PER_NS));
hw/timer/hpet.c:return (muldiv64(value, FS_PER_NS, HPET_CLK_PERIOD));
One of the two must disprove your assertion. :)
Unless FS_PER_NS == HPET_CLK_PERIOD :)
But it's true that we expect no overflow.
This is enough!
This lead to no integer overflow in the last division. However
the compiler does not know this so it does the entire (a*b) / c
division which is mainly consists in two integer division instead of
one (not taking into account that is implemented using a helper
function).
I think that I'll write two patches. One implementing using the int128
as you suggested (which is much easier to read that current one and
assembly ones) that another for x86_64 optimization.
Right, that's even better.
Out of curiosity, have you seen it in some profiles?
Paolo
No, just looked at the complicate code and generated code and though
why using dozen of instructions if two are enough? :)
Frediano
Re: [Qemu-devel] [PATCH 2/2] qemu-common.h: optimise muldiv64 for x86_64 architecture
2015-01-09 11:43 GMT+00:00 Paolo Bonzini pbonz...@redhat.com:
On 09/01/2015 12:25, Frediano Ziglio wrote:
/* compute with 96 bit intermediate result: (a*b)/c */
-#ifdef CONFIG_INT128
+#if defined(CONFIG_INT128) !defined(__x86_64__)
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
return (__int128)a * b / c;
}
+#elif defined(__x86_64__)
+/* Optimised x64 version. This assume that a*b/c fits in 64 bit */
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
Reorder it the other way, and you can simplify the first #if.
Yes, I was however thinking about people reading code. The int128
version is much easier to read so I put it first.
I applied patch 1 locally, and will send a pull request once the tree is
thawed.
Paolo
Frediano
[Qemu-devel] [PATCH] x86_64: optimise muldiv64 for x86_64 architecture
As this platform can do multiply/divide using 128 bit precision use
these instruction to implement it.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..5366220 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,7 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifndef __x86_64__
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +393,18 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b,
uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#else
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
+#endif
/* Round number down to multiple */
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
--
1.9.1
[Qemu-devel] [PATCH 1/2] qemu-common.h: optimise muldiv64 if int128 is available
Let compiler do the job to optimise the function.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..f3033ae 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,12 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifdef CONFIG_INT128
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+return (__uint128_t)a * b / c;
+}
+#else
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +398,7 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b,
uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#endif
/* Round number down to multiple */
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
--
1.9.1
Re: [Qemu-devel] [PATCH] x86_64: optimise muldiv64 for x86_64 architecture
2015-01-09 10:35 GMT+00:00 Paolo Bonzini pbonz...@redhat.com:
On 09/01/2015 11:27, Frediano Ziglio wrote:
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..5366220 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,7 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifndef __x86_64__
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +393,18 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b,
uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#else
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
+#endif
Good idea. However, if you have __int128, you can just do
return (__int128)a * b / c
and the compiler should generate the right code. Conveniently, there is
already CONFIG_INT128 that you can use.
Paolo
Well, it works but in our case b = c, that is a * b / c is always
2^64. This lead to no integer overflow in the last division. However
the compiler does not know this so it does the entire (a*b) / c
division which is mainly consists in two integer division instead of
one (not taking into account that is implemented using a helper
function).
I think that I'll write two patches. One implementing using the int128
as you suggested (which is much easier to read that current one and
assembly ones) that another for x86_64 optimization.
Frediano
[Qemu-devel] [PATCH 2/2] qemu-common.h: optimise muldiv64 for x86_64 architecture
As this platform can do multiply/divide using 128 bit precision use
these instructions to implement it.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f3033ae..880659d 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,11 +370,23 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
-#ifdef CONFIG_INT128
+#if defined(CONFIG_INT128) !defined(__x86_64__)
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
return (__int128)a * b / c;
}
+#elif defined(__x86_64__)
+/* Optimised x64 version. This assume that a*b/c fits in 64 bit */
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+uint64_t res;
+
+asm (mulq %2\n\tdivq %3
+ : =a(res)
+ : a(a), qm((uint64_t) b), qm((uint64_t)c)
+ : rdx, cc);
+return res;
+}
#else
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
--
1.9.1
[Qemu-devel] [PATCH 1/2] qemu-common.h: optimise muldiv64 if int128 is available
Let compiler do the job to optimise the function.
Signed-off-by: Frediano Ziglio frediano.zig...@huawei.com
---
include/qemu-common.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f862214..f3033ae 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -370,6 +370,12 @@ static inline uint8_t from_bcd(uint8_t val)
}
/* compute with 96 bit intermediate result: (a*b)/c */
+#ifdef CONFIG_INT128
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+return (__int128)a * b / c;
+}
+#else
static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
{
union {
@@ -392,6 +398,7 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b,
uint32_t c)
res.l.low = (((rh % c) 32) + (rl 0x)) / c;
return res.ll;
}
+#endif
/* Round number down to multiple */
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
--
1.9.1
[Qemu-devel] [RFC PATCH v3] tests: rtl8139: test timers and interrupt
Test behaviour of timers and interrupts related to timeouts.
Signed-off-by: Frediano Ziglio fredd...@gmail.com
---
tests/Makefile | 2 +-
tests/rtl8139-test.c | 181 +++
2 files changed, 182 insertions(+), 1 deletion(-)
This patch was derived from a test I did while implementing timer in
rtl8139 code. Now that there is support for integrated testing I converted
it. The test was tested on a real NIC.
As if it's the first test I wrote I don't know if syntax and details are
fine. For instance should I remove nop test? Should I split my test?
Changed from v2:
- style (variable declaration, Perl script not able to spot it)
Changed from v1:
- style
diff --git a/tests/Makefile b/tests/Makefile
index e4ddb6a..8858407 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -320,7 +320,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o
$(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
tests/e1000-test$(EXESUF): tests/e1000-test.o
-tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o
+tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o $(libqos-pc-obj-y)
tests/pcnet-test$(EXESUF): tests/pcnet-test.o
tests/eepro100-test$(EXESUF): tests/eepro100-test.o
tests/vmxnet3-test$(EXESUF): tests/vmxnet3-test.o
diff --git a/tests/rtl8139-test.c b/tests/rtl8139-test.c
index f6a1be3..4e0bf02 100644
--- a/tests/rtl8139-test.c
+++ b/tests/rtl8139-test.c
@@ -10,19 +10,200 @@
#include glib.h
#include string.h
#include libqtest.h
+#include libqos/pci-pc.h
#include qemu/osdep.h
+#include qemu-common.h
/* Tests only initialization so far. TODO: Replace with functional tests */
static void nop(void)
{
}
+#define CLK 3300
+#define NS_PER_SEC 10ULL
+
+static QPCIBus *pcibus;
+static QPCIDevice *dev;
+static void *dev_base;
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+QPCIDevice **pdev = (QPCIDevice **) data;
+
+*pdev = dev;
+}
+
+static QPCIDevice *get_device(void)
+{
+QPCIDevice *dev;
+
+pcibus = qpci_init_pc();
+qpci_device_foreach(pcibus, 0x10ec, 0x8139, save_fn, dev);
+g_assert(dev != NULL);
+
+return dev;
+}
+
+#define PORT(name, len, val) \
+static unsigned __attribute__((unused)) in_##name(void) \
+{ \
+unsigned res = qpci_io_read##len(dev, dev_base+(val)); \
+g_test_message(*%s - %x\n, #name, res); \
+return res; \
+} \
+static void out_##name(unsigned v) \
+{ \
+g_test_message(%x - *%s\n, v, #name); \
+qpci_io_write##len(dev, dev_base+(val), v); \
+}
+
+PORT(Timer, l, 0x48)
+PORT(IntrMask, w, 0x3c)
+PORT(IntrStatus, w, 0x3E)
+PORT(TimerInt, l, 0x54)
+
+#define fatal(...) do { g_test_message(__VA_ARGS__); g_assert(0); } while (0)
+
+static void test_timer(void)
+{
+const unsigned from = 0.95 * CLK;
+const unsigned to = 1.6 * CLK;
+unsigned prev, curr, next;
+unsigned cnt, diff;
+
+out_IntrMask(0);
+
+in_IntrStatus();
+in_Timer();
+in_Timer();
+
+/* Test 1. test counter continue and continue */
+out_TimerInt(0); /* disable timer */
+out_IntrStatus(0x4000);
+out_Timer(12345); /* reset timer to 0 */
+curr = in_Timer();
+if (curr 0.1 * CLK) {
+fatal(time too big %u\n, curr);
+}
+for (cnt = 0; ; ) {
+clock_step(1 * NS_PER_SEC);
+prev = curr;
+curr = in_Timer();
+
+/* test skip is in a specific range */
+diff = (curr-prev) 0xu;
+if (diff from || diff to) {
+fatal(Invalid diff %u (%u-%u)\n, diff, from, to);
+}
+if (curr prev ++cnt == 3) {
+break;
+}
+}
+
+/* Test 2. Check we didn't get an interrupt with TimerInt == 0 */
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test 3. Setting TimerInt to 1 and Timer to 0 get interrupt */
+out_TimerInt(1);
+out_Timer(0);
+clock_step(40);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test 3. Check acknowledge */
+out_IntrStatus(0x4000);
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test. Status set after Timer reset */
+out_Timer(0);
+out_TimerInt(0);
+out_IntrStatus(0x4000);
+curr = in_Timer();
+out_TimerInt(curr + 0.5 * CLK);
+clock_step(1 * NS_PER_SEC);
+out_Timer(0);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test. Status set after TimerInt reset */
+out_Timer(0);
+out_TimerInt(0);
+out_IntrStatus(0x4000);
+curr = in_Timer();
+out_TimerInt(curr + 0.5 * CLK);
+clock_step(1 * NS_PER_SEC);
+out_TimerInt(0);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test 4. Increment TimerInt we should see
[Qemu-devel] [RFC PATCH v2] tests: rtl8139: test timers and interrupt
Test behaviour of timers and interrupts related to timeouts.
Signed-off-by: Frediano Ziglio fredd...@gmail.com
---
tests/Makefile | 2 +-
tests/rtl8139-test.c | 180 +++
2 files changed, 181 insertions(+), 1 deletion(-)
This patch was derived from a test I did while implementing timer in
rtl8139 code. Now that there is support for integrated testing I converted
it. The test was tested on a real NIC.
As if it's the first test I wrote I don't know if syntax and details are
fine. For instance should I remove nop test? Should I split my test?
Changed from v1:
- style
diff --git a/tests/Makefile b/tests/Makefile
index e4ddb6a..8858407 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -320,7 +320,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o
$(libqos-omap-obj-y)
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
tests/e1000-test$(EXESUF): tests/e1000-test.o
-tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o
+tests/rtl8139-test$(EXESUF): tests/rtl8139-test.o $(libqos-pc-obj-y)
tests/pcnet-test$(EXESUF): tests/pcnet-test.o
tests/eepro100-test$(EXESUF): tests/eepro100-test.o
tests/vmxnet3-test$(EXESUF): tests/vmxnet3-test.o
diff --git a/tests/rtl8139-test.c b/tests/rtl8139-test.c
index f6a1be3..f0044bb 100644
--- a/tests/rtl8139-test.c
+++ b/tests/rtl8139-test.c
@@ -10,19 +10,199 @@
#include glib.h
#include string.h
#include libqtest.h
+#include libqos/pci-pc.h
#include qemu/osdep.h
+#include qemu-common.h
/* Tests only initialization so far. TODO: Replace with functional tests */
static void nop(void)
{
}
+#define CLK 3300
+#define NS_PER_SEC 10ULL
+
+static QPCIBus *pcibus;
+static QPCIDevice *dev;
+static void *dev_base;
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+QPCIDevice **pdev = (QPCIDevice **) data;
+
+*pdev = dev;
+}
+
+static QPCIDevice *get_device(void)
+{
+QPCIDevice *dev;
+
+pcibus = qpci_init_pc();
+qpci_device_foreach(pcibus, 0x10ec, 0x8139, save_fn, dev);
+g_assert(dev != NULL);
+
+return dev;
+}
+
+#define PORT(name, len, val) \
+static unsigned __attribute__((unused)) in_##name(void) \
+{ \
+unsigned res = qpci_io_read##len(dev, dev_base+(val)); \
+g_test_message(*%s - %x\n, #name, res); \
+return res; \
+} \
+static void out_##name(unsigned v) \
+{ \
+g_test_message(%x - *%s\n, v, #name); \
+qpci_io_write##len(dev, dev_base+(val), v); \
+}
+
+PORT(Timer, l, 0x48)
+PORT(IntrMask, w, 0x3c)
+PORT(IntrStatus, w, 0x3E)
+PORT(TimerInt, l, 0x54)
+
+#define fatal(...) do { g_test_message(__VA_ARGS__); g_assert(0); } while (0)
+
+static void test_timer(void)
+{
+const unsigned from = 0.95 * CLK;
+const unsigned to = 1.6 * CLK;
+
+out_IntrMask(0);
+
+in_IntrStatus();
+in_Timer();
+in_Timer();
+
+/* Test 1. test counter continue and continue */
+out_TimerInt(0); /* disable timer */
+out_IntrStatus(0x4000);
+out_Timer(12345); /* reset timer to 0 */
+unsigned curr = in_Timer();
+unsigned cnt;
+if (curr 0.1 * CLK) {
+fatal(time too big %u\n, curr);
+}
+for (cnt = 0; ; ) {
+clock_step(1 * NS_PER_SEC);
+unsigned prev = curr;
+curr = in_Timer();
+
+/* test skip is in a specific range */
+unsigned diff = (curr-prev) 0xu;
+if (diff from || diff to) {
+fatal(Invalid diff %u (%u-%u)\n, diff, from, to);
+}
+if (curr prev ++cnt == 3) {
+break;
+}
+}
+
+/* Test 2. Check we didn't get an interrupt with TimerInt == 0 */
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test 3. Setting TimerInt to 1 and Timer to 0 get interrupt */
+out_TimerInt(1);
+out_Timer(0);
+clock_step(40);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test 3. Check acknowledge */
+out_IntrStatus(0x4000);
+if (in_IntrStatus() 0x4000) {
+fatal(got an interrupt\n);
+}
+
+/* Test. Status set after Timer reset */
+out_Timer(0);
+out_TimerInt(0);
+out_IntrStatus(0x4000);
+curr = in_Timer();
+out_TimerInt(curr + 0.5 * CLK);
+clock_step(1 * NS_PER_SEC);
+out_Timer(0);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test. Status set after TimerInt reset */
+out_Timer(0);
+out_TimerInt(0);
+out_IntrStatus(0x4000);
+curr = in_Timer();
+out_TimerInt(curr + 0.5 * CLK);
+clock_step(1 * NS_PER_SEC);
+out_TimerInt(0);
+if ((in_IntrStatus() 0x4000) == 0) {
+fatal(we should have an interrupt here!\n);
+}
+
+/* Test 4. Increment TimerInt we should see an interrupt */
+curr = in_Timer();
+unsigned next = curr + 5.0 * CLK;
+out_TimerInt(next
