[Spice-devel] [RFC] register vmc interface early for name != vdagent [was: Re: Read data out of the Virtqueue]
Alon Levy
alevy at redhat.com
Thu Apr 5 08:36:33 PDT 2012
On Thu, Apr 05, 2012 at 06:22:47AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> Alon,
>
> Can you tell me more specific point what you meant " bottomhalf"?
>
> I am testing my modified code to send a message thru the main channel and it works for me.
> However, if there is a better approach to fix this problem, I will take it.
Something like this:
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 1e735ff..560127e 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -25,6 +25,7 @@ typedef struct SpiceCharDriver {
uint8_t *datapos;
ssize_t bufsize, datalen;
uint32_t debug;
+ QEMUBH *bh_register;
} SpiceCharDriver;
static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
@@ -188,6 +189,14 @@ static void print_allowed_subtypes(void)
fprintf(stderr, "\n");
}
+static void bh_vmc_register_interface(void *opaque)
+{
+ SpiceCharDriver *s = opaque;
+
+ vmc_register_interface(s);
+ qemu_bh_delete(s->bh_register);
+}
+
CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
{
CharDriverState *chr;
@@ -226,12 +235,15 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
chr->chr_guest_open = spice_chr_guest_open;
chr->chr_guest_close = spice_chr_guest_close;
-#if SPICE_SERVER_VERSION < 0x000901
- /* See comment in vmc_state() */
if (strcmp(subtype, "vdagent") == 0) {
+#if SPICE_SERVER_VERSION < 0x000901
+ /* See comment in vmc_state() */
qemu_chr_generic_open(chr);
- }
#endif
+ } else {
+ s->bh_register = qemu_bh_new(bh_vmc_register_interface, s);
+ qemu_bh_schedule(s->bh_register);
+ }
return chr;
}
>
>
> -----Original Message-----
> From: Alon Levy [mailto:alevy at redhat.com]
> Sent: Thursday, April 05, 2012 2:17 PM
> To: Charles.Tsai-蔡清海-研究發展部
> Cc: Hans de Goede; spice-devel at lists.freedesktop.org
> Subject: Re: [Spice-devel] [RFC] register vmc interface early for name != vdagent [was: Re: Read data out of the Virtqueue]
>
> On Thu, Apr 05, 2012 at 01:28:21AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > Alon,
> >
> > Your suggested approach does not work since the call of " vmc_register_interface" is too early before spice server is initialized.
> > Please see function "qemu_spice_add_interface(SpiceBaseInstance *sin)" for the following Ooops message.
> > "Oops: spice configured but not active"
>
> ok. Thanks for checking. Actually, a really quick fix would be to use a bottomhalf, that's guranteed to run only when the main event loop is started, which is after spice server is initialized (I think..)
>
> >
> > ============== Fatal error
> > ===================================================================
> > ctsai at ctsai-ubuntu:~$ ./Target32.sh
> > Starting QEMU with...
> > -localtime -drive file=/var/lib/libvirt/images/Windows-7-32.img -vga
> > qxl -spice port=5900,disable-ticketing -device virtio-serial-pci,ioeventfd=off -chardev spicevmc,id=vdagent,name=vdagent,debug=2 -device virtserialport,chardev=vdagent,name=com.redhat.spice.0 -chardev spicevmc,id=vdprint,name=vdprint,debug=2 -device virtserialport,chardev=vdprint,name=com.redhat.print.0 -smp 1,cores=2 -m 2048 -enable-kvm ....
> > scd: 1: qemu_chr_open_spice: vdprint
> > scd: 1: vmc_register_interface
> > Oops: spice configured but not active
> >
> >
> > -----Original Message-----
> > From: Alon Levy [mailto:alevy at redhat.com]
> > Sent: Tuesday, April 03, 2012 9:33 PM
> > To: Charles.Tsai-蔡清海-研究發展部
> > Cc: Hans de Goede; spice-devel at lists.freedesktop.org
> > Subject: Re: [Spice-devel] [RFC] register vmc interface early for name
> > != vdagent [was: Re: Read data out of the Virtqueue]
> >
> > On Tue, Apr 03, 2012 at 01:21:09PM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > Alon,
> > >
> > > >>when " spice_chr_guest_open" is called, it is ignored if subtype == vdprint.
> > > >>chr->chr_guest_open = spice_chr_guest_open;
> > >
> > > Here, I just want to avoid "vdprint" from being created twice when "chr->chr_guest_open" is called.
> > > "chr->chr_guest_open" is still called when gust OS open the printing channel, isn't it?
> >
> > It won't be created twice, there is a flag protecting it:
> >
> > static void vmc_register_interface(SpiceCharDriver *scd) {
> > if (scd->active) {
> > return;
> > }
> > ...
> > scd->active = true;
> > ...
> > }
> >
> > >
> > >
> > >
> > > -----Original Message-----
> > > From: Alon Levy [mailto:alevy at redhat.com]
> > > Sent: Tuesday, April 03, 2012 9:14 PM
> > > To: Charles.Tsai-蔡清海-研究發展部
> > > Cc: Hans de Goede; spice-devel at lists.freedesktop.org
> > > Subject: Re: [Spice-devel] [RFC] register vmc interface early for
> > > name != vdagent [was: Re: Read data out of the Virtqueue]
> > >
> > > On Tue, Apr 03, 2012 at 11:13:41AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > > Alon,
> > > >
> > > > After I read the code, I got some idea on how to modify the code.
> > > > For the printing channel, I will call "spice_chr_guest_open" early than it supposed to be.
> > > > As such, the spice client can see the printing channel and initialize it.
> > > >
> > > > when " spice_chr_guest_open" is called, it is ignored if subtype == vdprint.
> > > > Don't know if there is any side effect until I test the code.
> > >
> > > Cahrles, I still don't understand what you are saying, can you please explain what it is you want to achieve?
> > >
> > > The patch below should register the channel as early as it gets, when the chardev is created, which is when the command line is parsed, before the vm is started, and before the main loop that listens for spice connections is started. The only missing bit is what you've already added (I assume) in spice-server's implmentation of spice_server_add_interface, specifically checking for subtype == "vdprint" and then registering a spicevmc channel.
> > >
> > > >
> > > >
> > > > -----Original Message-----
> > > > From: Alon Levy [mailto:alevy at redhat.com]
> > > > Sent: Tuesday, April 03, 2012 6:52 PM
> > > > To: Charles.Tsai-蔡清海-研究發展部
> > > > Cc: Hans de Goede; spice-devel at lists.freedesktop.org
> > > > Subject: Re: [Spice-devel] [RFC] register vmc interface early for
> > > > name != vdagent [was: Re: Read data out of the Virtqueue]
> > > >
> > > > On Tue, Apr 03, 2012 at 10:16:46AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > > > Alon,
> > > > >
> > > > > I am sorry. I don't see your patch.
> > > > > I will take a look at the following code and see how I can apply it for printing channel.
> > > > The diff below is the patch. It doesn't need any changes, if subtype != vdagent it will call vmc_register_interface at chardev creation time, well before spice starts listening to connections.
> > > >
> > > >
> > > > > ====================
> > > > > > diff --git a/spice-qemu-char.c b/spice-qemu-char.c index
> > > > > > 1e735ff..d3ae0b7 100644
> > > > > > --- a/spice-qemu-char.c
> > > > > > +++ b/spice-qemu-char.c
> > > > > > @@ -226,12 +226,14 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> > > > > > chr->chr_guest_open = spice_chr_guest_open;
> > > > > > chr->chr_guest_close = spice_chr_guest_close;
> > > > > >
> > > > > > -#if SPICE_SERVER_VERSION < 0x000901
> > > > > > - /* See comment in vmc_state() */
> > > > > > if (strcmp(subtype, "vdagent") == 0) {
> > > > > > +#if SPICE_SERVER_VERSION < 0x000901
> > > > > > + /* See comment in vmc_state() */
> > > > > > qemu_chr_generic_open(chr);
> > > > > > - }
> > > > > > #endif
> > > > > > + } else {
> > > > > > + vmc_register_interface(s);
> > > > > > + }
> > > > > >
> > > > > > return chr;
> > > > > > }
> > > > >
> > > > > -----Original Message-----
> > > > > From: Alon Levy [mailto:alevy at redhat.com]
> > > > > Sent: Tuesday, April 03, 2012 5:30 PM
> > > > > To: Hans de Goede; Charles.Tsai-蔡清海-研究發展部;
> > > > > spice-devel at lists.freedesktop.org
> > > > > Subject: Re: [Spice-devel] [RFC] register vmc interface early
> > > > > for name != vdagent [was: Re: Read data out of the Virtqueue]
> > > > >
> > > > > On Tue, Apr 03, 2012 at 12:17:32PM +0300, Alon Levy wrote:
> > > > > > On Tue, Apr 03, 2012 at 08:40:26AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > > > > > Alon,
> > > > > > >
> > > > > > > Our Windows client code is based on 0.10.1 release and that is, I believe, the latest one.
> > > > > > > "spice-gtk" is running in LINUX and I believe it is not applicable to Windows platform.
> > > > > >
> > > > > > That is wrong. It works on windows as well, there is also a
> > > > > > prebuilt binary on http://www.spice-space.org/downloads.html
> > > > > > of remote-viewer.exe
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Please refer to the following trace to understand what I said.
> > > > > > > As you can see, printing channel is registered to spice
> > > > > > > server after the following trace
> > > > > > > "spice_server_char_device_add_interface: Connect SPICE_CHANNEL_PRINT".
> > > > > > >
> > > > > > > Spice server report the number of channels to the client using the following main channel message "SPICE_MSG_MAIN_CHANNELS_LIST" that happens before the printing channel registers to the spice server. As such, the spice client cannot create the "print channel object" when it is launched.
> > > > > >
> > > > > > ok, my bad. The problem is not a race, the problem is that the
> > > > > > channel only gets registered when the guest opens the device,
> > > > > > not during initialization of the vm (before it starts running,
> > > > > > and before spice server listens to connections).
> > > > > >
> > > > > > The channel is registered via spice_server_add_interface:
> > > > > > spice_server_add_interface
> > > > > > -> spice_server_char_device_add_interface
> > > > > > -> spicevmc_device_connect
> > > > > > -> reds_register_channel
> > > > > >
> > > > > > spice_server_add_interface is called by qemu:
> > > > > > spice_chr_guest_open / spice_chr_write
> > > > > > -> vmc_register_interface
> > > > > > -> qemu_spice_add_interface
> > > > > > -> spice_server_add_interface
> > > > > >
> > > > > > So one way would be to get spice_chr_guest_open to be called early.
> > > > > > We could have the initialization of the channel done
> > > > > > unconditionally during chardev creation. The way it should affect users:
> > > > > > usbredir - already triggers the channel creation via doing a
> > > > > > smartcard
> > > > > > - I think it does the same as usbredir.
> > > > > > vdagent - can't do it, server uses the interface registration
> > > > > > as guest agent exists. So will have to special case.
> > > > > >
> > > > > > vdagent - doesn't have a dedicated channel so not really a problem.
> > > > >
> > > > > Eek, somehow I mangled the above paragraph. It should read:
> > > > >
> > > > > usbredir - already triggers the channel creation via doing a
> > > > > qemu_chr_fe_open in usbredir_initfn (see
> > > > > hw/usb/redirect.c)
> > > > >
> > > > > vdagent - can't do it, server uses the interface registration as guest
> > > > > agent exists. So will have to special case.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > I am thinking to remedy such a problem by sending a message to spice client thru the main channel. After spice client receives that message, it creates and initialize the "printing object". Although it is not good approach, it is the easy way to fix this issue for us. Let me know if you have a better idea. Thanks.
> > > > > >
> > > > > > So I guess something like the following would be a better solution:
> > > > > >
> > > > > > diff --git a/spice-qemu-char.c b/spice-qemu-char.c index
> > > > > > 1e735ff..d3ae0b7 100644
> > > > > > --- a/spice-qemu-char.c
> > > > > > +++ b/spice-qemu-char.c
> > > > > > @@ -226,12 +226,14 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> > > > > > chr->chr_guest_open = spice_chr_guest_open;
> > > > > > chr->chr_guest_close = spice_chr_guest_close;
> > > > > >
> > > > > > -#if SPICE_SERVER_VERSION < 0x000901
> > > > > > - /* See comment in vmc_state() */
> > > > > > if (strcmp(subtype, "vdagent") == 0) {
> > > > > > +#if SPICE_SERVER_VERSION < 0x000901
> > > > > > + /* See comment in vmc_state() */
> > > > > > qemu_chr_generic_open(chr);
> > > > > > - }
> > > > > > #endif
> > > > > > + } else {
> > > > > > + vmc_register_interface(s);
> > > > > > + }
> > > > > >
> > > > > > return chr;
> > > > > > }
> > > > > >
> > > > > >
> > > > > > Hans, any comments?
> > > > > >
> > > > > > >
> > > > > > > ============================================================
> > > > > > > ==
> > > > > > > ==
> > > > > > > == ==
> > > > > > > ============================================================
> > > > > > > == ctsai at ctsai-ubuntu:~$ ./Target32.sh Starting QEMU with...
> > > > > > > -localtime -drive
> > > > > > > file=/var/lib/libvirt/images/Windows-7-32.img
> > > > > > > -vga qxl -spice port=5900,disable-ticketing -device virtio-serial-pci,ioeventfd=off -chardev spicevmc,id=vdagent,name=vdagent,debug=2 -device virtserialport,chardev=vdagent,name=com.redhat.spice.0 -chardev spicevmc,id=vdprint,name=vdprint,debug=2 -device virtserialport,chardev=vdprint,name=com.redhat.print.0 -smp 1,cores=2 -m 2048 -enable-kvm ....
> > > > > > > do_spice_init: starting 0.10.1
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=migration
> > > > > > >
> > > > > > > spice_server_add_interface: SPICE_INTERFACE_MIGRATION
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=keyboard
> > > > > > >
> > > > > > > spice_server_add_interface: SPICE_INTERFACE_KEYBOARD
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=mouse
> > > > > > >
> > > > > > > spice_server_add_interface: SPICE_INTERFACE_MOUSE
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=qxl
> > > > > > >
> > > > > > > spice_server_add_interface: SPICE_INTERFACE_QXL
> > > > > > > red_worker_main: begin
> > > > > > > display_channel_create: create display channel
> > > > > > > cursor_channel_create: create cursor channel
> > > > > > > scd: 1: vmc_register_interface
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=char_device
> > > > > > >
> > > > > > > spice_server_char_device_add_interface: CHAR_DEVICE vdagent
> > > > > > > scd: 2: vmc_register_interface
> > > > > > > spice_server_add_interface: spice_server_add_interface:
> > > > > > > type=char_device
> > > > > > >
> > > > > > > spice_server_char_device_add_interface: CHAR_DEVICE vdprint
> > > > > > > spice_server_char_device_add_interface: Connect
> > > > > > > SPICE_CHANNEL_PRINT spice_chr_write
> > > > > > > scd: 1: spice_chr_write: 36
> > > > > > > scd: 1: vmc_read: vdagent 0x7f7ff3f98c80 8/8/36
> > > > > > > scd: 2: vmc_read: vdagent 0x7f7ff3f98c88 28/28/28
> > > > > > > scd: 3: vmc_read: vdagent (nil) 8/0/0
> > > > > > > scd: 4: vmc_read: vdagent (nil) 8/0/0
> > > > > > > spice_chr_write
> > > > > > > scd: 2: spice_chr_write: 36
> > > > > > > scd: 5: vmc_read: vdagent 0x7f7ff3f98c80 8/8/36
> > > > > > > scd: 6: vmc_read: vdagent 0x7f7ff3f98c88 28/28/28
> > > > > > > scd: 7: vmc_read: vdagent (nil) 8/0/0
> > > > > > > scd: 8: vmc_read: vdagent (nil) 8/0/0
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Alon Levy [mailto:alevy at redhat.com]
> > > > > > > Sent: Tuesday, April 03, 2012 3:54 PM
> > > > > > > To: Charles.Tsai-蔡清海-研究發展部
> > > > > > > Cc: spice-devel at lists.freedesktop.org
> > > > > > > Subject: Re: Read data out of the Virtqueue
> > > > > > >
> > > > > > > On Mon, Apr 02, 2012 at 07:44:46AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > > > > > > Alon,
> > > > > > > >
> > > > > > > > In spice client, the creation of the channel is based on "init->num_of_channels" that is returned from the spice server.
> > > > > > > > Please see the following function to create the spice
> > > > > > > > channel from the client
> > > > > > > >
> > > > > > > > void RedClient::handle_channels(RedPeer::InMessage* message) {
> > > > > > > > SpiceMsgChannels *init = (SpiceMsgChannels *)message->data();
> > > > > > > > SpiceChannelId* channels = init->channels;
> > > > > > > > for (unsigned int i = 0; i < init->num_of_channels; i++) {
> > > > > > > > create_channel(channels[i].type, channels[i].id);
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > For the printing channel, it is created and registered by
> > > > > > > > function spicevmc_device_connect(char_device, SPICE_CHANNEL_PRINT); (see in reds.c file) Because the printing channel is registered and created in spice server lately after spice client is up, the creation of the printing channel is thus skipped by the spice client.
> > > > > > >
> > > > > > > Two things: First, you are using the old client code base as an example.
> > > > > > > All well and good, but if possible it's better to use spice-gtk as a base for new features, because that's the client we are going with forwward (or actually the client enabling library, with remote-viewer being the preffered client).
> > > > > > > Second, what do you mean registered after the spice client is up?
> > > > > > > If there is some race window where the server is already
> > > > > > > listening to clients but the printing channel is not
> > > > > > > registered, that is bad and needs fixing, but in general the
> > > > > > > server is initialized
> > > > > > > *before* the client is connected. The right order is 1.
> > > > > > > server channels are registered 2. server starts listening 3.
> > > > > > > client connects
> > > > > > >
> > > > > > > #3 cannot come before #2 so in general there is no way to have the situation you describe (unless there is a race and #1 happens after #2 and then both orders with respect to #3 are possible).
> > > > > > >
> > > > > > > > Can you tell me how to resolve this problem based on the spice architecture? Thanks.
> > > > > > > >
> > > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Charles.Tsai-蔡清海-研究發展部
> > > > > > > > Sent: Friday, March 30, 2012 9:27 AM
> > > > > > > > To: 'Alon Levy'
> > > > > > > > Cc: spice-devel at lists.freedesktop.org
> > > > > > > > Subject: RE: Read data out of the Virtqueue
> > > > > > > >
> > > > > > > > Alon,
> > > > > > > >
> > > > > > > > It is a bug that I modified for printing channel. BTW, can a spice channel be brought up or down based on a request from a spice client?
> > > > > > > > The reason that I am asking for such a question is because the printing channel might be brought up based on a configuration policy. I would like to know if a spice channel can be brought up dynamically during the run time.
> > > > > > > >
> > > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Alon Levy [mailto:alevy at redhat.com]
> > > > > > > > Sent: Friday, March 30, 2012 4:16 AM
> > > > > > > > To: Charles.Tsai-蔡清海-研究發展部
> > > > > > > > Cc: spice-devel at lists.freedesktop.org
> > > > > > > > Subject: Re: Read data out of the Virtqueue
> > > > > > > >
> > > > > > > > On Thu, Mar 29, 2012 at 09:42:32AM +0000, Charles.Tsai-蔡清海-研究發展部 wrote:
> > > > > > > > > Alon,
> > > > > > > > >
> > > > > > > > > Forget my previous mail. When I dig into the vdservice, I found there was one bug to check the overlay I/O status after calling the VirtIO write function.
> > > > > > > > > After I fixed the bug, I can see more printing raw data inside the Qemu now. Although I still find some issues, hopefully I can fix it as I debug more codes in both spice client and spice server. Thank you for your time.
> > > > > > > >
> > > > > > > > Great that you managed to progress. Can you send a patch for the vdservice issue? Is it a bug in the original or just in the one you modified for printer channel?
> > > > > > > >
> > > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Charles.Tsai-蔡清海-研究發展部
> > > > > > > > > Sent: Wednesday, March 28, 2012 6:07 PM
> > > > > > > > > To: 'Alon Levy'
> > > > > > > > > Cc: spice-devel at lists.freedesktop.org
> > > > > > > > > Subject: Read data out of the Virtqueue
> > > > > > > > >
> > > > > > > > > Alon,
> > > > > > > > >
> > > > > > > > > My printer driver can write the printing raw data into virtIO driver. But I cannot see the found the printing raw data in the spice server. For the vdagent, I found the following code segment which is a callback to read the data from the vdi_port. Do I need to add a similar code to read the data from the Virtio?
> > > > > > > > >
> > > > > > > > > In Qemu, I do see vmc_write is called when the printer driver writes the printing raw data into the virtIOdevice.
> > > > > > > > > Consequently, function "spicevmc_red_channel_send_item" should be called to send the payload to the spice client. But I did not see function "spicevmc_red_channel_send_item" is called either. It looks like the printing rawa data is still inside the VirtIO queue. What function I need to add so as to pull the data out of the VirtIO queue?
> > > > > > > > >
> > > > > > > > > ========================================================
> > > > > > > > > == == == == ==== == ================= struct
> > > > > > > > > SpiceCharDeviceState vdagent_char_device_state = {
> > > > > > > > > .wakeup = &vdagent_char_device_wakeup, };
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > void vdagent_char_device_wakeup(SpiceCharDeviceInstance *sin) {
> > > > > > > > > while (read_from_vdi_port()); }
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel at lists.freedesktop.org
> > > > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list