Re: [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus

[Jes Sorensen <Jes.Sorensen@redhat.com>]

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