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

[Alon Levy]

> 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
> 
>