[Qemu-devel] [Bug 1655708] Re: target/ppc/int_helper.c:2806: strange expression ?

[Jose R. Ziviani]

** Changed in: qemu
 Assignee: (unassigned) => Jose R. Ziviani (jrziviani)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1655708

Title:
  target/ppc/int_helper.c:2806: strange expression ?

Status in QEMU:
  New

Bug description:
  target/ppc/int_helper.c:2806:25: warning: ‘*’ in boolean context,
  suggest ‘&&’ instead [-Wint-in-bool-context]

  Source code is

 zone_digit = (i * 2) ? b->u8[BCD_DIG_BYTE(i * 2)] >> 4 :
  zone_lead;

  Which I read as

 zone_digit = (i * 2) ? (b->u8[BCD_DIG_BYTE(i * 2)] >> 4) :
  zone_lead;

  so I think the compiler warning is for the i * 2 lhs of the ?.

  I am not sure what to suggest as a bugfix.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1655708/+subscriptions

[PATCH] tcg/arm: Fix tcg_out_op function signature

[Jose R. Ziviani]

Commit 5e8892db93 fixed several function signatures but tcg_out_op for
arm is missing. This patch fixes it as well.

Signed-off-by: Jose R. Ziviani 
---
 tcg/arm/tcg-target.c.inc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tcg/arm/tcg-target.c.inc b/tcg/arm/tcg-target.c.inc
index f4c9cb8f9f..5157143246 100644
--- a/tcg/arm/tcg-target.c.inc
+++ b/tcg/arm/tcg-target.c.inc
@@ -1984,7 +1984,8 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg 
*args, bool is64)
 static void tcg_out_epilogue(TCGContext *s);
 
 static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
-const TCGArg *args, const int *const_args)
+const TCGArg args[TCG_MAX_OP_ARGS],
+const int const_args[TCG_MAX_OP_ARGS])
 {
 TCGArg a0, a1, a2, a3, a4, a5;
 int c;
-- 
2.31.1

Re: [PATCH v3 03/24] modules: generate modinfo.c

[Jose R. Ziviani]

Hello,

Just a small change.

On Fri, Jun 18, 2021 at 06:53:32AM +0200, Gerd Hoffmann wrote:
> Add script to generate C source with a small
> database containing the module meta-data.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  scripts/modinfo-generate.py | 84 +
>  include/qemu/module.h   | 17 
>  softmmu/vl.c|  4 ++
>  util/module.c   | 11 +
>  meson.build | 13 +-
>  5 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100755 scripts/modinfo-generate.py
> 
> diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
> new file mode 100755
> index ..2b925432655a
> --- /dev/null
> +++ b/scripts/modinfo-generate.py
> @@ -0,0 +1,84 @@
> +#!/usr/bin/env python3
> +# -*- coding: utf-8 -*-
> +
> +import os
> +import sys
> +
> +def print_array(name, values):
> +if len(values) == 0:
> +return
> +list = ", ".join(values)
> +print(".%s = ((const char*[]){ %s, NULL })," % (name, list))
> +
> +def parse_line(line):
> +kind = ""
> +data = ""
> +get_kind = False
> +get_data = False
> +for item in line.split():
> +if item == "MODINFO_START":
> +get_kind = True
> +continue
> +if item.startswith("MODINFO_END"):
> +get_data = False
> +continue
> +if get_kind:
> +kind = item
> +get_kind = False
> +get_data = True
> +continue
> +if get_data:
> +data += " " + item
> +continue
> +return (kind, data)
> +
> +def generate(name, lines):
> +arch = ""
> +objs = []
> +deps = []
> +opts = []
> +for line in lines:
> +if line.find("MODINFO_START") != -1:
> +(kind, data) = parse_line(line)
> +if kind == 'obj':
> +objs.append(data)
> +elif kind == 'dep':
> +deps.append(data)
> +elif kind == 'opts':
> +opts.append(data)
> +elif kind == 'arch':
> +arch = data;
> +else:
> +print("unknown:", kind)
> +exit(1)
> +
> +print(".name = \"%s\"," % name)
> +if arch != "":
> +print(".arch = %s," % arch)
> +print_array("objs", objs)
> +print_array("deps", deps)
> +print_array("opts", opts)
> +print("},{");
> +
> +def print_pre():
> +print("/* generated by scripts/modinfo.py */")

generated by scripts/modinfo-generate.py

> +print("#include \"qemu/osdep.h\"")
> +print("#include \"qemu/module.h\"")
> +print("const QemuModinfo qemu_modinfo[] = {{")
> +
> +def print_post():
> +print("/* end of list */")
> +print("}};")
> +
> +def main(args):
> +print_pre()
> +for modinfo in args:
> +with open(modinfo) as f:
> +lines = f.readlines()
> +print("/* %s */" % modinfo)
> +(basename, ext) = os.path.splitext(modinfo)
> +generate(basename, lines)
> +print_post()
> +
> +if __name__ == "__main__":
> +main(sys.argv[1:])
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 81ef086da023..a98748d501d3 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -98,4 +98,21 @@ void module_load_qom_all(void);
>  /* module registers QemuOpts  */
>  #define module_opts(name) modinfo(opts, name)
>  
> +/*
> + * module info database
> + *
> + * scripts/modinfo-generate.c will build this using the data collected
> + * by scripts/modinfo-collect.py
> + */
> +typedef struct QemuModinfo QemuModinfo;
> +struct QemuModinfo {
> +const char *name;
> +const char *arch;
> +const char **objs;
> +const char **deps;
> +const char **opts;
> +};
> +extern const QemuModinfo qemu_modinfo[];
> +void module_init_info(const QemuModinfo *info);
> +
>  #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 326c1e908008..a4857ec43ff3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2740,6 +2740,10 @@ void qemu_init(int argc, char **argv, char **envp)
>  error_init(argv[0]);
>  qemu_init_exec_dir(argv[0]);
>  
> +#ifdef CONFIG_MODULES
> +module_init_info(qemu_modinfo);
> +#endif
> +
>  qemu_init_subsystems();
>  
>  /* first pass of option parsing */
> diff --git a/util/module.c b/util/module.c
> index eee8ff2de136..8d3e8275b9f7 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -110,6 +110,17 @@ void module_call_init(module_init_type type)
>  }
>  
>  #ifdef CONFIG_MODULES
> +
> +static const QemuModinfo module_info_stub[] = { {
> +/* end of list */
> +} };
> +static const QemuModinfo *module_info = module_info_stub;
> +
> +void module_init_info(const QemuModinfo *info)
> +{
> +module_info = info;
> +}
> +
>  static int module_load_file(const char *fname, bool mayfail, bool 
> export_symbols)
>  {
>  GModule *g_module;
> diff --git a/meson.build 

Re: [PATCH 3/4] modules: module.h kerneldoc annotations

[Jose R. Ziviani]

Hello Gerd,

On Tue, Jun 22, 2021 at 02:51:09PM +0200, Gerd Hoffmann wrote:
> ---
>  include/qemu/module.h | 59 +--
>  1 file changed, 45 insertions(+), 14 deletions(-)

This header has a copyright date from 2009. Not sure if it requires an
update.

> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 7f4b1af8198c..8bc80535a4d4 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -74,11 +74,18 @@ void module_load_qom_one(const char *type);
>  void module_load_qom_all(void);
>  void module_allow_arch(const char *arch);
>  
> -/*
> - * module info annotation macros
> +/**
> + * DOC: module info annotation macros
>   *
> - * scripts/modinfo-collect.py will collect module info,
> - * using the preprocessor and -DQEMU_MODINFO
> + * `scripts/modinfo-collect.py` will collect module info,
> + * using the preprocessor and -DQEMU_MODINFO.
> + *
> + * `scripts/modinfo-generate.py` will create a module meta-data database
> + * from the collected information so qemu knows about module
> + * dependencies and QOM objects implemented by modules.
> + *
> + * See `*.modinfo` and `modinfo.c` in the build directory to check the
> + * script results.
>   */
>  #ifdef QEMU_MODINFO
>  # define modinfo(kind, value) \
> @@ -87,24 +94,48 @@ void module_allow_arch(const char *arch);
>  # define modinfo(kind, value)
>  #endif
>  
> -/* module implements QOM type  */
> +/**
> + * module_obj
> + *
> + * @name: QOM type.
> + *
> + * This module implements QOM type @name.
> + */
>  #define module_obj(name) modinfo(obj, name)
>  
> -/* module has a dependency on  */
> +/**
> + * module_dep
> + *
> + * @name: module name
> + *
> + * This module depends on module @name.
> + */
>  #define module_dep(name) modinfo(dep, name)
>  
> -/* module is for target architecture  */
> +/**
> + * module_arch
> + *
> + * @arch: target architecture
> + *
> + * This module is for target architecture @arch.
> + *
> + * Note that target-dependent modules are tagged automatically, so
> + * this is only needed in case target-independent modules should be
> + * restricted.  Use case example: the ccw bus is implemented by s390x
> + * only.
> + */
>  #define module_arch(name) modinfo(arch, name)
>  
> -/* module registers QemuOpts  */
> +/**
> + * module_opts
> + *
> + * @name: QemuOpts name
> + *
> + * This module registers QemuOpts @name.
> + */
>  #define module_opts(name) modinfo(opts, name)
>  
> -/*
> - * module info database
> - *
> - * scripts/modinfo-generate.c will build this using the data collected
> - * by scripts/modinfo-collect.py
> - */
> +/* module info database (created by scripts/modinfo-generate.py) */
>  typedef struct QemuModinfo QemuModinfo;
>  struct QemuModinfo {
>  const char *name;
> -- 
> 2.31.1
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH v3 03/24] modules: generate modinfo.c

[Jose R. Ziviani]

);
>  /* module registers QemuOpts  */
>  #define module_opts(name) modinfo(opts, name)
>  
> +/*
> + * module info database
> + *
> + * scripts/modinfo-generate.c will build this using the data collected
> + * by scripts/modinfo-collect.py
> + */
> +typedef struct QemuModinfo QemuModinfo;
> +struct QemuModinfo {
> +const char *name;
> +const char *arch;
> +const char **objs;
> +const char **deps;
> +const char **opts;
> +};
> +extern const QemuModinfo qemu_modinfo[];
> +void module_init_info(const QemuModinfo *info);
> +
>  #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 326c1e908008..a4857ec43ff3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2740,6 +2740,10 @@ void qemu_init(int argc, char **argv, char **envp)
>  error_init(argv[0]);
>  qemu_init_exec_dir(argv[0]);
>  
> +#ifdef CONFIG_MODULES
> +module_init_info(qemu_modinfo);
> +#endif
> +
>  qemu_init_subsystems();
>  
>  /* first pass of option parsing */
> diff --git a/util/module.c b/util/module.c
> index eee8ff2de136..8d3e8275b9f7 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -110,6 +110,17 @@ void module_call_init(module_init_type type)
>  }
>  
>  #ifdef CONFIG_MODULES
> +
> +static const QemuModinfo module_info_stub[] = { {
> +/* end of list */
> +} };
> +static const QemuModinfo *module_info = module_info_stub;
> +
> +void module_init_info(const QemuModinfo *info)
> +{
> +module_info = info;
> +}
> +
>  static int module_load_file(const char *fname, bool mayfail, bool 
> export_symbols)
>  {
>  GModule *g_module;
> diff --git a/meson.build b/meson.build
> index bb99619257d5..9cf50a50d39a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2022,6 +2022,7 @@ subdir('tests/qtest/fuzz')
>  
>  
>  modinfo_collect = find_program('scripts/modinfo-collect.py')
> +modinfo_generate = find_program('scripts/modinfo-generate.py')
>  modinfo_files = []
>  
>  block_mods = []
> @@ -2042,7 +2043,6 @@ foreach d, list : modules
> output: d + '-' + m + '.modinfo',
> input: module_ss.sources(),
> capture: true,
> -   build_by_default: true, # to be 
> removed when added to a target
> command: [modinfo_collect, '@INPUT@'])
>endif
>  else
> @@ -2055,6 +2055,17 @@ foreach d, list : modules
>endforeach
>  endforeach
>  
> +if enable_modules
> +  modinfo_src = custom_target('modinfo.c',
> +  output: 'modinfo.c',
> +  input: modinfo_files,
> +  command: [modinfo_generate, '@INPUT@'],
> +  capture: true)
> +  modinfo_lib = static_library('modinfo', modinfo_src)
> +  modinfo_dep = declare_dependency(link_whole: modinfo_lib)
> +  softmmu_ss.add(modinfo_dep)
> +endif
> +
>  nm = find_program('nm')
>  undefsym = find_program('scripts/undefsym.py')
>  block_syms = custom_target('block.syms', output: 'block.syms',
> -- 
> 2.31.1
> 
> 
From 0e6866bcae2d6576e7dbc08d8bbd2837f655d5a3 Mon Sep 17 00:00:00 2001
From: "Jose R. Ziviani" 
Date: Tue, 22 Jun 2021 17:37:42 -0300
Subject: [PATCH] modules: check if all dependencies can be satisfied

Verifies if all dependencies are correctly listed in the modinfo.c too
and stop the builds if they're not.

Signed-off-by: Jose R. Ziviani 
---
 scripts/modinfo-generate.py | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index 2b92543265..a36ddb77dd 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -59,6 +59,7 @@ def generate(name, lines):
 print_array("deps", deps)
 print_array("opts", opts)
 print("},{");
+return deps
 
 def print_pre():
 print("/* generated by scripts/modinfo.py */")
@@ -71,14 +72,26 @@ def print_post():
 print("}};")
 
 def main(args):
+deps = {}
 print_pre()
 for modinfo in args:
 with open(modinfo) as f:
 lines = f.readlines()
 print("/* %s */" % modinfo)
 (basename, ext) = os.path.splitext(modinfo)
-generate(basename, lines)
+deps[basename] = generate(basename, lines)
 print_post()
 
+flattened_deps = {flat.strip('" ') for dep in deps.values() for flat in dep}
+error = False
+for dep in flattened_deps:
+if dep not in deps.keys():
+print("Dependency {} cannot be satisfied".format(dep),
+  file=sys.stderr)
+error = True
+
+if error:
+exit(1)
+
 if __name__ == "__main__":
 main(sys.argv[1:])
-- 
2.32.0



signature.asc
Description: Digital signature

Re: [PATCH v4 00/34] modules: add meta-data database

[Jose R. Ziviani]

Hello Gerd,

Reviewed and tested successfully here. Thank you!

Reviewed-by: Jose R. Ziviani 


On Thu, Jun 24, 2021 at 12:38:02PM +0200, Gerd Hoffmann wrote:
> This patch series adds support for module meta-data.  Today this is
> either hard-coded in qemu (see qemu_load_module_for_opts) or handled
> with manually maintained lists in util/module (see module_deps[] and
> qom_modules[]).  This series replaced that scheme with annotation
> macros, so the meta-data can go into the module source code and -- for
> example -- the module_obj() annotations can go next to the TypeInfo
> struct for the object class.
> 
> Patches 1-3 put the infrastructure in place:  Add the annotation macros,
> add a script to collect the meta-data, add a script to compile the
> meta-data into C source code which we can then add to qemu.
> 
> Patch 4 - check module dependencies (Jose, new in v4).
> 
> Patches 5-13 add annotations macros to the modules we have.
> 
> Patches 14-16 put the modinfo database into use and remove the
> module_deps[] and qom_modules[] lists.
> 
> Patch 16 adds two tracepoints for easier trouble-shooting.
> 
> Patches 18-20 add support for target-specific modules.
> 
> Patches 21-24 add documentation for all of the above (new in v4, was
> separate series).
> 
> Patches 25-29 start building accelerators modular.  So far it is
> only qtest (all archs) and a small fraction of tcg (x86 only).
> 
> Patches 30-34 add support for registering hmp commands so they can
> be implemented as module (new in v4, was separate series).
> 
> take care,
>   Gerd
> 
> Gerd Hoffmann (33):
>   modules: add modinfo macros
>   modules: collect module meta-data
>   modules: generate modinfo.c
>   modules: add qxl module annotations
>   modules: add virtio-gpu module annotations
>   modules: add chardev module annotations
>   modules: add audio module annotations
>   modules: add usb-redir module annotations
>   modules: add ccid module annotations
>   modules: add ui module annotations
>   modules: add s390x module annotations
>   modules: add block module annotations
>   modules: use modinfo for dependencies
>   modules: use modinfo for qom load
>   modules: use modinfo for qemu opts load
>   modules: add tracepoints
>   modules: check arch and block load on mismatch
>   modules: check arch on qom lookup
>   modules: target-specific module build infrastructure
>   modules: add documentation for module sourcesets
>   modules: add module_obj() note to QOM docs
>   modules: module.h kerneldoc annotations
>   modules: hook up modules.h to docs build
>   accel: autoload modules
>   accel: add qtest module annotations
>   accel: build qtest modular
>   accel: add tcg module annotations
>   accel: build tcg modular
>   monitor: allow register hmp commands
>   usb: drop usb_host_dev_is_scsi_storage hook
>   monitor/usb: register 'info usbhost' dynamically
>   usb: build usb-host as module
>   monitor/tcg: move tcg hmp commands to accel/tcg, register them
> dynamically
> 
> Jose R. Ziviani (1):
>   modules: check if all dependencies can be satisfied
> 
>  scripts/modinfo-collect.py  |  67 +++
>  scripts/modinfo-generate.py |  97 
>  include/hw/usb.h|   7 +-
>  include/monitor/monitor.h   |   3 +
>  include/qemu/module.h   |  74 
>  accel/accel-common.c|   2 +-
>  accel/accel-softmmu.c   |   2 +-
>  accel/qtest/qtest.c |   2 +
>  accel/tcg/hmp.c |  29 +
>  accel/tcg/tcg-accel-ops.c   |   1 +
>  accel/tcg/tcg-all.c |   1 +
>  audio/spiceaudio.c  |   2 +
>  block/iscsi-opts.c  |   1 +
>  chardev/baum.c  |   1 +
>  chardev/spice.c |   4 +
>  hw/display/qxl.c|   4 +
>  hw/display/vhost-user-gpu-pci.c |   1 +
>  hw/display/vhost-user-gpu.c |   1 +
>  hw/display/vhost-user-vga.c |   1 +
>  hw/display/virtio-gpu-base.c|   1 +
>  hw/display/virtio-gpu-gl.c  |   3 +
>  hw/display/virtio-gpu-pci-gl.c  |   3 +
>  hw/display/virtio-gpu-pci.c |   2 +
>  hw/display/virtio-gpu.c |   1 +
>  hw/display/virtio-vga-gl.c  |   3 +
>  hw/display/virtio-vga.c |   2 +
>  hw/ppc/spapr.c  |   2 +-
>  hw/s390x/virtio-ccw-gpu.c   |   3 +
>  hw/usb/ccid-card-emulated.c |   1 +
>  hw/usb/ccid-card-passthru.c |   1 +
>  hw/usb/dev-storage-bot.c|   1 +
>  hw/usb/dev-storage-classic.c|   1 +
>  hw/usb/dev-uas.c|   1 +
>  hw/usb/host-libusb.c|  38 ++
>  hw/usb/host-stub.c  |  45 ---
>  hw/usb/redirect.c

Performance issue with qcow2/raid

[Jose R. Ziviani]

Hello team,

I'm currently investigating a performance regression detected by iozone 
filesystem benchmark (https://www.iozone.org/).

Basically, if I format a QCOW2 image with XFS filesystem in my guest and run 
iozone I'll get the following result:

$ mkfs.xfs -f /dev/xvdb1 && \
  mount -t xfs /dev/xvdb1 /mnt && \
  /opt/iozone/bin/iozone -a -e -s 16777216 -y 4 -q 8 -i 0 -i 1 -f 
/mnt/iozone.dat

kBblock len  read reread
16777216  4K 354790   348796
16777216  8K 362356   364818

However, if I revert the commit 46cd1e8a47 (qcow2: Skip copy-on-write when 
allocating a zero cluster) and run the same, I see a huge improvement:

$ mkfs.xfs -f /dev/xvdb1 && \
  mount -t xfs /dev/xvdb1 /mnt && \
  /opt/iozone/bin/iozone -a -e -s 16777216 -y 4 -q 8 -i 0 -i 1 -f 
/mnt/iozone.dat

kBblock len  read reread
16777216  4K 524067   560057
16777216  8K 538661   537004

Note that if I run iozone without re-formating the disk, I'll get results 
similar to last formatting. In other words, if I my current QEMU executable 
doesn't have commit 46cd1e8a47 and I format the disk, iozone will continue 
showing good results even if I reboot to use QEMU with that commit patched.

My system has a RAID controller[1] and runs QEMU/Xen. I'm not able to reproduce 
such behavior in other systems. 

Do you have any suggestion to help debugging this? What more info could help to 
understand it better?
My next approach is using perf, but I appreciate if you have any hints measure 
qcow efficiently.

[1]
# lspci -vv | grep -i raid
1a:00.0 RAID bus controller: Broadcom / LSI MegaRAID SAS-3 3108 [Invader] (rev 
02)
Kernel driver in use: megaraid_sas
Kernel modules: megaraid_sas

Thank you very much!

Jose R. Ziviani


signature.asc
Description: Digital signature

[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 
---
 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 
---
 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 
 
 #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 
---
 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

[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 0/2] Option to build native TCG with --enable-modules

[Jose R. Ziviani]

Hello!

I'm sending this simple patchset based on a patch still on review[1]
just to understand if it's something that makes sense to the community.

If so, I think it could be included in Gerd's patchset.

Thank you!

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

Jose R. Ziviani (2):
  modules: Option to build native TCG with --enable-modules
  modules: Fix warning in module_arch documentation

 configure | 12 ++--
 include/qemu/module.h |  2 +-
 meson.build   |  7 ++-
 meson_options.txt |  2 ++
 4 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.32.0

[RFC 1/2] modules: Option to build native TCG with --enable-modules

[Jose R. Ziviani]

Adds an option (--enable-tcg-builtin) to build TCG natively when
--enable-modules argument is passed to the build system. It gives
the opportunity to have the accelerator built-in and still take
advantage of the new modular system.

Signed-off-by: Jose R. Ziviani 
---
 configure | 12 ++--
 meson.build   |  7 ++-
 meson_options.txt |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 38704b4e11..add8cf4970 100755
--- a/configure
+++ b/configure
@@ -344,6 +344,7 @@ tsan="no"
 fortify_source="$default_feature"
 strip_opt="yes"
 tcg_interpreter="false"
+tcg_builtin="false"
 bigendian="no"
 mingw32="no"
 gcov="no"
@@ -1107,6 +1108,8 @@ for opt do
   ;;
   --enable-tcg) tcg="enabled"
   ;;
+  --enable-tcg-builtin) tcg_builtin="true"
+  ;;
   --disable-malloc-trim) malloc_trim="disabled"
   ;;
   --enable-malloc-trim) malloc_trim="enabled"
