Re: [Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
>
> On Thu, Apr 12, 2018 at 05:50:12AM -0400, Frediano Ziglio wrote:
> > >
> > > On Wed, Apr 11, 2018 at 01:24:59PM -0400, Frediano Ziglio wrote:
> > > > >
> > > > > There's an implicit API/ABI contract between QEMU and SPICE that
> > > > > SPICE
> > > > > will keep the guest QXL resources alive as long as QEMU can hold a
> > > > > pointer to them. This implicit contract was broken in 1c6e7cf7
> > > > > "Release
> > > > > cursor as soon as possible", causing crashes at migration time.
> > > > > While the proper fix would be in QEMU so that spice-server does not
> > > > > need
> > > > > to have that kind of knowledge regarding QEMU internal
> > > > > implementation,
> > > > > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a
> > > > > regression
> > > > > while QEMU is being fixed.
> > > > >
> > > > > This version of the fix is based on a suggestion from Frediano
> > > > > Ziglio.
> > > > >
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> > > > >
> > > > > Signed-off-by: Christophe Fergeau
> > > > > ---
> > > > > server/red-parse-qxl.c | 3 +++
> > > > > server/red-parse-qxl.h | 1 +
> > > > > server/red-worker.c| 2 +-
> > > > > 3 files changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > > > > index d0e7eb718..ebd7dcee7 100644
> > > > > --- a/server/red-parse-qxl.c
> > > > > +++ b/server/red-parse-qxl.c
> > > > > @@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
> > > > > red_put_cursor(&red->u.set.shape);
> > > > > break;
> > > > > }
> > > > > +if (red->qxl) {
> > > > > +red_qxl_release_resource(red->qxl, red->release_info_ext);
> > > > > +}
> > > > > }
> > > >
> > > > Yes, fix of my code is correct.
> > > > However I cannot compile without the include!
> > > > Why is compiling for you? Maybe you have another commit in?
> > >
> > > Yep, my bad, I did not test it independantly of other commits, I
> > > squashed the missing #include in this commit.
> > >
> > > Christophe
> > >
> >
> > Should not be #include "red-qxl.h" ?
>
> Yes, that, which is what I had squashed in ;)
>
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index d0e7eb718..9f1303da3 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -24,6 +24,7 @@
> #include
> #include "spice-bitmap-utils.h"
> #include "red-common.h"
> +#include "red-qxl.h"
> #include "memslot.h"
> #include "red-parse-qxl.h"
>
>
With that change:
Acked-by: Frediano Ziglio
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
On Thu, Apr 12, 2018 at 05:50:12AM -0400, Frediano Ziglio wrote:
> >
> > On Wed, Apr 11, 2018 at 01:24:59PM -0400, Frediano Ziglio wrote:
> > > >
> > > > There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> > > > will keep the guest QXL resources alive as long as QEMU can hold a
> > > > pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> > > > cursor as soon as possible", causing crashes at migration time.
> > > > While the proper fix would be in QEMU so that spice-server does not need
> > > > to have that kind of knowledge regarding QEMU internal implementation,
> > > > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> > > > while QEMU is being fixed.
> > > >
> > > > This version of the fix is based on a suggestion from Frediano Ziglio.
> > > >
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> > > >
> > > > Signed-off-by: Christophe Fergeau
> > > > ---
> > > > server/red-parse-qxl.c | 3 +++
> > > > server/red-parse-qxl.h | 1 +
> > > > server/red-worker.c| 2 +-
> > > > 3 files changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > > > index d0e7eb718..ebd7dcee7 100644
> > > > --- a/server/red-parse-qxl.c
> > > > +++ b/server/red-parse-qxl.c
> > > > @@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
> > > > red_put_cursor(&red->u.set.shape);
> > > > break;
> > > > }
> > > > +if (red->qxl) {
> > > > +red_qxl_release_resource(red->qxl, red->release_info_ext);
> > > > +}
> > > > }
> > >
> > > Yes, fix of my code is correct.
> > > However I cannot compile without the include!
> > > Why is compiling for you? Maybe you have another commit in?
> >
> > Yep, my bad, I did not test it independantly of other commits, I
> > squashed the missing #include in this commit.
> >
> > Christophe
> >
>
> Should not be #include "red-qxl.h" ?
Yes, that, which is what I had squashed in ;)
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index d0e7eb718..9f1303da3 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -24,6 +24,7 @@
#include
#include "spice-bitmap-utils.h"
#include "red-common.h"
+#include "red-qxl.h"
#include "memslot.h"
#include "red-parse-qxl.h"
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
>
> On Wed, Apr 11, 2018 at 01:24:59PM -0400, Frediano Ziglio wrote:
> > >
> > > There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> > > will keep the guest QXL resources alive as long as QEMU can hold a
> > > pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> > > cursor as soon as possible", causing crashes at migration time.
> > > While the proper fix would be in QEMU so that spice-server does not need
> > > to have that kind of knowledge regarding QEMU internal implementation,
> > > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> > > while QEMU is being fixed.
> > >
> > > This version of the fix is based on a suggestion from Frediano Ziglio.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> > >
> > > Signed-off-by: Christophe Fergeau
> > > ---
> > > server/red-parse-qxl.c | 3 +++
> > > server/red-parse-qxl.h | 1 +
> > > server/red-worker.c| 2 +-
> > > 3 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > > index d0e7eb718..ebd7dcee7 100644
> > > --- a/server/red-parse-qxl.c
> > > +++ b/server/red-parse-qxl.c
> > > @@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
> > > red_put_cursor(&red->u.set.shape);
> > > break;
> > > }
> > > +if (red->qxl) {
> > > +red_qxl_release_resource(red->qxl, red->release_info_ext);
> > > +}
> > > }
> >
> > Yes, fix of my code is correct.
> > However I cannot compile without the include!
> > Why is compiling for you? Maybe you have another commit in?
>
> Yep, my bad, I did not test it independantly of other commits, I
> squashed the missing #include in this commit.
>
> Christophe
>
Should not be #include "red-qxl.h" ?
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
On Wed, Apr 11, 2018 at 01:24:59PM -0400, Frediano Ziglio wrote:
> >
> > There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> > will keep the guest QXL resources alive as long as QEMU can hold a
> > pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> > cursor as soon as possible", causing crashes at migration time.
> > While the proper fix would be in QEMU so that spice-server does not need
> > to have that kind of knowledge regarding QEMU internal implementation,
> > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> > while QEMU is being fixed.
> >
> > This version of the fix is based on a suggestion from Frediano Ziglio.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> >
> > Signed-off-by: Christophe Fergeau
> > ---
> > server/red-parse-qxl.c | 3 +++
> > server/red-parse-qxl.h | 1 +
> > server/red-worker.c| 2 +-
> > 3 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > index d0e7eb718..ebd7dcee7 100644
> > --- a/server/red-parse-qxl.c
> > +++ b/server/red-parse-qxl.c
> > @@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
> > red_put_cursor(&red->u.set.shape);
> > break;
> > }
> > +if (red->qxl) {
> > +red_qxl_release_resource(red->qxl, red->release_info_ext);
> > +}
> > }
>
> Yes, fix of my code is correct.
> However I cannot compile without the include!
> Why is compiling for you? Maybe you have another commit in?
Yep, my bad, I did not test it independantly of other commits, I
squashed the missing #include in this commit.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
>
> There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> will keep the guest QXL resources alive as long as QEMU can hold a
> pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> cursor as soon as possible", causing crashes at migration time.
> While the proper fix would be in QEMU so that spice-server does not need
> to have that kind of knowledge regarding QEMU internal implementation,
> this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> while QEMU is being fixed.
>
> This version of the fix is based on a suggestion from Frediano Ziglio.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1540919
>
> Signed-off-by: Christophe Fergeau
> ---
> server/red-parse-qxl.c | 3 +++
> server/red-parse-qxl.h | 1 +
> server/red-worker.c| 2 +-
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index d0e7eb718..ebd7dcee7 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
> red_put_cursor(&red->u.set.shape);
> break;
> }
> +if (red->qxl) {
> +red_qxl_release_resource(red->qxl, red->release_info_ext);
> +}
> }
Yes, fix of my code is correct.
However I cannot compile without the include!
Why is compiling for you? Maybe you have another commit in?
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 4a576ca07..f0407b54a 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -99,6 +99,7 @@ typedef struct RedSurfaceCmd {
> } RedSurfaceCmd;
>
> typedef struct RedCursorCmd {
> +QXLInstance *qxl;
> QXLReleaseInfoExt release_info_ext;
> uint8_t type;
> union {
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 387f500e8..eb927f3e0 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -118,7 +118,7 @@ static gboolean red_process_cursor_cmd(RedWorker *worker,
> const QXLCommandExt *e
> g_free(cursor_cmd);
> return FALSE;
> }
> -red_qxl_release_resource(worker->qxl, cursor_cmd->release_info_ext);
> +cursor_cmd->qxl = worker->qxl;
> cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
> return TRUE;
> }
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice-server 1/2] cursor: Delay release of QXL guest cursor resources
There's an implicit API/ABI contract between QEMU and SPICE that SPICE
will keep the guest QXL resources alive as long as QEMU can hold a
pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
cursor as soon as possible", causing crashes at migration time.
While the proper fix would be in QEMU so that spice-server does not need
to have that kind of knowledge regarding QEMU internal implementation,
this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
while QEMU is being fixed.
This version of the fix is based on a suggestion from Frediano Ziglio.
https://bugzilla.redhat.com/show_bug.cgi?id=1540919
Signed-off-by: Christophe Fergeau
---
server/red-parse-qxl.c | 3 +++
server/red-parse-qxl.h | 1 +
server/red-worker.c| 2 +-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index d0e7eb718..ebd7dcee7 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1497,4 +1497,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
red_put_cursor(&red->u.set.shape);
break;
}
+if (red->qxl) {
+red_qxl_release_resource(red->qxl, red->release_info_ext);
+}
}
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 4a576ca07..f0407b54a 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -99,6 +99,7 @@ typedef struct RedSurfaceCmd {
} RedSurfaceCmd;
typedef struct RedCursorCmd {
+QXLInstance *qxl;
QXLReleaseInfoExt release_info_ext;
uint8_t type;
union {
diff --git a/server/red-worker.c b/server/red-worker.c
index 387f500e8..eb927f3e0 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -118,7 +118,7 @@ static gboolean red_process_cursor_cmd(RedWorker *worker,
const QXLCommandExt *e
g_free(cursor_cmd);
return FALSE;
}
-red_qxl_release_resource(worker->qxl, cursor_cmd->release_info_ext);
+cursor_cmd->qxl = worker->qxl;
cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
return TRUE;
}
--
2.14.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
