diff mbox

hw/usb/dev-hid: add a Mac guest compatibility option to usb-tablet

Message ID 1484749850-87206-1-git-send-email-phil@philjordan.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Phil Dennis-Jordan Jan. 18, 2017, 2:30 p.m. UTC
Darwin/OS X/macOS's HID driver stack does not correctly drive Qemu's simulated USB Tablet. This adds a boolean option "mac_compat" which subtly changes the device so it behaves in a way that Mac guests can handle.

The specific incompatibilities with the regular Qemu USB tablet are:

 1. Absolute pointing devices with HID Report Descriptor usage page of 0x01 (pointing) are handled by the macOS HID driver as analog sticks, so the movement of the cursor ends up being the cumulative deviance from the centre position.
 2. The bInterfaceProtocol of 0x02 enables a particular macOS HID driver mode which only works properly with mice (relative motion) not absolute pointing devices, so spurious events with relative coordinates are generated in addition to absolute ones. This manifests as a very jittery cursor.

The workaround is to report a usage page of 0x02 (mouse) and a bInterfaceProtocol value of 0x00.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>

---
 hw/usb/dev-hid.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 136 insertions(+), 2 deletions(-)

Comments

Gerd Hoffmann Jan. 19, 2017, 9:06 a.m. UTC | #1
On Mi, 2017-01-18 at 15:30 +0100, Phil Dennis-Jordan wrote:
> Darwin/OS X/macOS's HID driver stack does not correctly drive Qemu's
> simulated USB Tablet. This adds a boolean option "mac_compat" which
> subtly changes the device so it behaves in a way that Mac guests can
> handle.
> 
> The specific incompatibilities with the regular Qemu USB tablet are:
> 
>  1. Absolute pointing devices with HID Report Descriptor usage page of
> 0x01 (pointing) are handled by the macOS HID driver as analog sticks,
> so the movement of the cursor ends up being the cumulative deviance
> from the centre position.
>  2. The bInterfaceProtocol of 0x02 enables a particular macOS HID
> driver mode which only works properly with mice (relative motion) not
> absolute pointing devices, so spurious events with relative
> coordinates are generated in addition to absolute ones. This manifests
> as a very jittery cursor.
> 
> The workaround is to report a usage page of 0x02 (mouse) and a
> bInterfaceProtocol value of 0x00.