@@ -1792,6 +1795,7 @@ Advanced options (experts only):
Default:trace-
   --disable-slirp  disable SLIRP userspace network connectivity
   --enable-tcg-interpreter enable TCI (TCG with bytecode interpreter, 
experimental and slow)
+  --enable-tcg-builtin force TCG builtin even with --enable-modules
   --enable-malloc-trim enable libc malloc_trim() for memory optimization
   --oss-libpath to OSS library
   --cpu=CPUBuild for host CPU [$cpu]
@@ -2288,7 +2292,11 @@ if test "$solaris" = "yes" ; then
   fi
 fi
 
-if test "$tcg" = "enabled"; then
+if test "$tcg" = "disabled"; then
+debug_tcg="no"
+tcg_interpreter="false"
+tcg_builtin="false"
+else
 git_submodules="$git_submodules tests/fp/berkeley-testfloat-3"
 git_submodules="$git_submodules tests/fp/berkeley-softfloat-3"
 fi
@@ -6449,7 +6457,7 @@ if test "$skip_meson" = no; then
 -Dvhost_user_blk_server=$vhost_user_blk_server 
-Dmultiprocess=$multiprocess \
 -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek 
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
 $(if test "$default_features" = no; then echo 
"-Dauto_features=disabled"; fi) \
-   -Dtcg_interpreter=$tcg_interpreter \
+-Dtcg_interpreter=$tcg_interpreter -Dtcg_builtin=$tcg_builtin \
 $cross_arg \
 "$PWD" "$source_path"
 
diff --git a/meson.build b/meson.build
index fe58793549..2d72b8cc06 100644
--- a/meson.build
+++ b/meson.build
@@ -93,6 +93,9 @@ if cpu in ['x86', 'x86_64']
 endif
 
 modular_tcg = ['i386-softmmu', 'x86_64-softmmu']
+is_tcg_modular = config_host.has_key('CONFIG_MODULES') \
+ and get_option('tcg').enabled() \
+ and not get_option('tcg_builtin')
 
 edk2_targets = [ 'arm-softmmu', 'aarch64-softmmu', 'i386-softmmu', 
'x86_64-softmmu' ]
 install_edk2_blobs = false
@@ -1313,7 +1316,7 @@ foreach target : target_dirs
   elif sym == 'CONFIG_XEN' and have_xen_pci_passthrough
 config_target += { 'CONFIG_XEN_PCI_PASSTHROUGH': 'y' }
   endif
-  if target in modular_tcg
+  if target in modular_tcg and is_tcg_modular
 config_target += { 'CONFIG_TCG_MODULAR': 'y' }
   else
 config_target += { 'CONFIG_TCG_BUILTIN': 'y' }
@@ -2698,6 +2701,8 @@ summary_info += {'TCG support':   
config_all.has_key('CONFIG_TCG')}
 if config_all.has_key('CONFIG_TCG')
   if get_option('tcg_interpreter')
 summary_info += {'TCG backend':   'TCI (TCG with bytecode interpreter, 
experimental and slow)'}
+  elif is_tcg_modular
+summary_info += {'TCG backend':   'module (@0@)'.format(cpu)}
   else
 summary_info += {'TCG backend':   'native (@0@)'.format(cpu)}
   endif
