[PATCH 0/3] CanoKey: Fix xHCI compatibility and CCID ZLP

[PATCH 0/3] CanoKey: Fix xHCI compatibility and CCID ZLP

[Hongren (Zenithal) Zheng]

In patch v5 [1] of Introduce CanoKey QEMU I said that canokey-qemu
was incompatible with qemu-xhci.

kraxel kindly suggested[2] that it should be the problem of usb_wakeup
So I fixed it in this patch set.

Now that the v5 patch has been in the process of git PULL [3],
I think it would be better to post a new patch set instead
of sending out v6, which would make maintainer's tree back and forth.

This patch should be applied after [1].

As for the CCID ZLP issue, it is described in the comment and commit
message.

I added a commit in https://github.com/canokeys/canokey-qemu
to export the EP num in the header, so hw/usb/canokey.c in qemu
could use it for CTAPHID quirks. If you want to compile
this version when --enable-canokey, make sure to use the latest
libcanokey-qemu.so

The CI result for this PATCH is at [4]. The failure for
amd64-debian-container seems irrelevent to this patchset.

[1] https://lore.kernel.org/qemu-devel/YoY5k0PQny8WtAHi@Sun/
[2] https://lore.kernel.org/qemu-devel/20220609095659.ulgk64bx3nlqzs2k@sirius.home.kraxel.org/
[3] https://lore.kernel.org/qemu-devel/20220610092043.1874654-1-kraxel@redhat.com/
[4] https://gitlab.com/ZenithalHourlyRate/qemu/-/pipelines/561801062

Hongren (Zenithal) Zheng (3):
  hw/usb/canokey: Fix CCID ZLP
  hw/usb/canokey: fix compatibility of qemu-xhci
  docs/system/devices/usb/canokey: remove limitations on qemu-xhci

 docs/system/devices/canokey.rst | 10 ----------
 hw/usb/canokey.c                | 35 +++++++++++++++++++++++++++++----
 hw/usb/canokey.h                |  1 +
 3 files changed, 32 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH 1/3] hw/usb/canokey: Fix CCID ZLP

[Hongren (Zenithal) Zheng]

CCID could send zero-length packet (ZLP)
if we invoke two data_in, two packets would be concated
and we could not distinguish them.

The CANOKEY_EMU_EP_CTAPHID is exported from canokey-qemu.h

Reported-by: MkfsSion <myychina28759@gmail.com>
Signed-off-by: Hongren (Zenithal) Zheng <i@zenithal.me>
---
 hw/usb/canokey.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/usb/canokey.c b/hw/usb/canokey.c
