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

[Hans de Goede]

Hi,

On 09/12/2012 10:24 AM, Alon Levy wrote:
>> 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.
>
> there is crc32 from zlib.h, used elsewhere. There are a few other implementation of crc32/16 in qemu. Linux has linux/crc32.h but surprise it and zlib don't match (well, it's expected - there is no crc32 spec, it depends on the polynomial, if you xor before or after).
>
> I don't really mind which solution we use, I can send patches for the crc32 one already.

If you've already looked into both, I would go with the one which
is simpler from an implementation pov. I've a feeling that the guest
code with the crc32 will be significantly simpler (not counting the
crc32 implementation itself, you can simply copy the kernels crc.h to
qemu), but it is your call.

>
>>
>> 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).
>>
>
> Yes, I queue the packets in order to assemble them for the client_monitors_config(not NULL) call, but it's still a bit (tiny bit, admitted) simpler not to pass it to the spice-server char device afterwords. I'll send the patches with the client_monitors_config(NULL) implementation.

Ok.

Regards,

Hans