diff --git a/meson_options.txt b/meson_options.txt
index 3d304cac96..fd9f92b333 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,8 @@ option('tcg', type: 'feature', value: 'auto',
description: 'TCG support')
 option('tcg_interpreter', type: 'boolean', value: false,
description: 'TCG with bytecode interpreter (experimental and slow)')
+option('tcg_builtin', type: 'boolean', value: 'false',
+   description: 'Force TCG builtin')
 option('cfi', type: 'boolean', value: 'false',
description: 'Control-Flow Integrity (CFI)')
 option('cfi_debug', type: 'boolean', value: 'false',
-- 
2.32.0

[RFC 2/2] modules: Fix warning in module_arch documentation

[Jose R. Ziviani]

Fixes a small issue in the module_arch documentation that
caused the build system to complain:
module.h:127: warning: Function parameter or member
  'name' not described in 'module_arch'
module.h:127: warning: Excess function parameter
  'arch' description in 'module_arch'

Signed-off-by: Jose R. Ziviani 
---
 include/qemu/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 8bc80535a4..456e190a55 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -115,7 +115,7 @@ void module_allow_arch(const char *arch);
 /**
  * module_arch
  *
- * @arch: target architecture
+ * @name: module name
  *
  * This module is for target architecture @arch.
  *
-- 
2.32.0

[PATCH v4] vga: don't abort when adding a duplicate isa-vga device

[Jose R. Ziviani]

If users try to add an isa-vga device that was already registered,
still in command line, qemu will crash:

$ qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

That particular board registers the device automaticaly, so it's
not obvious that a VGA device already exists. This patch changes
this behavior by displaying a message and exiting without crashing.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Jose R. Ziviani 
---
v3 to v4: Used object_resolve_path_type instead of qemu_ram_block_by_name
  and copied the message from virgl, to give the same user exp.
v2 to v3: Improved error message
v1 to v2: Use error_setg instead of error_report

 hw/display/vga-isa.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..8cea84f2be 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -33,6 +33,7 @@
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "qapi/error.h"
 
 #define TYPE_ISA_VGA "isa-vga"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAVGAState, ISA_VGA)
@@ -61,6 +62,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 MemoryRegion *vga_io_memory;
 const MemoryRegionPortio *vga_ports, *vbe_ports;
 
+/*
+ * make sure this device is not being added twice, if so
+ * exit without crashing qemu
+ */
+if (object_resolve_path_type("", TYPE_ISA_VGA, NULL)) {
+error_setg(errp, "at most one %s device is permitted", TYPE_ISA_VGA);
+return;
+}
+
 s->global_vmstate = true;
 vga_common_init(s, OBJECT(dev));
 s->legacy_address_space = isa_address_space(isadev);
-- 
2.32.0

Re: [PATCH v3] vga: don't abort when adding a duplicate isa-vga device

[Jose R. Ziviani]

On Tue, Aug 17, 2021 at 10:07:55AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/17/21 9:36 AM, Mark Cave-Ayland wrote:
> > On 17/08/2021 08:25, Thomas Huth wrote:
> > 
> >> On 16/08/2021 15.55, Jose R. Ziviani wrote:
> >>> If users try to add an isa-vga device that was already registered,
> >>> still in command line, qemu will crash:
> >>>
> >>> $ qemu-system-mips64el -M pica61 -device isa-vga
> >>> RAMBlock "vga.vram" already registered, abort!
> >>> Aborted (core dumped)
> >>>
> >>> That particular board registers the device automaticaly, so it's
> >>> not obvious that a VGA device already exists. This patch changes
> >>> this behavior by displaying a message and exiting without crashing.
> >>>
> >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
> >>> Reviewed-by: Philippe Mathieu-Daudé 
> >>> Signed-off-by: Jose R. Ziviani 
> >>> ---
> >>> v2 to v3: Improved error message
> >>> v1 to v2: Use error_setg instead of error_report
> >>>
> >>>   hw/display/vga-isa.c | 10 ++
> >>>   1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> >>> index 90851e730b..30d55b41c3 100644
> >>> --- a/hw/display/vga-isa.c
> >>> +++ b/hw/display/vga-isa.c
> >>> @@ -33,6 +33,7 @@
> >>>   #include "hw/loader.h"
> >>>   #include "hw/qdev-properties.h"
> >>>   #include "qom/object.h"
> >>> +#include "qapi/error.h"
> >>>   #define TYPE_ISA_VGA "isa-vga"
> >>>   OBJECT_DECLARE_SIMPLE_TYPE(ISAVGAState, ISA_VGA)
> >>> @@ -61,6 +62,15 @@ static void vga_isa_realizefn(DeviceState *dev,
> >>> Error **errp)
> >>>   MemoryRegion *vga_io_memory;
> >>>   const MemoryRegionPortio *vga_ports, *vbe_ports;
> >>> +    /*
> >>> + * make sure this device is not being added twice, if so
> >>> + * exit without crashing qemu
> >>> + */
> >>> +    if (qemu_ram_block_by_name("vga.vram")) {
> >>> +    error_setg(errp, "'isa-vga' device already registered");
> >>> +    return;
> >>> +    }
> >>> +
> >>>   s->global_vmstate = true;
> >>>   vga_common_init(s, OBJECT(dev));
> >>>   s->legacy_address_space = isa_address_space(isadev);
> >>>
> >>
> >> Reviewed-by: Thomas Huth 
> > 
> > Instead of checking for the presence of the vga.vram block, would it be
> > better to use the standard object_resolve_path_type() method to check
> > for the presence of the existing isa-vga device instead? See
> > https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg00717.html for
> > how this was done for virgl.
> 
> I remembered there was a nicer way but couldn't find it.
> If this patch were for 6.1, it was good enough. Now it
> will be merged in 6.2, I prefer Mark's suggestion.
> Jose, do you mind a v4?
> 

Hello people! Thanks for reviewing it.

Sure, I'll send a v4. It's not for 6.1 anyway.

Thank you very much


signature.asc
Description: Digital signature

[PATCH v2] vga: don't abort when adding a duplicate isa-vga device

[Jose R. Ziviani]

If users try to add an isa-vga device that was already registered,
still in command line, qemu will crash:

$ qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

That particular board registers the device automaticaly, so it's
not obvious that a VGA device already exists. This patch changes
this behavior by displaying a message and exiting without crashing.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Jose R. Ziviani 
---
 hw/display/vga-isa.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..1fba33b22b 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -33,6 +33,7 @@
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "qapi/error.h"
 
 #define TYPE_ISA_VGA "isa-vga"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAVGAState, ISA_VGA)
@@ -61,6 +62,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 MemoryRegion *vga_io_memory;
 const MemoryRegionPortio *vga_ports, *vbe_ports;
 
+/*
+ * make sure this device is not being added twice, if so
+ * exit without crashing qemu
+ */
+if (qemu_ram_block_by_name("vga.vram")) {
+error_setg(errp, "vga.vram is already registered");
+return;
+}
+
 s->global_vmstate = true;
 vga_common_init(s, OBJECT(dev));
 s->legacy_address_space = isa_address_space(isadev);
-- 
2.32.0

[PATCH v3] vga: don't abort when adding a duplicate isa-vga device

[Jose R. Ziviani]

If users try to add an isa-vga device that was already registered,
still in command line, qemu will crash:

$ qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

That particular board registers the device automaticaly, so it's
not obvious that a VGA device already exists. This patch changes
this behavior by displaying a message and exiting without crashing.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Jose R. Ziviani 
---
v2 to v3: Improved error message
v1 to v2: Use error_setg instead of error_report

 hw/display/vga-isa.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..30d55b41c3 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -33,6 +33,7 @@
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
+#include "qapi/error.h"
 
 #define TYPE_ISA_VGA "isa-vga"
 OBJECT_DECLARE_SIMPLE_TYPE(ISAVGAState, ISA_VGA)
@@ -61,6 +62,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 MemoryRegion *vga_io_memory;
 const MemoryRegionPortio *vga_ports, *vbe_ports;
 
+/*
+ * make sure this device is not being added twice, if so
+ * exit without crashing qemu
+ */
+if (qemu_ram_block_by_name("vga.vram")) {
+error_setg(errp, "'isa-vga' device already registered");
+return;
+}
+
 s->global_vmstate = true;
 vga_common_init(s, OBJECT(dev));
 s->legacy_address_space = isa_address_space(isadev);
-- 
2.32.0

[PATCH] vga: don't abort when adding a duplicate isa-vga device

[Jose R. Ziviani]

If users try to add an isa-vga device that was already registered,
still in command line, qemu will crash:

$ qemu-system-mips64el -M pica61 -device isa-vga
RAMBlock "vga.vram" already registered, abort!
Aborted (core dumped)

That particular board registers such device automaticaly, so it's
not obvious that a VGA device already exists. This patch changes
this behavior by displaying a message and ignoring that device,
starting qemu normally.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/44
Signed-off-by: Jose R. Ziviani 
---
 hw/display/vga-isa.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 90851e730b..69db502dde 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -61,6 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp)
 MemoryRegion *vga_io_memory;
 const MemoryRegionPortio *vga_ports, *vbe_ports;
 
+/*
+ * some machines register VGA by default, so instead of aborting
+ * it, show a message and ignore this device.
+ */
+if (qemu_ram_block_by_name("vga.vram")) {
+error_report("vga.vram is already registered, ignoring this device");
+return;
+}
+
 s->global_vmstate = true;
 vga_common_init(s, OBJECT(dev));
 s->legacy_address_space = isa_address_space(isadev);
-- 
2.32.0

Re: [PATCH] qemu-img: Allow target be aligned to sector size

[Jose R. Ziviani]

Hello Hanna,

On Thu, Aug 19, 2021 at 12:12:00PM +0200, Hanna Reitz wrote:
> We cannot write to images opened with O_DIRECT unless we allow them to
> be resized so they are aligned to the sector size: Since 9c60a5d1978,
> bdrv_node_refresh_perm() ensures that for nodes whose length is not
> aligned to the request alignment and where someone has taken a WRITE
> permission, the RESIZE permission is taken, too).
> 
> Let qemu-img convert pass the BDRV_O_RESIZE flag (which causes
> blk_new_open() to take the RESIZE permission) when using cache=none for
> the target, so that when writing to it, it can be aligned to the target
> sector size.
> 
> Without this patch, an error is returned:
> 
> $ qemu-img convert -f raw -O raw -t none foo.img /mnt/tmp/foo.img
> qemu-img: Could not open '/mnt/tmp/foo.img': Cannot get 'write'
> permission without 'resize': Image size is not a multiple of request
> alignment
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1994266
> Signed-off-by: Hanna Reitz 
> ---
> As I have written in the BZ linked above, I am not sure what behavior we
> want.  It can be argued that the current behavior is perfectly OK
> because we want the target to have the same size as the source, so if
> this cannot be done, we should just print the above error (which I think
> explains the problem well enough that users can figure out they need to
> resize the source image).
> 
> OTOH, it is difficult for me to imagine a case where the user would
> prefer the above error to just having qemu-img align the target image's
> length.

This is timely convenient because I'm currently investigating an issue detected
by a libvirt-tck test:

https://gitlab.com/libvirt/libvirt-tck/-/blob/master/scripts/qemu/200-qcow2-single-backing-file.t

It fails with the same message:
qemu-system-x86_64: -device 
virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk1,write-cache=on:
 Cannot get 'write' permission without 'resize': Image size is not a multiple 
of request alignment

Libvirt-tck basically wants to make sure that libvirt won't pass a 'backing'
argument if we force a QCOW2 image to be interpreted as a RAW image. But, the
actual size of a (not preallocated) QCOW2 is usually unaligned so we ended up
failing at that requirement.

I crafted a reproducer (tck-main is a QCOW2 image):
 $ ./qemu-system-x86_64 \
  -machine pc-i440fx-6.0,accel=kvm -m 1024 -display none -no-user-config 
-nodefaults \
  -kernel vmlinuz -initrd initrd \
  -blockdev 
driver=file,filename=/var/cache/libvirt-tck/storage-fs/tck/tck-main,node-name=a,cache.direct=on
 \
  -blockdev node-name=name,driver=raw,file=a \
  -device virtio-blk-pci,drive=name

And if I remove 'cache.direct=on' OR if I remove the device virtio-blk-pci I
don't hit the failure.

In order to fix the libvirt-tck test case, I think that creating a preallocated
QCOW2 image is the best approach, considering their test case goal. But, if you
don't mind, could you explain why cache.direct=on doesn't set resize permission
with virtio-blk-pci?

Thank you!

> ---
>  qemu-img.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 908fd0cce5..d4b29bf73e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2628,6 +2628,14 @@ static int img_convert(int argc, char **argv)
>  goto out;
>  }
>  
> +if (flags & BDRV_O_NOCACHE) {
> +/*
> + * If we open the target with O_DIRECT, it may be necessary to
> + * extend its size to align to the physical sector size.
> + */
> +flags |= BDRV_O_RESIZE;
> +}
> +
>  if (skip_create) {
>  s.target = img_open(tgt_image_opts, out_filename, out_fmt,
>  flags, writethrough, s.quiet, false);
> -- 
> 2.31.1
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH] qemu-img: Allow target be aligned to sector size

[Jose R. Ziviani]

