From patchwork Wed Feb 2 20:28:06 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alon Levy X-Patchwork-Id: 81536 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1A20CB70E9 for ; Thu, 3 Feb 2011 07:40:00 +1100 (EST) Received: from localhost ([127.0.0.1]:51961 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkjUA-0003gj-Mi for incoming@patchwork.ozlabs.org; Wed, 02 Feb 2011 15:39:30 -0500 Received: from [140.186.70.92] (port=54119 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PkjJl-0007Wf-8G for qemu-devel@nongnu.org; Wed, 02 Feb 2011 15:28:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PkjJe-00009F-TK for qemu-devel@nongnu.org; Wed, 02 Feb 2011 15:28:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:20439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PkjJe-000091-MO for qemu-devel@nongnu.org; Wed, 02 Feb 2011 15:28:38 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p12KSckx006195 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 2 Feb 2011 15:28:38 -0500 Received: from playa.redhat.com (vpn-200-14.tlv.redhat.com [10.35.200.14]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p12KSLpW025755 for ; Wed, 2 Feb 2011 15:28:36 -0500 From: Alon Levy To: qemu-devel@nongnu.org Date: Wed, 2 Feb 2011 22:28:06 +0200 Message-Id: <1296678500-19497-7-git-send-email-alevy@redhat.com> In-Reply-To: <1296678500-19497-1-git-send-email-alevy@redhat.com> References: <1296678500-19497-1-git-send-email-alevy@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 06/20] usb-ccid: review fixes (v15-v16) X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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 * 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);