Waded through the specs.  Setting bInterfaceProtocol to 0x00 should be
done anyway.  Only boot protocol devices (which the tablet isn't) should
set this to 1 (kbd) or 2 (mouse).  No need to hide that behind a macos
option, we can do that for everybody.

The other one is more tricky.  Declaring the device as "mouse" is
clearly wrong according to the specs.  There is no explicit "tablet"
device.  "pointing" comes closest.

Does the issue still show up with bInterfaceProtocol being fixed?

cheers,
  Gerd
Phil Dennis-Jordan Jan. 19, 2017, 1:27 p.m. UTC | #2
Thanks for looking into this, Gerd.

On 19 January 2017 at 10:06, Gerd Hoffmann <kraxel@redhat.com> wrote:

> Waded through the specs.  Setting bInterfaceProtocol to 0x00 should be
> done anyway.  Only boot protocol devices (which the tablet isn't) should
> set this to 1 (kbd) or 2 (mouse).  No need to hide that behind a macos
> option, we can do that for everybody.
>

OK, that shortens the patch considerably!


> The other one is more tricky.  Declaring the device as "mouse" is
> clearly wrong according to the specs.  There is no explicit "tablet"
> device.  "pointing" comes closest.
>


> Does the issue still show up with bInterfaceProtocol being fixed?
>

I just tested it with macOS 10.12, and yes, unfortunately it does. Or
rather, the cursor now zips off (I think) the bottom right corner of the
screen as soon as you try to move it for the first time, never to be seen
again. I don't know if the "mouse" usage is strictly correct for a tablet;
the spec does appear somewhat ambiguous on this - none of the examples it
gives for "pointer" usage seem to match Qemu's usb-tablet. It would appear
that OSX/macOS follows the following sentence from chapter 2's overview:

"For example, a gaming device driver might look for Joystick and Game Pad
usages, while a system mouse driver might look for Mouse, Digitizer Tablet
and Touch Screen usages."

Note that "Pointer" usage is not listed for the system mouse driver; I'm
not sure Digitizer Tablet and Touch Screen are any more accurate though,
considering we have more mouse-like button events.

I do know for certain that VMWare also uses 0x02 for its absolute pointing
device. WinXP, Win7, and Win10 certainly appear to have no problem with
mac_compat from my patch enabled. I haven't checked other windowing systems
yet.

(So far I've been hacking around this issue with a special guest driver,
https://github.com/pmj/QemuUSBTablet-OSX but having a workaround in
mainline Qemu would be much more convenient, particularly as it'll work
during OS install etc.)

Any thoughts on how to proceed?

Thanks,
Phil
diff mbox

Patch

diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index 24d05f7..0f5b796 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -51,6 +51,7 @@  typedef struct USBHIDState {
     uint32_t usb_version;
     char *display;
     uint32_t head;
+    bool mac_compat;
 } USBHIDState;
 
 #define TYPE_USB_HID "usb-hid"
@@ -200,6 +201,66 @@  static const USBDescIface desc_iface_tablet2 = {
     },
 };
 
+static const USBDescIface desc_iface_tablet_mac_compat = {
+    .bInterfaceNumber              = 0,
+    .bNumEndpoints                 = 1,
+    .bInterfaceClass               = USB_CLASS_HID,
+    .bInterfaceProtocol            = 0x00, /* OSX/macOS can't handle 2 here */
+    .ndesc                         = 1,
+    .descs = (USBDescOther[]) {
+        {
+            /* HID descriptor */
+            .data = (uint8_t[]) {
+                0x09,          /*  u8  bLength */
+                USB_DT_HID,    /*  u8  bDescriptorType */
+                0x01, 0x00,    /*  u16 HID_class */
+                0x00,          /*  u8  country_code */
+                0x01,          /*  u8  num_descriptors */
+                USB_DT_REPORT, /*  u8  type: Report */
+                74, 0,         /*  u16 len */
+            },
+        },
+    },
+    .eps = (USBDescEndpoint[]) {
+        {
+            .bEndpointAddress      = USB_DIR_IN | 0x01,
+            .bmAttributes          = USB_ENDPOINT_XFER_INT,
+            .wMaxPacketSize        = 8,
+            .bInterval             = 0x0a,
+        },
+    },
+};
+
+static const USBDescIface desc_iface_tablet_mac_compat2 = {
+    .bInterfaceNumber              = 0,
+    .bNumEndpoints                 = 1,
+    .bInterfaceClass               = USB_CLASS_HID,
+    .bInterfaceProtocol            = 0x00, /* OSX/macOS can't handle 2 here */
+    .ndesc                         = 1,
+    .descs = (USBDescOther[]) {
+        {
+            /* HID descriptor */
+            .data = (uint8_t[]) {
+                0x09,          /*  u8  bLength */
+                USB_DT_HID,    /*  u8  bDescriptorType */
+                0x01, 0x00,    /*  u16 HID_class */
+                0x00,          /*  u8  country_code */
+                0x01,          /*  u8  num_descriptors */
+                USB_DT_REPORT, /*  u8  type: Report */
+                74, 0,         /*  u16 len */
+            },
+        },
+    },
+    .eps = (USBDescEndpoint[]) {
+        {
+            .bEndpointAddress      = USB_DIR_IN | 0x01,
+            .bmAttributes          = USB_ENDPOINT_XFER_INT,
+            .wMaxPacketSize        = 8,
+            .bInterval             = 4, /* 2 ^ (4-1) * 125 usecs = 1 ms */
+        },
+    },
+};
+
 static const USBDescIface desc_iface_keyboard = {
     .bInterfaceNumber              = 0,
     .bNumEndpoints                 = 1,
@@ -330,6 +391,40 @@  static const USBDescDevice desc_device_tablet2 = {
     },
 };
 
+static const USBDescDevice desc_device_tablet_mac_compat = {
+    .bcdUSB                        = 0x0100,
+    .bMaxPacketSize0               = 8,
+    .bNumConfigurations            = 1,
+    .confs = (USBDescConfig[]) {
+        {
+            .bNumInterfaces        = 1,
+            .bConfigurationValue   = 1,
+            .iConfiguration        = STR_CONFIG_TABLET,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
+            .bMaxPower             = 50,
+            .nif = 1,
+            .ifs = &desc_iface_tablet_mac_compat,
+        },
+    },
+};
+
+static const USBDescDevice desc_device_tablet_mac_compat2 = {
+    .bcdUSB                        = 0x0200,
+    .bMaxPacketSize0               = 64,
+    .bNumConfigurations            = 1,
+    .confs = (USBDescConfig[]) {
+        {
+            .bNumInterfaces        = 1,
+            .bConfigurationValue   = 1,
+            .iConfiguration        = STR_CONFIG_TABLET,
+            .bmAttributes          = USB_CFG_ATT_ONE | USB_CFG_ATT_WAKEUP,
+            .bMaxPower             = 50,
+            .nif = 1,
+            .ifs = &desc_iface_tablet_mac_compat2,
+        },
+    },
+};
+
 static const USBDescDevice desc_device_keyboard = {
     .bcdUSB                        = 0x0100,
     .bMaxPacketSize0               = 8,
@@ -426,6 +521,35 @@  static const USBDesc desc_tablet2 = {
     .msos = &desc_msos_suspend,
 };
 
+static const USBDesc desc_tablet_mac_compat = {
+    .id = {
+        .idVendor          = 0x0627,
+        .idProduct         = 0x0002,
+        .bcdDevice         = 0,
+        .iManufacturer     = STR_MANUFACTURER,
+        .iProduct          = STR_PRODUCT_TABLET,
+        .iSerialNumber     = STR_SERIALNUMBER,
+    },
+    .full = &desc_device_tablet_mac_compat,
+    .str  = desc_strings,
+    .msos = &desc_msos_suspend,
+};
+
+static const USBDesc desc_tablet_mac_compat2 = {
+    .id = {
+        .idVendor          = 0x0627,
+        .idProduct         = 0x0002,
+        .bcdDevice         = 0,
+        .iManufacturer     = STR_MANUFACTURER,
+        .iProduct          = STR_PRODUCT_TABLET,
+        .iSerialNumber     = STR_SERIALNUMBER,
+    },
+    .full = &desc_device_tablet_mac_compat,
+    .high = &desc_device_tablet_mac_compat2,
+    .str  = desc_strings,
+    .msos = &desc_msos_suspend,
+};
+
 static const USBDesc desc_keyboard = {
     .id = {
         .idVendor          = 0x0627,
@@ -599,6 +723,9 @@  static void usb_hid_handle_control(USBDevice *dev, USBPacket *p,
                 memcpy(data, qemu_tablet_hid_report_descriptor,
 		       sizeof(qemu_tablet_hid_report_descriptor));
                 p->actual_length = sizeof(qemu_tablet_hid_report_descriptor);
+                if (us->mac_compat) {
+                    data[3] = 0x02; /* Set usage to mouse, not pointing (1) */
+                }
             } else if (hs->kind == HID_KEYBOARD) {
                 memcpy(data, qemu_keyboard_hid_report_descriptor,
                        sizeof(qemu_keyboard_hid_report_descriptor));
@@ -731,8 +858,14 @@  static void usb_hid_initfn(USBDevice *dev, int kind,
 
 static void usb_tablet_realize(USBDevice *dev, Error **errp)
 {
-
-    usb_hid_initfn(dev, HID_TABLET, &desc_tablet, &desc_tablet2, errp);
+    USBHIDState *us = USB_HID(dev);
+    if (us->mac_compat) {
+        usb_hid_initfn(
+            dev, HID_TABLET,
+            &desc_tablet_mac_compat, &desc_tablet_mac_compat2, errp);
+    } else {
+        usb_hid_initfn(dev, HID_TABLET, &desc_tablet, &desc_tablet2, errp);
+    }
 }
 
 static void usb_mouse_realize(USBDevice *dev, Error **errp)
@@ -801,6 +934,7 @@  static Property usb_tablet_properties[] = {
         DEFINE_PROP_UINT32("usb_version", USBHIDState, usb_version, 2),
         DEFINE_PROP_STRING("display", USBHIDState, display),
         DEFINE_PROP_UINT32("head", USBHIDState, head, 0),
+        DEFINE_PROP_BOOL("mac_compat", USBHIDState, mac_compat, false),
         DEFINE_PROP_END_OF_LIST(),
 };