On Thu, Aug 19, 2021 at 05:14:30PM +0200, Hanna Reitz wrote:
> On 19.08.21 16:31, Jose R. Ziviani wrote:
> > Hello Hanna,
> > 
> > On Thu, Aug 19, 2021 at 12:12:00PM +0200, Hanna Reitz wrote:
> > > We cannot write to images opened with O_DIRECT unless we allow them to
> > > be resized so they are aligned to the sector size: Since 9c60a5d1978,
> > > bdrv_node_refresh_perm() ensures that for nodes whose length is not
> > > aligned to the request alignment and where someone has taken a WRITE
> > > permission, the RESIZE permission is taken, too).
> > > 
> > > Let qemu-img convert pass the BDRV_O_RESIZE flag (which causes
> > > blk_new_open() to take the RESIZE permission) when using cache=none for
> > > the target, so that when writing to it, it can be aligned to the target
> > > sector size.
> > > 
> > > Without this patch, an error is returned:
> > > 
> > > $ qemu-img convert -f raw -O raw -t none foo.img /mnt/tmp/foo.img
> > > qemu-img: Could not open '/mnt/tmp/foo.img': Cannot get 'write'
> > > permission without 'resize': Image size is not a multiple of request
> > > alignment
> > > 
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1994266
> > > Signed-off-by: Hanna Reitz 
> > > ---
> > > As I have written in the BZ linked above, I am not sure what behavior we
> > > want.  It can be argued that the current behavior is perfectly OK
> > > because we want the target to have the same size as the source, so if
> > > this cannot be done, we should just print the above error (which I think
> > > explains the problem well enough that users can figure out they need to
> > > resize the source image).
> > > 
> > > OTOH, it is difficult for me to imagine a case where the user would
> > > prefer the above error to just having qemu-img align the target image's
> > > length.
> > This is timely convenient because I'm currently investigating an issue 
> > detected
> > by a libvirt-tck test:
> > 
> > https://gitlab.com/libvirt/libvirt-tck/-/blob/master/scripts/qemu/200-qcow2-single-backing-file.t
> > 
> > It fails with the same message:
> > qemu-system-x86_64: -device 
> > virtio-blk-pci,bus=pci.0,addr=0x3,drive=libvirt-1-format,id=virtio-disk1,write-cache=on:
> >  Cannot get 'write' permission without 'resize': Image size is not a 
> > multiple of request alignment
> > 
> > Libvirt-tck basically wants to make sure that libvirt won't pass a 'backing'
> > argument if we force a QCOW2 image to be interpreted as a RAW image. But, 
> > the
> > actual size of a (not preallocated) QCOW2 is usually unaligned so we ended 
> > up
> > failing at that requirement.
> > 
> > I crafted a reproducer (tck-main is a QCOW2 image):
> >   $ ./qemu-system-x86_64 \
> >-machine pc-i440fx-6.0,accel=kvm -m 1024 -display none -no-user-config 
> > -nodefaults \
> >-kernel vmlinuz -initrd initrd \
> >-blockdev 
> > driver=file,filename=/var/cache/libvirt-tck/storage-fs/tck/tck-main,node-name=a,cache.direct=on
> >  \
> >-blockdev node-name=name,driver=raw,file=a \
> >-device virtio-blk-pci,drive=name
> > 
> > And if I remove 'cache.direct=on' OR if I remove the device virtio-blk-pci I
> > don't hit the failure.
> > 
> > In order to fix the libvirt-tck test case, I think that creating a 
> > preallocated
> > QCOW2 image is the best approach, considering their test case goal. But, if 
> > you
> > don't mind, could you explain why cache.direct=on doesn't set resize 
> > permission
> > with virtio-blk-pci?
> 
> Well, users only take the permissions they need.  Because the device doesn’t
> actively want to resize the disk, it doesn’t take the permission (because
> other simultaneous users might not share that permission, and so taking more
> permissions than you need may cause problems).
> 
> So the decision whether to take the permission or not is a tradeoff.  I
> mean, there’s always a work-around: The error message tells you that the
> image is not aligned to the request alignment, so you can align the image
> size manually.  The question is whether taking the permissions may be
> problematic, and whether the user can be expected to align the image size.
> 
> (And we also need to keep in mind that this case is extremely rare. I don’t
> think that users in practice will ever have images that are not 4k-aligned. 
> What the test is doing is of course something that would never happen in a
> practical set-up, in fact, I believe attaching a qcow

[PATCH] tcg/arm: Fix tcg_out_vec_op function signature

[Jose R. Ziviani]

Commit 5e8892db93 fixed several function signatures but tcg_out_vec_op
for arm is missing. It causes a build error on armv6 and armv7:

tcg-target.c.inc:2718:42: error: argument 5 of type 'const TCGArg *'
{aka 'const unsigned int *'} declared as a pointer [-Werror=array-parameter=]
   const TCGArg *args, const int *const_args)
  ~~^~~~
../tcg/tcg.c:120:41: note: previously declared as an array 'const TCGArg[16]'
{aka 'const unsigned int[16]'}
   const TCGArg args[TCG_MAX_OP_ARGS],
  ~~^~~~

Signed-off-by: Jose R. Ziviani 
---
 tcg/arm/tcg-target.c.inc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tcg/arm/tcg-target.c.inc b/tcg/arm/tcg-target.c.inc
index 007ceee68e..e5b4f86841 100644
--- a/tcg/arm/tcg-target.c.inc
+++ b/tcg/arm/tcg-target.c.inc
@@ -2715,7 +2715,8 @@ static const ARMInsn vec_cmp0_insn[16] = {
 
 static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
unsigned vecl, unsigned vece,
-   const TCGArg *args, const int *const_args)
+   const TCGArg args[TCG_MAX_OP_ARGS],
+   const int const_args[TCG_MAX_OP_ARGS])
 {
 TCGType type = vecl + TCG_TYPE_V64;
 unsigned q = vecl;
-- 
2.33.0

[PATCH 2/2] modules: use a list of supported arch for each module

[Jose R. Ziviani]

When compiling QEMU with more than one target, for instance,
--target-list=s390x-softmmu,x86_64-softmmu, modinfo.c will be
filled with modules available for both, with no specification
of what modules can/cannot be loaded for a particular target.

This will cause message errors when executing the target that
shouldn't be loading that module, such as:

$ qemu-system-s390x -nodefaults -display none -accel qtest -M none -device help
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common

This patch changes the module infrastructure to use a list of
architectures, obtained during the build time, to specify what
targets can load each module.

Signed-off-by: Jose R. Ziviani 
---
 include/qemu/module.h   |  2 +-
 meson.build | 18 +-
 scripts/modinfo-collect.py  | 10 ++
 scripts/modinfo-generate.py |  7 +++
 util/module.c   | 18 +-
 5 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 3deac0078b..3b487c646c 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -144,7 +144,7 @@ void module_allow_arch(const char *arch);
 typedef struct QemuModinfo QemuModinfo;
 struct QemuModinfo {
 const char *name;
-const char *arch;
+const char **archs;
 const char **objs;
 const char **deps;
 const char **opts;
diff --git a/meson.build b/meson.build
index d1d3fd84ec..efba275092 100644
--- a/meson.build
+++ b/meson.build
@@ -2343,11 +2343,19 @@ foreach d, list : modules
 # unique when it comes to lookup in compile_commands.json.
 # Depnds on a mesion version with
 # https://github.com/mesonbuild/meson/pull/8900
-modinfo_files += custom_target(d + '-' + m + '.modinfo',
-   output: d + '-' + m + '.modinfo',
-   input: module_ss.sources() + genh,
-   capture: true,
-   command: [modinfo_collect, 
module_ss.sources()])
+if modules_arch.has_key(m)
+  modinfo_files += custom_target(d + '-' + m + '.modinfo',
+output: d + '-' + m + '.modinfo',
+input: module_ss.sources() + genh,
+capture: true,
+command: [modinfo_collect, 
module_ss.sources(), '--archs', modules_arch[m]])
+else
+  modinfo_files += custom_target(d + '-' + m + '.modinfo',
+output: d + '-' + m + '.modinfo',
+input: module_ss.sources() + genh,
+capture: true,
+command: [modinfo_collect, 
module_ss.sources()])
+endif
   endif
 else
   if d == 'block'
diff --git a/scripts/modinfo-collect.py b/scripts/modinfo-collect.py
index 4acb188c3e..739cd23e2f 100755
--- a/scripts/modinfo-collect.py
+++ b/scripts/modinfo-collect.py
@@ -50,6 +50,16 @@ def main(args):
 print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
 with open('compile_commands.json') as f:
 compile_commands = json.load(f)
+
+try:
+arch_idx = args.index('--archs')
+archs = args[arch_idx + 1:]
+args = args[:arch_idx]
+for arch in archs:
+print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
+except ValueError:
+pass
+
 for src in args:
 print("MODINFO_DEBUG src %s" % src)
 command = find_command(src, target, compile_commands)
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index f559eed007..e1d13acd92 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -33,7 +33,7 @@ def parse_line(line):
 return (kind, data)
 
 def generate(name, lines):
-arch = ""
+archs = []
 objs = []
 deps = []
 opts = []
@@ -47,14 +47,13 @@ def generate(name, lines):
 elif kind == 'opts':
 opts.append(data)
 elif kind == 'arch':
-arch = data;
+archs.append(data);
 else:
 print("unknown:", kind)
 exit(1)
 
 print(".name = \"%s\"," % name)
-if arch != "":
-print(".arch = %s," % arch)
+print_array("archs", archs)
 print_array("objs", objs)
 print_array("deps", deps)
 print_array("opts", opts)
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..7009143bfc 100644
--- a/util/module.c
+++ b/util/module.c
@@ -131,16 +131,24 @@ void module_allow_arch(const char *arch)
 
 static bool module_check_arch(const QemuModinfo *modinfo)
 {

[PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

This variable keeps track of all modules enabled for a target
architecture. This will be used in modinfo to refine the
architectures that can really load the .so to avoid errors.

Signed-off-by: Jose R. Ziviani 
---
 hw/display/meson.build | 48 ++
 hw/usb/meson.build | 36 +++
 meson.build|  1 +
 3 files changed, 85 insertions(+)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 861c43ff98..ba06f58ff1 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -43,6 +43,18 @@ if config_all_devices.has_key('CONFIG_QXL')
   qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 
'qxl-render.c'),
pixman, spice])
   hw_display_modules += {'qxl': qxl_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_QXL') and cfg_target['CONFIG_QXL'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'qxl': archs}
 endif
 
 softmmu_ss.add(when: 'CONFIG_DPCD', if_true: files('dpcd.c'))
@@ -65,6 +77,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
if_true: [files('virtio-gpu-gl.c', 
'virtio-gpu-virgl.c'), pixman, virgl])
   hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_VIRTIO_GPU') and 
cfg_target['CONFIG_VIRTIO_GPU'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'virtio-gpu': archs, 'virtio-gpu-gl': archs}
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
@@ -79,6 +103,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
   virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', 
virgl, opengl],
if_true: [files('virtio-gpu-pci-gl.c'), pixman])
   hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_VIRTIO_PCI') and 
