Re: [PATCH 3/3] usbredir: avoid queuing hello packet on snapshot restore

[Victor Toso]

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