[RFC 0/3] Improve module accelerator error message

[RFC 0/3] Improve module accelerator error message

[Jose R. Ziviani]

Hello!

I'm sending this as RFC because it's based on a patch still on
review[1], so I'd like to see if it makes sense.

Tt will improve the error message when an accelerator module could
not be loaded. Instead of the current assert error, a formated
message will be displayed.

[1] https://patchwork.kernel.org/project/qemu-devel/list/?series=506379

Jose R. Ziviani (3):
  modules: Add CONFIG_TCG_MODULAR in config_host
  modules: Implement module_is_loaded function
  qom: Improve error message in module_object_class_by_name()

 include/qemu/module.h |  3 +++
 meson.build           |  3 +++
 qom/object.c          | 30 ++++++++++++++++++++++++++++++
 util/module.c         | 28 +++++++++++++++++++++-------
 4 files changed, 57 insertions(+), 7 deletions(-)

-- 
2.32.0

[RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host

[Jose R. Ziviani]

CONFIG_TCG_MODULAR is a complement to CONFIG_MODULES, in order to
know if TCG will be a module, even if --enable-modules option was
set.

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 meson.build | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meson.build b/meson.build
index 2d72b8cc06..c37a2358d4 100644
--- a/meson.build
+++ b/meson.build
@@ -277,6 +277,9 @@ if not get_option('tcg').disabled()
 
   accelerators += 'CONFIG_TCG'
   config_host += { 'CONFIG_TCG': 'y' }
+  if is_tcg_modular
+    config_host += { 'CONFIG_TCG_MODULAR': 'y' }
+  endif
 endif
 
 if 'CONFIG_KVM' not in accelerators and get_option('kvm').enabled()
-- 
2.32.0

[RFC 2/3] modules: Implement module_is_loaded function

[Jose R. Ziviani]

The function module_load_one() fills a hash table will all modules that
were successfuly loaded. However, that table is a static variable of
module_load_one(). This patch changes it and creates a function that
informs whether a given module was loaded or not.

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 include/qemu/module.h |  3 +++
 util/module.c         | 28 +++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 456e190a55..01779cc7fb 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,6 +14,7 @@
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#include <stdbool.h>
 
 #define DSO_STAMP_FUN         glue(qemu_stamp, CONFIG_STAMP)
 #define DSO_STAMP_FUN_STR     stringify(DSO_STAMP_FUN)
@@ -74,6 +75,8 @@ void module_load_qom_one(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
 
+bool module_is_loaded(const char *name);
+
 /**
  * DOC: module info annotation macros
  *
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..64307b7a25 100644
--- a/util/module.c
+++ b/util/module.c
@@ -119,6 +119,8 @@ static const QemuModinfo module_info_stub[] = { {
 static const QemuModinfo *module_info = module_info_stub;
 static const char *module_arch;
 
+static GHashTable *loaded_modules;
+
 void module_init_info(const QemuModinfo *info)
 {
     module_info = info;
@@ -206,13 +208,10 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
 out:
     return ret;
 }
-#endif
 
 bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
 {
     bool success = false;
-
-#ifdef CONFIG_MODULES
     char *fname = NULL;
 #ifdef CONFIG_MODULE_UPGRADES
     char *version_dir;
@@ -223,7 +222,6 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     int i = 0, n_dirs = 0;
     int ret;
     bool export_symbols = false;
-    static GHashTable *loaded_modules;
     const QemuModinfo *modinfo;
     const char **sl;
 
@@ -307,12 +305,9 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
         g_free(dirs[i]);
     }
 
-#endif
     return success;
 }
 
-#ifdef CONFIG_MODULES
-
 static bool module_loaded_qom_all;
 
 void module_load_qom_one(const char *type)
@@ -377,6 +372,15 @@ void qemu_load_module_for_opts(const char *group)
     }
 }
 
+bool module_is_loaded(const char *name)
+{
+    if (!loaded_modules || !g_hash_table_contains(loaded_modules, name)) {
+        return false;
+    }
+
+    return true;
+}
+
 #else
 
 void module_allow_arch(const char *arch) {}
@@ -384,4 +388,14 @@ void qemu_load_module_for_opts(const char *group) {}
 void module_load_qom_one(const char *type) {}
 void module_load_qom_all(void) {}
 
+bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
+{
+    return false;
+}
+
+bool module_is_loaded(const char *name)
+{
+    return false;
+}
+
 #endif
-- 
2.32.0

[RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Jose R. Ziviani]

module_object_class_by_name() calls module_load_qom_one if the object
is provided by a dynamically linked library. Such library might not be
available at this moment - for instance, it can be a package not yet
installed. Thus, instead of assert error messages, this patch outputs
more friendly messages.

Current error messages:
$ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
...
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
[1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

New error message:
$ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
accel-tcg-x86_64 module is missing, install the package or config the library path correctly.

$ make check
...
Running test qtest-x86_64/test-filter-mirror
Running test qtest-x86_64/endianness-test
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
...

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 qom/object.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..2d40245af9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
     return type->class;
 }
 
+char *get_accel_module_name(const char *ac_name);
+
+char *get_accel_module_name(const char *ac_name)
+{
+    size_t len = strlen(ac_name);
+    char *module_name = NULL;
+
+    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
+#ifdef CONFIG_TCG_MODULAR
+        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
+#endif
+    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
+        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
+    }
+
+    return module_name;
+}
+
 ObjectClass *module_object_class_by_name(const char *typename)
 {
     ObjectClass *oc;
@@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
     oc = object_class_by_name(typename);
 #ifdef CONFIG_MODULES
     if (!oc) {
+        char *module_name;
         module_load_qom_one(typename);
         oc = object_class_by_name(typename);
+        module_name = get_accel_module_name(typename);
+        if (module_name) {
+            if (!module_is_loaded(module_name)) {
+                fprintf(stderr, "%s module is missing, install the "
+                                "package or config the library path "
+                                "correctly.\n", module_name);
+                g_free(module_name);
+                exit(1);
+            }
+            g_free(module_name);
+        }
     }
 #endif
     return oc;
-- 
2.32.0

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Claudio Fontana]

On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
> module_object_class_by_name() calls module_load_qom_one if the object
> is provided by a dynamically linked library. Such library might not be
> available at this moment - for instance, it can be a package not yet
> installed. Thus, instead of assert error messages, this patch outputs
> more friendly messages.
> 
> Current error messages:
> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> ...
> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> 
> New error message:
> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> 
> $ make check
> ...
> Running test qtest-x86_64/test-filter-mirror
> Running test qtest-x86_64/endianness-test
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> ...
> 
> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> ---
>  qom/object.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 6a01d56546..2d40245af9 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>  
> +char *get_accel_module_name(const char *ac_name);
> +
> +char *get_accel_module_name(const char *ac_name)
> +{
> +    size_t len = strlen(ac_name);
> +    char *module_name = NULL;
> +
> +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
> +#ifdef CONFIG_TCG_MODULAR
> +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
> +#endif
> +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
> +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
> +    }
> +
> +    return module_name;
> +}
> +
>  ObjectClass *module_object_class_by_name(const char *typename)
>  {
>      ObjectClass *oc;
> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>      oc = object_class_by_name(typename);
>  #ifdef CONFIG_MODULES
>      if (!oc) {
> +        char *module_name;
>          module_load_qom_one(typename);
>          oc = object_class_by_name(typename);
> +        module_name = get_accel_module_name(typename);
> +        if (module_name) {
> +            if (!module_is_loaded(module_name)) {
> +                fprintf(stderr, "%s module is missing, install the "
> +                                "package or config the library path "
> +                                "correctly.\n", module_name);
> +                g_free(module_name);
> +                exit(1);
> +            }
> +            g_free(module_name);
> +        }
>      }
>  #endif
>      return oc;
> 

Hi Jose,

this inserts accel logic into module_object_class_by_name,
maybe some other place would be better to insert this check,
like when trying to select an accelerator?

Thanks,

CLaudio

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 4264 bytes --]

On Mon, Jul 19, 2021 at 05:29:49PM +0200, Claudio Fontana wrote:
> On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
> > module_object_class_by_name() calls module_load_qom_one if the object
> > is provided by a dynamically linked library. Such library might not be
> > available at this moment - for instance, it can be a package not yet
> > installed. Thus, instead of assert error messages, this patch outputs
> > more friendly messages.
> > 
> > Current error messages:
> > $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> > ...
> > ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> > 
> > New error message:
> > $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > 
> > $ make check
> > ...
> > Running test qtest-x86_64/test-filter-mirror
> > Running test qtest-x86_64/endianness-test
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > ...
> > 
> > Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> > ---
> >  qom/object.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/qom/object.c b/qom/object.c
> > index 6a01d56546..2d40245af9 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
> >      return type->class;
> >  }
> >  
> > +char *get_accel_module_name(const char *ac_name);
> > +
> > +char *get_accel_module_name(const char *ac_name)
> > +{
> > +    size_t len = strlen(ac_name);
> > +    char *module_name = NULL;
> > +
> > +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
> > +#ifdef CONFIG_TCG_MODULAR
> > +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
> > +#endif
> > +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
> > +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
> > +    }
> > +
> > +    return module_name;
> > +}
> > +
> >  ObjectClass *module_object_class_by_name(const char *typename)
> >  {
> >      ObjectClass *oc;
> > @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
> >      oc = object_class_by_name(typename);
> >  #ifdef CONFIG_MODULES
> >      if (!oc) {
> > +        char *module_name;
> >          module_load_qom_one(typename);
> >          oc = object_class_by_name(typename);
> > +        module_name = get_accel_module_name(typename);
> > +        if (module_name) {
> > +            if (!module_is_loaded(module_name)) {
> > +                fprintf(stderr, "%s module is missing, install the "
> > +                                "package or config the library path "
> > +                                "correctly.\n", module_name);
> > +                g_free(module_name);
> > +                exit(1);
> > +            }
> > +            g_free(module_name);
> > +        }
> >      }
> >  #endif
> >      return oc;
> > 
> 
> Hi Jose,
> 
> this inserts accel logic into module_object_class_by_name,
> maybe some other place would be better to insert this check,
> like when trying to select an accelerator?


Hello Claudio,

Yes, I think that 'get_accel_module_name()' may be a more generic
'get_module_name()' to handle any module failure (not only
accelerators).

I'll improve it and send a v2. Thank you for reviewing it.

> 
> Thanks,
> 
> CLaudio

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Claudio Fontana]

On 7/20/21 3:26 AM, Jose R. Ziviani wrote:
> On Mon, Jul 19, 2021 at 05:29:49PM +0200, Claudio Fontana wrote:
>> On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
>>> module_object_class_by_name() calls module_load_qom_one if the object
>>> is provided by a dynamically linked library. Such library might not be
>>> available at this moment - for instance, it can be a package not yet
>>> installed. Thus, instead of assert error messages, this patch outputs
>>> more friendly messages.
>>>
>>> Current error messages:
>>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>>> ...
>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>>
>>> New error message:
>>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>>
>>> $ make check
>>> ...
>>> Running test qtest-x86_64/test-filter-mirror
>>> Running test qtest-x86_64/endianness-test
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>> ...
>>>
>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>>> ---
>>>  qom/object.c | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 6a01d56546..2d40245af9 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
>>>      return type->class;
>>>  }
>>>  
>>> +char *get_accel_module_name(const char *ac_name);
>>> +
>>> +char *get_accel_module_name(const char *ac_name)
>>> +{
>>> +    size_t len = strlen(ac_name);
>>> +    char *module_name = NULL;
>>> +
>>> +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
>>> +#ifdef CONFIG_TCG_MODULAR
>>> +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
>>> +#endif
>>> +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
>>> +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
>>> +    }
>>> +
>>> +    return module_name;
>>> +}
>>> +
>>>  ObjectClass *module_object_class_by_name(const char *typename)
>>>  {
>>>      ObjectClass *oc;
>>> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>>>      oc = object_class_by_name(typename);
>>>  #ifdef CONFIG_MODULES
>>>      if (!oc) {
>>> +        char *module_name;
>>>          module_load_qom_one(typename);
>>>          oc = object_class_by_name(typename);
>>> +        module_name = get_accel_module_name(typename);
>>> +        if (module_name) {
>>> +            if (!module_is_loaded(module_name)) {
>>> +                fprintf(stderr, "%s module is missing, install the "
>>> +                                "package or config the library path "
>>> +                                "correctly.\n", module_name);
>>> +                g_free(module_name);
>>> +                exit(1);
>>> +            }
>>> +            g_free(module_name);
>>> +        }
>>>      }
>>>  #endif
>>>      return oc;
>>>
>>
>> Hi Jose,
>>
>> this inserts accel logic into module_object_class_by_name,
>> maybe some other place would be better to insert this check,
>> like when trying to select an accelerator?
> 
> 
> Hello Claudio,
> 
> Yes, I think that 'get_accel_module_name()' may be a more generic
> 'get_module_name()' to handle any module failure (not only
> accelerators).
> 
> I'll improve it and send a v2. Thank you for reviewing it.
> 

I also wonder, and @Gerd , should "tcg_enabled()", tcg_allowed etc as a mechanism be rethought,
in light of modular TCG?

Probably would be easier to do if we had a general mechanism for CONFIG_TCG=m CONFIG_KVM=m ... which can separately be set as modules.

Thanks,

Claudio

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Gerd Hoffmann]

>  ObjectClass *module_object_class_by_name(const char *typename)
>  {
>      ObjectClass *oc;
> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>      oc = object_class_by_name(typename);
>  #ifdef CONFIG_MODULES
>      if (!oc) {
> +        char *module_name;
>          module_load_qom_one(typename);
>          oc = object_class_by_name(typename);
> +        module_name = get_accel_module_name(typename);
> +        if (module_name) {
> +            if (!module_is_loaded(module_name)) {
> +                fprintf(stderr, "%s module is missing, install the "
> +                                "package or config the library path "
> +                                "correctly.\n", module_name);
> +                g_free(module_name);
> +                exit(1);
> +            }
> +            g_free(module_name);
> +        }

This error logging should IMHO be moved to util/module.c.  Either have a
helper function to print the error message, or have
module_load_qom_one() print it.

There is also no need to hard-code the module names.  We have the module
database and module_load_qom_one() uses it to figure which module must
be loaded for a specific qom object.  We can likewise use the database
for printing the error message.

take care,
  Gerd

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Daniel P. Berrangé]

On Wed, Jul 21, 2021 at 11:54:45AM +0200, Gerd Hoffmann wrote:
> >  ObjectClass *module_object_class_by_name(const char *typename)
> >  {
> >      ObjectClass *oc;
> > @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
> >      oc = object_class_by_name(typename);
> >  #ifdef CONFIG_MODULES
> >      if (!oc) {
> > +        char *module_name;
> >          module_load_qom_one(typename);
> >          oc = object_class_by_name(typename);
> > +        module_name = get_accel_module_name(typename);
> > +        if (module_name) {
> > +            if (!module_is_loaded(module_name)) {
> > +                fprintf(stderr, "%s module is missing, install the "
> > +                                "package or config the library path "
> > +                                "correctly.\n", module_name);
> > +                g_free(module_name);
> > +                exit(1);
> > +            }
> > +            g_free(module_name);
> > +        }
> 
> This error logging should IMHO be moved to util/module.c.  Either have a
> helper function to print the error message, or have
> module_load_qom_one() print it.
> 
> There is also no need to hard-code the module names.  We have the module
> database and module_load_qom_one() uses it to figure which module must
> be loaded for a specific qom object.  We can likewise use the database
> for printing the error message.

IIUC, module loading can be triggered from hotplug of backends/devices,
If so, we really shouldn't be printing to stderr, but using Error *errp
throughout, so that QMP can give back useful error messages


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()

[Jose R. Ziviani]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

On Wed, Jul 21, 2021 at 10:57:37AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 21, 2021 at 11:54:45AM +0200, Gerd Hoffmann wrote:
> > >  ObjectClass *module_object_class_by_name(const char *typename)
> > >  {
> > >      ObjectClass *oc;
> > > @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
> > >      oc = object_class_by_name(typename);
> > >  #ifdef CONFIG_MODULES
> > >      if (!oc) {
> > > +        char *module_name;
> > >          module_load_qom_one(typename);
> > >          oc = object_class_by_name(typename);
> > > +        module_name = get_accel_module_name(typename);
> > > +        if (module_name) {
> > > +            if (!module_is_loaded(module_name)) {
> > > +                fprintf(stderr, "%s module is missing, install the "
> > > +                                "package or config the library path "
> > > +                                "correctly.\n", module_name);
> > > +                g_free(module_name);
> > > +                exit(1);
> > > +            }
> > > +            g_free(module_name);
> > > +        }
> > 
> > This error logging should IMHO be moved to util/module.c.  Either have a
> > helper function to print the error message, or have
> > module_load_qom_one() print it.
> > 
> > There is also no need to hard-code the module names.  We have the module
> > database and module_load_qom_one() uses it to figure which module must
> > be loaded for a specific qom object.  We can likewise use the database
> > for printing the error message.
> 
> IIUC, module loading can be triggered from hotplug of backends/devices,
> If so, we really shouldn't be printing to stderr, but using Error *errp
> throughout, so that QMP can give back useful error messages

Thank you Gerd and Daniel,

I'll improve it and send a v2.

Thank you very much,

Jose

> 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)

[Claudio Fontana]

On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
> Hello!
> 
> I'm sending this as RFC because it's based on a patch still on
> review[1], so I'd like to see if it makes sense.
> 
> Tt will improve the error message when an accelerator module could
> not be loaded. Instead of the current assert error, a formated
> message will be displayed.
> 
> [1] https://patchwork.kernel.org/project/qemu-devel/list/?series=506379
> 
> Jose R. Ziviani (3):
>   modules: Add CONFIG_TCG_MODULAR in config_host
>   modules: Implement module_is_loaded function
>   qom: Improve error message in module_object_class_by_name()
> 
>  include/qemu/module.h |  3 +++
>  meson.build           |  3 +++
>  qom/object.c          | 30 ++++++++++++++++++++++++++++++
>  util/module.c         | 28 +++++++++++++++++++++-------
>  4 files changed, 57 insertions(+), 7 deletions(-)
> 

Open question to all,

why don't we have/add the ability to configure

CONFIG_XXX=m

for all potentially modular pieces?

It should be possible to say, I want to build the storage plugins as modules, TCG I would like it built-in, and KVM as a module,
or any combination of these.

The most useful combination I see for virtualization use of qemu is with TCG as a module (M), KVM as built-in (Y), and various other optional pieces as modules (M).

Should this not be the vision? To me it looks that way.

Thanks,

Claudio

Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)

[Gerd Hoffmann]

  Hi,

> Open question to all,
> 
> why don't we have/add the ability to configure
> 
> CONFIG_XXX=m
> 
> for all potentially modular pieces?
> 
> It should be possible to say, I want to build the storage plugins as modules, TCG I would like it built-in, and KVM as a module,
> or any combination of these.
>
> The most useful combination I see for virtualization use of qemu is with TCG as a module (M), KVM as built-in (Y), and various other optional pieces as modules (M).

Surely doable.  Comes with maintenance and testing cost though.

For example you'll get new kinds of dependencies: when building foo as
module stuff depending on foo must be built modular too (spice-core=m +
qxl=y doesn't work).

I see mainly two use cases:

  (1) distro builds.  Those would enable most features and also modules
      for fine-grained rpm/deb packaging.

  (2) builds for specific use cases.  Those would disable modules and
      just use CONFIG_FOO=n for things they don't need.

Being able to set CONFIG_FOO=y for features used in >90% of the use
cases (kvm, some virtio devices come to mind) might be useful for (1).
Distros do that with linux kernel builds too (Fedora kernel has
CONFIG_SATA_AHCI=y, CONFIG_USB_XHCI_PCI=y, ...).

But the question is: Are the benefits worth the effort?

take care,
  Gerd

Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)

[Claudio Fontana]

On 7/21/21 12:35 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> Open question to all,
>>
>> why don't we have/add the ability to configure
>>
>> CONFIG_XXX=m
>>
>> for all potentially modular pieces?
>>
>> It should be possible to say, I want to build the storage plugins as modules, TCG I would like it built-in, and KVM as a module,
>> or any combination of these.
>>
>> The most useful combination I see for virtualization use of qemu is with TCG as a module (M), KVM as built-in (Y), and various other optional pieces as modules (M).
> 
> Surely doable.  Comes with maintenance and testing cost though.
> 
> For example you'll get new kinds of dependencies: when building foo as
> module stuff depending on foo must be built modular too (spice-core=m +
> qxl=y doesn't work).
> 
> I see mainly two use cases:
> 
>   (1) distro builds.  Those would enable most features and also modules
>       for fine-grained rpm/deb packaging.
> 
>   (2) builds for specific use cases.  Those would disable modules and
>       just use CONFIG_FOO=n for things they don't need.
> 
> Being able to set CONFIG_FOO=y for features used in >90% of the use
> cases (kvm, some virtio devices come to mind) might be useful for (1).
> Distros do that with linux kernel builds too (Fedora kernel has
> CONFIG_SATA_AHCI=y, CONFIG_USB_XHCI_PCI=y, ...).
> 
> But the question is: Are the benefits worth the effort?
> 
> take care,
>   Gerd
> 

I generally agree with your use cases as we see it right now from a distro perspective, I suspect there are more,
especially thinking of modeling, testing/builds etc on the TCG side of things.

I think that eventually we will end up there anyway due to the requirements being so vastly different for all possible uses of QEMU.

Doing a proper design of this will allow I think to come to the right conclusions on how to correctly check for accelerators etc,
without creating a one-off solution for each single feature.

KConfig should probably be driving this from day 1 right?

Yeah, it's tough, but I think we would otherwise just drive circles around this, implement a lot of provisional stuff,
and still end up there sooner or later in my opinion.

Ciao,

CLaudio

Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)

[Claudio Fontana]

On 7/21/21 12:47 PM, Claudio Fontana wrote:
> On 7/21/21 12:35 PM, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> Open question to all,
>>>
>>> why don't we have/add the ability to configure
>>>
>>> CONFIG_XXX=m
>>>
>>> for all potentially modular pieces?
>>>
>>> It should be possible to say, I want to build the storage plugins as modules, TCG I would like it built-in, and KVM as a module,
>>> or any combination of these.
>>>
>>> The most useful combination I see for virtualization use of qemu is with TCG as a module (M), KVM as built-in (Y), and various other optional pieces as modules (M).
>>
>> Surely doable.  Comes with maintenance and testing cost though.
>>
>> For example you'll get new kinds of dependencies: when building foo as
>> module stuff depending on foo must be built modular too (spice-core=m +
>> qxl=y doesn't work).
>>
>> I see mainly two use cases:
>>
>>   (1) distro builds.  Those would enable most features and also modules
>>       for fine-grained rpm/deb packaging.
>>
>>   (2) builds for specific use cases.  Those would disable modules and
>>       just use CONFIG_FOO=n for things they don't need.
>>
>> Being able to set CONFIG_FOO=y for features used in >90% of the use
>> cases (kvm, some virtio devices come to mind) might be useful for (1).
>> Distros do that with linux kernel builds too (Fedora kernel has
>> CONFIG_SATA_AHCI=y, CONFIG_USB_XHCI_PCI=y, ...).
>>
>> But the question is: Are the benefits worth the effort?
>>
>> take care,
>>   Gerd
>>
> 
> I generally agree with your use cases as we see it right now from a distro perspective, I suspect there are more,
> especially thinking of modeling, testing/builds etc on the TCG side of things.
> 
> I think that eventually we will end up there anyway due to the requirements being so vastly different for all possible uses of QEMU.
> 
> Doing a proper design of this will allow I think to come to the right conclusions on how to correctly check for accelerators etc,
> without creating a one-off solution for each single feature.
> 
> KConfig should probably be driving this from day 1 right?


Before this, though, the KConfig stuff should be all-ok for ARM and possibly other archs, I am not sure where we are there..

> 
> Yeah, it's tough, but I think we would otherwise just drive circles around this, implement a lot of provisional stuff,
> and still end up there sooner or later in my opinion.
> 
> Ciao,
> 
> CLaudio