cfg_target['CONFIG_VIRTIO_PCI'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'virtio-gpu-pci': archs, 'virtio-gpu-pci-gl': archs}
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
@@ -93,6 +129,18 @@ if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
   virtio_vga_gl_ss.add(when: ['CONFIG_VIRTIO_VGA', virgl, opengl],
if_true: [files('virtio-vga-gl.c'), pixman])
   hw_display_modules += {'virtio-vga-gl': virtio_vga_gl_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_VIRTIO_VGA') and 
cfg_target['CONFIG_VIRTIO_VGA'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'virtio-vga': archs, 'virtio-vga-gl': archs}
 endif
 
 specific_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_lcdc.c'))
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index de853d780d..6b889d2ee2 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -54,6 +54,18 @@ if cacard.found()
   usbsmartcard_ss.add(when: 'CONFIG_USB_SMARTCARD',
   if_true: [cacard, files('ccid-card-emulated.c', 
'ccid-card-passthru.c')])
   hw_usb_modules += {'smartcard': usbsmartcard_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_USB_SMARTCARD') and 
cfg_target['CONFIG_USB_SMARTCARD'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'smartcard': archs}
 endif
 
 # U2F
@@ -69,6 +81,18 @@ if usbredir.found()
   usbredir_ss.add(when: 'CONFIG_USB',
   if_true: [usbredir, files('redirect.c', 'quirks.c')])
   hw_usb_modules += {'redirect': usbredir_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith('-softmmu')
+  cfg_target = config_target_mak[target]
+  if cfg_target.has_key('CONFIG_USB') and cfg_target['CONFIG_USB'] == 'y'
+archs += [cfg_target['TARGET_NAME']]
+  endif
+endif
+  endforeach
+
+  modules_arch += {'redirect': archs}
 endif
 
 # usb pass-through
@@ -77,6 +101,18 @@ if libusb.found()
   usbhost_ss.add(when: ['CONFIG_USB', libusb],
  if_true: files('host-libusb.c'))
   hw_usb_modules += {'host': usbhost_ss}
+
+  archs = []
+  foreach target: target_dirs
+if target.endswith

[PATCH 0/2] modules: Improve modinfo.c architecture support

[Jose R. Ziviani]

When building a single target, the build system detects the architecture
and generates a modinfo.c with modules related to that arch only.
However, when more than one target is built, modinfo.c is generated with
modules available for each architecture - without any way to know what
arch supports what.

The problem is when executing the target, it will try to load modules
that is not supported by it and will throw errors to users, for
instance:

$ ./configure --enable-modules # all targets
$ ./qemu-system-avr -nodefaults -display none -accel tcg -M none -device help | 
head
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-gpu.so: undefined symbol: 
virtio_vmstate_info
Failed to open module: /.../hw-display-virtio-gpu.so: undefined symbol: 
virtio_vmstate_info
Failed to open module: /.../hw-display-virtio-gpu-gl.so: undefined symbol: 
virtio_gpu_ctrl_response
Failed to open module: /.../hw-display-virtio-gpu-pci.so: undefined symbol: 
virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-gpu-pci.so: undefined symbol: 
virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-gpu-pci-gl.so: undefined symbol: 
virtio_instance_init_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: 
have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: 
ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: 
vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device
...

$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help 
| head 
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: 
have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: 
ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: 
vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device

This patchset tries to improve by collecting the modules are currently
enabled for each target, obtaining information in meson from kconfig
and passing that to modinfo.c, which now uses a list to store supported
architectures for each module.

$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help 
| head
Controller/Bridge/Hub devices:
name "pci-bridge", bus PCI, desc "Standard PCI Bridge"
name "pci-bridge-seat", bus PCI, desc "Standard PCI Bridge (multiseat)"
...

$ ./qemu-system-avr -nodefaults -display none -accel tcg -M none -device help | 
head
...
Misc devices:
name "guest-loader", desc "Guest Loader"
name "loader", desc "Generic Loader"

Jose R. Ziviani (2):
  meson: introduce modules_arch
  modules: use a list of supported arch for each module

 hw/display/meson.build  | 48 +
 hw/usb/meson.build  | 36 
 include/qemu/module.h   |  2 +-
 meson.build | 19 +++
 scripts/modinfo-collect.py  | 10 
 scripts/modinfo-generate.py |  7 +++---
 util/module.c   | 18 ++
 7 files changed, 125 insertions(+), 15 deletions(-)

-- 
2.33.0

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

Hello!

On Fri, Sep 17, 2021 at 09:14:04AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > This variable keeps track of all modules enabled for a target
> > architecture. This will be used in modinfo to refine the
> > architectures that can really load the .so to avoid errors.
> 
> I think this is the wrong approach.  The reason why modules are
> not loading is typically *not* the architecture, but a feature
> or subsystem the device needs not being compiled in.  Often the
> subsystem is a bus (pci, usb, ccw), but there are also other
> cases (virtio, vga).
> 
> We can stick that into modinfo, simliar to module_dep() but for bits
> provided by core qemu instead of other modules.  i.e. add something
> along the lines of ...

Yes, I really like your approach, makes more sense indeed. But, how do I
get the core modules that other modules depend on?

I see that Kconfig already has something in this line:

config VGA  (from hw/display)
bool

config PCI  (from hw/pci)
bool

config QXL  (from hw/display)
bool
depends on SPICE && PCI
select VGA

I assume that independent entries (like VGA and PCI) are core and that I
can rely on it to add
  module_need(PCI)
  module_need(VGA)
for hw-display-qxl. Am I right?

Thanks for reviewing it!!

> 
>   module_need(BUS_PCI);
> 
> ... to the modules, store that in modinfo and check it before trying
> to load.
> 
> That would also allow to remove hacks like commit 2dd9d8cfb4f3 ("s390x:
> add have_virtio_ccw")
> 
> take care,
>   Gerd
> 


signature.asc
Description: Digital signature

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

On Mon, Sep 20, 2021 at 07:15:32AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Yes, I really like your approach, makes more sense indeed. But, how do I
> > get the core modules that other modules depend on?
> > 
> > I see that Kconfig already has something in this line:
> > 
> > config VGA  (from hw/display)
> > bool
> > 
> > config PCI  (from hw/pci)
> > bool
> > 
> > config QXL  (from hw/display)
> > bool
> > depends on SPICE && PCI
> > select VGA
> > 
> > I assume that independent entries (like VGA and PCI) are core and that I
> > can rely on it to add
> >   module_need(PCI)
> >   module_need(VGA)
> > for hw-display-qxl. Am I right?
> 
> Yes, looking at kconfig for core dependencies makes sense.

But, in anyway, I'll still need to store the target architecture that
can use such core module, like I did here in this patch. Otherwise,
if I compile different targets at the same time, I'll end up with the
same problem of targets trying to load wrong modules.

I thought of using qom, but I think it will pollute module.c.

What do you think if I simply create one modinfo.c per target, like
modinfo-s390x.c, modinfo-avr.c, etc? Each will only have the data
structure filled with the right modules and linked only to its own
qemu-system-arch.

Best regards,

Jose R Ziviani

> 
> take care,
>   Gerd
> 


signature.asc
Description: Digital signature

Re: [PATCH v2 0/2] modules: Improve modinfo.c support

[Jose R. Ziviani]

Hello, Gerd!

On Tue, Sep 28, 2021 at 07:06:28AM +0200, Gerd Hoffmann wrote:
> On Mon, Sep 27, 2021 at 11:11:58AM -0300, Jose R. Ziviani wrote:
> > This patchset introduces the modinfo_need and changes
> > modinfo-generate.py/meson.build to generate/link one modinfo per target.
> > 
> > modinfo-generate.py will know, thanks to modinfo_need, which modules are
> > currently enabled for a given target before adding it in the array of
> > modules. It will give a hint about why some modules failed, so
> > developers can have a clue about it:
> 
> The approach looks good to me.

Awesome, I'll apply your review and send a new version.

Thank you!

> 
> > /* hw-display-qxl.modinfo */
> > /* module  QXL is missing. */
> 
> You are using kconfig symbols here, so the comment should say so ;)
> 
> Renaming modinfo_need to modinfo_kconfig will probably also help
> to make that clear.
> 
> > /* hw-display-virtio-gpu.modinfo */
> > .name = "hw-display-virtio-gpu",
> > .objs = ((const char*[]){  "virtio-gpu-base",  "virtio-gpu-device",  
> > "vhost-user-gpu", NULL }),
> 
> Hmm?  Leftover from an older version of the series?
> 
> >  - accelerators can be filtered as well (this only covers the device
> >part), then the field QemuModinfo.arch can be removed.
> 
> It's target-specific modules.  Although accelerators are the only
> in-tree users right this is not limited to accelerators.
> 
> take care,
>   Gerd
> 


signature.asc
Description: Digital signature

[PATCH v3 1/2] modules: introduces module_kconfig directive

[Jose R. Ziviani]

module_kconfig is a new directive that should be used with module_obj
whenever that module depends on the Kconfig to be enabled.

When the module is enabled in Kconfig we are sure that its dependencies
will be enabled as well, thus the module will be loaded without any
problem.

The correct way to use module_kconfig is by passing the Kconfig option
to module_kconfig (or the *config-devices.mak without CONFIG_).

Signed-off-by: Jose R. Ziviani 
---
 hw/display/qxl.c|  1 +
 hw/display/vhost-user-gpu-pci.c |  1 +
 hw/display/vhost-user-gpu.c |  1 +
 hw/display/vhost-user-vga.c |  1 +
 hw/display/virtio-gpu-base.c|  1 +
 hw/display/virtio-gpu-gl.c  |  1 +
 hw/display/virtio-gpu-pci-gl.c  |  1 +
 hw/display/virtio-gpu-pci.c |  1 +
 hw/display/virtio-gpu.c |  1 +
 hw/display/virtio-vga-gl.c  |  1 +
 hw/display/virtio-vga.c |  1 +
 hw/s390x/virtio-ccw-gpu.c   |  1 +
 hw/usb/ccid-card-emulated.c |  1 +
 hw/usb/ccid-card-passthru.c |  1 +
 hw/usb/host-libusb.c|  1 +
 hw/usb/redirect.c   |  1 +
 include/qemu/module.h   | 10 ++
 scripts/modinfo-generate.py |  2 ++
 18 files changed, 28 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 29c80b4289..d02d50014b 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2494,6 +2494,7 @@ static const TypeInfo qxl_primary_info = {
 .class_init= qxl_primary_class_init,
 };
 module_obj("qxl-vga");
+module_kconfig(QXL);
 
 static void qxl_secondary_class_init(ObjectClass *klass, void *data)
 {
diff --git a/hw/display/vhost-user-gpu-pci.c b/hw/display/vhost-user-gpu-pci.c
index daefcf7101..d119bcae45 100644
--- a/hw/display/vhost-user-gpu-pci.c
+++ b/hw/display/vhost-user-gpu-pci.c
@@ -44,6 +44,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_gpu_pci_info 
= {
 .instance_init = vhost_user_gpu_pci_initfn,
 };
 module_obj(TYPE_VHOST_USER_GPU_PCI);
+module_kconfig(VHOST_USER_GPU);
 
 static void vhost_user_gpu_pci_register_types(void)
 {
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 49df56cd14..c961d2a2d8 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -599,6 +599,7 @@ static const TypeInfo vhost_user_gpu_info = {
 .class_init = vhost_user_gpu_class_init,
 };
 module_obj(TYPE_VHOST_USER_GPU);
+module_kconfig(VHOST_USER_GPU);
 
 static void vhost_user_gpu_register_types(void)
 {
diff --git a/hw/display/vhost-user-vga.c b/hw/display/vhost-user-vga.c
index 072c9c65bc..0c146080fd 100644
--- a/hw/display/vhost-user-vga.c
+++ b/hw/display/vhost-user-vga.c
@@ -45,6 +45,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_vga_info = {
 .instance_init = vhost_user_vga_inst_initfn,
 };
 module_obj(TYPE_VHOST_USER_VGA);
+module_kconfig(VHOST_USER_VGA);
 
 static void vhost_user_vga_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index c8da4806e0..cde96325ca 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -257,6 +257,7 @@ static const TypeInfo virtio_gpu_base_info = {
 .abstract = true
 };
 module_obj(TYPE_VIRTIO_GPU_BASE);
+module_kconfig(VIRTIO_GPU);
 
 static void
 virtio_register_types(void)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 6cc4313b1a..f7837cc44d 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -160,6 +160,7 @@ static const TypeInfo virtio_gpu_gl_info = {
 .class_init = virtio_gpu_gl_class_init,
 };
 module_obj(TYPE_VIRTIO_GPU_GL);
+module_kconfig(VIRTIO_GPU);
 
 static void virtio_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-pci-gl.c b/hw/display/virtio-gpu-pci-gl.c
index 99b14a0718..a2819e1ca9 100644
--- a/hw/display/virtio-gpu-pci-gl.c
+++ b/hw/display/virtio-gpu-pci-gl.c
@@ -47,6 +47,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_gl_pci_info = 
{
 .instance_init = virtio_gpu_gl_initfn,
 };
 module_obj(TYPE_VIRTIO_GPU_GL_PCI);
+module_kconfig(VIRTIO_PCI);
 
 static void virtio_gpu_gl_pci_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index e36eee0c40..93f214ff58 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -65,6 +65,7 @@ static const TypeInfo virtio_gpu_pci_base_info = {
 .abstract = true
 };
 module_obj(TYPE_VIRTIO_GPU_PCI_BASE);
+module_kconfig(VIRTIO_PCI);
 
 #define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
 typedef struct VirtIOGPUPCI VirtIOGPUPCI;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 182e0868b0..6cc6bb1f1f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1450,6 +1450,7 @@ static const TypeInfo virtio_gpu_info = {
 .class_init = virtio_gpu_class_init,
 };
 module_obj(TYPE_VIRTIO_GPU);
+module_kconfig(VIRTIO_GPU);
 
 static void virtio_register_types(void)
 {
diff --git a/hw/display/virtio-vga-gl.c b/hw/display/virtio-vga-gl.c
index f2

[PATCH v3 2/2] modules: generates per-target modinfo

[Jose R. Ziviani]

This patch changes the way modinfo is generated and built. Instead of
one modinfo.c it generates one modinfo--softmmu.c per target. It
aims a fine-tune control of modules by configuring Kconfig.

Signed-off-by: Jose R. Ziviani 
---
 meson.build | 25 +++---
 scripts/modinfo-generate.py | 42 ++---
 2 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/meson.build b/meson.build
index 15ef4d3c41..a617be73de 100644
--- a/meson.build
+++ b/meson.build
@@ -2403,14 +2403,23 @@ foreach d, list : target_modules
 endforeach
 
 if enable_modules
-  modinfo_src = custom_target('modinfo.c',
-  output: 'modinfo.c',
-  input: modinfo_files,
-  command: [modinfo_generate, '@INPUT@'],
-  capture: true)
-  modinfo_lib = static_library('modinfo', modinfo_src)
-  modinfo_dep = declare_dependency(link_whole: modinfo_lib)
-  softmmu_ss.add(modinfo_dep)
+  foreach target : target_dirs
+if target.endswith('-softmmu')
+  config_target = config_target_mak[target]
+  config_devices_mak = target + '-config-devices.mak'
+  modinfo_src = custom_target('modinfo-' + target + '.c',
+  output: 'modinfo-' + target + '.c',
+  input: modinfo_files,
+  command: [modinfo_generate, '--devices', 
config_devices_mak, '@INPUT@'],
+  capture: true)
+
+  modinfo_lib = static_library('modinfo-' + target + '.c', modinfo_src)
+  modinfo_dep = declare_dependency(link_with: modinfo_lib)
+
+  arch = config_target['TARGET_NAME'] == 'sparc64' ? 'sparc64' : 
config_target['TARGET_BASE_ARCH']
+  hw_arch[arch].add(modinfo_dep)
+endif
+  endforeach
 endif
 
 nm = find_program('nm')
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index 689f33c0f2..a0c09edae1 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -32,7 +32,7 @@ def parse_line(line):
 continue
 return (kind, data)
 
-def generate(name, lines):
+def generate(name, lines, core_modules):
 arch = ""
 objs = []
 deps = []
@@ -49,7 +49,13 @@ def generate(name, lines):
 elif kind == 'arch':
 arch = data;
 elif kind == 'kconfig':
-pass # ignore
+# don't add a module which dependency is not enabled
+# in kconfig
+if data.strip() not in core_modules:
+print("/* module {} isn't enabled in Kconfig. */"
+  .format(data.strip()))
+print("/* },{ */")
+return []
 else:
 print("unknown:", kind)
 exit(1)
@@ -60,7 +66,7 @@ def generate(name, lines):
 print_array("objs", objs)
 print_array("deps", deps)
 print_array("opts", opts)
-print("},{");
+print("},{")
 return deps
 
 def print_pre():
@@ -74,26 +80,28 @@ def print_post():
 print("}};")
 
 def main(args):
+if len(args) < 3 or args[0] != '--devices':
+print('Expected: modinfo-generate.py --devices '
+  'config-device.mak [modinfo files]', file=sys.stderr)
+exit(1)
+
+# get all devices enabled in kconfig, from *-config-device.mak
+enabled_core_modules = set()
+with open(args[1]) as file:
+for line in file.readlines():
+config = line.split('=')
+if config[1].rstrip() == 'y':
+enabled_core_modules.add(config[0][7:]) # remove CONFIG_
+
 deps = {}
 print_pre()
-for modinfo in args:
+for modinfo in args[2:]:
 with open(modinfo) as f:
 lines = f.readlines()
 print("/* %s */" % modinfo)
-(basename, ext) = os.path.splitext(modinfo)
-deps[basename] = generate(basename, lines)
+(basename, _) = os.path.splitext(modinfo)
+deps[basename] = generate(basename, lines, enabled_core_modules)
 print_post()
 
-flattened_deps = {flat.strip('" ') for dep in deps.values() for flat in 
dep}
-error = False
-for dep in flattened_deps:
-if dep not in deps.keys():
-print("Dependency {} cannot be satisfied".format(dep),
-  file=sys.stderr)
-error = True
-
-if error:
-exit(1)
-
 if __name__ == "__main__":
 main(sys.argv[1:])
-- 
2.33.0

[PATCH v3 0/2] modules: Improve modinfo.c support

[Jose R. Ziviani]

This patchset introduces the modinfo_kconfig aiming for a fine-tune
control of module loading by simply checking Kconfig options during the
compile time, then generates one modinfo--softmmu.c per target.

The main reason of this change is to fix problems like:
$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help 
| head
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: 
have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: 
ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: 
vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device

With this patch, I run this small script successfuly:
#!/bin/bash
pushd ~/suse/virtualization/qemu/build
for qemu in qemu-system-*
do
[[ -f "$qemu" ]] || continue
res=$(./$qemu -nodefaults -display none -accel tcg -M none -device help 
2>&1 | grep "Failed to" > /dev/null; echo $?)
[[ $res -eq 0 ]] && echo "Error: $qemu"
done
popd

Also run 'make check' and 'check-acceptance' without any failures.

v2 -> v3:
 - Renamed module_needs to module_kconfig [Gerd]
 - Reworded the commit message a bit to improve a better understanding [myself]

v1 -> v2:
 - Changed the approach to this problem after suggestions made by Paolo and 
Gerd.

Thank you!

Jose R. Ziviani (2):
  modules: introduces module_kconfig directive
  modules: generates per-target modinfo

 hw/display/qxl.c|  1 +
 hw/display/vhost-user-gpu-pci.c |  1 +
 hw/display/vhost-user-gpu.c |  1 +
 hw/display/vhost-user-vga.c |  1 +
 hw/display/virtio-gpu-base.c|  1 +
 hw/display/virtio-gpu-gl.c  |  1 +
 hw/display/virtio-gpu-pci-gl.c  |  1 +
 hw/display/virtio-gpu-pci.c |  1 +
 hw/display/virtio-gpu.c |  1 +
 hw/display/virtio-vga-gl.c  |  1 +
 hw/display/virtio-vga.c |  1 +
 hw/s390x/virtio-ccw-gpu.c   |  1 +
 hw/usb/ccid-card-emulated.c |  1 +
 hw/usb/ccid-card-passthru.c |  1 +
 hw/usb/host-libusb.c|  1 +
 hw/usb/redirect.c   |  1 +
 include/qemu/module.h   | 10 
 meson.build | 25 +---
 scripts/modinfo-generate.py | 42 -
 19 files changed, 69 insertions(+), 24 deletions(-)

-- 
2.33.0

[PATCH v2 0/2] modules: Improve modinfo.c support

[Jose R. Ziviani]

This patchset introduces the modinfo_need and changes
modinfo-generate.py/meson.build to generate/link one modinfo per target.

modinfo-generate.py will know, thanks to modinfo_need, which modules are
currently enabled for a given target before adding it in the array of
modules. It will give a hint about why some modules failed, so
developers can have a clue about it:

},{
/* hw-display-qxl.modinfo */
/* module  QXL is missing. */

/* hw-display-virtio-gpu.modinfo */
.name = "hw-display-virtio-gpu",
.objs = ((const char*[]){  "virtio-gpu-base",  "virtio-gpu-device",  
"vhost-user-gpu", NULL }),
},{

The main reason of this change is to fix problems like:
$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help 
| head
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: 
have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: 
ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: 
vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device

With this patch, I run this small script successfuly:
#!/bin/bash
pushd ~/suse/virtualization/qemu/build
for qemu in qemu-system-*
do
[[ -f "$qemu" ]] || continue
res=$(./$qemu -nodefaults -display none -accel tcg -M none -device help 
2>&1 | grep "Failed to" > /dev/null; echo $?)
[[ $res -eq 0 ]] && echo "Error: $qemu"
done
popd

Also run make check and check-acceptance without any failures.

Todo:
 - accelerators can be filtered as well (this only covers the device
   part), then the field QemuModinfo.arch can be removed.

v1 -> v2:
 - Changed the approach to this problem after suggestions made by Paolo and 
Gerd.

Thank you!

Jose R. Ziviani (2):
  modules: introduces module_needs directive
  modules: generates per-target modinfo

 hw/display/qxl.c|  1 +
 hw/display/vhost-user-gpu-pci.c |  1 +
 hw/display/vhost-user-gpu.c |  1 +
 hw/display/vhost-user-vga.c |  1 +
 hw/display/virtio-gpu-base.c|  1 +
 hw/display/virtio-gpu-gl.c  |  1 +
 hw/display/virtio-gpu-pci-gl.c  |  1 +
 hw/display/virtio-gpu-pci.c |  1 +
 hw/display/virtio-gpu.c |  1 +
 hw/display/virtio-vga-gl.c  |  1 +
 hw/display/virtio-vga.c |  1 +
 hw/s390x/virtio-ccw-gpu.c   |  1 +
 hw/usb/ccid-card-emulated.c |  1 +
 hw/usb/ccid-card-passthru.c |  1 +
 hw/usb/host-libusb.c|  1 +
 hw/usb/redirect.c   |  1 +
 include/qemu/module.h   | 10 +
 meson.build | 25 ++---
 scripts/modinfo-generate.py | 40 -
 19 files changed, 67 insertions(+), 24 deletions(-)

-- 
2.33.0

[PATCH v2 0/2] modules: Improve modinfo.c support

[Jose R. Ziviani]

This patchset introduces the modinfo_need and changes
modinfo-generate.py/meson.build to generate/link one modinfo per target.

modinfo-generate.py will know, thanks to modinfo_need, which modules are
currently enabled for a given target before adding it in the array of
modules. It will give a hint about why some modules failed, so
developers can have a clue about it:

},{
/* hw-display-qxl.modinfo */
/* module  QXL is missing. */

/* hw-display-virtio-gpu.modinfo */
.name = "hw-display-virtio-gpu",
.objs = ((const char*[]){  "virtio-gpu-base",  "virtio-gpu-device",  
"vhost-user-gpu", NULL }),
},{

The main reason of this change is to fix problems like:
$ ./qemu-system-s390x -nodefaults -display none -accel tcg -M none -device help 
| head
Failed to open module: /.../hw-display-qxl.so: undefined symbol: vga_ioport_read
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga.so: undefined symbol: 
vmstate_vga_common
Failed to open module: /.../hw-display-virtio-vga-gl.so: undefined symbol: 
have_vga
Failed to open module: /.../hw-usb-smartcard.so: undefined symbol: 
ccid_card_ccid_attach
Failed to open module: /.../hw-usb-redirect.so: undefined symbol: 
vmstate_usb_device
Failed to open module: /.../hw-usb-host.so: undefined symbol: vmstate_usb_device

With this patch, I run this small script successfuly:
#!/bin/bash
pushd ~/suse/virtualization/qemu/build
for qemu in qemu-system-*
do
[[ -f "$qemu" ]] || continue
res=$(./$qemu -nodefaults -display none -accel tcg -M none -device help 
2>&1 | grep "Failed to" > /dev/null; echo $?)
[[ $res -eq 0 ]] && echo "Error: $qemu"
done
popd

Also run make check and check-acceptance without any failures.

Todo:
 - accelerators can be filtered as well (this only covers the device
   part), then the field QemuModinfo.arch can be removed.

v1 -> v2:
 - Changed the approach to this problem after suggestions made by Paolo and 
Gerd.

Thank you!

Jose R. Ziviani (2):
  modules: introduces module_needs directive
  modules: generates per-target modinfo

 hw/display/qxl.c|  1 +
 hw/display/vhost-user-gpu-pci.c |  1 +
 hw/display/vhost-user-gpu.c |  1 +
 hw/display/vhost-user-vga.c |  1 +
 hw/display/virtio-gpu-base.c|  1 +
 hw/display/virtio-gpu-gl.c  |  1 +
 hw/display/virtio-gpu-pci-gl.c  |  1 +
 hw/display/virtio-gpu-pci.c |  1 +
 hw/display/virtio-gpu.c |  1 +
 hw/display/virtio-vga-gl.c  |  1 +
 hw/display/virtio-vga.c |  1 +
 hw/s390x/virtio-ccw-gpu.c   |  1 +
 hw/usb/ccid-card-emulated.c |  1 +
 hw/usb/ccid-card-passthru.c |  1 +
 hw/usb/host-libusb.c|  1 +
 hw/usb/redirect.c   |  1 +
 include/qemu/module.h   | 10 +
 meson.build | 25 ++---
 scripts/modinfo-generate.py | 40 -
 19 files changed, 67 insertions(+), 24 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] modules: introduces module_needs directive

[Jose R. Ziviani]

module_needs is a new directive that shoule be used with module_obj
whenever that module depends on the Kconfig to be enabled.

When the module is enabled in Kconfig, we are sure that all of its
dependencies will be enabled as well, and that the program will be
able to load that module without any problem.

The correct way to use module_needs is by passing the Kconfig option
(or the *config-devices.mak without CONFIG_).

Signed-off-by: Jose R. Ziviani 
---
 hw/display/qxl.c|  1 +
 hw/display/vhost-user-gpu-pci.c |  1 +
 hw/display/vhost-user-gpu.c |  1 +
 hw/display/vhost-user-vga.c |  1 +
 hw/display/virtio-gpu-base.c|  1 +
 hw/display/virtio-gpu-gl.c  |  1 +
 hw/display/virtio-gpu-pci-gl.c  |  1 +
 hw/display/virtio-gpu-pci.c |  1 +
 hw/display/virtio-gpu.c |  1 +
 hw/display/virtio-vga-gl.c  |  1 +
 hw/display/virtio-vga.c |  1 +
 hw/s390x/virtio-ccw-gpu.c   |  1 +
 hw/usb/ccid-card-emulated.c |  1 +
 hw/usb/ccid-card-passthru.c |  1 +
 hw/usb/host-libusb.c|  1 +
 hw/usb/redirect.c   |  1 +
 include/qemu/module.h   | 10 ++
 scripts/modinfo-generate.py |  2 ++
 18 files changed, 28 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 29c80b4289..33647d59ad 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2494,6 +2494,7 @@ static const TypeInfo qxl_primary_info = {
 .class_init= qxl_primary_class_init,
 };
 module_obj("qxl-vga");
+module_needs(QXL);
 
 static void qxl_secondary_class_init(ObjectClass *klass, void *data)
 {
diff --git a/hw/display/vhost-user-gpu-pci.c b/hw/display/vhost-user-gpu-pci.c
index daefcf7101..d47219f294 100644
--- a/hw/display/vhost-user-gpu-pci.c
+++ b/hw/display/vhost-user-gpu-pci.c
@@ -44,6 +44,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_gpu_pci_info 
= {
 .instance_init = vhost_user_gpu_pci_initfn,
 };
 module_obj(TYPE_VHOST_USER_GPU_PCI);
+module_needs(VHOST_USER_GPU);
 
 static void vhost_user_gpu_pci_register_types(void)
 {
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 49df56cd14..6a229f2b34 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -599,6 +599,7 @@ static const TypeInfo vhost_user_gpu_info = {
 .class_init = vhost_user_gpu_class_init,
 };
 module_obj(TYPE_VHOST_USER_GPU);
+module_needs(VHOST_USER_GPU);
 
 static void vhost_user_gpu_register_types(void)
 {
diff --git a/hw/display/vhost-user-vga.c b/hw/display/vhost-user-vga.c
index 072c9c65bc..a7b9f3580d 100644
--- a/hw/display/vhost-user-vga.c
+++ b/hw/display/vhost-user-vga.c
@@ -45,6 +45,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_vga_info = {
 .instance_init = vhost_user_vga_inst_initfn,
 };
 module_obj(TYPE_VHOST_USER_VGA);
+module_needs(VHOST_USER_VGA);
 
 static void vhost_user_vga_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index c8da4806e0..9c485151fc 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -257,6 +257,7 @@ static const TypeInfo virtio_gpu_base_info = {
 .abstract = true
 };
 module_obj(TYPE_VIRTIO_GPU_BASE);
+module_needs(VIRTIO_GPU);
 
 static void
 virtio_register_types(void)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 6cc4313b1a..c57707f6f1 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -160,6 +160,7 @@ static const TypeInfo virtio_gpu_gl_info = {
 .class_init = virtio_gpu_gl_class_init,
 };
 module_obj(TYPE_VIRTIO_GPU_GL);
+module_needs(VIRTIO_GPU);
 
 static void virtio_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-pci-gl.c b/hw/display/virtio-gpu-pci-gl.c
index 99b14a0718..3817f0dd9d 100644
--- a/hw/display/virtio-gpu-pci-gl.c
+++ b/hw/display/virtio-gpu-pci-gl.c
@@ -47,6 +47,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_gl_pci_info = 
{
 .instance_init = virtio_gpu_gl_initfn,
 };
 module_obj(TYPE_VIRTIO_GPU_GL_PCI);
+module_needs(VIRTIO_PCI);
 
 static void virtio_gpu_gl_pci_register_types(void)
 {
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index e36eee0c40..3219adcf2d 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -65,6 +65,7 @@ static const TypeInfo virtio_gpu_pci_base_info = {
 .abstract = true
 };
 module_obj(TYPE_VIRTIO_GPU_PCI_BASE);
+module_needs(VIRTIO_PCI);
 
 #define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
 typedef struct VirtIOGPUPCI VirtIOGPUPCI;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 182e0868b0..874808326f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1450,6 +1450,7 @@ static const TypeInfo virtio_gpu_info = {
 .class_init = virtio_gpu_class_init,
 };
 module_obj(TYPE_VIRTIO_GPU);
+module_needs(VIRTIO_GPU);
 
 static void virtio_register_types(void)
 {
diff --git a/hw/display/virtio-vga-gl.c b/hw/display/virtio-vga-gl.c
index f22549097

[PATCH v2 2/2] modules: generates per-target modinfo

[Jose R. Ziviani]

This patch changes the way modinfo is generated and built. Today we have
only modinfo.c being genereated and linked to all targets, now it
generates (and link) one modinfo per target.

It also makes use of the module_need to add modules that makes sense for
the selected target.

Signed-off-by: Jose R. Ziviani 
---
 meson.build | 25 +++
 scripts/modinfo-generate.py | 40 +
 2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/meson.build b/meson.build
index 2711cbb789..9d25ebb2f9 100644
--- a/meson.build
+++ b/meson.build
@@ -2395,14 +2395,23 @@ foreach d, list : target_modules
 endforeach
 
 if enable_modules
-  modinfo_src = custom_target('modinfo.c',
-  output: 'modinfo.c',
-  input: modinfo_files,
-  command: [modinfo_generate, '@INPUT@'],
-  capture: true)
-  modinfo_lib = static_library('modinfo', modinfo_src)
-  modinfo_dep = declare_dependency(link_whole: modinfo_lib)
-  softmmu_ss.add(modinfo_dep)
+  foreach target : target_dirs
+if target.endswith('-softmmu')
+  config_target = config_target_mak[target]
+  config_devices_mak = target + '-config-devices.mak'
+  modinfo_src = custom_target('modinfo-' + target + '.c',
+  output: 'modinfo-' + target + '.c',
+  input: modinfo_files,
+  command: [modinfo_generate, '--devices', 
config_devices_mak, '@INPUT@'],
+  capture: true)
+
+  modinfo_lib = static_library('modinfo-' + target + '.c', modinfo_src)
+  modinfo_dep = declare_dependency(link_with: modinfo_lib)
+
+  arch = config_target['TARGET_NAME'] == 'sparc64' ? 'sparc64' : 
config_target['TARGET_BASE_ARCH']
+  hw_arch[arch].add(modinfo_dep)
+endif
+  endforeach
 endif
 
 nm = find_program('nm')
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index 9d3e037b15..25fb241b2d 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -32,7 +32,7 @@ def parse_line(line):
 continue
 return (kind, data)
 
-def generate(name, lines):
+def generate(name, lines, core_modules):
 arch = ""
 objs = []
 deps = []
@@ -49,7 +49,11 @@ def generate(name, lines):
 elif kind == 'arch':
 arch = data;
 elif kind == 'need':
-pass # ignore
+# don't add a module which dependency is not enabled
+# in kconfig
+if data.strip() not in core_modules:
+print("/* module {} is missing. */\n".format(data))
+return []
 else:
 print("unknown:", kind)
 exit(1)
@@ -60,7 +64,7 @@ def generate(name, lines):
 print_array("objs", objs)
 print_array("deps", deps)
 print_array("opts", opts)
-print("},{");
+print("},{")
 return deps
 
 def print_pre():
@@ -74,26 +78,28 @@ def print_post():
 print("}};")
 
 def main(args):
+if len(args) < 3 or args[0] != '--devices':
+print('Expected: modinfo-generate.py --devices '
+  'config-device.mak [modinfo files]', file=sys.stderr)
+exit(1)
+
+# get all devices enabled in kconfig, from *-config-device.mak
+enabled_core_modules = set()
+with open(args[1]) as file:
+for line in file.readlines():
+config = line.split('=')
+if config[1].rstrip() == 'y':
+enabled_core_modules.add(config[0][7:]) # remove CONFIG_
+
 deps = {}
 print_pre()
-for modinfo in args:
+for modinfo in args[2:]:
 with open(modinfo) as f:
 lines = f.readlines()
 print("/* %s */" % modinfo)
-(basename, ext) = os.path.splitext(modinfo)
-deps[basename] = generate(basename, lines)
+(basename, _) = os.path.splitext(modinfo)
+deps[basename] = generate(basename, lines, enabled_core_modules)
 print_post()
 
-flattened_deps = {flat.strip('" ') for dep in deps.values() for flat in 
dep}
-error = False
-for dep in flattened_deps:
-if dep not in deps.keys():
-print("Dependency {} cannot be satisfied".format(dep),
-  file=sys.stderr)
-error = True
-
-if error:
-exit(1)
-
 if __name__ == "__main__":
 main(sys.argv[1:])
-- 
2.33.0

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

Hello!!

On Tue, Sep 21, 2021 at 07:25:42AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > But, in anyway, I'll still need to store the target architecture that
> > can use such core module, like I did here in this patch. Otherwise,
> > if I compile different targets at the same time, I'll end up with the
> > same problem of targets trying to load wrong modules.
> 
> That all works just fine today.  If you have target-specific modules
> (i.e. source files added to specific_ss instead of softmmu_ss when
> compiling into core qemu) you only need to add those to the
> target_modules[] (instead of modules[]) and you are set.
> 
> In-tree example: qtest accelerator.

Exactly, but what it doesn't seem to work (or I'm not understanding it
well) is: how the target will know whether it can or cannot load a
module.

For example, suppose I build target-list=s390x-softmmu,x86_64-softmmu.
Both targets will be linked to the same modinfo.c object, which will
have a 'hw-display-virtio-gpu-pci' entry (it wouldn't if I build only
s390x-softmmu). When I execute ./qemu-system-s390x, it will try to
load hw-display-virtio-gpu-pci and will throw the errors I mentioned
earlier.

If, for example, I add that module_need(PCI_BUS), we will continue
not knowing whether the target in execution has the required bus
(without injecting dependencies in module.c). But it would be easier if
we have such information in modinfo.c directly (of an modinfo-.c).
I understood that it's not an arch issue, it's the target that doesn't
current implement such core module. But, in practice, we will end up
need to query that information.

Please, correct me if I'm not getting the point correctly.

Thank you!!


> 
> take care,
>   Gerd
> 


signature.asc
Description: Digital signature

Re: [PATCH 1/2] meson: introduce modules_arch

[Jose R. Ziviani]

Hello!!

On Mon, Sep 20, 2021 at 09:03:28PM +0200, Paolo Bonzini wrote:
> On 20/09/21 15:02, Jose R. Ziviani wrote:
> > But, in anyway, I'll still need to store the target architecture that
> > can use such core module, like I did here in this patch. Otherwise,
> > if I compile different targets at the same time, I'll end up with the
> > same problem of targets trying to load wrong modules.
> > 
> > I thought of using qom, but I think it will pollute module.c.
> 
> Alternatively, you could C-ify the contents of config-devices.mak, and embed
> them in the per-arch modinfo-*.c; and record CONFIG_* symbols for each
> module (e.g. '{ "CONFIG_QXL", "hw-display-qxl" }' from a
> 'module_config("CONFIG_QXL")' line in the global modinfo.c file.  Then
> before loading a module you do a binary search on the per-arch
> config-devices array.

With a per-arch modinfo-*.c we don't even need a modinfo.c global, do
we?

Each target could be linked to its own modinfo-target.c only.

> 
> I hope the above is readable. :)

Absolutely, thank you for your suggestion!!

> 
> Paolo


signature.asc
Description: Digital signature

[PATCH 0/1]

[Jose R. Ziviani]

Hello!

This patch gives the ability to build TCG builtin even if
--enable-modules is selected. This is useful to have a base
QEMU with TCG native product but still using the benefits of
modules.

Thank you!

Jose R. Ziviani (1):
  modules: Option to build native TCG with --enable-modules

 configure | 12 ++--
 meson.build   | 11 ++-
 meson_options.txt |  2 ++
 3 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.32.0

[PATCH 1/1] modules: Option to build native TCG with --enable-modules

[Jose R. Ziviani]

Adds an option (--enable-tcg-builtin) to build TCG natively when
--enable-modules argument is passed to the build system. It gives
the opportunity to have this important accelerator built-in and
still take advantage of the new modular system.

Signed-off-by: Jose R. Ziviani 
---
 configure | 12 ++--
 meson.build   | 11 ++-
 meson_options.txt |  2 ++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 232c54dcc1..64d7a909ce 100755
--- a/configure
+++ b/configure
@@ -345,6 +345,7 @@ tsan="no"
 fortify_source="$default_feature"
 strip_opt="yes"
 tcg_interpreter="false"
+tcg_builtin="false"
 bigendian="no"
 mingw32="no"
 gcov="no"
@@ -1120,6 +1121,8 @@ for opt do
   ;;
   --enable-tcg) tcg="enabled"
   ;;
+  --enable-tcg-builtin) tcg_builtin="true"
+  ;;
   --disable-malloc-trim) malloc_trim="disabled"
   ;;
   --enable-malloc-trim) malloc_trim="enabled"
@@ -1817,6 +1820,7 @@ Advanced options (experts only):
Default:trace-
   --disable-slirp  disable SLIRP userspace network connectivity
   --enable-tcg-interpreter enable TCI (TCG with bytecode interpreter, 
experimental and slow)
+  --enable-tcg-builtin force TCG builtin even with --enable-modules
   --enable-malloc-trim enable libc malloc_trim() for memory optimization
   --oss-libpath to OSS library
   --cpu=CPUBuild for host CPU [$cpu]
@@ -2318,7 +2322,11 @@ if test "$solaris" = "yes" ; then
   fi
 fi
 
-if test "$tcg" = "enabled"; then
+if test "$tcg" = "disabled"; then
+debug_tcg="no"
+tcg_interpreter="false"
+tcg_builtin="false"
+else
 git_submodules="$git_submodules tests/fp/berkeley-testfloat-3"
 git_submodules="$git_submodules tests/fp/berkeley-softfloat-3"
 fi
@@ -5229,7 +5237,7 @@ if test "$skip_meson" = no; then
 -Dvhost_user_blk_server=$vhost_user_blk_server 
-Dmultiprocess=$multiprocess \
 -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek 
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
 $(if test "$default_features" = no; then echo 
"-Dauto_features=disabled"; fi) \
-   -Dtcg_interpreter=$tcg_interpreter \
+-Dtcg_interpreter=$tcg_interpreter -Dtcg_builtin=$tcg_builtin \
 $cross_arg \
 "$PWD" "$source_path"
 
diff --git a/meson.build b/meson.build
index 2f377098d7..2909043aab 100644
--- a/meson.build
+++ b/meson.build
@@ -93,9 +93,13 @@ if cpu in ['x86', 'x86_64']
 endif
 
 modular_tcg = []
+is_tcg_modular = false
 # Darwin does not support references to thread-local variables in modules
 if targetos != 'darwin'
   modular_tcg = ['i386-softmmu', 'x86_64-softmmu']
+  is_tcg_modular = config_host.has_key('CONFIG_MODULES') \
+   and get_option('tcg').enabled() \
+   and not get_option('tcg_builtin')
 endif
 
 edk2_targets = [ 'arm-softmmu', 'aarch64-softmmu', 'i386-softmmu', 
'x86_64-softmmu' ]
@@ -279,6 +283,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()
@@ -1567,7 +1574,7 @@ foreach target : target_dirs
   elif sym == 'CONFIG_XEN' and have_xen_pci_passthrough
 config_target += { 'CONFIG_XEN_PCI_PASSTHROUGH': 'y' }
   endif
-  if target in modular_tcg
+  if target in modular_tcg and is_tcg_modular
 config_target += { 'CONFIG_TCG_MODULAR': 'y' }
   else
 config_target += { 'CONFIG_TCG_BUILTIN': 'y' }
@@ -2976,6 +2983,8 @@ summary_info += {'TCG support':   
config_all.has_key('CONFIG_TCG')}
 if config_all.has_key('CONFIG_TCG')
   if get_option('tcg_interpreter')
 summary_info += {'TCG backend':   'TCI (TCG with bytecode interpreter, 
experimental and slow)'}
+  elif is_tcg_modular
+summary_info += {'TCG backend':   'module (@0@)'.format(cpu)}
   else
 summary_info += {'TCG backend':   'native (@0@)'.format(cpu)}
   endif
diff --git a/meson_options.txt b/meson_options.txt
index a9a9b8f4c6..c27749b864 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,8 @@ option('tcg', type: 'feature', value: 'auto',
description: 'TCG support')
 option('tcg_interpreter', type: 'boolean', value: false,
description: 'TCG with bytecode interpreter (experimental and slow)')
+option('tcg_builtin', type: 'boolean', value: 'false',
+   description: 'Force TCG builtin')
 option('cfi', type: 'boolean', value: 'false',
description: 'Control-Flow Integrity (CFI)')
 option('cfi_debug', type: 'boolean', value: 'false',
-- 
2.32.0

[PATCH 0/2] Improve module accelerator error message

[Jose R. Ziviani]

The main objective here is to fix an user issue when trying to load TCG
that was built as module, but it's not installed or found in the library
path.

For example:

$ ./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 ...

The new error message becomes:

$ ./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.

Jose R. Ziviani (2):
  modules: Implement new helper functions
  qom: Improve error message in module_object_class_by_name()

 accel/accel-softmmu.c |  5 +++-
 include/qemu/module.h |  4 +++
 qom/object.c  |  9 +++
 util/module.c | 57 +--
 4 files changed, 67 insertions(+), 8 deletions(-)

-- 
2.32.0

[PATCH 1/2] modules: Implement new helper functions

[Jose R. Ziviani]

The function module_load_one() fills a hash table with 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.

It also creates a function that returns the module name given the
typename.

Signed-off-by: Jose R. Ziviani 
---
 include/qemu/module.h |  4 +++
 util/module.c | 57 +--
 2 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 3deac0078b..f45773718d 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,6 +14,7 @@
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#include 
 
 #define DSO_STAMP_FUN glue(qemu_stamp, CONFIG_STAMP)
 #define DSO_STAMP_FUN_STR stringify(DSO_STAMP_FUN)
@@ -74,6 +75,9 @@ 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);
+const char *module_get_name_from_obj(const char *obj);
+
 /**
  * DOC: module info annotation macros
  *
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..f23adc65bf 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,39 @@ 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;
+}
+
+const char *module_get_name_from_obj(const char *obj)
+{
+const QemuModinfo *modinfo;
+const char **sl;
+
+if (!obj) {
+return NULL;
+}
+
+for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (!modinfo->objs) {
+continue;
+}
+
+for (sl = modinfo->objs; *sl != NULL; sl++) {
+if (strcmp(obj, *sl) == 0) {
+return modinfo->name;
+}
+}
+}
+
+return NULL;
+}
+
 #else
 
 void module_allow_arch(const char *arch) {}
@@ -384,4 +412,19 @@ 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;
+}
+
+char *module_get_name_from_obj(const char *obj)
+{
+return NULL;
+}
+
 #endif
-- 
2.32.0

[PATCH 2/2] 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.

Or with other modules, when possible:
$ ./qemu-system-x86_64 -machine q35 -accel kvm -kernel /boot/vmlinuz -vga qxl   
  ✹
hw-display-qxl module is missing, install the package or config the library 
path correctly.
qemu-system-x86_64: QXL VGA not available

$ 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 
---
 accel/accel-softmmu.c | 5 -
 qom/object.c  | 9 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..52449ac2d0 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
  * all accelerators need to define ops, providing at least a mandatory
  * non-NULL create_vcpu_thread operation.
  */
-g_assert(ops != NULL);
+if (ops == NULL) {
+exit(1);
+}
+
 if (ops->ops_init) {
 ops->ops_init(ops);
 }
diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..3a170ea9df 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -10,6 +10,7 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include "qemu/module.h"
 #include "qemu/osdep.h"
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
@@ -1031,8 +1032,16 @@ ObjectClass *module_object_class_by_name(const char 
*typename)
 oc = object_class_by_name(typename);
 #ifdef CONFIG_MODULES
 if (!oc) {
+const char *module_name = module_get_name_from_obj(typename);
 module_load_qom_one(typename);
 oc = object_class_by_name(typename);
+if (!oc && 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);
+}
+}
 }
 #endif
 return oc;
-- 
2.32.0

Re: [PATCH 0/1]

[Jose R. Ziviani]

On Wed, Jul 21, 2021 at 07:24:02AM +0200, Thomas Huth wrote:
> On 21/07/2021 00.13, Jose R. Ziviani wrote:
> > Hello!
> > 
> > This patch gives the ability to build TCG builtin even if
> > --enable-modules is selected. This is useful to have a base
> > QEMU with TCG native product but still using the benefits of
> > modules.
> 
> Could you please elaborate why this is required? Did you see a performance
> improvement? Or is there another problem?

Hello Thomas,

Please, disconsider this patch. There's a more general discussion about
modules happening here:

https://lists.nongnu.org/archive/html/qemu-devel/2021-07/msg00632.html

A more general solution may be required to actually give us a
fine-grained control on modules.

The case is to allow us to generate customized QEMU packages attending
different user needs.

Thank you very much!!

Jose

> 
>  Thomas
> 


signature.asc
Description: Digital signature

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

[Jose R. Ziviani]

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 :|
> 


signature.asc
Description: Digital signature

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

[Jose R. Ziviani]

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 
> > ---
> >  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


signature.asc
Description: Digital signature

[PATCH v2 0/1] Improve module accelerator error message

[Jose R. Ziviani]

v1 -> v2:
* Moved the code to module.c
* Simplified a lot by using current module DB to get info

The main objective is to improve the error message when trying to
load a not found/not installed module TCG.

For example:

$ qemu-system-x86_64 -accel tcg
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 ...

To:
$ qemu-system-x86_64 -accel tcg
accel-tcg-x86_64 module is missing, install the package or config the library 
path correctly.

Jose R. Ziviani (1):
  modules: Improve error message when module is not found

 accel/accel-softmmu.c |  5 -
 util/module.c | 14 --
 2 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.32.0

[PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

When a module is not found, specially accelerators, QEMU displays
a error message that not easy to understand[1]. This patch improves
the readability by offering a user-friendly message[2].

This patch also moves the accelerator ops check to runtine (instead
of the original g_assert) because it works better with dynamic
modules.

[1] qemu-system-x86_64 -accel tcg
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)
31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

[2] qemu-system-x86_64 -accel tcg
accel-tcg-x86_64 module is missing, install the package or config the library 
path correctly.

Signed-off-by: Jose R. Ziviani 
---
 accel/accel-softmmu.c |  5 -
 util/module.c | 14 --
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..52449ac2d0 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
  * all accelerators need to define ops, providing at least a mandatory
  * non-NULL create_vcpu_thread operation.
  */
-g_assert(ops != NULL);
+if (ops == NULL) {
+exit(1);
+}
+
 if (ops->ops_init) {
 ops->ops_init(ops);
 }
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..268a8563fd 100644
--- a/util/module.c
+++ b/util/module.c
@@ -206,13 +206,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;
@@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char 
*lib_name, bool mayfail)
 
 if (!success) {
 g_hash_table_remove(loaded_modules, module_name);
+fprintf(stderr, "%s module is missing, install the "
+"package or config the library path "
+"correctly.\n", module_name);
 g_free(module_name);
 }
 
@@ -307,12 +307,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)
@@ -384,4 +381,9 @@ 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;
+}
+
 #endif
-- 
2.32.0

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> > When a module is not found, specially accelerators, QEMU displays
> > a error message that not easy to understand[1]. This patch improves
> > the readability by offering a user-friendly message[2].
> > 
> > This patch also moves the accelerator ops check to runtine (instead
> > of the original g_assert) because it works better with dynamic
> > modules.
> > 
> > [1] qemu-system-x86_64 -accel tcg
> > 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)
> > 31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> > 
> > [2] qemu-system-x86_64 -accel tcg
> > accel-tcg-x86_64 module is missing, install the package or config the 
> > library path correctly.
> > 
> > Signed-off-by: Jose R. Ziviani 
> > ---
> >  accel/accel-softmmu.c |  5 -
> >  util/module.c | 14 --
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> > index 67276e4f52..52449ac2d0 100644
> > --- a/accel/accel-softmmu.c
> > +++ b/accel/accel-softmmu.c
> > @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >   * all accelerators need to define ops, providing at least a mandatory
> >   * non-NULL create_vcpu_thread operation.
> >   */
> > -g_assert(ops != NULL);
> > +if (ops == NULL) {
> > +exit(1);
> > +}
> > +
> 
> 
> Ah, again, why?
> This change looks wrong to me, 
> 
> the ops code should be present when ops interfaces are initialized:
> it should be a code level assertion, as it has to do with the proper order of 
> initializations in QEMU,
> 
> why would we want to do anything else but to assert here?
> 
> Am I blind to something obvious?

