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

Ripke, Klaus posted 1 patch 1 year, 2 months ago
hw/usb/dev-smartcard-reader.c | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)
[PATCH] usb-ccid: make ids and descriptor configurable
Posted by Ripke, Klaus 1 year, 2 months ago
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
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago
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
Posted by Ripke, Klaus 1 year, 2 months ago
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
Posted by Ripke, Klaus 1 year ago
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
Posted by Ripke, Klaus 1 year, 2 months ago
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