[Qemu-devel] [PATCH 06/20] usb-ccid: review fixes (v15-v16)

[Alon Levy]

I'll fold it before submitting the version to be applied, but
I hope keeping it as a separate patch will make reviewing easier.

Behavioral changes:
 * fix abort on client answer after card remove
 * enable migration
 * remove side affect code from asserts
 * return consistent self-powered state
 * mask out reserved bits in ccid_set_parameters
 * add missing abRFU in SetParameters (no affect on linux guest)

whitefixes / comments / consts defines:
 * remove stale comment
 * remove ccid_print_pending_answers if no DEBUG_CCID
 * replace printf's with DPRINTF, remove DEBUG_CCID, add verbosity defines
 * use error_report
 * update copyright (most of the code is not original)
 * reword known bug comment
 * add missing closing quote in comment
 * add missing whitespace on one line
 * s/CCID_SetParameter/CCID_SetParameters/
 * add comments
 * use define for max packet size

Comment for "return consistent self-powered state":

the Configuration Descriptor bmAttributes claims we are self powered,
but we were returning not self powered to USB_REQ_GET_STATUS control message.

In practice, this message is not sent by a linux 2.6.35.10-74.fc14.x86_64
guest (not tested on other guests), unless you issue lsusb -v as root (for
example).

Signed-off-by: Alon Levy <address@hidden>
---
 hw/ccid.h     |    7 +++
 hw/usb-ccid.c |  115 ++++++++++++++++++++++++++++-----------------------------
 2 files changed, 63 insertions(+), 59 deletions(-)

diff --git a/hw/ccid.h b/hw/ccid.h
index af59070..f5dcfae 100644
--- a/hw/ccid.h
+++ b/hw/ccid.h
@@ -6,11 +6,16 @@
 typedef struct CCIDCardState CCIDCardState;
 typedef struct CCIDCardInfo CCIDCardInfo;
 
+/* state of the CCID Card device (i.e. hw/ccid-card-*.c)
+ */
 struct CCIDCardState {
     DeviceState qdev;
     uint32_t    slot; // For future use with multiple slot reader.
 };
 
