diff mbox series

[v4,3/7] ccid: build smartcard as module

Message ID 20200622135601.12433-4-kraxel@redhat.com
State New, archived
Headers show
Series build some devices as modules. | expand

Commit Message

Gerd Hoffmann June 22, 2020, 1:55 p.m. UTC
Drops libcacard.so dependency from core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs        | 1 +
 hw/core/qdev.c       | 2 ++
 hw/Makefile.objs     | 1 +
 hw/usb/Makefile.objs | 4 +++-
 4 files changed, 7 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé June 23, 2020, 3:28 p.m. UTC | #1
Hi Gerd,

On 6/22/20 3:55 PM, Gerd Hoffmann wrote:
> Drops libcacard.so dependency from core qemu.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.objs        | 1 +
>  hw/core/qdev.c       | 2 ++
>  hw/Makefile.objs     | 1 +
>  hw/usb/Makefile.objs | 4 +++-
>  4 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 7ce2588b89a3..ca555ede0710 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -59,6 +59,7 @@ common-obj-y += migration/
>  common-obj-y += audio/
>  common-obj-m += audio/
>  common-obj-y += hw/
> +common-obj-m += hw/
>  
>  common-obj-y += replay/
>  
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6119e25e0eeb..d6459e4aa8e8 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -172,6 +172,8 @@ static struct {
>      const char *type;
>      const char *mod;
>  } const hwmodules[] = {
> +    { .type = "ccid-card-passthru",    .mod = "usb-smartcard"         },
> +    { .type = "ccid-card-emulated",    .mod = "usb-smartcard"         },

We want to use type definitions here (such TYPE_CCID_PASSTHRU),
as we don't guaranty them stable.

We don't want to include various "hw/x/y.h" in this core file.

Since there is a relation between QOM type and the module,
can we store/use the module name in the TypeInfo declaration?

  static const TypeInfo passthru_card_info = {
      .name          = TYPE_CCID_PASSTHRU,
      .parent        = TYPE_CCID_CARD,
      .instance_size = sizeof(PassthruState),
      .class_init    = passthru_class_initfn,
      .module_name   = "usb-smartcard",        <=====
  };

Actually this modularization is not specific to QDEV
and can be used to all QOM right? I.e:

  static const TypeInfo qcrypto_tls_creds_x509_info = {
      .parent = TYPE_QCRYPTO_TLS_CREDS,
      .name = TYPE_QCRYPTO_TLS_CREDS_X509,
      .module_name = "gnu-tls",
      ...
  }

>  };
>  
>  static bool qdev_module_loaded_all;
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 4cbe5e4e57d6..af8fd9a510ed 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,4 +43,5 @@ devices-dirs-y += smbios/
>  endif
>  
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m += usb/
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index fa5c3fa1b877..3c5b3d4fadd3 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -29,11 +29,13 @@ common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>  
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
>  common-obj-y                          += dev-smartcard-reader.o
> -common-obj-$(CONFIG_SMARTCARD)        += smartcard.mo
> +ifeq ($(CONFIG_SMARTCARD),y)
> +common-obj-m                          += smartcard.mo
>  smartcard.mo-objs := ccid-card-passthru.o ccid-card-emulated.o
>  smartcard.mo-cflags := $(SMARTCARD_CFLAGS)
>  smartcard.mo-libs := $(SMARTCARD_LIBS)
>  endif
> +endif
>  
>  ifeq ($(CONFIG_POSIX),y)
>  common-obj-$(CONFIG_USB_STORAGE_MTP)  += dev-mtp.o
>
Gerd Hoffmann June 23, 2020, 5:12 p.m. UTC | #2
Hi,

> > +    { .type = "ccid-card-passthru",    .mod = "usb-smartcard"         },
> > +    { .type = "ccid-card-emulated",    .mod = "usb-smartcard"         },
> 
> We want to use type definitions here (such TYPE_CCID_PASSTHRU),
> as we don't guaranty them stable.