Hello!

Thank you for reviewing it!

The problem is that if your TCG module is not installed and you start
QEMU like:

./qemu-system-x86_64 -accel tcg

You'll get the error message + a crash with a core dump:

accel-tcg-x86_64 module is missing, install the package or config the library 
path correctly.
**
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]5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg

I was digging a little bit more in order to move this responsibility to
module.c but there isn't enough information there to safely exit() in
all situations that a module may be loaded. As Gerd mentioned, more work
is needed in order to achieve that.

However, it's not nice to have a crash due to an optional module missing.
It's specially confusing because TCG has always been native. Considering
also that we're already in hard freeze for 6.1, I thought to have this
simpler check instead.

What do you think if we have something like:

/* FIXME: this isn't the right place to handle a missing module and
   must be reverted when the module refactoring is completely done */
#ifdef CONFIG_MODULES
if (ops == NULL) {
exit(1);
}
#else
g_assert(ops != NULL);
#endif

Regards!

> 
> >  if (ops->ops_init) {
> >  ops->ops_init(ops);
> >  }
> > diff --git a/util/module.c b/util/module.c
> > index 6bb4ad915a..268a8563fd 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -206,13 +206,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;
> > @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char 
> > *lib_name, bool mayfail)
> >  
> >  if (!success) {
> >  g_hash_table_remove(loaded_modules, module_name);
> > +fprintf(stderr, "%s module is missing, install the "
> > +"package or config the library path "
> > +"correctly.\n", module_name);
> >  g_free(module_name);
> >  }
> >  
> > @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, c

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

