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

[Blue Swirl]

On Sun, Nov 28, 2010 at 10:53 AM, Alon Levy <address@hidden> wrote:
> 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).

At least endianness should be taken care of, so perhaps
packing/unpacking can be done in the same pass. Or you could use
uint8_t fields for each byte in the word.

Poor design by USB committees IMHO.