Hmm?  I'm pretty sure '-device ccid-card-passthru' *is* stable ABI.

> Since there is a relation between QOM type and the module,
> can we store/use the module name in the TypeInfo declaration?
> 
>   static const TypeInfo passthru_card_info = {
>       .name          = TYPE_CCID_PASSTHRU,
>       .parent        = TYPE_CCID_CARD,
>       .instance_size = sizeof(PassthruState),
>       .class_init    = passthru_class_initfn,
>       .module_name   = "usb-smartcard",        <=====
>   };

That doesn't buy us much, the TypeInfo ends up in the module not qemu.
So qemu can't access it without loading the module.

We do *not* want load all modules on startup though.  Which means we
need a such list in qemu.  The struct above is just that.  There
certainly is room for improvement, building that list automatically
somehow for example.

Given that most devices don't depend on external shared libraries I
expect the list of device modules will stay relatively short.  So I've
decided to start with something simple and see how it goes (see also
patch 1/7).

> Actually this modularization is not specific to QDEV
> and can be used to all QOM right? I.e:
> 
>   static const TypeInfo qcrypto_tls_creds_x509_info = {
>       .parent = TYPE_QCRYPTO_TLS_CREDS,
>       .name = TYPE_QCRYPTO_TLS_CREDS_X509,
>       .module_name = "gnu-tls",
>       ...
>   }

Not as-is.  You'll need module load hooks in more places then and some
code tweaks to move it from qdev level (loading hw-* module only) to qom
level.

But, yes, moving the infrastructure to some qom-module.c file might be
useful when modularizing non-device objects.  Do you have any candidates
in mind?

take care,
  Gerd
Philippe Mathieu-Daudé June 30, 2020, 9:44 a.m. UTC | #3
On 6/23/20 7:12 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> +    { .type = "ccid-card-passthru",    .mod = "usb-smartcard"         },
>>> +    { .type = "ccid-card-emulated",    .mod = "usb-smartcard"         },
>>
>> We want to use type definitions here (such TYPE_CCID_PASSTHRU),
>> as we don't guaranty them stable.
> 
> Hmm?  I'm pretty sure '-device ccid-card-passthru' *is* stable ABI.

Asking on IRC, there is no explicit contract.

But as you remarked, doing so would break the CLI, so we should
some day clarify that objects implementing TYPE_USER_CREATABLE
can not have their typename changed. For the rest, there is no
restriction.

>> Since there is a relation between QOM type and the module,
>> can we store/use the module name in the TypeInfo declaration?
>>
>>   static const TypeInfo passthru_card_info = {
>>       .name          = TYPE_CCID_PASSTHRU,
>>       .parent        = TYPE_CCID_CARD,
>>       .instance_size = sizeof(PassthruState),
>>       .class_init    = passthru_class_initfn,
>>       .module_name   = "usb-smartcard",        <=====
>>   };
> 
> That doesn't buy us much, the TypeInfo ends up in the module not qemu.
> So qemu can't access it without loading the module.
> 
> We do *not* want load all modules on startup though.  Which means we
> need a such list in qemu.  The struct above is just that.  There
> certainly is room for improvement, building that list automatically
> somehow for example.

OK.

> Given that most devices don't depend on external shared libraries I
> expect the list of device modules will stay relatively short.  So I've
> decided to start with something simple and see how it goes (see also
> patch 1/7).
> 
>> Actually this modularization is not specific to QDEV
>> and can be used to all QOM right? I.e:
>>
>>   static const TypeInfo qcrypto_tls_creds_x509_info = {
>>       .parent = TYPE_QCRYPTO_TLS_CREDS,
>>       .name = TYPE_QCRYPTO_TLS_CREDS_X509,
>>       .module_name = "gnu-tls",
>>       ...
>>   }
> 
> Not as-is.  You'll need module load hooks in more places then and some
> code tweaks to move it from qdev level (loading hw-* module only) to qom
> level.
> 
> But, yes, moving the infrastructure to some qom-module.c file might be
> useful when modularizing non-device objects.  Do you have any candidates
> in mind?

So far I was only thinking of gnutls.
Gerd Hoffmann June 30, 2020, 4:07 p.m. UTC | #4
> >>   static const TypeInfo qcrypto_tls_creds_x509_info = {
> >>       .parent = TYPE_QCRYPTO_TLS_CREDS,
> >>       .name = TYPE_QCRYPTO_TLS_CREDS_X509,
> >>       .module_name = "gnu-tls",
> >>       ...
> >>   }
> > 
> > Not as-is.  You'll need module load hooks in more places then and some
> > code tweaks to move it from qdev level (loading hw-* module only) to qom
> > level.

[ note: v5 of the series does this ]

> > But, yes, moving the infrastructure to some qom-module.c file might be
> > useful when modularizing non-device objects.  Do you have any candidates
> > in mind?
> 
> So far I was only thinking of gnutls.

Looks challenging on a quick glance ...

take care,
  Gerd
Daniel P. Berrangé June 30, 2020, 4:24 p.m. UTC | #5
On Tue, Jun 30, 2020 at 06:07:51PM +0200, Gerd Hoffmann wrote:
> > >>   static const TypeInfo qcrypto_tls_creds_x509_info = {
> > >>       .parent = TYPE_QCRYPTO_TLS_CREDS,
> > >>       .name = TYPE_QCRYPTO_TLS_CREDS_X509,
> > >>       .module_name = "gnu-tls",
> > >>       ...
> > >>   }
> > > 
> > > Not as-is.  You'll need module load hooks in more places then and some
> > > code tweaks to move it from qdev level (loading hw-* module only) to qom
> > > level.
> 
> [ note: v5 of the series does this ]
> 
> > > But, yes, moving the infrastructure to some qom-module.c file might be
> > > useful when modularizing non-device objects.  Do you have any candidates
> > > in mind?
> > 
> > So far I was only thinking of gnutls.
> 
> Looks challenging on a quick glance ...

Yeah, I'm not convinced modularizing that is a good use of time. It is
plumbed in across multiple backends (chardev, migration, block, ui)
and also providing the secure RNG used across QEMU.

Regards,
Daniel
diff mbox series

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 7ce2588b89a3..ca555ede0710 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -59,6 +59,7 @@  common-obj-y += migration/
 common-obj-y += audio/
 common-obj-m += audio/
 common-obj-y += hw/
+common-obj-m += hw/
 
 common-obj-y += replay/
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6119e25e0eeb..d6459e4aa8e8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -172,6 +172,8 @@  static struct {
     const char *type;
     const char *mod;
 } const hwmodules[] = {
+    { .type = "ccid-card-passthru",    .mod = "usb-smartcard"         },
+    { .type = "ccid-card-emulated",    .mod = "usb-smartcard"         },
 };
 
 static bool qdev_module_loaded_all;
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 4cbe5e4e57d6..af8fd9a510ed 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -43,4 +43,5 @@  devices-dirs-y += smbios/
 endif
 
 common-obj-y += $(devices-dirs-y)
+common-obj-m += usb/
 obj-y += $(devices-dirs-y)
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index fa5c3fa1b877..3c5b3d4fadd3 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -29,11 +29,13 @@  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
 
 ifeq ($(CONFIG_USB_SMARTCARD),y)
 common-obj-y                          += dev-smartcard-reader.o
-common-obj-$(CONFIG_SMARTCARD)        += smartcard.mo
+ifeq ($(CONFIG_SMARTCARD),y)
+common-obj-m                          += smartcard.mo
 smartcard.mo-objs := ccid-card-passthru.o ccid-card-emulated.o
 smartcard.mo-cflags := $(SMARTCARD_CFLAGS)
 smartcard.mo-libs := $(SMARTCARD_LIBS)
 endif
+endif
 
 ifeq ($(CONFIG_POSIX),y)
 common-obj-$(CONFIG_USB_STORAGE_MTP)  += dev-mtp.o