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

[Alon Levy]

On Fri, Nov 26, 2010 at 07:05:02PM +0000, Blue Swirl wrote:
> On Thu, Nov 25, 2010 at 4:22 PM, Alon Levy <address@hidden> wrote:
> > A CCID device is a smart card reader. It is a USB device, defined at [1].
> > This patch introduces the usb-ccid device that is a ccid bus. Next patches 
> > will
> > introduce two card types to use it, a passthru card and an emulated card.
> >
> >  [1] http://www.usb.org/developers/devclass_docs/DWG_Smart-Card_CCID_Rev110.
> >
> > Signed-off-by: Alon Levy <address@hidden>
> > ---
> >  Makefile.objs |    1 +
> >  configure     |   12 +
> >  hw/ccid.h     |   34 ++
> >  hw/usb-ccid.c | 1342 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1389 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/ccid.h
> >  create mode 100644 hw/usb-ccid.c
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 23b17ce..713131f 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -183,6 +183,7 @@ hw-obj-$(CONFIG_FDC) += fdc.o
> >  hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o
> >  hw-obj-$(CONFIG_APM) += pm_smbus.o apm.o
> >  hw-obj-$(CONFIG_DMA) += dma.o
> > +hw-obj-$(CONFIG_SMARTCARD) += usb-ccid.o
> >
> >  # PPC devices
> >  hw-obj-$(CONFIG_OPENPIC) += openpic.o
> > diff --git a/configure b/configure
> > index 2917874..fb9eac2 100755
> > --- a/configure
> > +++ b/configure
> > @@ -332,6 +332,7 @@ zero_malloc=""
> >  trace_backend="nop"
> >  trace_file="trace"
> >  spice=""
> > +smartcard="no"
> >
> >  # OS specific
> >  if check_define __linux__ ; then
> > @@ -739,6 +740,10 @@ for opt do
> >   ;;
> >   --enable-vhost-net) vhost_net="yes"
> >   ;;
> > +  --disable-smartcard) smartcard="no"
> > +  ;;
> > +  --enable-smartcard) smartcard="yes"
> > +  ;;
> >   --*dir)
> >   ;;
> >   *) echo "ERROR: unknown option $opt"; show_help="yes"
> > @@ -934,6 +939,8 @@ echo "  --trace-file=NAME        Full PATH,NAME of file 
> > to store traces"
> >  echo "                           Default:trace-<pid>"
> >  echo "  --disable-spice          disable spice"
> >  echo "  --enable-spice           enable spice"
> > +echo "  --disable-smartcard      disable smartcard support"
> > +echo "  --enable-smartcard       enable smartcard support"
> >  echo ""
> >  echo "NOTE: The object files are built at the place where configure is 
> > launched"
> >  exit 1
> > @@ -2354,6 +2361,7 @@ echo "vhost-net support $vhost_net"
> >  echo "Trace backend     $trace_backend"
> >  echo "Trace output file $trace_file-<pid>"
> >  echo "spice support     $spice"
> > +echo "smartcard support $smartcard"
> >
> >  if test $sdl_too_old = "yes"; then
> >  echo "-> Your SDL version is too old - please upgrade to have SDL support"
> > @@ -2617,6 +2625,10 @@ if test "$spice" = "yes" ; then
> >   echo "CONFIG_SPICE=y" >> $config_host_mak
> >  fi
> >
> > +if test "$smartcard" = "yes" ; then
> > +  echo "CONFIG_SMARTCARD=y" >> $config_host_mak
> > +fi
> > +
> >  # XXX: suppress that
> >  if [ "$bsd" = "yes" ] ; then
> >   echo "CONFIG_BSD=y" >> $config_host_mak
> > diff --git a/hw/ccid.h b/hw/ccid.h
> > new file mode 100644
> > index 0000000..a38f971
> > --- /dev/null
> > +++ b/hw/ccid.h
> > @@ -0,0 +1,34 @@
> > +#ifndef __CCID_H__
> > +#define __CCID_H__
> > +
> > +#include "qdev.h"
> > +
> > +typedef struct CCIDCardState CCIDCardState;
> > +typedef struct CCIDCardInfo CCIDCardInfo;
> > +
> > +struct CCIDCardState {
> > +    DeviceState qdev;
> > +};
> > +
> > +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);
> > +};
> > +
> > +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) */
> > +int ccid_card_ccid_attach(CCIDCardState *card);
> > +void ccid_card_ccid_detach(CCIDCardState *card);
> > +
> > +#endif // __CCID_H__
> > +
> > diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
> > new file mode 100644
> > index 0000000..66c1dba
> > --- /dev/null
> > +++ b/hw/usb-ccid.c
> > @@ -0,0 +1,1342 @@
> > +/*
> > + * CCID Device emulation
> > + *
> > + * Based on usb-serial.c:
> > + * Copyright (c) 2006 CodeSourcery.
> > + * Copyright (c) 2008 Samuel Thibault <address@hidden>
> > + * Written by Paul Brook, reused for FTDI by Samuel Thibault,
> > + * Reused for CCID by Alon Levy.
> > + * Contributed to by Robert Relyea
> > + * Copyright (c) 2010 Red Hat.
> > + *
> > + * This code is licenced under the LGPL.
> > + */
> > +
> > +/* References:
> > + *
> > + * CCID Specification Revision 1.1 April 22nd 2005
> > + *  "Universal Serial Bus, Device Class: Smart Card"
> > + *  Specification for Integrated Circuit(s) Cards Interface Devices
> > + *
> > + * KNOWN BUGS
> > + * 1. remove/insert can sometimes result in removed state instead of 
> > inserted.
> > + * This is a result of the following:
> > + *  symptom: dmesg shows ERMOTEIO (-121), pcscd shows -99. This happens
> > + *  when we send a too short packet, seen in uhci-usb.c, resulting from
> > + *  a urb requesting SPD and us returning a smaller packet.
> > + *  Not sure which messages trigger this.
> > + *
> > + */
> > +
> > +#include "qemu-common.h"
> > +#include "qemu-error.h"
> > +#include "usb.h"
> > +#include "monitor.h"
> > +
> > +#include "hw/ccid.h"
> > +
> > +//#define DEBUG_CCID
> > +
> > +#define DPRINTF(s, lvl, fmt, ...) \
> > +do { if (lvl <= s->debug) { printf("usb-ccid: " fmt , ## __VA_ARGS__); } } 
> > while (0)
> > +
> > +#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. */
> > +#define BULK_OUT_DATA_SIZE 65536
> > +#define PENDING_ANSWERS_NUM 128
> > +
> > +#define BULK_IN_BUF_SIZE 384
> > +#define BULK_IN_PENDING_NUM 8
> > +
> > +#define InterfaceOutClass    
> > ((USB_DIR_OUT|USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
> > +#define InterfaceInClass     ((USB_DIR_IN 
> > |USB_TYPE_CLASS|USB_RECIP_INTERFACE)<<8)
> > +
> > +#define CCID_CONTROL_ABORT                  0x1
> > +#define CCID_CONTROL_GET_CLOCK_FREQUENCIES  0x2
> > +#define CCID_CONTROL_GET_DATA_RATES         0x3
> > +
> > +#define CCID_PRODUCT_DESCRIPTION        "QEMU USB CCID"
> > +#define CCID_VENDOR_DESCRIPTION         "QEMU " QEMU_VERSION
> > +#define CCID_INTERFACE_NAME             "CCID Interface"
> > +#define CCID_SERIAL_NUMBER_STRING       "1"
> > +/* 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.
> > + */
> > +#define CCID_VENDOR_ID                  0x08e6
> > +#define CCID_PRODUCT_ID                 0x4433
> > +#define CCID_DEVICE_VERSION             0x0000
> > +
> > +/* BULK_OUT messages from PC to Reader
> > +   Defined in CCID Rev 1.1 6.1 (page 26)
> > + */
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOn              0x62
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_IccPowerOff             0x63
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_GetSlotStatus           0x65
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_XfrBlock                0x6f
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_GetParameters           0x6c
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_ResetParameters         0x6d
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_SetParameters           0x61
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_Escape                  0x6b
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_IccClock                0x6e
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_T0APDU                  0x6a
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_Secure                  0x69
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_Mechanical              0x71
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_Abort                   0x72
> > +#define CCID_MESSAGE_TYPE_PC_to_RDR_SetDataRateAndClockFrequency 0x73
> > +
> > +/* BULK_IN messages from Reader to PC
> > +   Defined in CCID Rev 1.1 6.2 (page 48)
> > + */
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_DataBlock               0x80
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_SlotStatus              0x81
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_Parameters              0x82
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_Escape                  0x83
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_DataRateAndClockFrequency 0x84
> > +
> > +/* INTERRUPT_IN messages from Reader to PC
> > +   Defined in CCID Rev 1.1 6.3 (page 56)
> > + */
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_NotifySlotChange        0x50
> > +#define CCID_MESSAGE_TYPE_RDR_to_PC_HardwareError           0x51
> > +
> > +/* Endpoints for CCID - addresses are up to us to decide.
> > +   To support slot insertion and removal we must have an interrupt in ep
> > +   in addition we need a bulk in and bulk out ep
> > +   5.2, page 20
> > + */
> > +#define CCID_INT_IN_EP       1
> > +#define CCID_BULK_IN_EP      2
> > +#define CCID_BULK_OUT_EP     3
> > +
> > +/* bmSlotICCState masks */
> > +#define SLOT_0_STATE_MASK    1
> > +#define SLOT_0_CHANGED_MASK  2
> > +
> > +/* Status codes that go in bStatus (see 6.2.6) */
> > +enum {
> > +    ICC_STATUS_PRESENT_ACTIVE = 0,
> > +    ICC_STATUS_PRESENT_INACTIVE,
> > +    ICC_STATUS_NOT_PRESENT
> > +};
> > +
> > +enum {
> > +    COMMAND_STATUS_NO_ERROR = 0,
> > +    COMMAND_STATUS_FAILED,
> > +    COMMAND_STATUS_TIME_EXTENSION_REQUIRED
> > +};
> > +
> > +/* Error codes that go in bError (see 6.2.6)
> > + */
> > +enum {
> > +    ERROR_CMD_NOT_SUPPORTED = 0,
> > +    ERROR_CMD_ABORTED       = -1,
> > +    ERROR_ICC_MUTE          = -2,
> > +    ERROR_XFR_PARITY_ERROR  = -3,
> > +    ERROR_XFR_OVERRUN       = -4,
> > +    ERROR_HW_ERROR          = -5,
> > +};
> > +
> > +/* 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__)) {
> 
> Please don't use anonymous structs.
> 
> > +    uint8_t     bMessageType;
> 
> Why aHungarian nNotation? It's not QEMU style.
> 
> > +    uint32_t    dwLength;
> > +    uint8_t     bSlot;
> > +    uint8_t     bSeq;
> > +} CCID_Header;
> 
> Packed structure with unaligned fields. Will this work?
> 

So by that you mean it could produce incorrect code on some cpu's? I'm asking 
because adding endianess correcting code is much simpler then removing the 
packed attribute, which requires computing the size of message in a way other 
then sizeof (which will be bigger) and copying each field separately (inducing 
a much larger packing/unpacking code, error prone of course). I assumed gcc 
knows to generate aligned accesses where required, and do bit twiddling to 
retrieve the specific field (even if it is 4 byte word at a 1 byte offset from 
a 4 byte multiple).