* [RFC 0/3] Improve module accelerator error message
@ 2021-06-30 23:27 Jose R. Ziviani
2021-06-30 23:27 ` [RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host Jose R. Ziviani
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Jose R. Ziviani @ 2021-06-30 23:27 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, Jose R. Ziviani, kraxel, pbonzini, cfontana
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host
2021-06-30 23:27 [RFC 0/3] Improve module accelerator error message Jose R. Ziviani
@ 2021-06-30 23:27 ` Jose R. Ziviani
2021-06-30 23:27 ` [RFC 2/3] modules: Implement module_is_loaded function Jose R. Ziviani
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Jose R. Ziviani @ 2021-06-30 23:27 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, Jose R. Ziviani, kraxel, pbonzini, cfontana
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC 2/3] modules: Implement module_is_loaded function
2021-06-30 23:27 [RFC 0/3] Improve module accelerator error message Jose R. Ziviani
2021-06-30 23:27 ` [RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host Jose R. Ziviani
@ 2021-06-30 23:27 ` Jose R. Ziviani
2021-06-30 23:27 ` [RFC 3/3] qom: Improve error message in module_object_class_by_name() Jose R. Ziviani
2021-07-05 8:18 ` QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message) Claudio Fontana
3 siblings, 0 replies; 14+ messages in thread
From: Jose R. Ziviani @ 2021-06-30 23:27 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, Jose R. Ziviani, kraxel, pbonzini, cfontana
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-06-30 23:27 [RFC 0/3] Improve module accelerator error message Jose R. Ziviani
2021-06-30 23:27 ` [RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host Jose R. Ziviani
2021-06-30 23:27 ` [RFC 2/3] modules: Implement module_is_loaded function Jose R. Ziviani
@ 2021-06-30 23:27 ` Jose R. Ziviani
2021-07-19 15:29 ` Claudio Fontana
2021-07-21 9:54 ` Gerd Hoffmann
2021-07-05 8:18 ` QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message) Claudio Fontana
3 siblings, 2 replies; 14+ messages in thread
From: Jose R. Ziviani @ 2021-06-30 23:27 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, Jose R. Ziviani, kraxel, pbonzini, cfontana
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)
2021-06-30 23:27 [RFC 0/3] Improve module accelerator error message Jose R. Ziviani
` (2 preceding siblings ...)
2021-06-30 23:27 ` [RFC 3/3] qom: Improve error message in module_object_class_by_name() Jose R. Ziviani
@ 2021-07-05 8:18 ` Claudio Fontana
2021-07-21 10:35 ` Gerd Hoffmann
3 siblings, 1 reply; 14+ messages in thread
From: Claudio Fontana @ 2021-07-05 8:18 UTC (permalink / raw)
To: Jose R. Ziviani, qemu-devel
Cc: Peter Maydell, pbonzini, berrange, kraxel, ehabkost
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-06-30 23:27 ` [RFC 3/3] qom: Improve error message in module_object_class_by_name() Jose R. Ziviani
@ 2021-07-19 15:29 ` Claudio Fontana
2021-07-20 1:26 ` Jose R. Ziviani
2021-07-21 9:54 ` Gerd Hoffmann
1 sibling, 1 reply; 14+ messages in thread
From: Claudio Fontana @ 2021-07-19 15:29 UTC (permalink / raw)
To: Jose R. Ziviani, qemu-devel; +Cc: pbonzini, berrange, ehabkost, kraxel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-07-19 15:29 ` Claudio Fontana
@ 2021-07-20 1:26 ` Jose R. Ziviani
2021-07-20 6:40 ` Claudio Fontana
0 siblings, 1 reply; 14+ messages in thread
From: Jose R. Ziviani @ 2021-07-20 1:26 UTC (permalink / raw)
To: Claudio Fontana; +Cc: pbonzini, kraxel, berrange, qemu-devel, ehabkost
[-- 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 --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-07-20 1:26 ` Jose R. Ziviani
@ 2021-07-20 6:40 ` Claudio Fontana
0 siblings, 0 replies; 14+ messages in thread
From: Claudio Fontana @ 2021-07-20 6:40 UTC (permalink / raw)
To: Jose R. Ziviani; +Cc: pbonzini, kraxel, berrange, qemu-devel, ehabkost
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-06-30 23:27 ` [RFC 3/3] qom: Improve error message in module_object_class_by_name() Jose R. Ziviani
2021-07-19 15:29 ` Claudio Fontana
@ 2021-07-21 9:54 ` Gerd Hoffmann
2021-07-21 9:57 ` Daniel P. Berrangé
1 sibling, 1 reply; 14+ messages in thread
From: Gerd Hoffmann @ 2021-07-21 9:54 UTC (permalink / raw)
To: Jose R. Ziviani; +Cc: pbonzini, berrange, qemu-devel, ehabkost, cfontana
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-07-21 9:54 ` Gerd Hoffmann
@ 2021-07-21 9:57 ` Daniel P. Berrangé
2021-07-21 13:36 ` Jose R. Ziviani
0 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrangé @ 2021-07-21 9:57 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: pbonzini, cfontana, qemu-devel, ehabkost, Jose R. Ziviani
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 :|
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)
2021-07-05 8:18 ` QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message) Claudio Fontana
@ 2021-07-21 10:35 ` Gerd Hoffmann
2021-07-21 10:47 ` Claudio Fontana
0 siblings, 1 reply; 14+ messages in thread
From: Gerd Hoffmann @ 2021-07-21 10:35 UTC (permalink / raw)
To: Claudio Fontana
Cc: Peter Maydell, berrange, ehabkost, Jose R. Ziviani, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)
2021-07-21 10:35 ` Gerd Hoffmann
@ 2021-07-21 10:47 ` Claudio Fontana
2021-07-21 10:50 ` Claudio Fontana
0 siblings, 1 reply; 14+ messages in thread
From: Claudio Fontana @ 2021-07-21 10:47 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Peter Maydell, berrange, ehabkost, Jose R. Ziviani, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message)
2021-07-21 10:47 ` Claudio Fontana
@ 2021-07-21 10:50 ` Claudio Fontana
0 siblings, 0 replies; 14+ messages in thread
From: Claudio Fontana @ 2021-07-21 10:50 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Peter Maydell, berrange, ehabkost, Jose R. Ziviani, qemu-devel,
Philippe Mathieu-Daudé,
pbonzini, Alex Bennee
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 3/3] qom: Improve error message in module_object_class_by_name()
2021-07-21 9:57 ` Daniel P. Berrangé
@ 2021-07-21 13:36 ` Jose R. Ziviani
0 siblings, 0 replies; 14+ messages in thread
From: Jose R. Ziviani @ 2021-07-21 13:36 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: pbonzini, cfontana, Gerd Hoffmann, ehabkost, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-07-21 13:37 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-30 23:27 [RFC 0/3] Improve module accelerator error message Jose R. Ziviani
2021-06-30 23:27 ` [RFC 1/3] modules: Add CONFIG_TCG_MODULAR in config_host Jose R. Ziviani
2021-06-30 23:27 ` [RFC 2/3] modules: Implement module_is_loaded function Jose R. Ziviani
2021-06-30 23:27 ` [RFC 3/3] qom: Improve error message in module_object_class_by_name() Jose R. Ziviani
2021-07-19 15:29 ` Claudio Fontana
2021-07-20 1:26 ` Jose R. Ziviani
2021-07-20 6:40 ` Claudio Fontana
2021-07-21 9:54 ` Gerd Hoffmann
2021-07-21 9:57 ` Daniel P. Berrangé
2021-07-21 13:36 ` Jose R. Ziviani
2021-07-05 8:18 ` QEMU modules improvements objective (Was: Re: [RFC 0/3] Improve module accelerator error message) Claudio Fontana
2021-07-21 10:35 ` Gerd Hoffmann
2021-07-21 10:47 ` Claudio Fontana
2021-07-21 10:50 ` Claudio Fontana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).