On Thu, Feb 03, 2011 at 10:48:03AM -0600, Anthony Liguori wrote:
> On 02/02/2011 02:28 PM, Alon Levy wrote:
> >I'll fold it before submitting the version to be applied, but
> >I hope keeping it as a separate patch will make reviewing easier.
>
> Hrm, can you just send out the new patches? It's actually harder
> to review like this.
Yes.
>
> Regards,
>
> Anthony Liguori
>
> >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<al...@redhat.com>
> >---
> > 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<samuel.thiba...@ens-lyon.org>
> > * 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);
>
