Re: [Qemu-devel] [PATCH 3/3] hw/qxl: support client monitor configuration via device

[Alon Levy <alevy@redhat.com>]

> On 09/11/12 08:56, Alon Levy wrote:
> > Until now we used only the agent to change the monitor count and
> > each
> > monitor resolution. This patch introduces the qemu part of using
> > the
> > device as the mediator instead of the agent via virtio-serial.
> > 
> > Spice (>=0.11.5) calls the new
> > QXLInterface::client_monitors_config,
> > generating an interrupt QXL_INTERRUPT_CLIENT_MONITORS_CONFIG which
> > the
> > client indicates handling of (after reading from
> > QXLRom::client_monitors_config) by
> > QXL_IO_CLIENT_MONITORS_CONFIG_DONE.
> 
> I don't think an explicit handshake via
> QXL_IO_CLIENT_MONITORS_CONFIG_DONE is a good idea.

Why? I don't see the below as being better - it just moves the checking to the guest, and racily.

> 
> How about this update protocol:
> 
> qemu:
>   (1) set QXLRom::client_monitors_config_updating
>   (2) fill QXLRom::client_monitors_config
>   (3) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>   (4) clear QXLRom::client_monitors_config_updating
> 
> guest:
>   (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>   (2) wait until QXLRom::client_monitors_config_updating is clear
>   (3) parse QXLRom::client_monitors_config
>   (4) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>       (a) when set, goto (1).
>       (b) when clear we are done.
> 

Hmm, you are making the guest more complicated instead of the host side, don't know if that's better.

Point (2) is a busy wait, no? Also, guest will have to handle half old / half new configurations:
qemu(1)
qemu(2)
qemu(3)
qemu(4)
qemu(1)
qemu(2) start
guest(1)
guest(2)
guest(3) reads half old half new
qemu(2) complete
qemu(3)
qemu(4)
guest(1)
guest(2)
guest(3) reads full new

Thinking about it some more, I can complete the handling of outstanding client_monitors_config requests by having an additional struct (we don't need to keep the whole bunch, just the current and pending). That would make the logic similar to what you outlined (but reverse - qemu does the waiting and re-interrupt-ing).

> While thinking about it:  I think we also don't need the guest
> capabilities.  With the handshake gone we can update
> QXLRom::client_monitors_config unconditionally.

The capability is there for spice, not for qemu, I used it in qemu since it was already there.

Spice needs it to know if to pass the message (VDAgentMonitorsConfig) to the guest agent or not.

But if you just meant we don't need to check the capability in qemu code, I understand.

> 
> We might want to notify spice-server when the guest flips the
> QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in the irq mask, so we can
> route the event accordingly.

That would mean even more complex logic, to delay spice-server from sending the monitors command to the guest while more data could be incoming (which can be multiple chunks comprising one message, so we must wait for it to complete before pushing the VDAgentMonitorsConfig message).

>  Or we just route it unconditionally
> both
> ways and let the guest sort it (i.e. make vdagent ignore monitors
> config
> when the qxl kms driver is active).

Routing it only one way is simpler in spice code. In other words, I had a buggy version doing that and decided that I should just do it one way or the other to not bang my head on it. But it's also simpler to handle - what order are the events going to happen in the guest? Also, not only spice-vdagent plus our driver, but with, for instance, gnome-settings-daemon listening to the uevent from the kernel it will do modesetting by itself, racing with spice-vdagent.

> 
> cheers,
>   Gerd
> 
>