| Message ID | 20220813011031.3744-4-j@getutm.app |
|---|---|
| State | New |
| Headers | show |
| Series | Set runstate to RUN_STATE_RESTORE_VM when started with "-loadvm" | expand |
Hi, On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote: > When launching QEMU with "-loadvm", usbredir_create_parser() should avoid > setting up the hello packet (just as with "-incoming". On the latest version > of libusbredir, usbredirparser_unserialize() will return error if the parser > is not "pristine." That was wrong in the usbredir side. The fix [0] was merged and included in the latest 0.13.0 release [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 > Signed-off-by: Joelle van Dyne <j@getutm.app> > --- > hw/usb/redirect.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > index fd7df599bc..47fac3895a 100644 > --- a/hw/usb/redirect.c > +++ b/hw/usb/redirect.c > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) > } > #endif > > - if (runstate_check(RUN_STATE_INMIGRATE)) { > + if (runstate_check(RUN_STATE_INMIGRATE) || > + runstate_check(RUN_STATE_RESTORE_VM)) { > flags |= usbredirparser_fl_no_hello; > } > usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, > -- > 2.28.0 > > Cheers, Victor
On Fri, Aug 12, 2022 at 10:30 PM Victor Toso <victortoso@redhat.com> wrote: > > Hi, > > On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote: > > When launching QEMU with "-loadvm", usbredir_create_parser() should avoid > > setting up the hello packet (just as with "-incoming". On the latest version > > of libusbredir, usbredirparser_unserialize() will return error if the parser > > is not "pristine." > > That was wrong in the usbredir side. The fix [0] was merged and > included in the latest 0.13.0 release This is good to know. Should the entire runstate_check in usbredir_create_parser be removed? > > [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 > > > Signed-off-by: Joelle van Dyne <j@getutm.app> > > --- > > hw/usb/redirect.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > > index fd7df599bc..47fac3895a 100644 > > --- a/hw/usb/redirect.c > > +++ b/hw/usb/redirect.c > > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) > > } > > #endif > > > > - if (runstate_check(RUN_STATE_INMIGRATE)) { > > + if (runstate_check(RUN_STATE_INMIGRATE) || > > + runstate_check(RUN_STATE_RESTORE_VM)) { > > flags |= usbredirparser_fl_no_hello; > > } > > usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, > > -- > > 2.28.0 > > > > > > Cheers, > Victor >
Hi, On Fri, Aug 12, 2022 at 10:33:54PM -0700, Joelle van Dyne wrote: > On Fri, Aug 12, 2022 at 10:30 PM Victor Toso <victortoso@redhat.com> wrote: > > > > Hi, > > > > On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote: > > > When launching QEMU with "-loadvm", usbredir_create_parser() should avoid > > > setting up the hello packet (just as with "-incoming". On the latest version > > > of libusbredir, usbredirparser_unserialize() will return error if the parser > > > is not "pristine." > > > > That was wrong in the usbredir side. The fix [0] was merged and > > included in the latest 0.13.0 release > > This is good to know. Should the entire runstate_check in > usbredir_create_parser be removed? From my POV your patch looks correct and would avoid migration issues such as [1] with usbredir 0.12.0 that was not patched [1] https://bugzilla.redhat.com/show_bug.cgi?id=2096008 So, I'd keep the check and the patch :) > > [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 > > > > > Signed-off-by: Joelle van Dyne <j@getutm.app> > > > --- > > > hw/usb/redirect.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > > > index fd7df599bc..47fac3895a 100644 > > > --- a/hw/usb/redirect.c > > > +++ b/hw/usb/redirect.c > > > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) > > > } > > > #endif > > > > > > - if (runstate_check(RUN_STATE_INMIGRATE)) { > > > + if (runstate_check(RUN_STATE_INMIGRATE) || > > > + runstate_check(RUN_STATE_RESTORE_VM)) { > > > flags |= usbredirparser_fl_no_hello; > > > } > > > usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, > > > -- > > > 2.28.0 > > > > > > Cheers, Victor
On Fri, Aug 12, 2022 at 10:50 PM Victor Toso <victortoso@redhat.com> wrote: > > Hi, > > On Fri, Aug 12, 2022 at 10:33:54PM -0700, Joelle van Dyne wrote: > > On Fri, Aug 12, 2022 at 10:30 PM Victor Toso <victortoso@redhat.com> wrote: > > > > > > Hi, > > > > > > On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote: > > > > When launching QEMU with "-loadvm", usbredir_create_parser() should avoid > > > > setting up the hello packet (just as with "-incoming". On the latest version > > > > of libusbredir, usbredirparser_unserialize() will return error if the parser > > > > is not "pristine." > > > > > > That was wrong in the usbredir side. The fix [0] was merged and > > > included in the latest 0.13.0 release > > > > This is good to know. Should the entire runstate_check in > > usbredir_create_parser be removed? > > From my POV your patch looks correct and would avoid migration > issues such as [1] with usbredir 0.12.0 that was not patched > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=2096008 > > So, I'd keep the check and the patch :) I have to admit I'm not too familiar with the inner workings of libusbredir. But is it desirable to skip the HELLO packet on "loadvm"? I wrote this on the assumption that it's correct because libusbredir enforces it, but if that's wrong, then I'm wondering if maybe we need the HELLO to re-establish communication (that was the issue I triaged with "-S", when USB devices did not work due to the HELLO packet not being sent). In a migration, it makes sense that a SPICE client has not reset the USB device. However, when re-starting QEMU with "-loadvm", it's possible the USB device has been disconnected and reconnected. Ideally, we report that to the guest and let it handle the reset. Would "usbredirparser_fl_no_hello" prevent that? > > > > [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 > > > > > > > Signed-off-by: Joelle van Dyne <j@getutm.app> > > > > --- > > > > hw/usb/redirect.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > > > > index fd7df599bc..47fac3895a 100644 > > > > --- a/hw/usb/redirect.c > > > > +++ b/hw/usb/redirect.c > > > > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) > > > > } > > > > #endif > > > > > > > > - if (runstate_check(RUN_STATE_INMIGRATE)) { > > > > + if (runstate_check(RUN_STATE_INMIGRATE) || > > > > + runstate_check(RUN_STATE_RESTORE_VM)) { > > > > flags |= usbredirparser_fl_no_hello; > > > > } > > > > usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, > > > > -- > > > > 2.28.0 > > > > > > > > > > Cheers, > Victor >
Hi, On Fri, Aug 12, 2022 at 10:57:08PM -0700, Joelle van Dyne wrote: > On Fri, Aug 12, 2022 at 10:50 PM Victor Toso <victortoso@redhat.com> wrote: > > > > Hi, > > > > On Fri, Aug 12, 2022 at 10:33:54PM -0700, Joelle van Dyne wrote: > > > On Fri, Aug 12, 2022 at 10:30 PM Victor Toso <victortoso@redhat.com> wrote: > > > > > > > > Hi, > > > > > > > > On Fri, Aug 12, 2022 at 06:10:31PM -0700, Joelle van Dyne wrote: > > > > > When launching QEMU with "-loadvm", usbredir_create_parser() should avoid > > > > > setting up the hello packet (just as with "-incoming". On the latest version > > > > > of libusbredir, usbredirparser_unserialize() will return error if the parser > > > > > is not "pristine." > > > > > > > > That was wrong in the usbredir side. The fix [0] was merged and > > > > included in the latest 0.13.0 release > > > > > > This is good to know. Should the entire runstate_check in > > > usbredir_create_parser be removed? > > > > From my POV your patch looks correct and would avoid migration > > issues such as [1] with usbredir 0.12.0 that was not patched > > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=2096008 > > > > So, I'd keep the check and the patch :) > > I have to admit I'm not too familiar with the inner workings of > libusbredir. But is it desirable to skip the HELLO packet on > "loadvm"? The hello package is to used for capabilities negotiation and then stablish a new connection. > I wrote this on the assumption that it's correct because > libusbredir enforces it, but if that's wrong, then I'm > wondering if maybe we need the HELLO to re-establish > communication (that was the issue I triaged with "-S", when USB > devices did not work due to the HELLO packet not being sent). > In a migration, it makes sense that a SPICE client has not > reset the USB device. However, when re-starting QEMU with > "-loadvm", it's possible the USB device has been disconnected > and reconnected. Yes, or it could be holding the device to reestablish the connection with the VM. Depends a bit on the backend? I'm not 100% sure. If user was using a TCP backend and connecting to usbredirserver (or usbredirect running as a TCP server with --as) then I assume that QEMU would try to reconnect and keep going like migration. > Ideally, we report that to the guest and let it handle the > reset. Would "usbredirparser_fl_no_hello" prevent that? usbredirparser_fl_no_hello is mainly meant for unserialization but it can also be used for the application to send/handle its own hello package too (used for emulated usb devices in spice-gtk [2]). All in all, if the device is no longer available on loadvm(), at the point the VM is restarted, the guest should be notified similarly to when the device is being unplugged. [2] https://gitlab.freedesktop.org/spice/spice-gtk/-/commit/78c5a2e93383bd21a121 > > > > [0] https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 > > > > > > > > > Signed-off-by: Joelle van Dyne <j@getutm.app> > > > > > --- > > > > > hw/usb/redirect.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > > > > > index fd7df599bc..47fac3895a 100644 > > > > > --- a/hw/usb/redirect.c > > > > > +++ b/hw/usb/redirect.c > > > > > @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) > > > > > } > > > > > #endif > > > > > > > > > > - if (runstate_check(RUN_STATE_INMIGRATE)) { > > > > > + if (runstate_check(RUN_STATE_INMIGRATE) || > > > > > + runstate_check(RUN_STATE_RESTORE_VM)) { > > > > > flags |= usbredirparser_fl_no_hello; > > > > > } > > > > > usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, > > > > > -- > > > > > 2.28.0 > > > > > Cheers, Victor
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index fd7df599bc..47fac3895a 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -1280,7 +1280,8 @@ static void usbredir_create_parser(USBRedirDevice *dev) } #endif - if (runstate_check(RUN_STATE_INMIGRATE)) { + if (runstate_check(RUN_STATE_INMIGRATE) || + runstate_check(RUN_STATE_RESTORE_VM)) { flags |= usbredirparser_fl_no_hello; } usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE,
When launching QEMU with "-loadvm", usbredir_create_parser() should avoid setting up the hello packet (just as with "-incoming". On the latest version of libusbredir, usbredirparser_unserialize() will return error if the parser is not "pristine." Signed-off-by: Joelle van Dyne <j@getutm.app> --- hw/usb/redirect.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)