On Fri, Jul 23, 2021 at 04:02:26PM +0200, Claudio Fontana wrote:
> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> > On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> >> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> >>> When a module is not found, specially accelerators, QEMU displays
> >>> a error message that not easy to understand[1]. This patch improves
> >>> the readability by offering a user-friendly message[2].
> >>>
> >>> This patch also moves the accelerator ops check to runtine (instead
> >>> of the original g_assert) because it works better with dynamic
> >>> modules.
> >>>
> >>> [1] qemu-system-x86_64 -accel tcg
> >>> 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)
> >>> 31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> >>>
> >>> [2] qemu-system-x86_64 -accel tcg
> >>> accel-tcg-x86_64 module is missing, install the package or config the 
> >>> library path correctly.
> >>>
> >>> Signed-off-by: Jose R. Ziviani 
> >>> ---
> >>>  accel/accel-softmmu.c |  5 -
> >>>  util/module.c | 14 --
> >>>  2 files changed, 12 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> >>> index 67276e4f52..52449ac2d0 100644
> >>> --- a/accel/accel-softmmu.c
> >>> +++ b/accel/accel-softmmu.c
> >>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >>>   * all accelerators need to define ops, providing at least a 
> >>> mandatory
> >>>   * non-NULL create_vcpu_thread operation.
> >>>   */
> >>> -g_assert(ops != NULL);
> >>> +if (ops == NULL) {
> >>> +exit(1);
> >>> +}
> >>> +
> >>
> >>
> >> Ah, again, why?
> >> This change looks wrong to me, 
> >>
> >> the ops code should be present when ops interfaces are initialized:
> >> it should be a code level assertion, as it has to do with the proper order 
> >> of initializations in QEMU,
> >>
> >> why would we want to do anything else but to assert here?
> >>
> >> Am I blind to something obvious?
> > 
> > Hello!
> > 
> > Thank you for reviewing it!
> > 
> > The problem is that if your TCG module is not installed and you start
> > QEMU like:
> > 
> > ./qemu-system-x86_64 -accel tcg
> > 
> > You'll get the error message + a crash with a core dump:
> > 
> > accel-tcg-x86_64 module is missing, install the package or config the 
> > library path correctly.
> > **
> > 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]5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> > 
> > I was digging a little bit more in order to move this responsibility to
> > module.c but there isn't enough information there to safely exit() in
> > all situations that a module may be loaded. As Gerd mentioned, more work
> > is needed in order to achieve that.
> > 
> > However, it's not nice to have a crash due to an optional module missing.
> > It's specially confusing because TCG has always been native. Considering
> > also that we're already in hard freeze for 6.1, I thought to have this
> > simpler check instead.
> > 
> > What do you think if we have something like:
> > 
> > /* FIXME: this isn't the right place to handle a missing module and
> >must be reverted when the module refactoring is completely done */
> > #ifdef CONFIG_MODULES
> > if (ops == NULL) {
> > exit(1);
> > }
> > #else
> > g_assert(ops != NULL);
> > #endif
> > 
> > Regards!
> 
> 
> For the normal builds (without modular tcg), this issue does not appear right?

