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
