> 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 > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel