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