On 02/23/11 12:20, Alon Levy wrote: > diff --git a/configure b/configure > index 791b71d..147aab3 100755 > --- a/configure > +++ b/configure > @@ -174,6 +174,7 @@ trace_backend="nop" > trace_file="trace" > spice="" > rbd="" > +smartcard="yes"
IMHO smartcard support shouldn't be enabled per default. The userbase is limited. > diff --git a/hw/ccid.h b/hw/ccid.h > new file mode 100644 > index 0000000..4350bc2 > --- /dev/null > +++ b/hw/ccid.h > @@ -0,0 +1,54 @@ > +/* > + * CCID Passthru Card Device emulation > + * > + * Copyright (c) 2011 Red Hat. > + * Written by Alon Levy. > + * > + * This code is licenced under the GNU LGPL, version 2 or later. > + */ > + [snip] > + > +/* callbacks to be used by the CCID device (hw/usb-ccid.c) to call > + * into the smartcard device (hw/ccid-card-*.c) > + */ This is inconsistent with the comment above. Normally multi-line comments in QEMU are like this: /* * foo * bar */ > +struct CCIDCardInfo { > + DeviceInfo qdev; > + void (*print)(Monitor *mon, CCIDCardState *card, int indent); > + const uint8_t *(*get_atr)(CCIDCardState *card, uint32_t *len); > + void (*apdu_from_guest)(CCIDCardState *card, > + const uint8_t *apdu, > + uint32_t len); > + int (*exitfn)(CCIDCardState *card); > + int (*initfn)(CCIDCardState *card); > +}; > + > +/* API for smartcard calling the CCID device (used by hw/ccid-card-*.c) > + */ again here > +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); > +void ccid_card_card_error(CCIDCardState *card, uint64_t error); > +void ccid_card_qdev_register(CCIDCardInfo *card); > + > +/* support guest visible insertion/removal of ccid devices based on actual > + * devices connected/removed. Called by card implementation (passthru, > local) */ and here > diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c > new file mode 100644 > index 0000000..bf4022a > --- /dev/null > +++ b/hw/usb-ccid.c > +#define CCID_DEV_NAME "usb-ccid" > + > +/* The two options for variable sized buffers: > + * make them constant size, for large enough constant, > + * or handle the migration complexity - VMState doesn't handle this case. > + * sizes are expected never to be exceeded, unless guest misbehaves. */ here again [snip] > +/* Using Gemplus Vendor and Product id > + Effect on various drivers: > + * usbccid.sys (winxp, others untested) is a class driver so it doesn't > care. > + * linux has a number of class drivers, but openct filters based on > + vendor/product (/etc/openct.conf under fedora), hence Gemplus. > + */ Something went totally boink with the comments there! > +/* 6.2.6 RDR_to_PC_SlotStatus definitions */ > +enum { > + CLOCK_STATUS_RUNNING = 0, > + /* 0 - Clock Running, 1 - Clock stopped in State L, 2 - H, > + 3 - unkonwn state. rest are RFU > + */ > +}; > + > +typedef struct __attribute__ ((__packed__)) CCID_Header { > + uint8_t bMessageType; > + uint32_t dwLength; > + uint8_t bSlot; > + uint8_t bSeq; > +} CCID_Header; Is this header decided upon by the CCID spec or the code? It seems suboptimal to have a uint8 in front of a uint32 like that. Inefficient structure alignment :( > + > +/* 6.1.4 PC_to_RDR_XfrBlock */ > +typedef struct __attribute__ ((__packed__)) CCID_XferBlock { > + CCID_Header hdr; > + uint8_t bBWI; /* Block Waiting Timeout */ > + uint16_t wLevelParameter; /* XXX currently unused */ > + uint8_t abData[0]; > +} CCID_XferBlock; > + > +typedef struct __attribute__ ((__packed__)) CCID_IccPowerOn { > + CCID_Header hdr; > + uint8_t bPowerSelect; > + uint16_t abRFU; > +} CCID_IccPowerOn; Same problem with the above two structs.... > +typedef struct __attribute__ ((__packed__)) CCID_IccPowerOff { > + CCID_Header hdr; > + uint16_t abRFU; > +} CCID_IccPowerOff; > + > +typedef struct __attribute__ ((__packed__)) CCID_SetParameters { > + CCID_Header hdr; > + uint8_t bProtocolNum; > + uint16_t abRFU; > + uint8_t abProtocolDataStructure[0]; > +} CCID_SetParameters; and again. > +/** > + * powered - defaults to true, changed by PowerOn/PowerOff messages > + */ > +struct USBCCIDState { > + USBDevice dev; > + CCIDBus *bus; > + CCIDCardState *card; > + CCIDCardInfo *cardinfo; /* caching the info pointer */ > + uint8_t debug; > + BulkIn bulk_in_pending[BULK_IN_PENDING_NUM]; /* circular */ > + uint32_t bulk_in_pending_start; > + uint32_t bulk_in_pending_end; /* first free */ > + uint32_t bulk_in_pending_num; > + BulkIn *current_bulk_in; > + uint8_t bulk_out_data[BULK_OUT_DATA_SIZE]; > + uint32_t bulk_out_pos; > + uint8_t bmSlotICCState; > + uint8_t powered; > + uint8_t notify_slot_change; > + uint64_t last_answer_error; > + Answer pending_answers[PENDING_ANSWERS_NUM]; > + uint32_t pending_answers_start; > + uint32_t pending_answers_end; > + uint32_t pending_answers_num; > + uint8_t bError; > + uint8_t bmCommandStatus; > + uint8_t bProtocolNum; > + uint8_t abProtocolDataStructure[MAX_PROTOCOL_SIZE]; > + uint32_t ulProtocolDataStructureSize; > + uint32_t state_vmstate; > + uint8_t migration_state; > + uint32_t migration_target_ip; > + uint16_t migration_target_port; > +}; Try to place the struct elements a little better so you don't end up with a lot of space wasted due to natural alignment by the compiler. > +static void ccid_bulk_in_get(USBCCIDState *s) > +{ > + if (s->current_bulk_in != NULL || s->bulk_in_pending_num == 0) { > + return; > + } > + assert(s->bulk_in_pending_num > 0); > + s->bulk_in_pending_num--; > + s->current_bulk_in = &s->bulk_in_pending[ > + (s->bulk_in_pending_start++) % BULK_IN_PENDING_NUM]; That line break is really not good :( Either break it after the '=' or calculate the index outside the assignment statement. > +static void ccid_write_data_block( > + USBCCIDState *s, uint8_t slot, uint8_t seq, > + const uint8_t *data, uint32_t len) Please fix this - keep some arguments on the first line, and align the following ones to match. > +/* handle a single USB_TOKEN_OUT, return value returned to guest. > + * 0 - all ok > + * USB_RET_STALL - failed to handle packet */ another badly formatted comment > +void ccid_card_card_error(CCIDCardState *card, uint64_t error) > +{ > + USBCCIDState *s = > + DO_UPCAST(USBCCIDState, dev.qdev, card->qdev.parent_bus->parent); > + > + s->bmCommandStatus = COMMAND_STATUS_FAILED; > + s->last_answer_error = 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. > + * */ > + /* we flush all pending answers on CardRemove message in > ccid-card-passthru, > + * so check that first to not trigger abort */ !!! there's more below. Except for the mostly cosmetic stuff, it looks ok to me. Cheers, Jes