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

[Alon Levy]

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

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

> cheers,
>   Gerd
> 
>