index 4a08b1cbd7..86548923eb 100644
--- a/hw/usb/canokey.c
+++ b/hw/usb/canokey.c
@@ -109,11 +109,10 @@ int canokey_emu_transmit(
      * Note: this is a quirk for CanoKey CTAPHID
      * because it calls multiple emu_transmit in one device_loop
      * but w/o data_in it would stuck in device_loop
-     * This has no side effect for CCID as it is strictly
-     * OUT then IN transfer
-     * However it has side effect for Control transfer
+     * This has side effect for CCID since CCID can send ZLP
+     * This also has side effect for Control transfer
      */
-    if (ep_in != 0) {
+    if (ep_in == CANOKEY_EMU_EP_CTAPHID) {
         canokey_emu_data_in(ep_in);
     }
     return 0;
-- 
2.35.1

[PATCH 2/3] hw/usb/canokey: fix compatibility of qemu-xhci

[Hongren (Zenithal) Zheng]

XHCI wont poll interrupt IN endpoint if NAKed, and needs wakeup

Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Hongren (Zenithal) Zheng <i@zenithal.me>
---
 hw/usb/canokey.c | 28 ++++++++++++++++++++++++++++
 hw/usb/canokey.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/hw/usb/canokey.c b/hw/usb/canokey.c
index 86548923eb..e5fa4a5ad2 100644
--- a/hw/usb/canokey.c
+++ b/hw/usb/canokey.c
@@ -103,6 +103,13 @@ int canokey_emu_transmit(
             pbuf, size);
     key->ep_in_size[ep_in] += size;
     key->ep_in_state[ep_in] = CANOKEY_EP_IN_READY;
+    /*
+     * wake up controller if we NAKed IN token before
+     * Note: this is a quirk for CanoKey CTAPHID
+     */
+    if (ep_in == CANOKEY_EMU_EP_CTAPHID &&
+            key->ep_in_pointer[ep_in] != NULL)
+        usb_wakeup(key->ep_in_pointer[ep_in], 0);
     /*
      * ready for more data in device loop
      *
@@ -135,6 +142,7 @@ static void canokey_handle_reset(USBDevice *dev)
         key->ep_in_state[i] = CANOKEY_EP_IN_WAIT;
         key->ep_in_pos[i] = 0;
         key->ep_in_size[i] = 0;
+        key->ep_in_pointer[i] = NULL;
     }
     canokey_emu_reset();
 }
@@ -163,6 +171,8 @@ static void canokey_handle_control(USBDevice *dev, USBPacket *p,
     switch (key->ep_in_state[0]) {
     case CANOKEY_EP_IN_WAIT:
         p->status = USB_RET_NAK;
+        /* store pointer here for later emu_transmit wakeup */
+        key->ep_in_pointer[0] = p->ep;
         break;
     case CANOKEY_EP_IN_STALL:
         p->status = USB_RET_STALL;
@@ -208,6 +218,22 @@ static void canokey_handle_data(USBDevice *dev, USBPacket *p)
             key->ep_out_size[ep_out] = out_len;
             canokey_emu_data_out(ep_out, NULL);
         }
+        /*
+         * Note: this is a quirk for CanoKey CTAPHID
+         *
+         * There is one code path that uses this device loop
+         * INTR IN -> useful data_in and useless device_loop -> NAKed
+         * INTR OUT -> useful device loop -> transmit -> wakeup
+         *   (this one thanks to both data_in and data_out being called)
+         * the next INTR IN -> actual data to guest
+         *
+         * if there is no such device loop, there would be no further
+         * INTR IN, no device loop, no transmit hence no usb_wakeup
+         * then qemu would hang
+         */
+        if (ep_in == CANOKEY_EMU_EP_CTAPHID) {
+            canokey_emu_device_loop(); /* may call transmit multiple times */
+        }
         break;
     case USB_TOKEN_IN:
         if (key->ep_in_pos[ep_in] == 0) { /* first time IN */
@@ -218,6 +244,8 @@ static void canokey_handle_data(USBDevice *dev, USBPacket *p)
         case CANOKEY_EP_IN_WAIT:
             /* NAK for early INTR IN */
             p->status = USB_RET_NAK;
+            /* store pointer here for later emu_transmit wakeup */
+            key->ep_in_pointer[ep_in] = p->ep;
             break;
         case CANOKEY_EP_IN_STALL:
             p->status = USB_RET_STALL;
diff --git a/hw/usb/canokey.h b/hw/usb/canokey.h
index 24cf304203..7261f81e80 100644
--- a/hw/usb/canokey.h
+++ b/hw/usb/canokey.h
@@ -55,6 +55,7 @@ typedef struct CanoKeyState {
      */
     uint32_t ep_in_pos[CANOKEY_EP_NUM];
     CanoKeyEPState ep_in_state[CANOKEY_EP_NUM];
+    USBEndpoint *ep_in_pointer[CANOKEY_EP_NUM];
 
     /* OUT pointer to canokey recv buffer */
     uint8_t *ep_out[CANOKEY_EP_NUM];
-- 
2.35.1

[PATCH 3/3] docs/system/devices/usb/canokey: remove limitations on qemu-xhci

[Hongren (Zenithal) Zheng]

Signed-off-by: Hongren (Zenithal) Zheng <i@zenithal.me>
---
 docs/system/devices/canokey.rst | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/docs/system/devices/canokey.rst b/docs/system/devices/canokey.rst
index 169f99b8eb..c2c58ae3e7 100644
--- a/docs/system/devices/canokey.rst
+++ b/docs/system/devices/canokey.rst
@@ -146,16 +146,6 @@ multiple CanoKey QEMU running, namely you can not
 Also, there is no lock on canokey-file, thus two CanoKey QEMU instance
 can not read one canokey-file at the same time.
 
-Another limitation is that this device is not compatible with ``qemu-xhci``,
-in that this device would hang when there are FIDO2 packets (traffic on
-interrupt endpoints). If you do not use FIDO2 then it works as intended,
-but for full functionality you should use old uhci/ehci bus and attach canokey
-to it, for example
-
-.. parsed-literal::
-
-   |qemu_system| -device piix3-usb-uhci,id=uhci -device canokey,bus=uhci.0
-
 References
 ==========
 
-- 
2.35.1

Re: [PATCH 2/3] hw/usb/canokey: fix compatibility of qemu-xhci

[Gerd Hoffmann]

  Hi,

>          case CANOKEY_EP_IN_WAIT:
>              /* NAK for early INTR IN */
>              p->status = USB_RET_NAK;
> +            /* store pointer here for later emu_transmit wakeup */
> +            key->ep_in_pointer[ep_in] = p->ep;

There is no need to fish the ep pointer out of usb packets.
You can just use usb_ep_get() instead.

take care,
  Gerd