[PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Yuri Benditovich]

If the redirected device has this capability, Windows guest may
place the device into D2 and expect it to wake when the device
becomes active, but this will never happen. For example, when
internal Bluetooth adapter is redirected, keyboards and mice
connected to it do not work. Setting global property
'usb-redir.nowake=off' keeps 'remote wake' as is.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 hw/usb/redirect.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index e0f5ca6f81..e95898fe80 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -113,6 +113,7 @@ struct USBRedirDevice {
     /* Properties */
     CharBackend cs;
     bool enable_streams;
+    bool suppress_remote_wake;
     uint8_t debug;
     int32_t bootindex;
     char *filter_str;
@@ -1989,6 +1990,23 @@ static void usbredir_control_packet(void *priv, uint64_t id,
             memcpy(dev->dev.data_buf, data, data_len);
         }
         p->actual_length = len;
+        /*
+         * If this is GET_DESCRIPTOR request for configuration descriptor,
+         * remove 'remote wakeup' flag from it to prevent idle power down
+         * in Windows guest
+         */
+        if (dev->suppress_remote_wake &&
+            control_packet->requesttype == USB_DIR_IN &&
+            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
+            control_packet->value == (USB_DT_CONFIG << 8) &&
+            control_packet->index == 0 &&
+            /* bmAttributes field of config descriptor */
+            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
+                DPRINTF("Removed remote wake %04X:%04X\n",
+                    dev->device_info.vendor_id,
+                    dev->device_info.product_id);
+                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
+            }
         usb_generic_async_ctrl_complete(&dev->dev, p);
     }
     free(data);
@@ -2530,6 +2548,7 @@ static Property usbredir_properties[] = {
     DEFINE_PROP_UINT8("debug", USBRedirDevice, debug, usbredirparser_warning),
     DEFINE_PROP_STRING("filter", USBRedirDevice, filter_str),
     DEFINE_PROP_BOOL("streams", USBRedirDevice, enable_streams, true),
+    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.17.1

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> If the redirected device has this capability, Windows guest may
> place the device into D2 and expect it to wake when the device
> becomes active, but this will never happen. For example, when
> internal Bluetooth adapter is redirected, keyboards and mice
> connected to it do not work. Setting global property
> 'usb-redir.nowake=off' keeps 'remote wake' as is.

"usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
name?  Naming is hard...  What about "usb-redir.wakeup=on"?

> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  hw/usb/redirect.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index e0f5ca6f81..e95898fe80 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -113,6 +113,7 @@ struct USBRedirDevice {
>      /* Properties */
>      CharBackend cs;
>      bool enable_streams;
> +    bool suppress_remote_wake;
>      uint8_t debug;
>      int32_t bootindex;
>      char *filter_str;
> @@ -1989,6 +1990,23 @@ static void usbredir_control_packet(void *priv, uint64_t id,
>              memcpy(dev->dev.data_buf, data, data_len);
>          }
>          p->actual_length = len;
> +        /*
> +         * If this is GET_DESCRIPTOR request for configuration descriptor,
> +         * remove 'remote wakeup' flag from it to prevent idle power down
> +         * in Windows guest
> +         */
> +        if (dev->suppress_remote_wake &&
> +            control_packet->requesttype == USB_DIR_IN &&
> +            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
> +            control_packet->value == (USB_DT_CONFIG << 8) &&
> +            control_packet->index == 0 &&
> +            /* bmAttributes field of config descriptor */
> +            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
> +                DPRINTF("Removed remote wake %04X:%04X\n",
> +                    dev->device_info.vendor_id,
> +                    dev->device_info.product_id);
> +                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
> +            }
>          usb_generic_async_ctrl_complete(&dev->dev, p);
>      }
>      free(data);
> @@ -2530,6 +2548,7 @@ static Property usbredir_properties[] = {
>      DEFINE_PROP_UINT8("debug", USBRedirDevice, debug, usbredirparser_warning),
>      DEFINE_PROP_STRING("filter", USBRedirDevice, filter_str),
>      DEFINE_PROP_BOOL("streams", USBRedirDevice, enable_streams, true),
> +    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };

The default is .nowake=on.  Is that a guest-visible change?  Do we need
compat properties to keep it off for existing machine types?

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Yuri Benditovich]

On Wed, Nov 27, 2019 at 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>
> > If the redirected device has this capability, Windows guest may
> > place the device into D2 and expect it to wake when the device
> > becomes active, but this will never happen. For example, when
> > internal Bluetooth adapter is redirected, keyboards and mice
> > connected to it do not work. Setting global property
> > 'usb-redir.nowake=off' keeps 'remote wake' as is.
>
> "usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
> with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
> name?  Naming is hard...  What about "usb-redir.wakeup=on"?
'"wakeup" is good but "wakeup=on" makes an impression that we add the
capability to the device even if it does not have one.
disable_wake? suppress_wake? clear_wake? wake_allowed?

