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

[Gerd Hoffmann]

On 09/11/12 13:29, Alon Levy wrote:
>> Hi,
>> 
>>>> 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.
>> 
>> It is more robust.
> 
> I suggested a way for it to be as robust - I take robust to mean "no
> lost messages except for intermediate ones", which both solutions
> suffer from anyway.
> 
>> We don't have to keep state in qxl for the handshake
> You mean it's preferable to have state on the device
> (QXLRom::client_monitors_config_updating) rather then private qxl
> device state?

The difference is that QXLRom::client_monitors_config_updating is
one-way, it informs the guest what qemu is doing.

qxl->client_monitors_config_isr_in_progress depends on the guest doing
the handshake correctly.

>> It is also closer to how real hardware handles this.
> 
> I don't know how it is in real hardware.

Modern hardware would stuff this into an event queue, simliar to a
virtqueue.  Raise a ring-full IRQ in case it can't.

Two monitor config updates in a row would simply result in two event
queue entries.

Setting up a new spice ring just for that is probably overkill though.
But (maybe you've seen it) for a virtio-based qxl design I would clearly
pass that kind of data as virtqueue payload in a event queue, so a new
update wouldn't overwrite the previous one.

>> When qemu updated the data while the guest was reading it one of
>> the two conditions will be true: either (3a) if qemu is still busy
>> updating or (4) if qemu finished updating.
> 
> Making things more complicated in the host, qemu, means making the
> kernel driver in the guest simpler, so even though you have a good
> solution for the race you discovered I don't see why the alternative
> is worse. (answered point to point above).

Real hardware tends to be the other way around.

>> I was thinking about spice-vdagent detecting the qxl kms driver is 
>> active, then just ignore all VDAgentMonitorsConfig messages 
>> unconditionally if this happens to be the case, so there will be
>> no race. But if you think it is too complicated, no problem.
> 
> This sounds like a good idea, didn't think of it. But that leaves me
> fixing the vdagent code now :) Also, I guess I still think doing just
> one message looks simpler.
> 
>> Was just an idea.  Notifying spice-server which way to route is
>> fine with me too.
> 
> OK, if it's all the same to you I'll stick with spice-server routing
> the message.
> 
> Overall, if you find this tedious I will switch to your suggestion
> since it isn't such a big deal.

I think sending both ways unconditionally could make things easier.

For starters you'll have valid data in QXLRom unconditionally, even in
case the spice client has sent it before the qxl kms driver was loaded
and signaled support (this will only work in case the update is
handshake-free).

You have a better view on the big picture though, how this all interacts
vdagent starting and vdagent capability negotiation.

cheers,
  Gerd