[PATCH] usb-ccid: make ids and descriptor configurable

[PATCH] usb-ccid: make ids and descriptor configurable

[Ripke, Klaus]

Signed-off-by: Klaus Ripke <klaus.ripke@secunet.com>

hw/usb/dev-smartcard-reader.c:
Set some static values from ccid_properties.

---
 hw/usb/dev-smartcard-reader.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
reader.c
index 28164d89be..4002157773 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -311,6 +311,11 @@ struct USBCCIDState {
     uint8_t  powered;
     uint8_t  notify_slot_change;
     uint8_t  debug;
+    /* the following are copied to static on initial realize */
+    uint16_t vendor;
+    uint16_t product;
+    uint8_t  maxslot;
+    uint8_t  feat2;
 };
 
 /*
@@ -323,7 +328,11 @@ struct USBCCIDState {
  *   0dc3:1004 Athena Smartcard Solutions, Inc.
  */
 
-static const uint8_t qemu_ccid_descriptor[] = {
+enum {
+    DESC_MAXSLOT = 4,
+    DESC_FEAT2 = 42 /* dwFeatures byte 2 */
+};
+static uint8_t qemu_ccid_descriptor[] = {
         /* Smart Card Device Class Descriptor */
         0x36,       /* u8  bLength; */
         0x21,       /* u8  bDescriptorType; Functional */
@@ -472,7 +481,7 @@ static const USBDescDevice desc_device = {
     },
 };
 
-static const USBDesc desc_ccid = {
+static USBDesc desc_ccid = {
     .id = {
         .idVendor          = CCID_VENDOR_ID,
         .idProduct         = CCID_PRODUCT_ID,
@@ -1295,9 +1304,10 @@ static void ccid_card_realize(DeviceState *qdev,
Error **errp)
     USBCCIDState *s = USB_CCID_DEV(dev);
     Error *local_err = NULL;
 
-    if (card->slot != 0) {
-        error_setg(errp, "usb-ccid supports one slot, can't add %d",
-                   card->slot);
+    DPRINTF(s, D_VERBOSE, "%s: slot %d\n", __func__, card->slot);
+    if (card->slot > qemu_ccid_descriptor[DESC_MAXSLOT]) {
+        error_setg(errp, "usb-ccid supports %d slot, can't add %d",
+                   qemu_ccid_descriptor[DESC_MAXSLOT] + 1, card-
>slot);
         return;
     }
     if (s->card != NULL) {
@@ -1317,6 +1327,14 @@ static void ccid_card_realize(DeviceState *qdev,
Error **errp)
 static void ccid_realize(USBDevice *dev, Error **errp)
 {
     USBCCIDState *s = USB_CCID_DEV(dev);
+    static int initialized;
+    if (!initialized) {
+        desc_ccid.id.idVendor = s->vendor;
+        desc_ccid.id.idProduct = s->product;
+        qemu_ccid_descriptor[DESC_MAXSLOT] = s->maxslot;
+        qemu_ccid_descriptor[DESC_FEAT2] = s->feat2;
+        initialized = !0;
+    }
 
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
@@ -1339,6 +1357,8 @@ static void ccid_realize(USBDevice *dev, Error
**errp)
     ccid_reset_parameters(s);
     ccid_reset(s);
     s->debug = parse_debug_env("QEMU_CCID_DEBUG", D_VERBOSE, s-
>debug);
+    DPRINTF(s, D_VERBOSE, "ccid_realize %d %x %x %x %x\n",
+        initialized, s->vendor, s->product, s->maxslot, s->feat2);
 }
 
 static int ccid_post_load(void *opaque, int version_id)
@@ -1434,9 +1454,14 @@ static const VMStateDescription ccid_vmstate = {
 
 static Property ccid_properties[] = {
     DEFINE_PROP_UINT8("debug", USBCCIDState, debug, 0),
+    DEFINE_PROP_UINT16("vendor", USBCCIDState, vendor,
CCID_VENDOR_ID),
+    DEFINE_PROP_UINT16("product", USBCCIDState, product,
CCID_PRODUCT_ID),
+    DEFINE_PROP_UINT8("maxslot", USBCCIDState, maxslot, 0),
+    DEFINE_PROP_UINT8("feat2", USBCCIDState, feat2, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+
 static void ccid_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-- 
2.34.1

-- 
Klaus Ripke
Senior Developer
Public Authorities Division
secunet Security Networks AG

Telefon:  +49 201 5454-2982

Re: [PATCH] usb-ccid: make ids and descriptor configurable

[Philippe Mathieu-Daudé]

Hi Klaus,

On 16/1/23 16:46, Ripke, Klaus wrote:
> Signed-off-by: Klaus Ripke <klaus.ripke@secunet.com>
> 
> hw/usb/dev-smartcard-reader.c:
> Set some static values from ccid_properties.
> 
> ---
>   hw/usb/dev-smartcard-reader.c | 35 ++++++++++++++++++++++++++++++-----
>   1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
> reader.c
> index 28164d89be..4002157773 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -311,6 +311,11 @@ struct USBCCIDState {
>       uint8_t  powered;
>       uint8_t  notify_slot_change;
>       uint8_t  debug;
> +    /* the following are copied to static on initial realize */
> +    uint16_t vendor;
> +    uint16_t product;
> +    uint8_t  maxslot;
> +    uint8_t  feat2;
>   };
>   
>   /*
> @@ -323,7 +328,11 @@ struct USBCCIDState {
>    *   0dc3:1004 Athena Smartcard Solutions, Inc.
>    */
>   
> -static const uint8_t qemu_ccid_descriptor[] = {
> +enum {
> +    DESC_MAXSLOT = 4,
> +    DESC_FEAT2 = 42 /* dwFeatures byte 2 */
> +};
> +static uint8_t qemu_ccid_descriptor[] = {

If you create 2 devices with different properties, the
first gets its properties overwritten with the second's
ones.

>           /* Smart Card Device Class Descriptor */
>           0x36,       /* u8  bLength; */
>           0x21,       /* u8  bDescriptorType; Functional */
> @@ -472,7 +481,7 @@ static const USBDescDevice desc_device = {
>       },
>   };
>   
> -static const USBDesc desc_ccid = {
> +static USBDesc desc_ccid = {
>       .id = {
>           .idVendor          = CCID_VENDOR_ID,
>           .idProduct         = CCID_PRODUCT_ID,
> @@ -1295,9 +1304,10 @@ static void ccid_card_realize(DeviceState *qdev,
> Error **errp)
>       USBCCIDState *s = USB_CCID_DEV(dev);
>       Error *local_err = NULL;
>   
> -    if (card->slot != 0) {
> -        error_setg(errp, "usb-ccid supports one slot, can't add %d",
> -                   card->slot);
> +    DPRINTF(s, D_VERBOSE, "%s: slot %d\n", __func__, card->slot);
> +    if (card->slot > qemu_ccid_descriptor[DESC_MAXSLOT]) {
> +        error_setg(errp, "usb-ccid supports %d slot, can't add %d",
> +                   qemu_ccid_descriptor[DESC_MAXSLOT] + 1, card-
>> slot);
>           return;
>       }
>       if (s->card != NULL) {
> @@ -1317,6 +1327,14 @@ static void ccid_card_realize(DeviceState *qdev,
> Error **errp)
>   static void ccid_realize(USBDevice *dev, Error **errp)
>   {
>       USBCCIDState *s = USB_CCID_DEV(dev);
> +    static int initialized;
> +    if (!initialized) {
> +        desc_ccid.id.idVendor = s->vendor;
> +        desc_ccid.id.idProduct = s->product;
> +        qemu_ccid_descriptor[DESC_MAXSLOT] = s->maxslot;
> +        qemu_ccid_descriptor[DESC_FEAT2] = s->feat2;
> +        initialized = !0;
> +    }
>   
>       usb_desc_create_serial(dev);
>       usb_desc_init(dev);
> @@ -1339,6 +1357,8 @@ static void ccid_realize(USBDevice *dev, Error
> **errp)
>       ccid_reset_parameters(s);
>       ccid_reset(s);
>       s->debug = parse_debug_env("QEMU_CCID_DEBUG", D_VERBOSE, s-
>> debug);
> +    DPRINTF(s, D_VERBOSE, "ccid_realize %d %x %x %x %x\n",
> +        initialized, s->vendor, s->product, s->maxslot, s->feat2);
>   }
>   
>   static int ccid_post_load(void *opaque, int version_id)
> @@ -1434,9 +1454,14 @@ static const VMStateDescription ccid_vmstate = {
>   
>   static Property ccid_properties[] = {
>       DEFINE_PROP_UINT8("debug", USBCCIDState, debug, 0),
> +    DEFINE_PROP_UINT16("vendor", USBCCIDState, vendor,
> CCID_VENDOR_ID),
> +    DEFINE_PROP_UINT16("product", USBCCIDState, product,
> CCID_PRODUCT_ID),
> +    DEFINE_PROP_UINT8("maxslot", USBCCIDState, maxslot, 0),
> +    DEFINE_PROP_UINT8("feat2", USBCCIDState, feat2, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> +
>   static void ccid_class_initfn(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);

Re: [PATCH] usb-ccid: make ids and descriptor configurable

[Ripke, Klaus]

hi Philippe,

On Tue, 2023-01-17 at 08:04 +0100, Philippe Mathieu-Daudé wrote:
> If you create 2 devices with different properties, the
> first gets its properties overwritten with the second's
> ones.
The !initialized guard should prevent that.
In practize you would not create more usb-ccid devices,
but usb-ccid-passthru on the same class,
slots are now supported in card_realize.
Abandoning all the static structures seems too big a change
for a little tweaking, and setting from ENV was refused.

> > +        qemu_ccid_descriptor[DESC_FEAT2] = s->feat2;
Some devices (libcacard vscclient) want 4 here for "Automatic IFSD
exchange".


best Klaus

-- 
Klaus Ripke
Senior Developer
Public Authorities Division
secunet Security Networks AG

Telefon:  +49 201 5454-2982

Re: [PATCH] usb-ccid: make ids and descriptor configurable

[Ripke, Klaus]

Hi Philippe,

so I guess it's rejected. Any suggestions?

TIA Klaus

On Tue, 2023-01-17 at 08:04 +0100, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 16/1/23 16:46, Ripke, Klaus wrote:
> > Signed-off-by: Klaus Ripke <klaus.ripke@secunet.com>
> > 
> > hw/usb/dev-smartcard-reader.c:
> > Set some static values from ccid_properties.
> > 
> > ---
> >   hw/usb/dev-smartcard-reader.c | 35
> > ++++++++++++++++++++++++++++++-----
> >   1 file changed, 30 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
> > reader.c
> > index 28164d89be..4002157773 100644
> > --- a/hw/usb/dev-smartcard-reader.c
> > +++ b/hw/usb/dev-smartcard-reader.c
> > @@ -311,6 +311,11 @@ struct USBCCIDState {
> >       uint8_t  powered;
> >       uint8_t  notify_slot_change;
> >       uint8_t  debug;
> > +    /* the following are copied to static on initial realize */
> > +    uint16_t vendor;
> > +    uint16_t product;
> > +    uint8_t  maxslot;
> > +    uint8_t  feat2;
> >   };
> >   
> >   /*
> > @@ -323,7 +328,11 @@ struct USBCCIDState {
> >    *   0dc3:1004 Athena Smartcard Solutions, Inc.
> >    */
> >   
> > -static const uint8_t qemu_ccid_descriptor[] = {
> > +enum {
> > +    DESC_MAXSLOT = 4,
> > +    DESC_FEAT2 = 42 /* dwFeatures byte 2 */
> > +};
> > +static uint8_t qemu_ccid_descriptor[] = {
> 
> If you create 2 devices with different properties, the
> first gets its properties overwritten with the second's
> ones.
> 
> >           /* Smart Card Device Class Descriptor */
> >           0x36,       /* u8  bLength; */
> >           0x21,       /* u8  bDescriptorType; Functional */
> > @@ -472,7 +481,7 @@ static const USBDescDevice desc_device = {
> >       },
> >   };
> >   
> > -static const USBDesc desc_ccid = {
> > +static USBDesc desc_ccid = {
> >       .id = {
> >           .idVendor          = CCID_VENDOR_ID,
> >           .idProduct         = CCID_PRODUCT_ID,
> > @@ -1295,9 +1304,10 @@ static void ccid_card_realize(DeviceState
> > *qdev,
> > Error **errp)
> >       USBCCIDState *s = USB_CCID_DEV(dev);
> >       Error *local_err = NULL;
> >   
> > -    if (card->slot != 0) {
> > -        error_setg(errp, "usb-ccid supports one slot, can't add
> > %d",
> > -                   card->slot);
> > +    DPRINTF(s, D_VERBOSE, "%s: slot %d\n", __func__, card->slot);
> > +    if (card->slot > qemu_ccid_descriptor[DESC_MAXSLOT]) {
> > +        error_setg(errp, "usb-ccid supports %d slot, can't add
> > %d",
> > +                   qemu_ccid_descriptor[DESC_MAXSLOT] + 1, card-
> > > slot);
> >           return;
> >       }
> >       if (s->card != NULL) {
> > @@ -1317,6 +1327,14 @@ static void ccid_card_realize(DeviceState
> > *qdev,
> > Error **errp)
> >   static void ccid_realize(USBDevice *dev, Error **errp)
> >   {
> >       USBCCIDState *s = USB_CCID_DEV(dev);
> > +    static int initialized;
> > +    if (!initialized) {
> > +        desc_ccid.id.idVendor = s->vendor;
> > +        desc_ccid.id.idProduct = s->product;
> > +        qemu_ccid_descriptor[DESC_MAXSLOT] = s->maxslot;
> > +        qemu_ccid_descriptor[DESC_FEAT2] = s->feat2;
> > +        initialized = !0;
> > +    }
> >   
> >       usb_desc_create_serial(dev);
> >       usb_desc_init(dev);
> > @@ -1339,6 +1357,8 @@ static void ccid_realize(USBDevice *dev,
> > Error
> > **errp)
> >       ccid_reset_parameters(s);
> >       ccid_reset(s);
> >       s->debug = parse_debug_env("QEMU_CCID_DEBUG", D_VERBOSE, s-
> > > debug);
> > +    DPRINTF(s, D_VERBOSE, "ccid_realize %d %x %x %x %x\n",
> > +        initialized, s->vendor, s->product, s->maxslot, s->feat2);
> >   }
> >   
> >   static int ccid_post_load(void *opaque, int version_id)
> > @@ -1434,9 +1454,14 @@ static const VMStateDescription ccid_vmstate
> > = {
> >   
> >   static Property ccid_properties[] = {
> >       DEFINE_PROP_UINT8("debug", USBCCIDState, debug, 0),
> > +    DEFINE_PROP_UINT16("vendor", USBCCIDState, vendor,
> > CCID_VENDOR_ID),
> > +    DEFINE_PROP_UINT16("product", USBCCIDState, product,
> > CCID_PRODUCT_ID),
> > +    DEFINE_PROP_UINT8("maxslot", USBCCIDState, maxslot, 0),
> > +    DEFINE_PROP_UINT8("feat2", USBCCIDState, feat2, 0),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> > +
> >   static void ccid_class_initfn(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> 

-- 
Klaus Ripke
Senior Developer
Public Authorities Division
secunet Security Networks AG

Telefon:  +49 201 5454-2982

seeking advice for configuring usb_desc in ccid / dev-smartcard-reader.c

[Ripke, Klaus]

Hello


in he/usb/dev-smartcard-reader.c: we need a slightly differing version
of the "Athena Smart Card Reader" as of qemu_ccid_descriptor with two
bytes changed to fixed "extended" values, 14 for max slot and 4 in
feature 2.
This data is shared by all ccid devices through a chain down to
usb_desc (which is klass->usb_desc for all ccid as of now).

Should we best follow the practice of dev-audio and dev-hid by using
another static config, selected by some device property?

Or better dynamically create and modify copies of all structures in
realize?

Or some other way?


many thanks for your help, kind regards
Klaus

Re: [PATCH] usb-ccid: make ids and descriptor configurable

[Marc-André Lureau]

Hi Klaus

On Tue, Jan 10, 2023 at 7:49 PM Ripke, Klaus <klaus.ripke@secunet.com> wrote:
>
> Signed-off-by: Klaus Ripke <klaus.ripke@secunet.com>
>
> hw/usb/dev-smartcard-reader.c:
> Set static values from env vars QEMU_CCID_VENDOR/PRODUCT_ID and
> _DESCRIPTOR

We don't use environment variables to configure devices. Please add
properties to ccid_properties[] instead.

thanks

>
> ---
>  hw/usb/dev-smartcard-reader.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
> reader.c
> index 28164d89be..0632ab44c6 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -323,7 +323,7 @@ struct USBCCIDState {
>   *   0dc3:1004 Athena Smartcard Solutions, Inc.
>   */
>
> -static const uint8_t qemu_ccid_descriptor[] = {
> +static uint8_t qemu_ccid_descriptor[] = {
>          /* Smart Card Device Class Descriptor */
>          0x36,       /* u8  bLength; */
>          0x21,       /* u8  bDescriptorType; Functional */
> @@ -472,7 +472,7 @@ static const USBDescDevice desc_device = {
>      },
>  };
>
> -static const USBDesc desc_ccid = {
> +static USBDesc desc_ccid = {
>      .id = {
>          .idVendor          = CCID_VENDOR_ID,
>          .idProduct         = CCID_PRODUCT_ID,
> @@ -1437,12 +1437,33 @@ static Property ccid_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static void env_uint16(uint16_t *val, const char *env)
> +{
> +    const char *str = getenv(env);
> +    if (str) {
> +        *val = qemu_strtoul(str, NULL, 16);
> +    }
> +}
> +
>  static void ccid_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
>      HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> -
> +    const char *dsc = getenv("QEMU_CCID_DESCRIPTOR");
> +
> +    if (dsc) {
> +        unsigned int idx = 0;
> +        unsigned int val = 0;
> +        int off = 0;
> +        for (; 2 == sscanf(dsc, "%u:%x%n", &idx, &val, &off); dsc +=
> off) {
> +            if (idx < sizeof qemu_ccid_descriptor) {
> +                qemu_ccid_descriptor[idx] = val;
> +            }
> +        }
> +    }
> +    env_uint16(&desc_ccid.id.idVendor, "QEMU_CCID_VENDOR_ID");
> +    env_uint16(&desc_ccid.id.idProduct, "QEMU_CCID_PRODUCT_ID");
>      uc->realize        = ccid_realize;
>      uc->product_desc   = "QEMU USB CCID";
>      uc->usb_desc       = &desc_ccid;
> --
> 2.34.1
>
>
>
> --
> Klaus Ripke
> Senior Developer
> Public Authorities Division
> secunet Security Networks AG
>
> Telefon:  +49 201 5454-2982



-- 
Marc-André Lureau

[PATCH] usb-ccid: make ids and descriptor configurable

[Ripke, Klaus]

Signed-off-by: Klaus Ripke <klaus.ripke@secunet.com>

hw/usb/dev-smartcard-reader.c:
Set static values from env vars QEMU_CCID_VENDOR/PRODUCT_ID and
_DESCRIPTOR

---
 hw/usb/dev-smartcard-reader.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-
reader.c
index 28164d89be..0632ab44c6 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -323,7 +323,7 @@ struct USBCCIDState {
  *   0dc3:1004 Athena Smartcard Solutions, Inc.
  */
 
-static const uint8_t qemu_ccid_descriptor[] = {
+static uint8_t qemu_ccid_descriptor[] = {
         /* Smart Card Device Class Descriptor */
         0x36,       /* u8  bLength; */
         0x21,       /* u8  bDescriptorType; Functional */
@@ -472,7 +472,7 @@ static const USBDescDevice desc_device = {
     },
 };
 
-static const USBDesc desc_ccid = {
+static USBDesc desc_ccid = {
     .id = {
         .idVendor          = CCID_VENDOR_ID,
         .idProduct         = CCID_PRODUCT_ID,
@@ -1437,12 +1437,33 @@ static Property ccid_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void env_uint16(uint16_t *val, const char *env)
+{
+    const char *str = getenv(env);
+    if (str) {
+        *val = qemu_strtoul(str, NULL, 16);
+    }
+}
+
 static void ccid_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
-
+    const char *dsc = getenv("QEMU_CCID_DESCRIPTOR");
+
+    if (dsc) {
+        unsigned int idx = 0;
+        unsigned int val = 0;
+        int off = 0;
+        for (; 2 == sscanf(dsc, "%u:%x%n", &idx, &val, &off); dsc +=
off) {
+            if (idx < sizeof qemu_ccid_descriptor) {
+                qemu_ccid_descriptor[idx] = val;
+            }
+        }
+    }
+    env_uint16(&desc_ccid.id.idVendor, "QEMU_CCID_VENDOR_ID");
+    env_uint16(&desc_ccid.id.idProduct, "QEMU_CCID_PRODUCT_ID");
     uc->realize        = ccid_realize;
     uc->product_desc   = "QEMU USB CCID";
     uc->usb_desc       = &desc_ccid;
-- 
2.34.1



-- 
Klaus Ripke
Senior Developer
Public Authorities Division
secunet Security Networks AG

Telefon:  +49 201 5454-2982