[Spice-devel] [spice-protocol PATCH v2 0.12.2 1/2] qxl_dev.h: add client monitors configuration notification to guest

[Gerd Hoffmann]

On 09/11/12 17:32, Alon Levy wrote:
>> Hi,
>>
>> On 09/11/2012 04:35 PM, Alon Levy wrote:
>>> So far we have used the agent to notify the guest of a request to
>>> change
>>> the monitors configurations (heads) on the qxl device. This patch
>>> introduces
>>> a new interrupt and new fields in the qxl rom to notify the guest
>>> about
>>> a new request, similarly to how physical hardware notifies the
>>> driver.
>>>
>>> To avoid overwriting the rom while the guest is reading it there is
>>> a
>>> client_monitors_config_updating field in ROM. The update protocol
>>> is:
>>>
>>> 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 that QXLRom::client_monitors_Config_updating is clear
>>>        (a) when set, goto (1)
>>>    (5) check QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>>        (a) when set, goto (1)
>>>        (b) when clear we are done
>>>
>>
>> This seems very complicated how about:
>>
>> qemu:
>>     (1) fill QXLRom::client_monitors_config, including a crc32 of the
>>     data
>>     (2) raise QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
>>
>> guest on interrupt:
>>     (1) clear QXL_INTERRUPT_CLIENT_MONITORS_CONFIG bit in irq status
>>     (2) read QXLRom::client_monitors_config
>>     (3) (verify-crc)? done : goto 2
>>
>> That seems more straight-forward to me.
> 
> Requires crc computing code in qemu and the guest. I know there is
> such in the kernel. I guess it's fine. It also has the chance of a
> mistake, I can let that slide I guess..

I'm not sure it is actually simpler.  Using
client_monitors_config_updating is just two lines of code on the qemu
side (set & clear the bit).  Dunno about crc32, maybe we have such a
function somewhere in the networking code which we can reuse.

On the guest side using client_monitors_config_updating isn't that
complicated too.  The only thing which might add significant complexity
is (2) as this pretty much requires to not run this from irq context.
Otherwise it just adds a loop (needed for crc32 too) and two simple checks.

> But more importantly I think I still need to change the protocol. I
> still need to know if the guest supports the client_monitors_config
> before issuing it, to take care of the case of a VDAgentMonitorsConfig
> arriving in multiple chunks, and for that I think one of:

>  QXLInterface::client_monit-rs_config_supported()
> 
> pro: straightforward
> con: add such for each cap? might as well add the guest_capabilities I introduced before, only this time set this cap from qemu instead of via a guest io (and don't introduce that guest io since we won't need it
> 
> alternative, reuse client_monitors_config, but have a NULL parameter just check and return without actually sending a message
> pro: no extra api
> con: abuse of a function
> 
> What do you think?

I'd tend to go with QXLInterface::client_monitors_config(NULL).

But can't you just queue up the packets in spice-server until
VDAgentMonitorsConfig is complete?  You must do that anyway to assemble
the data for the client_monitors_config call, right?  Then send all
packets in one go after the client_monitors_config call returns (or drop
them, depending on the return value).

cheers,
  Gerd