Yes, but OpenSUSE already builds with --enable-modules, we've already been 
shipping
several modules as optional RPMs, like qemu-hw-display-virtio-gpu for example. 
I sent
a patch some weeks ago to add "--enable-tcg-builtin" in the build system but 
there're
more work required in that area 

Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Jose R. Ziviani]

On Fri, Jul 23, 2021 at 05:27:25PM +0200, Claudio Fontana wrote:
> On 7/23/21 4:36 PM, Jose R. Ziviani wrote:
> > On Fri, Jul 23, 2021 at 04:02:26PM +0200, Claudio Fontana wrote:
> >> On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> >>> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
> >>>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
> >>>>> When a module is not found, specially accelerators, QEMU displays
> >>>>> a error message that not easy to understand[1]. This patch improves
> >>>>> the readability by offering a user-friendly message[2].
> >>>>>
> >>>>> This patch also moves the accelerator ops check to runtine (instead
> >>>>> of the original g_assert) because it works better with dynamic
> >>>>> modules.
> >>>>>
> >>>>> [1] qemu-system-x86_64 -accel tcg
> >>>>> 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)
> >>>>> 31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> >>>>>
> >>>>> [2] qemu-system-x86_64 -accel tcg
> >>>>> accel-tcg-x86_64 module is missing, install the package or config the 
> >>>>> library path correctly.
> >>>>>
> >>>>> Signed-off-by: Jose R. Ziviani 
> >>>>> ---
> >>>>>  accel/accel-softmmu.c |  5 -
> >>>>>  util/module.c | 14 --
> >>>>>  2 files changed, 12 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> >>>>> index 67276e4f52..52449ac2d0 100644
> >>>>> --- a/accel/accel-softmmu.c
> >>>>> +++ b/accel/accel-softmmu.c
> >>>>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
> >>>>>   * all accelerators need to define ops, providing at least a 
> >>>>> mandatory
> >>>>>   * non-NULL create_vcpu_thread operation.
> >>>>>   */
> >>>>> -g_assert(ops != NULL);
> >>>>> +if (ops == NULL) {
> >>>>> +exit(1);
> >>>>> +}
> >>>>> +
> >>>>
> >>>>
> >>>> Ah, again, why?
> >>>> This change looks wrong to me, 
> >>>>
> >>>> the ops code should be present when ops interfaces are initialized:
> >>>> it should be a code level assertion, as it has to do with the proper 
> >>>> order of initializations in QEMU,
> >>>>
> >>>> why would we want to do anything else but to assert here?
> >>>>
> >>>> Am I blind to something obvious?
> >>>
> >>> Hello!
> >>>
> >>> Thank you for reviewing it!
> >>>
> >>> The problem is that if your TCG module is not installed and you start
> >>> QEMU like:
> >>>
> >>> ./qemu-system-x86_64 -accel tcg
> >>>
> >>> You'll get the error message + a crash with a core dump:
> >>>
> >>> accel-tcg-x86_64 module is missing, install the package or config the 
> >>> library path correctly.
> >>> **
> >>> 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]5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> >>>
> >>> I was digging a little bit more in order to move this responsibility to
> >>> module.c but there isn't enough information there to safely exit() in
> >>> all situations that a module may be loaded. As Gerd mentioned, more work
> >>> is needed in order to achieve that.
> >>>
> >>> However, it's not nice to have a crash due to an optional module missing.
> >>> It's specially confusing because TCG has always been native. Considering
> >>> also that we're already in hard freeze for 6.1, I thought to have this
> >>> simpler check instead.
> >>>
>