>
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  hw/usb/redirect.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> > index e0f5ca6f81..e95898fe80 100644
> > --- a/hw/usb/redirect.c
> > +++ b/hw/usb/redirect.c
> > @@ -113,6 +113,7 @@ struct USBRedirDevice {
> >      /* Properties */
> >      CharBackend cs;
> >      bool enable_streams;
> > +    bool suppress_remote_wake;
> >      uint8_t debug;
> >      int32_t bootindex;
> >      char *filter_str;
> > @@ -1989,6 +1990,23 @@ static void usbredir_control_packet(void *priv, uint64_t id,
> >              memcpy(dev->dev.data_buf, data, data_len);
> >          }
> >          p->actual_length = len;
> > +        /*
> > +         * If this is GET_DESCRIPTOR request for configuration descriptor,
> > +         * remove 'remote wakeup' flag from it to prevent idle power down
> > +         * in Windows guest
> > +         */
> > +        if (dev->suppress_remote_wake &&
> > +            control_packet->requesttype == USB_DIR_IN &&
> > +            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
> > +            control_packet->value == (USB_DT_CONFIG << 8) &&
> > +            control_packet->index == 0 &&
> > +            /* bmAttributes field of config descriptor */
> > +            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
> > +                DPRINTF("Removed remote wake %04X:%04X\n",
> > +                    dev->device_info.vendor_id,
> > +                    dev->device_info.product_id);
> > +                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
> > +            }
> >          usb_generic_async_ctrl_complete(&dev->dev, p);
> >      }
> >      free(data);
> > @@ -2530,6 +2548,7 @@ static Property usbredir_properties[] = {
> >      DEFINE_PROP_UINT8("debug", USBRedirDevice, debug, usbredirparser_warning),
> >      DEFINE_PROP_STRING("filter", USBRedirDevice, filter_str),
> >      DEFINE_PROP_BOOL("streams", USBRedirDevice, enable_streams, true),
> > +    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
>
> The default is .nowake=on.  Is that a guest-visible change?  Do we need
> compat properties to keep it off for existing machine types?

Guest will see the device as one without 'remote wake' capability.
IMO, in the worst case this does not change anything, in the best case
this will suppress device power transition to D2 and the device will
work.
Including existing machine types.
Probably I did not understand the idea of 'compat property', can you
please provide an example of some existing compat property?
And, of course, we can keep existing behavior by default and advise to
turn this property on to make these devices work.
>

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Markus Armbruster]

Yuri Benditovich <yuri.benditovich@daynix.com> writes:

> On Wed, Nov 27, 2019 at 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>>
>> > If the redirected device has this capability, Windows guest may
>> > place the device into D2 and expect it to wake when the device
>> > becomes active, but this will never happen. For example, when
>> > internal Bluetooth adapter is redirected, keyboards and mice
>> > connected to it do not work. Setting global property
>> > 'usb-redir.nowake=off' keeps 'remote wake' as is.
>>
>> "usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
>> with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
>> name?  Naming is hard...  What about "usb-redir.wakeup=on"?
> '"wakeup" is good but "wakeup=on" makes an impression that we add the
> capability to the device even if it does not have one.

True.

> disable_wake? suppress_wake? clear_wake? wake_allowed?

Let's have a look at what the property does:

>> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>> > ---
>> >  hw/usb/redirect.c | 19 +++++++++++++++++++
>> >  1 file changed, 19 insertions(+)
>> >
>> > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
>> > index e0f5ca6f81..e95898fe80 100644
>> > --- a/hw/usb/redirect.c
>> > +++ b/hw/usb/redirect.c
>> > @@ -113,6 +113,7 @@ struct USBRedirDevice {
>> >      /* Properties */
>> >      CharBackend cs;
>> >      bool enable_streams;
>> > +    bool suppress_remote_wake;
>> >      uint8_t debug;
>> >      int32_t bootindex;
>> >      char *filter_str;
>> > @@ -1989,6 +1990,23 @@ static void usbredir_control_packet(void *priv, uint64_t id,
>> >              memcpy(dev->dev.data_buf, data, data_len);
>> >          }
>> >          p->actual_length = len;
>> > +        /*
>> > +         * If this is GET_DESCRIPTOR request for configuration descriptor,
>> > +         * remove 'remote wakeup' flag from it to prevent idle power down
>> > +         * in Windows guest
>> > +         */
>> > +        if (dev->suppress_remote_wake &&
>> > +            control_packet->requesttype == USB_DIR_IN &&
>> > +            control_packet->request == USB_REQ_GET_DESCRIPTOR &&
>> > +            control_packet->value == (USB_DT_CONFIG << 8) &&
>> > +            control_packet->index == 0 &&
>> > +            /* bmAttributes field of config descriptor */
>> > +            len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
>> > +                DPRINTF("Removed remote wake %04X:%04X\n",
>> > +                    dev->device_info.vendor_id,
>> > +                    dev->device_info.product_id);
>> > +                dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
>> > +            }

If the property is true, and this is a GET_DESCRIPTOR control packet
with USB_CFG_ATT_WAKEUP bit set, unset it.  Correct?

Assuming it is: "suppress_wakup" feels okay to me.

Whatever we pick, I recommend naming the USBRedirDevice member like the
property.  It's currently named @suppress_remote_wake.

>> >          usb_generic_async_ctrl_complete(&dev->dev, p);
>> >      }
>> >      free(data);
>> > @@ -2530,6 +2548,7 @@ static Property usbredir_properties[] = {
>> >      DEFINE_PROP_UINT8("debug", USBRedirDevice, debug, usbredirparser_warning),
>> >      DEFINE_PROP_STRING("filter", USBRedirDevice, filter_str),
>> >      DEFINE_PROP_BOOL("streams", USBRedirDevice, enable_streams, true),
>> > +    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
>> >      DEFINE_PROP_END_OF_LIST(),
>> >  };
>>
>> The default is .nowake=on.  Is that a guest-visible change?  Do we need
>> compat properties to keep it off for existing machine types?
>
> Guest will see the device as one without 'remote wake' capability.
> IMO, in the worst case this does not change anything, in the best case
> this will suppress device power transition to D2 and the device will
> work.
> Including existing machine types.
> Probably I did not understand the idea of 'compat property', can you
> please provide an example of some existing compat property?
> And, of course, we can keep existing behavior by default and advise to
> turn this property on to make these devices work.

Guest-visible changes require care.  Consider:

* Live migration

  This is meant to be transparent to the guest, even when we migrate to
  a different version of QEMU.  Guest-visible hardware changes are
  no-no.

* Cold reboot ("dead" migration)

  Guests should cope with hardware changes on cold reboot.
  Nevertheless, users do not appreciate surprise changes, so we better
  control them.  Also, the Windows reactivation spectre lurks.

Our general rule is to keep the guest ABI stable for released machine
types, and change it only in the latest, not-yet-released machine type.

To achieve this, we guard the change by a device property, which
defaults to the new behavior (your patch does that already).  We use
compat properties to flip the default to old behavior for released
machine types.

We occasionally make exceptions for sufficiently harmless guest-visible
changes.  If you think yours is, make your case in your commit message.

Example: USB device property "full-path"

hw/usb/bus.c has

    static Property usb_props[] = {
        DEFINE_PROP_STRING("port", USBDevice, port_path),
        DEFINE_PROP_STRING("serial", USBDevice, serial),
--->    DEFINE_PROP_BIT("full-path", USBDevice, flags,
--->                    USB_DEV_FLAG_FULL_PATH, true),
        DEFINE_PROP_BIT("msos-desc", USBDevice, flags,
                        USB_DEV_FLAG_MSOS_DESC_ENABLE, true),
        DEFINE_PROP_END_OF_LIST()
    };

The property is on by default.  We flip the default to off for machine
type pc-1.0 and older:

    static void pc_i440fx_1_0_machine_options(MachineClass *m)
    {
        static GlobalProperty compat[] = {
            PC_CPU_MODEL_IDS("1.0")
            { TYPE_ISA_FDC, "check_media_rate", "off" },
            { "virtio-balloon-pci", "class", stringify(PCI_CLASS_MEMORY_RAM) },
            { "apic-common", "vapic", "off" },
--->        { TYPE_USB_DEVICE, "full-path", "no" },
        };

        pc_i440fx_1_1_machine_options(m);
        m->hw_version = "1.0";
        compat_props_add(m->compat_props, compat, G_N_ELEMENTS(compat));
    }

Done in commit eeb0cf9abf "usb/vmstate: add parent dev path" (v1.1.0).

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Gerd Hoffmann]

