From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42679 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pzmq5-0005Ij-A9 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:16:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pzmq2-0004I6-B7 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:16:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61406) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pzmq2-0004I0-16 for qemu-devel@nongnu.org; Wed, 16 Mar 2011 05:16:18 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2G9GHDG016204 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 16 Mar 2011 05:16:17 -0400 Date: Wed, 16 Mar 2011 11:15:51 +0200 From: Alon Levy Subject: Re: [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus Message-ID: <20110316091551.GC7413@playa.tlv.redhat.com> References: <1298460024-23591-1-git-send-email-alevy@redhat.com> <1298460024-23591-2-git-send-email-alevy@redhat.com> <4D7E1E33.1070801@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D7E1E33.1070801@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: qemu-devel@nongnu.org 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 > >