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

[Alon Levy <alevy@redhat.com>]

On Mon, Mar 14, 2011 at 02:54:59PM +0100, Jes Sorensen wrote:
> 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 :(
> 

In the spec.

> > +
> > +/* 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....
> 

spec spec.

> > +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.
> 

guess ;)

> > +/**
> > + * 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.
> 

ok, this one's me. I'm really not sure except for stuff that goes on the wire
or get's allocated a bazillion times that this is worth the change in general,
but since I'm respinning anyway I'll do it. (unless you're saying it should be
a habit).

> > +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.

ok, after the =, but then I think the rest is >80, so it will neccessitate another
break.
> 
> > +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.

Is that a coding style thing I missed or personal preferance? my personal preferance
here is the way it is, since it looks shorter/more readable, but I don't care that
much.

> 
> > +/* 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
> 
fixing them all.

> > +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.
? more badly formated comments? more todos? more flushing?

> 
> Except for the mostly cosmetic stuff, it looks ok to me.
> 
> Cheers,
> Jes
> 
>