On Wed, Nov 27, 2019 at 09:36:21AM +0200, Yuri Benditovich wrote:
> On Wed, Nov 27, 2019 at 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Yuri Benditovich <yuri.benditovich@daynix.com> writes:
> >
> > > If the redirected device has this capability, Windows guest may
> > > place the device into D2 and expect it to wake when the device
> > > becomes active, but this will never happen. For example, when
> > > internal Bluetooth adapter is redirected, keyboards and mice
> > > connected to it do not work. Setting global property
> > > 'usb-redir.nowake=off' keeps 'remote wake' as is.
> >
> > "usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
> > with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
> > name?  Naming is hard...  What about "usb-redir.wakeup=on"?
> '"wakeup" is good but "wakeup=on" makes an impression that we add the
> capability to the device even if it does not have one.
> disable_wake? suppress_wake? clear_wake? wake_allowed?

remote-wakeup=on,off ?

> > > +    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> >
> > The default is .nowake=on.  Is that a guest-visible change?

Yes, usb descriptors change, which the guest can see.

> And, of course, we can keep existing behavior by default and advise to
> turn this property on to make these devices work.

In that case a compat property would not be needed.

But, after all the question is whenever that is the best way to solve
the problem.  Most likely there is just a usb_wakeup() call missing
somewhere ...

cheers,
  Gerd

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Yuri Benditovich]

On Wed, Nov 27, 2019 at 11:40 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Nov 27, 2019 at 09:36:21AM +0200, Yuri Benditovich wrote:
> > On Wed, Nov 27, 2019 at 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > Yuri Benditovich <yuri.benditovich@daynix.com> writes:
> > >
> > > > If the redirected device has this capability, Windows guest may
> > > > place the device into D2 and expect it to wake when the device
> > > > becomes active, but this will never happen. For example, when
> > > > internal Bluetooth adapter is redirected, keyboards and mice
> > > > connected to it do not work. Setting global property
> > > > 'usb-redir.nowake=off' keeps 'remote wake' as is.
> > >
> > > "usb-redir.nowake=off" is a double negation.  Gets weirder when dusted
> > > with syntactic sugar: "usb-redir.nonowake".  Can we think of a better
> > > name?  Naming is hard...  What about "usb-redir.wakeup=on"?
> > '"wakeup" is good but "wakeup=on" makes an impression that we add the
> > capability to the device even if it does not have one.
> > disable_wake? suppress_wake? clear_wake? wake_allowed?
>
> remote-wakeup=on,off ?

This is like wakeup=on, suggesting that we turn wake on even if it is
not supported.
Anyway, I agree with any name.

>
> > > > +    DEFINE_PROP_BOOL("nowake", USBRedirDevice, suppress_remote_wake, true),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > >
> > > The default is .nowake=on.  Is that a guest-visible change?
>
> Yes, usb descriptors change, which the guest can see.
>
> > And, of course, we can keep existing behavior by default and advise to
> > turn this property on to make these devices work.
>
> In that case a compat property would not be needed.
>
> But, after all the question is whenever that is the best way to solve
> the problem.  Most likely there is just a usb_wakeup() call missing
> somewhere ...
>

Indeed, it would be good to call usb_wakeup(), but ... there is no
trigger to do that.

When the guest places the device to D2, it cancels all the urbs that
were pending, so there are no request that will be completed on spice
client side that can call usb_wakeup on qemu side.
The device on spice client side is powered up without any active
request so the device will not produce wake up event.
Usb-redir protocol, and client-side libusb and its kernel partner of
libusb (in case of Windows client - UsbDk/winusb) can't process such
flow correctly.
Of course, AFAIK.

Similar problem happens with local redirection, BTW. But this is for
another patch.

> cheers,
>   Gerd
>

Re: [PATCH] usbredir: remove 'remote wake' capability from configuration descriptor

[Gerd Hoffmann]

  Hi,

> Indeed, it would be good to call usb_wakeup(), but ... there is no
> trigger to do that.
> 
> When the guest places the device to D2, it cancels all the urbs that
> were pending, so there are no request that will be completed on spice
> client side that can call usb_wakeup on qemu side.
> The device on spice client side is powered up without any active
> request so the device will not produce wake up event.

Well, the device will produce a wakeup event for sure (unless it is
buggy), so the kernel will notice.  It might be that the kernel doesn't
forward the event to userspace though.

For "normal" usb applications the kernel can transparently handle
suspend+wakeup and just re-submit pending urbs to the hardware after
receiving a wakeup event, so powermanagement works automagically without
the app doing anything special.

For qemu + spice-client that approach doesn't work though, we need
explicit instead of automatic power management so we can properly
forward any events to the guest and let the guest deal with it.

Skimming the libusb docs it seems there is no support for that though.
So no easy way out.  Guess patching the descriptor is the best thing we
can do here.

> Similar problem happens with local redirection, BTW. But this is for
> another patch.

Can you make a small series for both?  I guess the actual descriptor
checking and patching is pretty simliar and can be handled by a shared
helper function.

thanks,
  Gerd