+/* callbacks to be used by the CCID device (hw/usb-ccid.c) to call
+ * into the smartcard device (hw/ccid-card-*.c)
+ */
 struct CCIDCardInfo {
     DeviceInfo qdev;
     void (*print)(Monitor *mon, CCIDCardState *card, int indent);
@@ -20,6 +25,8 @@ struct CCIDCardInfo {
     int (*initfn)(CCIDCardState *card);
 };
 
+/* API for smartcard calling the CCID device (used by hw/ccid-card-*.c)
+ */
 void ccid_card_send_apdu_to_guest(CCIDCardState *card, uint8_t* apdu, uint32_t 
len);
 void ccid_card_card_removed(CCIDCardState *card);
 void ccid_card_card_inserted(CCIDCardState *card);
diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
index 58f69a6..09e39ac 100644
--- a/hw/usb-ccid.c
+++ b/hw/usb-ccid.c
@@ -1,15 +1,18 @@
 /*
  * CCID Device emulation
  *
- * Based on usb-serial.c:
+ * Written by Alon Levy, with contributions from Robert Relyea.
+ *
+ * Based on usb-serial.c, see it's copyright and attributions below.
+ *
+ * This code is licenced under the GNU LGPL, version 2 or later.
+ *
+ * -------
+ *
+ * usb-serial.c copyright and attribution:
  * Copyright (c) 2006 CodeSourcery.
  * Copyright (c) 2008 Samuel Thibault <address@hidden>
  * Written by Paul Brook, reused for FTDI by Samuel Thibault,
- * Reused for CCID by Alon Levy.
- * Contributed to by Robert Relyea
- * Copyright (c) 2010 Red Hat.
- *
- * This code is licenced under the LGPL.
  */
 
 /* References:
@@ -19,22 +22,16 @@
  *  Specification for Integrated Circuit(s) Cards Interface Devices
  *
  * Endianess note: from the spec (1.3)
- *  "Fields that are larger than a byte are stored in little endian
+ *  "Fields that are larger than a byte are stored in little endian"
  *
  * KNOWN BUGS
  * 1. remove/insert can sometimes result in removed state instead of inserted.
  * This is a result of the following:
- *  symptom: dmesg shows ERMOTEIO (-121), pcscd shows -99. This happens
- *  when we send a too short packet, seen in uhci-usb.c, resulting from
- *  a urb requesting SPD and us returning a smaller packet.
+ *  symptom: dmesg shows ERMOTEIO (-121), pcscd shows -99. This can happen
+ *  when a short packet is sent, as seen in uhci-usb.c, resulting from a urb
+ *  from the guest requesting SPD and us returning a smaller packet.
  *  Not sure which messages trigger this.
  *
- * Migration note:
- *
- * All the VMStateDescription's are left here for future use, but
- * not enabled right now since there is no support for USB migration.
- *
- * To enable define ENABLE_MIGRATION
  */
 
 #include "qemu-common.h"
@@ -44,11 +41,14 @@
 
 #include "hw/ccid.h"
 
-//#define DEBUG_CCID
-
 #define DPRINTF(s, lvl, fmt, ...) \
 do { if (lvl <= s->debug) { printf("usb-ccid: " fmt , ## __VA_ARGS__); } } 
while (0)
 
+#define D_WARN 1
+#define D_INFO 2
+#define D_MORE_INFO 3
+#define D_VERBOSE 4
+
 #define CCID_DEV_NAME "usb-ccid"
 
 /* The two options for variable sized buffers:
@@ -64,6 +64,8 @@ do { if (lvl <= s->debug) { printf("usb-ccid: " fmt , ## 
__VA_ARGS__); } } while
 #define InterfaceOutClass    
((USB_DIR_OUT|USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
 #define InterfaceInClass     ((USB_DIR_IN 
|USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
 
+#define CCID_MAX_PACKET_SIZE                64
+
 #define CCID_CONTROL_ABORT                  0x1
 #define CCID_CONTROL_GET_CLOCK_FREQUENCIES  0x2
 #define CCID_CONTROL_GET_DATA_RATES         0x3
@@ -209,11 +211,12 @@ typedef struct __attribute__ ((__packed__)) 
CCID_IccPowerOff {
     uint16_t    abRFU;
 } CCID_IccPowerOff;
 
-typedef struct __attribute__ ((__packed__)) CCID_SetParameter {
+typedef struct __attribute__ ((__packed__)) CCID_SetParameters {
     CCID_Header hdr;
     uint8_t     bProtocolNum;
+    uint16_t   abRFU;
     uint8_t    abProtocolDataStructure[0];
-} CCID_SetParameter;
+} CCID_SetParameters;
 
 typedef struct CCID_Notify_Slot_Change {
     uint8_t     bMessageType; /* CCID_MESSAGE_TYPE_RDR_to_PC_NotifySlotChange 
*/
@@ -278,10 +281,6 @@ struct USBCCIDState {
     uint16_t migration_target_port;
 };
 
-/* Slot specific variables. We emulate a single slot card reader.
- */
-
-
 /* CCID Spec chapter 4: CCID uses a standard device descriptor per Chapter 9,
  * "USB Device Framework", section 9.6.1, in the Universal Serial Bus
  * Specification.
@@ -416,7 +415,8 @@ static const uint8_t qemu_ccid_config_descriptor[] = {
                     /*  u8  ep_bEndpointAddress; IN Endpoint 1 */
         0x80 | CCID_INT_IN_EP,
         0x03,       /*  u8  ep_bmAttributes; Interrupt */
-        0x40, 0x00, /*  u16 ep_wMaxPacketSize; */
+                    /*  u16 ep_wMaxPacketSize; */
+        CCID_MAX_PACKET_SIZE & 0xff, (CCID_MAX_PACKET_SIZE >> 8),
         0xff,       /*  u8  ep_bInterval; */
 
         /* Bulk-In endpoint */
@@ -455,32 +455,31 @@ static void ccid_clear_pending_answers(USBCCIDState *s)
 
 static void ccid_print_pending_answers(USBCCIDState *s)
 {
-#ifdef DEBUG_CCID
     Answer *answer;
     int i, count;
 
-    printf("usb-ccid: pending answers:");
+    DPRINTF(s, D_VERBOSE, "usb-ccid: pending answers:");
     if (!ccid_has_pending_answers(s)) {
-        printf(" empty\n");
+        DPRINTF(s, D_VERBOSE, " empty\n");
         return;
     }
     for (i = s->pending_answers_start, count=s->pending_answers_num ;
          count > 0; count--, i++) {
         answer = &s->pending_answers[i % PENDING_ANSWERS_NUM];
         if (count == 1) {
-            printf("%d:%d\n", answer->slot, answer->seq);
+            DPRINTF(s, D_VERBOSE, "%d:%d\n", answer->slot, answer->seq);
         } else {
-            printf("%d:%d,", answer->slot, answer->seq);
+            DPRINTF(s, D_VERBOSE, "%d:%d,", answer->slot, answer->seq);
         }
     }
-#endif
 }
 
 static void ccid_add_pending_answer(USBCCIDState *s, CCID_Header *hdr)
 {
     Answer* answer;
 
-    assert(s->pending_answers_num++ < PENDING_ANSWERS_NUM);
+    assert(s->pending_answers_num < PENDING_ANSWERS_NUM);
+    s->pending_answers_num++;
     answer = &s->pending_answers[(s->pending_answers_end++) % 
PENDING_ANSWERS_NUM];
     answer->slot = hdr->bSlot;
     answer->seq = hdr->bSeq;
@@ -492,7 +491,8 @@ static void ccid_remove_pending_answer(USBCCIDState *s,
 {
     Answer *answer;
 
-    assert(s->pending_answers_num-- > 0);
+    assert(s->pending_answers_num > 0);
+    s->pending_answers_num--;
     answer = &s->pending_answers[(s->pending_answers_start++) % 
PENDING_ANSWERS_NUM];
     *slot = answer->slot;
     *seq = answer->seq;
@@ -528,16 +528,16 @@ static void* ccid_reserve_recv_buf(USBCCIDState* s, 
uint16_t len)
 {
     BulkIn* bulk_in;
 
-    DPRINTF(s, 4, "%s: QUEUE: reserve %d bytes\n", __func__, len);
+    DPRINTF(s, D_VERBOSE, "%s: QUEUE: reserve %d bytes\n", __func__, len);
 
     /* look for an existing element */
     if (len > BULK_IN_BUF_SIZE) {
-        printf("usb-ccid.c: %s: len larger then max (%d>%d). discarding 
message.\n",
+        DPRINTF(s, D_WARN, "usb-ccid.c: %s: len larger then max (%d>%d). 
discarding message.\n",
             __func__, len, BULK_IN_BUF_SIZE);
         return NULL;
     }
     if (s->bulk_in_pending_num >= BULK_IN_PENDING_NUM) {
-        printf("usb-ccid.c: %s: No free bulk_in buffers. discarding 
message.\n",
+        DPRINTF(s, D_WARN, "usb-ccid.c: %s: No free bulk_in buffers. 
discarding message.\n",
                 __func__);
         return NULL;
     }
@@ -576,7 +576,7 @@ static int ccid_handle_control(USBDevice *dev, int request, 
int value,
     DPRINTF(s, 1, "got control %x, value %x\n",request, value);
     switch (request) {
     case DeviceRequest | USB_REQ_GET_STATUS:
-        data[0] = (0 << USB_DEVICE_SELF_POWERED) |
+        data[0] = (1 << USB_DEVICE_SELF_POWERED) |
             (dev->remote_wakeup << USB_DEVICE_REMOTE_WAKEUP);
         data[1] = 0x00;
         ret = 2;
@@ -709,7 +709,7 @@ static uint8_t ccid_calc_status(USBCCIDState *s)
        bmCommandStatus
      */
     uint8_t ret = ccid_card_status(s) | (s->bmCommandStatus << 6);
-    DPRINTF(s, 4, "status = %d\n", ret);
+    DPRINTF(s, D_VERBOSE, "status = %d\n", ret);
     return ret;
 }
 
@@ -770,11 +770,9 @@ static void ccid_write_data_block(
     p->b.hdr.bSeq = seq;
     p->b.bStatus = ccid_calc_status(s);
     p->b.bError = s->bError;
-#ifdef DEBUG_CCID
     if (p->b.bError) {
-        DPRINTF(s, 4, "error %d", p->b.bError);
+        DPRINTF(s, D_VERBOSE, "error %d", p->b.bError);
     }
-#endif
     memcpy(p->abData, data, len);
     ccid_reset_error_status(s);
 }
@@ -805,12 +803,12 @@ static void ccid_write_data_block_atr(USBCCIDState* s, 
CCID_Header* recv)
 
 static void ccid_set_parameters(USBCCIDState *s, CCID_Header *recv)
 {
-    CCID_SetParameter *ph = (CCID_SetParameter *) recv;
+    CCID_SetParameters *ph = (CCID_SetParameters *) recv;
     uint32_t len = 0;
-    if (ph->bProtocolNum == 0) {
+    if ((ph->bProtocolNum & 3) == 0) {
         len = 5;
     }
-    if (ph->bProtocolNum == 1) {
+    if ((ph->bProtocolNum & 3) == 1) {
         len = 7;
     }
     if (len == 0) {
@@ -884,7 +882,7 @@ static void ccid_on_apdu_from_guest(USBCCIDState *s, 
CCID_XferBlock *recv)
     if (s->card) {
         s->cardinfo->apdu_from_guest(s->card, recv->abData, len);
     } else {
-        printf("warning: discarded apdu\n");
+        DPRINTF(s, D_WARN, "warning: discarded apdu\n");
     }
 }
 
@@ -901,15 +899,15 @@ static int ccid_handle_bulk_out(USBCCIDState *s, 
USBPacket *p)
     ccid_header = (CCID_Header*)s->bulk_out_data;
     memcpy(s->bulk_out_data + s->bulk_out_pos, p->data, p->len);
     s->bulk_out_pos += p->len;
-    if (p->len == 64) {
-        DPRINTF(s, 4, "usb-ccid: bulk_in: expecting more packets (%d/%d)\n",
+    if (p->len == CCID_MAX_PACKET_SIZE) {
+        DPRINTF(s, D_VERBOSE, "usb-ccid: bulk_in: expecting more packets 
(%d/%d)\n",
             p->len, ccid_header->dwLength);
         return 0;
     }
     if (s->bulk_out_pos < 10) {
         DPRINTF(s, 1, "%s: bad USB_TOKEN_OUT length, should be at least 10 
bytes\n", __func__);
     } else {
-        DPRINTF(s, 3, "%s %x\n", __func__, ccid_header->bMessageType);
+        DPRINTF(s, D_MORE_INFO, "%s %x\n", __func__, 
ccid_header->bMessageType);
         switch (ccid_header->bMessageType) {
             case CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus:
                 ccid_write_slot_status(s, ccid_header);
@@ -965,7 +963,7 @@ static int ccid_bulk_in_copy_to_guest(USBCCIDState *s, 
uint8_t *data, int len)
 {
     int ret = 0;
 
-    assert(len>0);
+    assert(len > 0);
     ccid_bulk_in_get(s);
     if (s->current_bulk_in != NULL) {
         ret = MIN(s->current_bulk_in->len - s->current_bulk_in->pos, len);
@@ -978,7 +976,7 @@ static int ccid_bulk_in_copy_to_guest(USBCCIDState *s, 
uint8_t *data, int len)
         ret = USB_RET_NAK; /* return when device has no data - usb 2.0 spec 
Table 8-4 */
     }
     if (ret > 0) {
-        DPRINTF(s, 3, "%s: %d/%d req/act to guest (BULK_IN)\n", __func__, len, 
ret);
+        DPRINTF(s, D_MORE_INFO, "%s: %d/%d req/act to guest (BULK_IN)\n", 
__func__, len, ret);
     }
     if (ret != USB_RET_NAK && ret < len) {
         DPRINTF(s, 1, "%s: returning short (EREMOTEIO) %d < %d\n", __func__, 
ret, len);
@@ -1015,7 +1013,7 @@ static int ccid_handle_data(USBDevice *dev, USBPacket *p)
                     ret = 2;
                     s->notify_slot_change = false;
                     s->bmSlotICCState &= ~SLOT_0_CHANGED_MASK;
-                    DPRINTF(s, 2, "handle_data: int_in: notify_slot_change %X, 
requested len %d\n",
+                    DPRINTF(s, D_INFO, "handle_data: int_in: 
notify_slot_change %X, requested len %d\n",
                             s->bmSlotICCState, len);
                 }
                 break;
@@ -1146,7 +1144,11 @@ void ccid_card_card_error(CCIDCardState *card, uint64_t 
error)
     DPRINTF(s, 1, "VSC_Error: %lX\n", s->last_answer_error);
     /* TODO: these error's should be more verbose and propogated to the guest.
      * */
-    ccid_write_data_block_answer(s, NULL, 0);
+    /* we flush all pending answers on CardRemove message in 
ccid-card-passthru,
+     * so check that first to not trigger abort */
+    if (ccid_has_pending_answers(s)) {
+        ccid_write_data_block_answer(s, NULL, 0);
+    }
 }
 
 void ccid_card_card_inserted(CCIDCardState *card)
@@ -1184,12 +1186,12 @@ static int ccid_card_init(DeviceState *qdev, DeviceInfo 
*base)
     int ret = 0;
 
     if (card->slot != 0) {
-        fprintf(stderr, "Warning: usb-ccid supports one slot, can't add %d",
+        error_report("Warning: usb-ccid supports one slot, can't add %d",
                 card->slot);
         return -1;
     }
     if (s->card != NULL) {
-        fprintf(stderr, "Warning: usb-ccid card already full, not adding\n");
+        error_report("Warning: usb-ccid card already full, not adding\n");
         return -1;
     }
     ret = info->initfn ? info->initfn(card) : ret;
@@ -1233,7 +1235,6 @@ static int ccid_initfn(USBDevice *dev)
     return 0;
 }
 
-#ifdef ENABLE_MIGRATION
 static int ccid_post_load(void *opaque, int version_id)
 {
     USBCCIDState *s = opaque;
@@ -1325,7 +1326,6 @@ static VMStateDescription ccid_vmstate = {
         VMSTATE_END_OF_LIST()
     }
 };
-#endif // ENABLE_MIGRATION
 
 static struct USBDeviceInfo ccid_info = {
     .product_desc   = "QEMU USB CCID",
@@ -1342,12 +1342,9 @@ static struct USBDeviceInfo ccid_info = {
         DEFINE_PROP_UINT8("debug", USBCCIDState, debug, 0),
         DEFINE_PROP_END_OF_LIST(),
     },
-#ifdef ENABLE_MIGRATION
     .qdev.vmsd      = &ccid_vmstate,
-#endif
 };
 
-
 static void ccid_register_devices(void)
 {
     usb_qdev_register(&ccid_info);
-- 
1.7.4