[Spice-devel] [PATCH] Don't close all but one display during reboot.

[Jonathon Jongsma]

On Thu, 2017-02-02 at 05:29 -0500, Frediano Ziglio wrote:
> > 
> > When a guest is rebooted, the QXL driver gets unloaded at some
> > point in
> > the reboot process. When the driver is unloaded, the spice server
> > sets a
> > single flag to FALSE: RedWorker::driver_cap_monitors_config. This
> > flag
> > indicates whether the driver is capable of sending its own monitors
> > config
> > messages to the client.
> > 
> > The only place this flag is used is when a new primary surface is
> > created. If
> > this flag is true, the server assumes that the driver will send its
> > own
> > monitors config very soon after the surface is created. If it's
> > false, the
> > server directly sends its own temporary monitors config message to
> > the client
> > since the driver cannot.  This temporary monitors config message is
> > based on
> > the size of the just-created primary surface and always has a
> > maximum monitor
> > count of 1.
> > 
> > This flag is initialized to false at startup so that in early boot
> > (e.g. vga
> > text mode), the server will send out these 'temporary' monitor
> > config
> > messages
> > to the client whenever the primary surface is destroyed and re-
> > created. This
> > causes the client to resize its window to match the size of the
> > guest. When
> > the
> > QXL driver is eventually loaded and starts taking over the monitors
> > config
> > responsibilities, we set this flag to true and the server stops
> > sending
> > monitors config messages out on its own.
> > 
> > If we reboot and set this flag to false, it will result in the
> > server sending
> > a
> > monitors config message to the client indicating that the guest now
> > supports
> > a
> > maximum of 1 monitor. If the guest happens to have more than one
> > display
> > window
> > open, it will destroy those extra windows because they exceed the
> > maximum
> > allowed number of monitors. This means that if you reboot a guest
> > with 4
> > monitors, after reboot it will only have 1 monitor.
> > 
> 
> After reading this paragraph and the bug I don't see the issue.
> I mean, if on my laptop I reboot when I reboot I get a single monitor
> till the OS and other stuff kick in. After a reboot the firmware use
> one monitor or if capable do the mirroring but always the same way.
> 
> I think the issue is that on first boot the guest activate the
> additional monitors and the client do the same while on second
> boot (reboot) not so to me looks like more an issue of the client
> instead of the server.
> 

Well, it could be considered a client issue to some extent, but not an
easy one to fix. 

> I would try to get a network capture to look at DisplayChannel
> messages if they are more or less the same for first boot and
> reboot. If they are the same I would look at the client state
> to check that while booting it's the same.

The initial boot and the reboot are the same, and that's basically why
the problem exists. At initial boot, it brings up a single display. And
on reboot, it also brings up a single display. The issue is that we
want the reboot to behave differently.

Initial boot:
-------------
 - In early boot, the server sends monitors config message when primary
surface is created. monitors=1, max-monitors=1
 - client only shows a single window, disables menu items for enabling
additional displays because the guest doesn't support more than 1
 - When QXL driver is loaded and takes over, it takes over sending
monitors config messages. monitors=1, max-monitors=4
 - client still shows a single window, but now the menu items for
enabling additional menus are enabled
 - client enables a second monitor. Client sends monitors-config
request to server 
 - QXL driver enables the second monitor and sends a new monitors-
config response to the client. monitors = 2, max-monitors=4

Reboot:
-------
 - At some point in the reboot process while the QXL driver is still
loaded, it disables all but the first monitor, but still claims to
support more than one. monitors=1, max-monitors=4
 - client keeps both display windows open, but the second one just says
"Waiting for display 2..."
 - eventually the QXL driver gets unloaded, and the
RedWorker::driver_cap_monitors_config flag gets set to false
 - The next time a primary surface is created (during early boot?) the
server sends out a new monitors config message to match the size of the
primary surface. monitors=1, max-monitors=1
 - the client sees that the guest only supports a single monitor, so it
closes that inactive second display window
 - when the qxl driver is loaded and takes over, it takes over sending
monitors-config messages. monitors=1, max-monitors=4.


Before qemu commit 0a2b5e3a7899b40d05d7c6c1c41eb4e64dd2ed4b, the QXL
driver never called _unload() during reboot. This meant that during
early boot, the server would never send out monitors-config messages
with max-monitors=1. So the client would have never closed its extra
inactive display windows. They would have simply remained open with a
"Waiting for display n..." message. And as soon as the vdagent was
reconnected, the client would have sent a new monitors-config request
attempting to enable displays for all open windows. But now that
doesn't happen because those windows get closed during reboot.

So my fix attempts to retain that behavior (while still fixing the bug
that was addressed by 0a2b5e3a7899b40d05d7c6c1c41eb4e64dd2ed4b) by
keeping the max-monitors value the same as it had been before
rebooting.  

You could possibly argue that you should instead fix this issue in the
client by refusing to close/destroy displays when we recieve a
monitors-config message with a max-monitors value that is less than the
 number of currently-open windows. But the design of the client is such
that when we recieve a monitors-config message from the server, we
automatically create N Display objects (where N is the value of max-
monitors) even if these displays are not yet enabled. The existence of
these Display objects determines whether or not the menu for enabling
additional displays are enabled, etc. So if we change the client to not
destroy extra displays when we receive max-monitors, I fear that we
will introduce some additional bugs by violating some hidden
assumptions. But we could try it if you think it's a better approach.
It would be a more complicated fix, however.


> 
> > To avoid this, we assume that if we had the ability to support
> > multiple
> > monitors at some point, that ability will return at some point. So
> > when the
> > server constructs its own temporary monitors config message to send
> > to the
> > client (when the driver_cap_monitors_config flag is false), we
> > continue to
> > send
> > the previous maximum monitor count instead of always sending a
> > maximum of 1.
> > 
> > Resolves: rhbz#1274447
> > ---
> > 
> > NOTE: this fix is a workaround that assumes some things that may
> > not be valid
> > in all cases.  The main assumption is that if the QXL driver was
> > used before
> > the reboot, it will continue to be used after the reboot. If this
> > assumption
> > is
> > violated, the server will continue to report that the guest
> > supports e.g. 4
> > displays even though the driver may only support 1. The client
> > would then be
> > able to attempt to enable additional displays, which would
> > inevitably fail
> > without explanation.  This is not a huge problem, but maybe it's
> > not
> > acceptable. If it's not acceptable, I think that fixing this bug
> > would
> > require
> > significantly more work and may not be worth the effort.
> > 
> >  server/display-channel.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index a554bfd..88ee194 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2459,15 +2459,18 @@ void
> > display_channel_set_monitors_config_to_primary(DisplayChannel
> > *display)
> >  {
> >      DrawContext *context = &display->priv->surfaces[0].context;
> >      QXLHead head = { 0, };
> > +    uint16_t old_max = 1;
> >  
> >      spice_return_if_fail(display->priv-
> > >surfaces[0].context.canvas);
> >  
> > -    if (display->priv->monitors_config)
> > +    if (display->priv->monitors_config) {
> > +        old_max = display->priv->monitors_config->max_allowed;
> >          monitors_config_unref(display->priv->monitors_config);
> > +    }
> >  
> >      head.width = context->width;
> >      head.height = context->height;
> > -    display->priv->monitors_config = monitors_config_new(&head, 1,
> > 1);
> > +    display->priv->monitors_config = monitors_config_new(&head, 1,
> > old_max);
> >  }
> >  
> >  void display_channel_reset_image_cache(DisplayChannel *self)
> 
> Frediano