[PATCH 00/10] RFC: Move SPICE to a load module

[PATCH 00/10] RFC: Move SPICE to a load module

[Christophe de Dinechin]

There has been a bit of discussion regarding the possibility to move
SPICE code to a module in order to reduce the attack surface and
memory usage when SPICE is not used.

This WIP series proposes one way to do it that is relatively cheap in
terms of code changes, relative to other ideas I tested, and seems to
be working on simple test cases, unlike a couple of more
"sophisiticated" ideas I tried where I ran into rather nasty SPICE
initialization order issues.

Furthermore, the approach presented here requires relatively few code
changes in order to apply to other components as well. It relies on a
couple of macros added to the module.h file, MODIFACE and MODIMPLE.

MODIFACE declare the interface for a module function. A module
function is transformed into a pointer when you build with modules,
and that pointer is used instead of the original function.

MODIMPL implements a MODIFACE, and patches the pointer at load time to
call the function in the shared library.

Thanks to some suggestions from Gerd, I also moved QXL to a module,
although at the moment it does not load correctly.

There are a few known hacks in the present state, including:

- Adding various non-UI files into spice-app.so, which required a
  couple of ../pwd/foo.o references in the makefile. Not very nice,
  but I did not want to spend too much time fixing the makefiles.

- qmp_query_spice had to be dealt with "manually" because its
  interface is generated.

With these changes, the following shared libraries are no longer
needed in the main binary:

 	libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
 	libopus.so.0 => /lib64/libopus.so.0 (HEX)
 	liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
 	libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
 	libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
 	libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
 	libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
 	libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
 	liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)

I will keep pushing updates on branch "modular-spice"
on https://github.com/c3d/qemu.git

Christophe de Dinechin (10):
  modules: Provide macros making it easier to identify module exports
  minikconf: Pass variables for modules
  spice: Make spice a module configuration
  spice: Move all the spice-related code in spice-app.so
  build: Avoid build failure when building drivers as modules
  trivial: Remove extra trailing whitespace
  qxl - FIXME: Build as module
  build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files
  spice: Put spice functions in a separate load module
  REMOVE: Instrumentation to show the module functions being replaced

 Makefile                 |  1 +
 Makefile.objs            |  2 ++
 Makefile.target          |  7 +++++++
 audio/Makefile.objs      |  2 +-
 chardev/Makefile.objs    |  2 +-
 configure                |  6 +++---
 hw/Makefile.objs         |  1 +
 hw/display/Makefile.objs |  4 +++-
 hw/display/qxl.c         |  2 +-
 hw/i386/Makefile.objs    |  1 +
 include/qemu/module.h    | 28 ++++++++++++++++++++++++++++
 include/ui/qemu-spice.h  | 24 +++++++++++++++---------
 monitor/Makefile.objs    |  3 +++
 monitor/hmp-cmds.c       |  6 ++++++
 scripts/minikconf.py     |  4 ++--
 softmmu/Makefile.objs    |  2 +-
 softmmu/vl.c             |  1 +
 stubs/Makefile.objs      |  2 +-
 ui/Makefile.objs         | 12 ++++++------
 ui/spice-core.c          | 31 +++++++++++++++++++++----------
 ui/spice-display.c       |  2 +-
 util/module.c            | 13 +++++++++++--
 22 files changed, 117 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

In order to facilitate the move of large chunks of functionality to
load modules, it is simpler to create a wrapper with the same name
that simply relays the implementation. For efficiency, this is
typically done using inline functions in the header for the
corresponding functionality. In that case, we rename the actual
implementation by appending _implementation to its name. This makes it
easier to select which function you want to put a breakpoint on.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/qemu/module.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 011ae1ae76..1922a0293c 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
 }
 #endif
 
+#ifdef CONFIG_MODULES
+/* Identify which functions are replaced by a callback stub */
+#ifdef MODULE_STUBS
+#define MODIFACE(Ret,Name,Args)                                         \
+    Ret (*Name)Args;                                                    \
+    extern Ret Name##_implementation Args
+#else /* !MODULE_STUBS */
+#define MODIFACE(Ret,Name,Args)                                         \
+    extern Ret (*Name)Args;                                             \
+    extern Ret Name##_implementation Args
+#endif /* MODULE_STUBS */
+
+#define MODIMPL(Ret,Name,Args)                                          \
+    static void __attribute__((constructor)) Name##_register(void)      \
+    {                                                                   \
+        Name = Name##_implementation;                                   \
+    }                                                                   \
+    Ret Name##_implementation Args
+#else /* !CONFIG_MODULES */
+/* When not using a module, such functions are called directly */
+#define MODIFACE(Ret,Name,Args)         Ret Name Args
+#define MODIMPL(Ret,Name,Args)          Ret Name Args
+#endif /* CONFIG_MODULES */
+
 typedef enum {
     MODULE_INIT_MIGRATION,
     MODULE_INIT_BLOCK,
-- 
2.26.2

[PATCH 02/10] minikconf: Pass variables for modules

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 scripts/minikconf.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/minikconf.py b/scripts/minikconf.py
index bcd91015d3..d60add97f6 100755
--- a/scripts/minikconf.py
+++ b/scripts/minikconf.py
@@ -690,10 +690,10 @@ if __name__ == '__main__':
     parser = KconfigParser(data)
     external_vars = set()
     for arg in argv[3:]:
-        m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([yn]?)$', arg)
+        m = re.match(r'^(CONFIG_[A-Z0-9_]+)=([ymn]?)$', arg)
         if m is not None:
             name, value = m.groups()
-            parser.do_assignment(name, value == 'y')
+            parser.do_assignment(name, value == 'y' or value == 'm')
             external_vars.add(name[7:])
         else:
             fp = open(arg, 'rt', encoding='utf-8')
-- 
2.26.2

[PATCH 03/10] spice: Make spice a module configuration

[Christophe de Dinechin]

This commit changes the spice configuration 'm' by default, and moves
the spice components to obj-m variables. It is sufficient to build
without modules enable, but does not link correctly yet, since no
shims have been created for the missing functions yet.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 Makefile              | 1 +
 Makefile.objs         | 1 +
 chardev/Makefile.objs | 3 ++-
 configure             | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b29b0eeefa..ee674971a5 100644
--- a/Makefile
+++ b/Makefile
@@ -477,6 +477,7 @@ dummy := $(call unnest-vars,, \
                 common-obj-m \
                 trace-obj-y)
 
+
 include $(SRC_PATH)/tests/Makefile.include
 
 all: $(DOCS) $(if $(BUILD_DOCS),sphinxdocs) $(TOOLS) $(HELPERS-y) recurse-all modules $(vhost-user-json-y)
diff --git a/Makefile.objs b/Makefile.objs
index 98383972ee..e38768c8d5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -70,6 +70,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += backends/
 common-obj-y += chardev/
+common-obj-m += chardev/
 
 common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
 qemu-seccomp.o-cflags := $(SECCOMP_CFLAGS)
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index d68e1347f9..fc9910d4f2 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -22,4 +22,5 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
 baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 
-common-obj-$(CONFIG_SPICE) += spice.o
+common-obj-$(CONFIG_SPICE) += spice.mo
+spice.mo-objs := spice.o
diff --git a/configure b/configure
index 130630b98f..2de1715800 100755
--- a/configure
+++ b/configure
@@ -7471,7 +7471,7 @@ if test "$posix_memalign" = "yes" ; then
 fi
 
 if test "$spice" = "yes" ; then
-  echo "CONFIG_SPICE=y" >> $config_host_mak
+  echo "CONFIG_SPICE=m" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
-- 
2.26.2

[PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

If we want to build spice as a separately loadable module, we need to
put all the spice code in one loadable module, because the build
system does not know how to deal with dependencies yet.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 audio/Makefile.objs   | 2 +-
 chardev/Makefile.objs | 3 +--
 ui/Makefile.objs      | 8 ++++----
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index b4a4c11f31..298c895ff5 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -1,5 +1,5 @@
 common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
-common-obj-$(CONFIG_SPICE) += spiceaudio.o
+spice-app.mo-objs += ../audio/spiceaudio.o
 common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
 common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
 common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index fc9910d4f2..955fac0cf9 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
 baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
 
-common-obj-$(CONFIG_SPICE) += spice.mo
-spice.mo-objs := spice.o
+spice-app.mo-objs += ../chardev/spice.o
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 504b196479..1ab515e23d 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
 common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
 common-obj-y += input-barrier.o
 common-obj-$(CONFIG_LINUX) += input-linux.o
-common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
 common-obj-$(CONFIG_COCOA) += cocoa.o
 common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
 common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
@@ -53,10 +52,11 @@ curses.mo-objs := curses.o
 curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
 curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
 
-ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
-common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
+common-obj-$(CONFIG_SPICE) += spice-app.mo
+spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
+ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
+spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-objs := spice-app.o
 spice-app.mo-cflags := $(GIO_CFLAGS)
 spice-app.mo-libs := $(GIO_LIBS)
 
-- 
2.26.2

[PATCH 05/10] build: Avoid build failure when building drivers as modules

[Christophe de Dinechin]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 Makefile.objs    | 1 +
 Makefile.target  | 7 +++++++
 hw/Makefile.objs | 1 +
 3 files changed, 9 insertions(+)

diff --git a/Makefile.objs b/Makefile.objs
index e38768c8d5..6703353493 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -86,6 +86,7 @@ endif # CONFIG_SOFTMMU
 # Target-independent parts used in system and user emulation
 common-obj-y += cpus-common.o
 common-obj-y += hw/
+common-obj-m += hw/
 common-obj-y += qom/
 common-obj-y += disas/
 
diff --git a/Makefile.target b/Makefile.target
index 8ed1eba95b..3f3b5ee058 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
 dummy := $(call unnest-vars,,obj-y)
 all-obj-y := $(obj-y)
 
+#
+# common-obj-m has some crap here, probably as side effect from
+# filling obj-y.  Clear it.  Fixes suspicious dependency errors when
+# building devices as modules.
+#
+common-obj-m :=
+
 include $(SRC_PATH)/Makefile.objs
 dummy := $(call unnest-vars,.., \
                authz-obj-y \
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 4cbe5e4e57..d6d387b74b 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -43,4 +43,5 @@ devices-dirs-y += smbios/
 endif
 
 common-obj-y += $(devices-dirs-y)
+common-obj-m := display/
 obj-y += $(devices-dirs-y)
-- 
2.26.2

[PATCH 06/10] trivial: Remove extra trailing whitespace

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 hw/display/qxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index d5627119ec..28caf878cd 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -51,7 +51,7 @@
 #undef ALIGN
 #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
 
-#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" 
+#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9"
 
 #define QXL_MODE(_x, _y, _b, _o)                  \
     {   .x_res = _x,                              \
-- 
2.26.2

[PATCH 07/10] qxl - FIXME: Build as module

[Christophe de Dinechin]

Forcibly build qxl as a module to see if we can load it

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 hw/display/Makefile.objs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 77a7d622bd..f51411619b 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -44,7 +44,8 @@ common-obj-$(CONFIG_ARTIST) += artist.o
 
 obj-$(CONFIG_VGA) += vga.o
 
-common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
+common-obj-$(CONFIG_QXL:y=m) += qxl.mo
+qxl.mo-objs := qxl.o qxl-logger.o qxl-render.o
 
 obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
 obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
-- 
2.26.2

[PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

Instead of adding the spice build flags to the top-level build
options, add them where they are necessary. This is a step to move the
burden of linking with spice libraries away from the top-level qemu.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 configure                |  4 ++--
 hw/display/Makefile.objs |  1 +
 hw/i386/Makefile.objs    |  1 +
 monitor/Makefile.objs    |  3 +++
 softmmu/Makefile.objs    |  2 +-
 stubs/Makefile.objs      |  2 +-
 ui/Makefile.objs         |  4 ++--
 util/module.c            | 13 +++++++++++--
 8 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/configure b/configure
index 2de1715800..ac83aea242 100755
--- a/configure
+++ b/configure
@@ -5148,8 +5148,6 @@ EOF
      $pkg_config --atleast-version=0.12.3 spice-protocol && \
      compile_prog "$spice_cflags" "$spice_libs" ; then
     spice="yes"
-    libs_softmmu="$libs_softmmu $spice_libs"
-    QEMU_CFLAGS="$QEMU_CFLAGS $spice_cflags"
     spice_protocol_version=$($pkg_config --modversion spice-protocol)
     spice_server_version=$($pkg_config --modversion spice-server)
   else
@@ -7472,6 +7470,8 @@ fi
 
 if test "$spice" = "yes" ; then
   echo "CONFIG_SPICE=m" >> $config_host_mak
+  echo "SPICE_CFLAGS=$spice_cflags" >> $config_host_mak
+  echo "SPICE_LIBS=$spice_libs" >> $config_host_mak
 fi
 
 if test "$smartcard" = "yes" ; then
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index f51411619b..273a956d96 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -46,6 +46,7 @@ obj-$(CONFIG_VGA) += vga.o
 
 common-obj-$(CONFIG_QXL:y=m) += qxl.mo
 qxl.mo-objs := qxl.o qxl-logger.o qxl-render.o
+qxl.mo-cflags += $(SPICE_CFLAGS)
 
 obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
 obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 6abc74551a..bf9856be2a 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -2,6 +2,7 @@ obj-$(CONFIG_KVM) += kvm/
 obj-y += e820_memory_layout.o multiboot.o
 obj-y += x86.o
 obj-$(CONFIG_PC) += pc.o pc_sysfw.o
+pc.o-cflags += $(SPICE_CFLAGS)
 obj-$(CONFIG_I440FX) += pc_piix.o
 obj-$(CONFIG_Q35) += pc_q35.o
 obj-$(CONFIG_MICROVM) += microvm.o
diff --git a/monitor/Makefile.objs b/monitor/Makefile.objs
index a8533c9dd7..fd58d80195 100644
--- a/monitor/Makefile.objs
+++ b/monitor/Makefile.objs
@@ -2,5 +2,8 @@ obj-y += misc.o
 common-obj-y += monitor.o qmp.o hmp.o
 common-obj-y += qmp-cmds.o qmp-cmds-control.o
 common-obj-y += hmp-cmds.o
+qmp-cmds.o-cflags += $(SPICE_CFLAGS)
+hmp-cmds.o-cflags += $(SPICE_CFLAGS)
+misc.o-cflags += $(SPICE_CFLAGS)
 
 storage-daemon-obj-y += monitor.o qmp.o qmp-cmds-control.o
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index dd15c24346..0e7605bd32 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -1,3 +1,3 @@
 softmmu-main-y = softmmu/main.o
 obj-y += vl.o
-vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
+vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f32b9e47a3..1df8bb3814 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,10 +19,10 @@ stub-obj-y += replay.o
 stub-obj-y += runstate-check.o
 stub-obj-$(CONFIG_SOFTMMU) += semihost.o
 stub-obj-y += set-fd-handler.o
-stub-obj-y += vmgenid.o
 stub-obj-y += sysbus.o
 stub-obj-y += tpm.o
 stub-obj-y += trace-control.o
+stub-obj-y += vmgenid.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
 
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 1ab515e23d..6a6fda2f06 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -57,8 +57,8 @@ spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
 ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
 spice-app.mo-objs += spice-app.o
 endif
-spice-app.mo-cflags := $(GIO_CFLAGS)
-spice-app.mo-libs := $(GIO_LIBS)
+spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
+spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)
 
 common-obj-$(CONFIG_OPENGL) += shader.o
 common-obj-$(CONFIG_OPENGL) += console-gl.o
diff --git a/util/module.c b/util/module.c
index 2fa93561fe..29b4806520 100644
--- a/util/module.c
+++ b/util/module.c
@@ -22,11 +22,11 @@
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
-#ifdef CONFIG_TRACE_RECORDER
 #include "trace/recorder.h"
-#endif
 
 
+RECORDER(modules, 16, "QEMU load modules");
+
 typedef struct ModuleEntry
 {
     void (*init)(void);
@@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
 {
     ModuleEntry *e;
 
+#ifdef CONFIG_TRACE_RECORDER
+    static const char *name[] = {
+        "MIGRATION", "BLOCK", "OPTS", "QOM",
+        "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
+        "MAX"
+    };
+#endif
+    record(modules, "Register DSO module init %p type %u %+s",
+           fn, type, name[type]);
     init_lists();
 
     e = g_malloc0(sizeof(*e));
-- 
2.26.2

[PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

Use the MODIFACE and MODIMPL macros to to redirect the highest-level
qemu_spice functions into the spice-app.so load module when SPICE is
compiled as a module.

With these changes, the following shared libraries are no longer
necessary in the top-level qemu binary:

 	libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
 	libopus.so.0 => /lib64/libopus.so.0 (HEX)
 	liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
 	libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
 	libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
 	libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
 	libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
 	libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
 	liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/ui/qemu-spice.h | 24 +++++++++++++++---------
 monitor/hmp-cmds.c      |  6 ++++++
 softmmu/vl.c            |  1 +
 ui/spice-core.c         | 31 +++++++++++++++++++++----------
 ui/spice-display.c      |  2 +-
 5 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index 8c23dfe717..0f7e139da5 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -24,22 +24,28 @@
 
 #include <spice.h>
 #include "qemu/config-file.h"
+#include "qemu/module.h"
 
-extern int using_spice;
+#define using_spice     (qemu_is_using_spice())
 
-void qemu_spice_init(void);
+MODIFACE(bool, qemu_is_using_spice,(void));
+MODIFACE(void, qemu_start_using_spice, (void));
+MODIFACE(void, qemu_spice_init, (void));
 void qemu_spice_input_init(void);
 void qemu_spice_audio_init(void);
-void qemu_spice_display_init(void);
-int qemu_spice_display_add_client(int csock, int skipauth, int tls);
+MODIFACE(void, qemu_spice_display_init, (void));
+MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls));
 int qemu_spice_add_interface(SpiceBaseInstance *sin);
 bool qemu_spice_have_display_interface(QemuConsole *con);
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
-int qemu_spice_set_passwd(const char *passwd,
-                          bool fail_if_connected, bool disconnect_if_connected);
-int qemu_spice_set_pw_expire(time_t expires);
-int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-                            const char *subject);
+MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
+                                      bool fail_if_connected,
+                                      bool disconnect_if_connected));
+MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
+MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
+                                       int port, int tls_port,
+                                       const char *subject));
+MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));
 
 #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
 #define SPICE_NEEDS_SET_MM_TIME 1
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2b0b58a336..6bd9c52658 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -56,6 +56,7 @@
 #include "migration/misc.h"
 
 #ifdef CONFIG_SPICE
+#include "ui/qemu-spice.h"
 #include <spice/enums.h>
 #endif
 
@@ -573,6 +574,11 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
 #endif
 
 #ifdef CONFIG_SPICE
+SpiceInfo *qmp_query_spice(Error **errp)
+{
+    return qemu_spice_query(errp);
+}
+
 void hmp_info_spice(Monitor *mon, const QDict *qdict)
 {
     SpiceChannelList *chan;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3e15ee2435..c94b4fa49b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 
+#define MODULE_STUBS
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/units.h"
diff --git a/ui/spice-core.c b/ui/spice-core.c
index ecc2ec2c55..dbc1886b77 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -48,7 +48,7 @@ static time_t auth_expires = TIME_MAX;
 static int spice_migration_completed;
 static int spice_display_is_running;
 static int spice_have_target_host;
-int using_spice = 0;
+static int is_using_spice = 0;
 
 static QemuThread me;
 
@@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
     },
 };
 
-SpiceInfo *qmp_query_spice(Error **errp)
+MODIMPL(SpiceInfo *,qemu_spice_query,(Error **errp))
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
     int port, tls_port;
@@ -579,8 +579,9 @@ static void migration_state_notifier(Notifier *notifier, void *data)
     }
 }
 
-int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
-                            const char *subject)
+MODIMPL(int, qemu_spice_migrate_info, (const char *hostname,
+                                       int port, int tls_port,
+                                       const char *subject))
 {
     int ret;
 
@@ -634,7 +635,17 @@ static void vm_change_state_handler(void *opaque, int running,
     }
 }
 
-void qemu_spice_init(void)
+MODIMPL(bool, qemu_is_using_spice, (void))
+{
+    return is_using_spice;
+}
+
+MODIMPL(void, qemu_start_using_spice, (void))
+{
+    is_using_spice = 1;
+}
+
+MODIMPL(void, qemu_spice_init, (void))
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
     const char *password, *str, *x509_dir, *addr,
@@ -796,7 +807,7 @@ void qemu_spice_init(void)
         error_report("failed to initialize spice server");
         exit(1);
     };
-    using_spice = 1;
+    qemu_start_using_spice();
 
     migration_state.notify = migration_state_notifier;
     add_migration_state_change_notifier(&migration_state);
@@ -945,8 +956,8 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
                                    fail_if_conn, disconnect_if_conn);
 }
 
-int qemu_spice_set_passwd(const char *passwd,
-                          bool fail_if_conn, bool disconnect_if_conn)
+MODIMPL(int, qemu_spice_set_passwd,(const char *passwd,
+                                    bool fail_if_conn, bool disconnect_if_conn))
 {
     if (strcmp(auth, "spice") != 0) {
         return -1;
@@ -957,13 +968,13 @@ int qemu_spice_set_passwd(const char *passwd,
     return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
 }
 
-int qemu_spice_set_pw_expire(time_t expires)
+MODIMPL(int, qemu_spice_set_pw_expire, (time_t expires))
 {
     auth_expires = expires;
     return qemu_spice_set_ticket(false, false);
 }
 
-int qemu_spice_display_add_client(int csock, int skipauth, int tls)
+MODIMPL(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls))
 {
     if (tls) {
         return spice_server_add_ssl_client(spice_server, csock, skipauth);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 19632fdf6c..90529695fe 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1164,7 +1164,7 @@ static void qemu_spice_display_init_one(QemuConsole *con)
     register_displaychangelistener(&ssd->dcl);
 }
 
-void qemu_spice_display_init(void)
+MODIMPL(void, qemu_spice_display_init,(void))
 {
     QemuOptsList *olist = qemu_find_opts("spice");
     QemuOpts *opts = QTAILQ_FIRST(&olist->head);
-- 
2.26.2

[PATCH 10/10] REMOVE: Instrumentation to show the module functions being replaced

[Christophe de Dinechin]

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/qemu/module.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 1922a0293c..8d6e10ba81 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,10 +14,13 @@
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#include "trace/recorder.h"
 
 #define DSO_STAMP_FUN         glue(qemu_stamp, CONFIG_STAMP)
 #define DSO_STAMP_FUN_STR     stringify(DSO_STAMP_FUN)
 
+RECORDER_DECLARE(modules);
+
 #ifdef BUILD_DSO
 void DSO_STAMP_FUN(void);
 /* This is a dummy symbol to identify a loaded DSO as a QEMU module, so we can
@@ -55,6 +58,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
     static void __attribute__((constructor)) Name##_register(void)      \
     {                                                                   \
         Name = Name##_implementation;                                   \
+        record(modules, "Setting " #Name " to %p", Name);               \
     }                                                                   \
     Ret Name##_implementation Args
 #else /* !CONFIG_MODULES */
-- 
2.26.2

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
> If we want to build spice as a separately loadable module, we need to
> put all the spice code in one loadable module, because the build
> system does not know how to deal with dependencies yet.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  audio/Makefile.objs   | 2 +-
>  chardev/Makefile.objs | 3 +--
>  ui/Makefile.objs      | 8 ++++----
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
> index b4a4c11f31..298c895ff5 100644
> --- a/audio/Makefile.objs
> +++ b/audio/Makefile.objs
> @@ -1,5 +1,5 @@
>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
> +spice-app.mo-objs += ../audio/spiceaudio.o

Explicitly showing paths in the variables doesn't look right. The
make recipes are supposed to automatically expand bare file names
to add the right path. This is usually dealt with by a call to
the "unnest-vars" function.

>  common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
>  common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
>  common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
> index fc9910d4f2..955fac0cf9 100644
> --- a/chardev/Makefile.objs
> +++ b/chardev/Makefile.objs
> @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
>  baum.o-cflags := $(SDL_CFLAGS)
>  baum.o-libs := $(BRLAPI_LIBS)
>  
> -common-obj-$(CONFIG_SPICE) += spice.mo
> -spice.mo-objs := spice.o
> +spice-app.mo-objs += ../chardev/spice.o
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index 504b196479..1ab515e23d 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
>  common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
>  common-obj-y += input-barrier.o
>  common-obj-$(CONFIG_LINUX) += input-linux.o
> -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
>  common-obj-$(CONFIG_COCOA) += cocoa.o
>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>  common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
> @@ -53,10 +52,11 @@ curses.mo-objs := curses.o
>  curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
>  curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
>  
> -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
> -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
> +common-obj-$(CONFIG_SPICE) += spice-app.mo
> +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
> +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
> +spice-app.mo-objs += spice-app.o
>  endif
> -spice-app.mo-objs := spice-app.o
>  spice-app.mo-cflags := $(GIO_CFLAGS)
>  spice-app.mo-libs := $(GIO_LIBS)
>  
> -- 
> 2.26.2
> 
> 

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

Re: [PATCH 05/10] build: Avoid build failure when building drivers as modules

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 06:43:02PM +0200, Christophe de Dinechin wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  Makefile.objs    | 1 +
>  Makefile.target  | 7 +++++++
>  hw/Makefile.objs | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index e38768c8d5..6703353493 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -86,6 +86,7 @@ endif # CONFIG_SOFTMMU
>  # Target-independent parts used in system and user emulation
>  common-obj-y += cpus-common.o
>  common-obj-y += hw/
> +common-obj-m += hw/
>  common-obj-y += qom/
>  common-obj-y += disas/
>  
> diff --git a/Makefile.target b/Makefile.target
> index 8ed1eba95b..3f3b5ee058 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
>  dummy := $(call unnest-vars,,obj-y)
>  all-obj-y := $(obj-y)
>  
> +#
> +# common-obj-m has some crap here, probably as side effect from
> +# filling obj-y.  Clear it.  Fixes suspicious dependency errors when
> +# building devices as modules.
> +#
> +common-obj-m :=

This comment doesn't fill me with confidence - makes it feel like there's
some more important root cause that needs addressing instead.

>  include $(SRC_PATH)/Makefile.objs
>  dummy := $(call unnest-vars,.., \
>                 authz-obj-y \
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 4cbe5e4e57..d6d387b74b 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,4 +43,5 @@ devices-dirs-y += smbios/
>  endif
>  
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m := display/
>  obj-y += $(devices-dirs-y)
> -- 
> 2.26.2
> 
> 

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

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 06:43:05PM +0200, Christophe de Dinechin wrote:
> Instead of adding the spice build flags to the top-level build
> options, add them where they are necessary. This is a step to move the
> burden of linking with spice libraries away from the top-level qemu.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  configure                |  4 ++--
>  hw/display/Makefile.objs |  1 +
>  hw/i386/Makefile.objs    |  1 +
>  monitor/Makefile.objs    |  3 +++
>  softmmu/Makefile.objs    |  2 +-
>  stubs/Makefile.objs      |  2 +-
>  ui/Makefile.objs         |  4 ++--
>  util/module.c            | 13 +++++++++++--
>  8 files changed, 22 insertions(+), 8 deletions(-)

> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f32b9e47a3..1df8bb3814 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>  stub-obj-y += runstate-check.o
>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>  stub-obj-y += set-fd-handler.o
> -stub-obj-y += vmgenid.o
>  stub-obj-y += sysbus.o
>  stub-obj-y += tpm.o
>  stub-obj-y += trace-control.o
> +stub-obj-y += vmgenid.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
>

This looks unrelated to this series.



> diff --git a/util/module.c b/util/module.c
> index 2fa93561fe..29b4806520 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -22,11 +22,11 @@
>  #ifdef CONFIG_MODULE_UPGRADES
>  #include "qemu-version.h"
>  #endif
> -#ifdef CONFIG_TRACE_RECORDER
>  #include "trace/recorder.h"
> -#endif
>  
>  
> +RECORDER(modules, 16, "QEMU load modules");
> +
>  typedef struct ModuleEntry
>  {
>      void (*init)(void);
> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
>  {
>      ModuleEntry *e;
>  
> +#ifdef CONFIG_TRACE_RECORDER
> +    static const char *name[] = {
> +        "MIGRATION", "BLOCK", "OPTS", "QOM",
> +        "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
> +        "MAX"
> +    };
> +#endif
> +    record(modules, "Register DSO module init %p type %u %+s",
> +           fn, type, name[type]);
>      init_lists();

This looks unrelated too, but in general debugging should go via QEMU's
standard trace backends.

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

Re: [PATCH 10/10] REMOVE: Instrumentation to show the module functions being replaced

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 06:43:07PM +0200, Christophe de Dinechin wrote:
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/qemu/module.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 1922a0293c..8d6e10ba81 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -14,10 +14,13 @@
>  #ifndef QEMU_MODULE_H
>  #define QEMU_MODULE_H
>  
> +#include "trace/recorder.h"
>  
>  #define DSO_STAMP_FUN         glue(qemu_stamp, CONFIG_STAMP)
>  #define DSO_STAMP_FUN_STR     stringify(DSO_STAMP_FUN)
>  
> +RECORDER_DECLARE(modules);
> +
>  #ifdef BUILD_DSO
>  void DSO_STAMP_FUN(void);
>  /* This is a dummy symbol to identify a loaded DSO as a QEMU module, so we can
> @@ -55,6 +58,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>      static void __attribute__((constructor)) Name##_register(void)      \
>      {                                                                   \
>          Name = Name##_implementation;                                   \
> +        record(modules, "Setting " #Name " to %p", Name);               \
>      }                                                                   \
>      Ret Name##_implementation Args
>  #else /* !CONFIG_MODULES */

Contrary to the commit $SUBJECT, I think you should keep this, not remove
it. It should use QEMU's trace backend though.

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

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Daniel P. Berrangé]

On Fri, Jun 26, 2020 at 06:43:06PM +0200, Christophe de Dinechin wrote:
> Use the MODIFACE and MODIMPL macros to to redirect the highest-level
> qemu_spice functions into the spice-app.so load module when SPICE is
> compiled as a module.
> 
> With these changes, the following shared libraries are no longer
> necessary in the top-level qemu binary:
> 
>  	libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
>  	libopus.so.0 => /lib64/libopus.so.0 (HEX)
>  	liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
>  	libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
>  	libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
>  	libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
>  	libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
>  	libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
>  	liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/ui/qemu-spice.h | 24 +++++++++++++++---------
>  monitor/hmp-cmds.c      |  6 ++++++
>  softmmu/vl.c            |  1 +
>  ui/spice-core.c         | 31 +++++++++++++++++++++----------
>  ui/spice-display.c      |  2 +-
>  5 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> index 8c23dfe717..0f7e139da5 100644
> --- a/include/ui/qemu-spice.h
> +++ b/include/ui/qemu-spice.h
> @@ -24,22 +24,28 @@
>  
>  #include <spice.h>
>  #include "qemu/config-file.h"
> +#include "qemu/module.h"
>  
> -extern int using_spice;
> +#define using_spice     (qemu_is_using_spice())
>  
> -void qemu_spice_init(void);
> +MODIFACE(bool, qemu_is_using_spice,(void));
> +MODIFACE(void, qemu_start_using_spice, (void));
> +MODIFACE(void, qemu_spice_init, (void));
>  void qemu_spice_input_init(void);
>  void qemu_spice_audio_init(void);
> -void qemu_spice_display_init(void);
> -int qemu_spice_display_add_client(int csock, int skipauth, int tls);
> +MODIFACE(void, qemu_spice_display_init, (void));
> +MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls));
>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>  bool qemu_spice_have_display_interface(QemuConsole *con);
>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
> -int qemu_spice_set_passwd(const char *passwd,
> -                          bool fail_if_connected, bool disconnect_if_connected);
> -int qemu_spice_set_pw_expire(time_t expires);
> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> -                            const char *subject);
> +MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
> +                                      bool fail_if_connected,
> +                                      bool disconnect_if_connected));
> +MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
> +MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
> +                                       int port, int tls_port,
> +                                       const char *subject));
> +MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));

This macro usage looks kind of unpleasant and its hard to understand
just what is going on, especially why some methods are changed but
others are not.

IIUC, the goal is to turn all these into weak symbols so they don't
need to be resolved immediately at startup, and instead have an
impl of them provided dynamically at runtime.

If so the more normal approach would be to have a struct defining
a set of callbacks, that can be registered. Or if there's a natural
fit with QOM, then a QOM interface that can then have a QOM object
impl registered as a singleton.

>  #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
>  #define SPICE_NEEDS_SET_MM_TIME 1
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 2b0b58a336..6bd9c52658 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -56,6 +56,7 @@
>  #include "migration/misc.h"
>  
>  #ifdef CONFIG_SPICE
> +#include "ui/qemu-spice.h"
>  #include <spice/enums.h>
>  #endif
>  
> @@ -573,6 +574,11 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>  #endif
>  
>  #ifdef CONFIG_SPICE
> +SpiceInfo *qmp_query_spice(Error **errp)
> +{
> +    return qemu_spice_query(errp);
> +}
> +
>  void hmp_info_spice(Monitor *mon, const QDict *qdict)
>  {
>      SpiceChannelList *chan;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 3e15ee2435..c94b4fa49b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  
> +#define MODULE_STUBS
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/units.h"
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index ecc2ec2c55..dbc1886b77 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -48,7 +48,7 @@ static time_t auth_expires = TIME_MAX;
>  static int spice_migration_completed;
>  static int spice_display_is_running;
>  static int spice_have_target_host;
> -int using_spice = 0;
> +static int is_using_spice = 0;
>  
>  static QemuThread me;
>  
> @@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
>      },
>  };
>  
> -SpiceInfo *qmp_query_spice(Error **errp)
> +MODIMPL(SpiceInfo *,qemu_spice_query,(Error **errp))
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>      int port, tls_port;
> @@ -579,8 +579,9 @@ static void migration_state_notifier(Notifier *notifier, void *data)
>      }
>  }
>  
> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> -                            const char *subject)
> +MODIMPL(int, qemu_spice_migrate_info, (const char *hostname,
> +                                       int port, int tls_port,
> +                                       const char *subject))
>  {
>      int ret;
>  
> @@ -634,7 +635,17 @@ static void vm_change_state_handler(void *opaque, int running,
>      }
>  }
>  
> -void qemu_spice_init(void)
> +MODIMPL(bool, qemu_is_using_spice, (void))
> +{
> +    return is_using_spice;
> +}
> +
> +MODIMPL(void, qemu_start_using_spice, (void))
> +{
> +    is_using_spice = 1;
> +}
> +
> +MODIMPL(void, qemu_spice_init, (void))
>  {
>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>      const char *password, *str, *x509_dir, *addr,
> @@ -796,7 +807,7 @@ void qemu_spice_init(void)
>          error_report("failed to initialize spice server");
>          exit(1);
>      };
> -    using_spice = 1;
> +    qemu_start_using_spice();
>  
>      migration_state.notify = migration_state_notifier;
>      add_migration_state_change_notifier(&migration_state);
> @@ -945,8 +956,8 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
>                                     fail_if_conn, disconnect_if_conn);
>  }
>  
> -int qemu_spice_set_passwd(const char *passwd,
> -                          bool fail_if_conn, bool disconnect_if_conn)
> +MODIMPL(int, qemu_spice_set_passwd,(const char *passwd,
> +                                    bool fail_if_conn, bool disconnect_if_conn))
>  {
>      if (strcmp(auth, "spice") != 0) {
>          return -1;
> @@ -957,13 +968,13 @@ int qemu_spice_set_passwd(const char *passwd,
>      return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
>  }
>  
> -int qemu_spice_set_pw_expire(time_t expires)
> +MODIMPL(int, qemu_spice_set_pw_expire, (time_t expires))
>  {
>      auth_expires = expires;
>      return qemu_spice_set_ticket(false, false);
>  }
>  
> -int qemu_spice_display_add_client(int csock, int skipauth, int tls)
> +MODIMPL(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls))
>  {
>      if (tls) {
>          return spice_server_add_ssl_client(spice_server, csock, skipauth);
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 19632fdf6c..90529695fe 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -1164,7 +1164,7 @@ static void qemu_spice_display_init_one(QemuConsole *con)
>      register_displaychangelistener(&ssd->dcl);
>  }
>  
> -void qemu_spice_display_init(void)
> +MODIMPL(void, qemu_spice_display_init,(void))
>  {
>      QemuOptsList *olist = qemu_find_opts("spice");
>      QemuOpts *opts = QTAILQ_FIRST(&olist->head);
> -- 
> 2.26.2
> 
> 

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

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
>> If we want to build spice as a separately loadable module, we need to
>> put all the spice code in one loadable module, because the build
>> system does not know how to deal with dependencies yet.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  audio/Makefile.objs   | 2 +-
>>  chardev/Makefile.objs | 3 +--
>>  ui/Makefile.objs      | 8 ++++----
>>  3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
>> index b4a4c11f31..298c895ff5 100644
>> --- a/audio/Makefile.objs
>> +++ b/audio/Makefile.objs
>> @@ -1,5 +1,5 @@
>>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
>> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
>> +spice-app.mo-objs += ../audio/spiceaudio.o
>
> Explicitly showing paths in the variables doesn't look right. The
> make recipes are supposed to automatically expand bare file names
> to add the right path. This is usually dealt with by a call to
> the "unnest-vars" function.

I agree. I mentioned this in the cover letter:

> - Adding various non-UI files into spice-app.so, which required a
>   couple of ../pwd/foo.o references in the makefile. Not very nice,
>   but I did not want to spend too much time fixing the makefiles.

I did not find an elegant way to integrate a non-UI file into a .mo that is
declared in the ui section.

I considered various solutions:

a) Having separate load modules for different source directories.
   This exposes details of the build system into the generated libs.

b) Moving the source
   This breaks the logical organization of the sources

c) Manually managing this specific .o one level up
   This seems even harder to read / understand

So I thought I would dedicate a bit more time to add that capability to the
unnest-vars function. I did not want to defer review for that, and vaguely
hoped that there was an existing correct way to do it that someone more
experienced in the build system could point me to.

>
>>  common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
>>  common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
>>  common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
>> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
>> index fc9910d4f2..955fac0cf9 100644
>> --- a/chardev/Makefile.objs
>> +++ b/chardev/Makefile.objs
>> @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
>>  baum.o-cflags := $(SDL_CFLAGS)
>>  baum.o-libs := $(BRLAPI_LIBS)
>>
>> -common-obj-$(CONFIG_SPICE) += spice.mo
>> -spice.mo-objs := spice.o
>> +spice-app.mo-objs += ../chardev/spice.o
>> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
>> index 504b196479..1ab515e23d 100644
>> --- a/ui/Makefile.objs
>> +++ b/ui/Makefile.objs
>> @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
>>  common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
>>  common-obj-y += input-barrier.o
>>  common-obj-$(CONFIG_LINUX) += input-linux.o
>> -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
>>  common-obj-$(CONFIG_COCOA) += cocoa.o
>>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>>  common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
>> @@ -53,10 +52,11 @@ curses.mo-objs := curses.o
>>  curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
>>  curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
>>
>> -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
>> -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
>> +common-obj-$(CONFIG_SPICE) += spice-app.mo
>> +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
>> +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
>> +spice-app.mo-objs += spice-app.o
>>  endif
>> -spice-app.mo-objs := spice-app.o
>>  spice-app.mo-cflags := $(GIO_CFLAGS)
>>  spice-app.mo-libs := $(GIO_LIBS)
>>
>> --
>> 2.26.2
>>
>>
>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

On 2020-06-26 at 19:26 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:05PM +0200, Christophe de Dinechin wrote:
>> Instead of adding the spice build flags to the top-level build
>> options, add them where they are necessary. This is a step to move the
>> burden of linking with spice libraries away from the top-level qemu.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  configure                |  4 ++--
>>  hw/display/Makefile.objs |  1 +
>>  hw/i386/Makefile.objs    |  1 +
>>  monitor/Makefile.objs    |  3 +++
>>  softmmu/Makefile.objs    |  2 +-
>>  stubs/Makefile.objs      |  2 +-
>>  ui/Makefile.objs         |  4 ++--
>>  util/module.c            | 13 +++++++++++--
>>  8 files changed, 22 insertions(+), 8 deletions(-)
>
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index f32b9e47a3..1df8bb3814 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>>  stub-obj-y += runstate-check.o
>>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>>  stub-obj-y += set-fd-handler.o
>> -stub-obj-y += vmgenid.o
>>  stub-obj-y += sysbus.o
>>  stub-obj-y += tpm.o
>>  stub-obj-y += trace-control.o
>> +stub-obj-y += vmgenid.o
>>  stub-obj-y += vmstate.o
>>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
>>
>
> This looks unrelated to this series.

I'll send a separate trivial patch to fix the alphabetical ordering.
I used to have a spice.c stub here, which conflicted every time. This is how
I noticed the alphabetical order was not respected here.

>
>
>
>> diff --git a/util/module.c b/util/module.c
>> index 2fa93561fe..29b4806520 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,11 +22,11 @@
>>  #ifdef CONFIG_MODULE_UPGRADES
>>  #include "qemu-version.h"
>>  #endif
>> -#ifdef CONFIG_TRACE_RECORDER
>>  #include "trace/recorder.h"
>> -#endif
>>
>>
>> +RECORDER(modules, 16, "QEMU load modules");
>> +
>>  typedef struct ModuleEntry
>>  {
>>      void (*init)(void);
>> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
>>  {
>>      ModuleEntry *e;
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +    static const char *name[] = {
>> +        "MIGRATION", "BLOCK", "OPTS", "QOM",
>> +        "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
>> +        "MAX"
>> +    };
>> +#endif
>> +    record(modules, "Register DSO module init %p type %u %+s",
>> +           fn, type, name[type]);
>>      init_lists();
>
> This looks unrelated too, but in general debugging should go via QEMU's
> standard trace backends.
>

Yes. I apparently botched a fixup. That was supposed to be a private patch
for my own use.



--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Daniel P. Berrangé]

On Mon, Jun 29, 2020 at 11:22:40AM +0200, Christophe de Dinechin wrote:
> 
> On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote...
> > On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
> >> If we want to build spice as a separately loadable module, we need to
> >> put all the spice code in one loadable module, because the build
> >> system does not know how to deal with dependencies yet.
> >>
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> >> ---
> >>  audio/Makefile.objs   | 2 +-
> >>  chardev/Makefile.objs | 3 +--
> >>  ui/Makefile.objs      | 8 ++++----
> >>  3 files changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
> >> index b4a4c11f31..298c895ff5 100644
> >> --- a/audio/Makefile.objs
> >> +++ b/audio/Makefile.objs
> >> @@ -1,5 +1,5 @@
> >>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
> >> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
> >> +spice-app.mo-objs += ../audio/spiceaudio.o
> >
> > Explicitly showing paths in the variables doesn't look right. The
> > make recipes are supposed to automatically expand bare file names
> > to add the right path. This is usually dealt with by a call to
> > the "unnest-vars" function.
> 
> I agree. I mentioned this in the cover letter:
> 
> > - Adding various non-UI files into spice-app.so, which required a
> >   couple of ../pwd/foo.o references in the makefile. Not very nice,
> >   but I did not want to spend too much time fixing the makefiles.
> 
> I did not find an elegant way to integrate a non-UI file into a .mo that is
> declared in the ui section.
> 
> I considered various solutions:
> 
> a) Having separate load modules for different source directories.
>    This exposes details of the build system into the generated libs.
> 
> b) Moving the source
>    This breaks the logical organization of the sources
> 
> c) Manually managing this specific .o one level up
>    This seems even harder to read / understand

I don't think any of these three should be required. The problem isn't
the structure of the makefiles or layout of the source, rather it is
a matter of expanding paths correctly in the build rules.

> So I thought I would dedicate a bit more time to add that capability to the
> unnest-vars function. I did not want to defer review for that, and vaguely
> hoped that there was an existing correct way to do it that someone more
> experienced in the build system could point me to.

With QEMU's build system, regardless of where the rules are defined,
they should get run in the context of the top level makefile, as all
the sub-dir Makefiles are just includes. The unnest-vars function
takes relative filenames and adds the correct directory prefix to
them.

IIUC, common-obj-m gets  spice-app.mo added. common-obj-m is processed
by unnest-vars, but spice-app.mo-objs is not, so all its .o files are
relative. So we just need to fix the way the *.mo-objs variables are
processed, such that unnest-vars is used to add the directory prefixes.


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

Re: [PATCH 06/10] trivial: Remove extra trailing whitespace

[Philippe Mathieu-Daudé]

On 6/26/20 6:43 PM, Christophe de Dinechin wrote:
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index d5627119ec..28caf878cd 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -51,7 +51,7 @@
>  #undef ALIGN
>  #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
>  
> -#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" 
> +#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9"

From CODING_STYLE.rst:

  Comment style
  =============

  We use traditional C-style /``*`` ``*``/ comments and avoid // comments.

Let's switch to the C-style format :)

>  
>  #define QXL_MODE(_x, _y, _b, _o)                  \
>      {   .x_res = _x,                              \
> 

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Claudio Fontana]

Hello Christophe,

On 6/26/20 6:42 PM, Christophe de Dinechin wrote:
> In order to facilitate the move of large chunks of functionality to
> load modules, it is simpler to create a wrapper with the same name
> that simply relays the implementation. For efficiency, this is
> typically done using inline functions in the header for the
> corresponding functionality. In that case, we rename the actual
> implementation by appending _implementation to its name. This makes it
> easier to select which function you want to put a breakpoint on.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/qemu/module.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae76..1922a0293c 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>  }
>  #endif
>  
> +#ifdef CONFIG_MODULES
> +/* Identify which functions are replaced by a callback stub */
> +#ifdef MODULE_STUBS
> +#define MODIFACE(Ret,Name,Args)                                         \
> +    Ret (*Name)Args;                                                    \
> +    extern Ret Name##_implementation Args
> +#else /* !MODULE_STUBS */
> +#define MODIFACE(Ret,Name,Args)                                         \
> +    extern Ret (*Name)Args;                                             \
> +    extern Ret Name##_implementation Args
> +#endif /* MODULE_STUBS */
> +
> +#define MODIMPL(Ret,Name,Args)                                          \
> +    static void __attribute__((constructor)) Name##_register(void)      \
> +    {                                                                   \
> +        Name = Name##_implementation;                                   \
> +    }                                                                   \
> +    Ret Name##_implementation Args
> +#else /* !CONFIG_MODULES */
> +/* When not using a module, such functions are called directly */
> +#define MODIFACE(Ret,Name,Args)         Ret Name Args
> +#define MODIMPL(Ret,Name,Args)          Ret Name Args
> +#endif /* CONFIG_MODULES */
> +
>  typedef enum {
>      MODULE_INIT_MIGRATION,
>      MODULE_INIT_BLOCK,
> 

Just as background, I am interested in all modules-related work, because of my long term plan to have target-specific modules as well:

 https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

I am not 100% clear on what is the goal and expected usage of this preprocessor code, despite the commit message,
maybe you could clarify a bit with more verbosity?

Additionally if you happen to be interested, maybe you know already or could think about what this could mean for target-specific modules,
which will require some improvements to the modules "subsystem"(?) as well.

In my experimentation I didn't have to do this preprocessor work, instead I had to fine tune a bit the makefile support (rules.mak and makefiles)
to be able to accomodate for (even large) modules in target/ as well.

Thanks!

CLaudio

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

On 2020-06-26 at 19:35 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:06PM +0200, Christophe de Dinechin wrote:
>> Use the MODIFACE and MODIMPL macros to to redirect the highest-level
>> qemu_spice functions into the spice-app.so load module when SPICE is
>> compiled as a module.
>>
>> With these changes, the following shared libraries are no longer
>> necessary in the top-level qemu binary:
>>
>>  	libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
>>  	libopus.so.0 => /lib64/libopus.so.0 (HEX)
>>  	liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
>>  	libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
>>  	libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
>>  	libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
>>  	libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
>>  	libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
>>  	liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  include/ui/qemu-spice.h | 24 +++++++++++++++---------
>>  monitor/hmp-cmds.c      |  6 ++++++
>>  softmmu/vl.c            |  1 +
>>  ui/spice-core.c         | 31 +++++++++++++++++++++----------
>>  ui/spice-display.c      |  2 +-
>>  5 files changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
>> index 8c23dfe717..0f7e139da5 100644
>> --- a/include/ui/qemu-spice.h
>> +++ b/include/ui/qemu-spice.h
>> @@ -24,22 +24,28 @@
>>
>>  #include <spice.h>
>>  #include "qemu/config-file.h"
>> +#include "qemu/module.h"
>>
>> -extern int using_spice;
>> +#define using_spice     (qemu_is_using_spice())
>>
>> -void qemu_spice_init(void);
>> +MODIFACE(bool, qemu_is_using_spice,(void));
>> +MODIFACE(void, qemu_start_using_spice, (void));
>> +MODIFACE(void, qemu_spice_init, (void));
>>  void qemu_spice_input_init(void);
>>  void qemu_spice_audio_init(void);
>> -void qemu_spice_display_init(void);
>> -int qemu_spice_display_add_client(int csock, int skipauth, int tls);
>> +MODIFACE(void, qemu_spice_display_init, (void));
>> +MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls));
>>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>>  bool qemu_spice_have_display_interface(QemuConsole *con);
>>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
>> -int qemu_spice_set_passwd(const char *passwd,
>> -                          bool fail_if_connected, bool disconnect_if_connected);
>> -int qemu_spice_set_pw_expire(time_t expires);
>> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
>> -                            const char *subject);
>> +MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
>> +                                      bool fail_if_connected,
>> +                                      bool disconnect_if_connected));
>> +MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
>> +MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
>> +                                       int port, int tls_port,
>> +                                       const char *subject));
>> +MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));
>
> This macro usage looks kind of unpleasant and its hard to understand
> just what is going on, especially why some methods are changed but
> others are not.

The functions that are changed are the module interface between qemu and the
DSO. For example, qemu_spice_init is called from vl.c, i.e. the main binary,
but qemu_spice_audio_init is called from ui/spice-core.c and
ui/spice-input.c, which are both in the DSO after the commit.

The existing function-based interface in qemu-spice.h was not really
carefully designed for modularization, so this list was determined by
following the initialization path. It may not be the smallest possible cut.
It may be neat to add a patch to reorder functions based on whether they are
inside the DSO or exported from it.

As for the macro syntax, I see it as somewhat transient. I wanted to propose
a working and scalable mechanism before adding some nice syntactic sugar
tooling to it. I expect the syntax to turn into something like:

MODIFACE void  qemu_spice_display_init (void);
MODIIMPL void  qemu_spice_display_init (void) { ... }

But it feels a bit too early to do that. I prefer to experiment with a
simple macro for now.

>
> IIUC, the goal is to turn all these into weak symbols so they don't
> need to be resolved immediately at startup, and instead have an
> impl of them provided dynamically at runtime.

My very first approach was indeed to use weak symbols, but then I learned
that ld.so no longer deals with weak symbols, apparently for security
reasons. See LD_DYNAMIC_WEAK in ld.so(8).

>
> If so the more normal approach would be to have a struct defining
> a set of callbacks, that can be registered. Or if there's a natural
> fit with QOM, then a QOM interface that can then have a QOM object
> impl registered as a singleton.

That was my second attempt (after the weak symbols). I cleaned it up a bit
and put it here: https://github.com/c3d/qemu/commits/spice-vtable.

What made me switch to the approach in this series is the following
considerations:

- A vtable is useful if there can be multiple values for a method, e.g. to
  implement inheritance, or if you have multiple instances. This is not the
  case here.

- A vtable adds one level of indirection, which does not seem to be
  particularly useful or helpful for this particular use case.

- Overloading QOM for that purpose looked more confusing than anything else.
  It looked like I was mixing unrelated concepts. Maybe that's just me.

- The required change with a vtable ends up being more extensive. Instead of
  changing a single line to put an entry point in a DSO, you need to create
  the vtable, add functions to it, add a register function, etc. I was
  looking for an easier and more scalable way.

- In particular, with a vtable, you cannot take advantage of the syntactic
  trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
  So for a vtable, you need to manually write wrappers.

This could be automated, of course, but so far I did not find any clear
benefit, and many drawbacks to using the vtable approach. As a quantitative
comparison point, the commit that does this same connection using the vtable
approach is:

 include/ui/qemu-spice.h | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 monitor/hmp-cmds.c      |   6 +++++
 softmmu/vl.c            |  10 +++++++
 ui/spice-core.c         |  38 ++++++++++++++++++++++++---
 4 files changed, 148 insertions(+), 10 deletions(-)

as opposed to what is presented in this series:

 include/ui/qemu-spice.h | 24 +++++++++++++++---------
 monitor/hmp-cmds.c      |  6 ++++++
 softmmu/vl.c            |  1 +
 ui/spice-core.c         | 31 +++++++++++++++++++++----------
 ui/spice-display.c      |  2 +-
 5 files changed, 44 insertions(+), 20 deletions(-)


>
>>  #if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
>>  #define SPICE_NEEDS_SET_MM_TIME 1
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 2b0b58a336..6bd9c52658 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -56,6 +56,7 @@
>>  #include "migration/misc.h"
>>
>>  #ifdef CONFIG_SPICE
>> +#include "ui/qemu-spice.h"
>>  #include <spice/enums.h>
>>  #endif
>>
>> @@ -573,6 +574,11 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>>  #endif
>>
>>  #ifdef CONFIG_SPICE
>> +SpiceInfo *qmp_query_spice(Error **errp)
>> +{
>> +    return qemu_spice_query(errp);
>> +}
>> +
>>  void hmp_info_spice(Monitor *mon, const QDict *qdict)
>>  {
>>      SpiceChannelList *chan;
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 3e15ee2435..c94b4fa49b 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -22,6 +22,7 @@
>>   * THE SOFTWARE.
>>   */
>>
>> +#define MODULE_STUBS
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qemu/units.h"
>> diff --git a/ui/spice-core.c b/ui/spice-core.c
>> index ecc2ec2c55..dbc1886b77 100644
>> --- a/ui/spice-core.c
>> +++ b/ui/spice-core.c
>> @@ -48,7 +48,7 @@ static time_t auth_expires = TIME_MAX;
>>  static int spice_migration_completed;
>>  static int spice_display_is_running;
>>  static int spice_have_target_host;
>> -int using_spice = 0;
>> +static int is_using_spice = 0;
>>
>>  static QemuThread me;
>>
>> @@ -503,7 +503,7 @@ static QemuOptsList qemu_spice_opts = {
>>      },
>>  };
>>
>> -SpiceInfo *qmp_query_spice(Error **errp)
>> +MODIMPL(SpiceInfo *,qemu_spice_query,(Error **errp))
>>  {
>>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>>      int port, tls_port;
>> @@ -579,8 +579,9 @@ static void migration_state_notifier(Notifier *notifier, void *data)
>>      }
>>  }
>>
>> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
>> -                            const char *subject)
>> +MODIMPL(int, qemu_spice_migrate_info, (const char *hostname,
>> +                                       int port, int tls_port,
>> +                                       const char *subject))
>>  {
>>      int ret;
>>
>> @@ -634,7 +635,17 @@ static void vm_change_state_handler(void *opaque, int running,
>>      }
>>  }
>>
>> -void qemu_spice_init(void)
>> +MODIMPL(bool, qemu_is_using_spice, (void))
>> +{
>> +    return is_using_spice;
>> +}
>> +
>> +MODIMPL(void, qemu_start_using_spice, (void))
>> +{
>> +    is_using_spice = 1;
>> +}
>> +
>> +MODIMPL(void, qemu_spice_init, (void))
>>  {
>>      QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
>>      const char *password, *str, *x509_dir, *addr,
>> @@ -796,7 +807,7 @@ void qemu_spice_init(void)
>>          error_report("failed to initialize spice server");
>>          exit(1);
>>      };
>> -    using_spice = 1;
>> +    qemu_start_using_spice();
>>
>>      migration_state.notify = migration_state_notifier;
>>      add_migration_state_change_notifier(&migration_state);
>> @@ -945,8 +956,8 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
>>                                     fail_if_conn, disconnect_if_conn);
>>  }
>>
>> -int qemu_spice_set_passwd(const char *passwd,
>> -                          bool fail_if_conn, bool disconnect_if_conn)
>> +MODIMPL(int, qemu_spice_set_passwd,(const char *passwd,
>> +                                    bool fail_if_conn, bool disconnect_if_conn))
>>  {
>>      if (strcmp(auth, "spice") != 0) {
>>          return -1;
>> @@ -957,13 +968,13 @@ int qemu_spice_set_passwd(const char *passwd,
>>      return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
>>  }
>>
>> -int qemu_spice_set_pw_expire(time_t expires)
>> +MODIMPL(int, qemu_spice_set_pw_expire, (time_t expires))
>>  {
>>      auth_expires = expires;
>>      return qemu_spice_set_ticket(false, false);
>>  }
>>
>> -int qemu_spice_display_add_client(int csock, int skipauth, int tls)
>> +MODIMPL(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls))
>>  {
>>      if (tls) {
>>          return spice_server_add_ssl_client(spice_server, csock, skipauth);
>> diff --git a/ui/spice-display.c b/ui/spice-display.c
>> index 19632fdf6c..90529695fe 100644
>> --- a/ui/spice-display.c
>> +++ b/ui/spice-display.c
>> @@ -1164,7 +1164,7 @@ static void qemu_spice_display_init_one(QemuConsole *con)
>>      register_displaychangelistener(&ssd->dcl);
>>  }
>>
>> -void qemu_spice_display_init(void)
>> +MODIMPL(void, qemu_spice_display_init,(void))
>>  {
>>      QemuOptsList *olist = qemu_find_opts("spice");
>>      QemuOpts *opts = QTAILQ_FIRST(&olist->head);
>> --
>> 2.26.2
>>
>>
>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Daniel P. Berrangé]

On Mon, Jun 29, 2020 at 07:19:41PM +0200, Christophe de Dinechin wrote:
> 
> On 2020-06-26 at 19:35 CEST, Daniel P. Berrangé wrote...
> > On Fri, Jun 26, 2020 at 06:43:06PM +0200, Christophe de Dinechin wrote:
> >> Use the MODIFACE and MODIMPL macros to to redirect the highest-level
> >> qemu_spice functions into the spice-app.so load module when SPICE is
> >> compiled as a module.
> >>
> >> With these changes, the following shared libraries are no longer
> >> necessary in the top-level qemu binary:
> >>
> >>  	libspice-server.so.1 => /lib64/libspice-server.so.1 (HEX)
> >>  	libopus.so.0 => /lib64/libopus.so.0 (HEX)
> >>  	liblz4.so.1 => /lib64/liblz4.so.1 (HEX)
> >>  	libgstapp-1.0.so.0 => /lib64/libgstapp-1.0.so.0 (HEX)
> >>  	libgstvideo-1.0.so.0 => /lib64/libgstvideo-1.0.so.0 (HEX)
> >>  	libgstbase-1.0.so.0 => /lib64/libgstbase-1.0.so.0 (HEX)
> >>  	libgstreamer-1.0.so.0 => /lib64/libgstreamer-1.0.so.0 (HEX)
> >>  	libssl.so.1.1 => /lib64/libssl.so.1.1 (HEX)
> >>  	liborc-0.4.so.0 => /lib64/liborc-0.4.so.0 (HEX)
> >>
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> >> ---
> >>  include/ui/qemu-spice.h | 24 +++++++++++++++---------
> >>  monitor/hmp-cmds.c      |  6 ++++++
> >>  softmmu/vl.c            |  1 +
> >>  ui/spice-core.c         | 31 +++++++++++++++++++++----------
> >>  ui/spice-display.c      |  2 +-
> >>  5 files changed, 44 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
> >> index 8c23dfe717..0f7e139da5 100644
> >> --- a/include/ui/qemu-spice.h
> >> +++ b/include/ui/qemu-spice.h
> >> @@ -24,22 +24,28 @@
> >>
> >>  #include <spice.h>
> >>  #include "qemu/config-file.h"
> >> +#include "qemu/module.h"
> >>
> >> -extern int using_spice;
> >> +#define using_spice     (qemu_is_using_spice())
> >>
> >> -void qemu_spice_init(void);
> >> +MODIFACE(bool, qemu_is_using_spice,(void));
> >> +MODIFACE(void, qemu_start_using_spice, (void));
> >> +MODIFACE(void, qemu_spice_init, (void));
> >>  void qemu_spice_input_init(void);
> >>  void qemu_spice_audio_init(void);
> >> -void qemu_spice_display_init(void);
> >> -int qemu_spice_display_add_client(int csock, int skipauth, int tls);
> >> +MODIFACE(void, qemu_spice_display_init, (void));
> >> +MODIFACE(int, qemu_spice_display_add_client, (int csock, int skipauth, int tls));
> >>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
> >>  bool qemu_spice_have_display_interface(QemuConsole *con);
> >>  int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con);
> >> -int qemu_spice_set_passwd(const char *passwd,
> >> -                          bool fail_if_connected, bool disconnect_if_connected);
> >> -int qemu_spice_set_pw_expire(time_t expires);
> >> -int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> >> -                            const char *subject);
> >> +MODIFACE(int, qemu_spice_set_passwd, (const char *passwd,
> >> +                                      bool fail_if_connected,
> >> +                                      bool disconnect_if_connected));
> >> +MODIFACE(int, qemu_spice_set_pw_expire,(time_t expires));
> >> +MODIFACE(int, qemu_spice_migrate_info,(const char *hostname,
> >> +                                       int port, int tls_port,
> >> +                                       const char *subject));
> >> +MODIFACE(struct SpiceInfo *,qemu_spice_query, (Error **errp));
> >
> > This macro usage looks kind of unpleasant and its hard to understand
> > just what is going on, especially why some methods are changed but
> > others are not.
> 
> The functions that are changed are the module interface between qemu and the
> DSO. For example, qemu_spice_init is called from vl.c, i.e. the main binary,
> but qemu_spice_audio_init is called from ui/spice-core.c and
> ui/spice-input.c, which are both in the DSO after the commit.
> 
> The existing function-based interface in qemu-spice.h was not really
> carefully designed for modularization, so this list was determined by
> following the initialization path. It may not be the smallest possible cut.
> It may be neat to add a patch to reorder functions based on whether they are
> inside the DSO or exported from it.
> 
> As for the macro syntax, I see it as somewhat transient. I wanted to propose
> a working and scalable mechanism before adding some nice syntactic sugar
> tooling to it. I expect the syntax to turn into something like:
> 
> MODIFACE void  qemu_spice_display_init (void);
> MODIIMPL void  qemu_spice_display_init (void) { ... }
> 
> But it feels a bit too early to do that. I prefer to experiment with a
> simple macro for now.
> 
> >
> > IIUC, the goal is to turn all these into weak symbols so they don't
> > need to be resolved immediately at startup, and instead have an
> > impl of them provided dynamically at runtime.
> 
> My very first approach was indeed to use weak symbols, but then I learned
> that ld.so no longer deals with weak symbols, apparently for security
> reasons. See LD_DYNAMIC_WEAK in ld.so(8).
> 
> >
> > If so the more normal approach would be to have a struct defining
> > a set of callbacks, that can be registered. Or if there's a natural
> > fit with QOM, then a QOM interface that can then have a QOM object
> > impl registered as a singleton.
> 
> That was my second attempt (after the weak symbols). I cleaned it up a bit
> and put it here: https://github.com/c3d/qemu/commits/spice-vtable.
> 
> What made me switch to the approach in this series is the following
> considerations:
> 
> - A vtable is useful if there can be multiple values for a method, e.g. to
>   implement inheritance, or if you have multiple instances. This is not the
>   case here.

IMHO a vtable is fine for singleton objects. It is a generic way to
remove tight coupling between caller(s) and an implementation. Just
having 1 implementation doesn't invalidate the model.

> - A vtable adds one level of indirection, which does not seem to be
>   particularly useful or helpful for this particular use case.
> 
> - Overloading QOM for that purpose looked more confusing than anything else.
>   It looked like I was mixing unrelated concepts. Maybe that's just me.
> 
> - The required change with a vtable ends up being more extensive. Instead of
>   changing a single line to put an entry point in a DSO, you need to create
>   the vtable, add functions to it, add a register function, etc. I was
>   looking for an easier and more scalable way.
> 
> - In particular, with a vtable, you cannot take advantage of the syntactic
>   trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
>   So for a vtable, you need to manually write wrappers.
> 
> This could be automated, of course, but so far I did not find any clear
> benefit, and many drawbacks to using the vtable approach. As a quantitative
> comparison point, the commit that does this same connection using the vtable
> approach is:
> 
>  include/ui/qemu-spice.h | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  monitor/hmp-cmds.c      |   6 +++++
>  softmmu/vl.c            |  10 +++++++
>  ui/spice-core.c         |  38 ++++++++++++++++++++++++---
>  4 files changed, 148 insertions(+), 10 deletions(-)
> 
> as opposed to what is presented in this series:
> 
>  include/ui/qemu-spice.h | 24 +++++++++++++++---------
>  monitor/hmp-cmds.c      |  6 ++++++
>  softmmu/vl.c            |  1 +
>  ui/spice-core.c         | 31 +++++++++++++++++++++----------
>  ui/spice-display.c      |  2 +-
>  5 files changed, 44 insertions(+), 20 deletions(-)

I much prefer the code in the vtable patch version. The larger number of
lines of code doesn't bother me, because the code is really simple and
clear to read. IOW I prefer the clarity of the longer code, over the
shorter code with magic hidden behind it.

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

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Gerd Hoffmann]

  Hi,

> > If so the more normal approach would be to have a struct defining
> > a set of callbacks, that can be registered. Or if there's a natural
> > fit with QOM, then a QOM interface that can then have a QOM object
> > impl registered as a singleton.
> 
> That was my second attempt (after the weak symbols). I cleaned it up a bit
> and put it here: https://github.com/c3d/qemu/commits/spice-vtable.

I think this is the direction we should take.

> What made me switch to the approach in this series is the following
> considerations:
> 
> - A vtable is useful if there can be multiple values for a method, e.g. to
>   implement inheritance, or if you have multiple instances. This is not the
>   case here.

Well, we'll have two.  The normal functions.  And the stubs.

The stubs are inline functions right now, in include/ui/qemu-spice.h, in
the !CONFIG_SPICE section.  We can turn them into normal functions, move
them to some C file.  Let QemuSpiceOpts function pointers point to the
stubs initially.  When spice initializes (no matter whenever modular or
not) it'll set QemuSpiceOpts to the normal implementation.

That way we'll also remove some spice #ifdefs as part of the spice
modularization effort.

Things like the "using_spice" variable which don't depend on the spice
shared libraries can also be moved to the new C file with the spice
stubs.

I don't think we need to hide QemuSpiceOpts with inline functions like
qemu_spice_migrate_info().  I would simply use ...

	struct QemuSpiceOps {
		[ ... ]
		int (*migrate_info)(...);
		[ ... ]
	} qemu_spice;

... then change the ...

	qemu_spice_migrate_info(...)

.. callsites into ...

	qemu_spice.migrate_info(...)

> - Overloading QOM for that purpose looked more confusing than anything else.
>   It looked like I was mixing unrelated concepts. Maybe that's just me.

Hmm?  Not sure what you mean.  There is no need for QOM here (and I
can't see anything like that in your spice-vtable branch either).

> - The required change with a vtable ends up being more extensive. Instead of
>   changing a single line to put an entry point in a DSO, you need to create
>   the vtable, add functions to it, add a register function, etc. I was
>   looking for an easier and more scalable way.

IMHO it isn't too much overhead, and I find the code is more readable
that way.

> - In particular, with a vtable, you cannot take advantage of the syntactic
>   trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
>   So for a vtable, you need to manually write wrappers.

See above, I don't think we need wrappers.

take care,
  Gerd

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Gerd Hoffmann]

  Hi,

>  obj-$(CONFIG_PC) += pc.o pc_sysfw.o
> +pc.o-cflags += $(SPICE_CFLAGS)

Hmm, looks strange.  Why does pc.c need spice?

> +qmp-cmds.o-cflags += $(SPICE_CFLAGS)
> +hmp-cmds.o-cflags += $(SPICE_CFLAGS)

spice monitor commands need this I guess?

> +misc.o-cflags += $(SPICE_CFLAGS)

Why this?

> +vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)

spice init probably.

> -stub-obj-y += vmgenid.o
>  stub-obj-y += sysbus.o
>  stub-obj-y += tpm.o
>  stub-obj-y += trace-control.o
> +stub-obj-y += vmgenid.o

Huh?

> -spice-app.mo-cflags := $(GIO_CFLAGS)
> -spice-app.mo-libs := $(GIO_LIBS)
> +spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
> +spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)

Good.

> --- a/util/module.c
> +++ b/util/module.c
> @@ -22,11 +22,11 @@
>  #ifdef CONFIG_MODULE_UPGRADES
>  #include "qemu-version.h"
>  #endif
> -#ifdef CONFIG_TRACE_RECORDER
>  #include "trace/recorder.h"
> -#endif
>  
>  
> +RECORDER(modules, 16, "QEMU load modules");
> +
>  typedef struct ModuleEntry
>  {
>      void (*init)(void);
> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
>  {
>      ModuleEntry *e;
>  
> +#ifdef CONFIG_TRACE_RECORDER
> +    static const char *name[] = {
> +        "MIGRATION", "BLOCK", "OPTS", "QOM",
> +        "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
> +        "MAX"
> +    };
> +#endif
> +    record(modules, "Register DSO module init %p type %u %+s",
> +           fn, type, name[type]);
>      init_lists();
>  
>      e = g_malloc0(sizeof(*e));

Unrelated change.

(the recorder stuff should probably integrate with qemu trace support,
so you can record any trace point qemu has, but that'll be another patch
series ...)

take care,
  Gerd

Re: [PATCH 05/10] build: Avoid build failure when building drivers as modules

[Gerd Hoffmann]

  Hi,

> > +#
> > +# common-obj-m has some crap here, probably as side effect from
> > +# filling obj-y.  Clear it.  Fixes suspicious dependency errors when
> > +# building devices as modules.
> > +#
> > +common-obj-m :=
> 
> This comment doesn't fill me with confidence - makes it feel like there's
> some more important root cause that needs addressing instead.

It's my fault, see also
  https://patchwork.ozlabs.org/project/qemu-devel/patch/20200624131045.14512-5-kraxel@redhat.com/

I think the underlying problem is that the functions building *-m
variables do not properly handle per-target objects.  Which has no bad
side effects as long as we never recurse into hw/, but if we want build
devices as modules we have to ...

Unless we want build per-target code as module the above should do the
trick I think.

Better suggestions are welcome of course.

take care,
  Gerd

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Gerd Hoffmann]

  Hi,

>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
> +spice-app.mo-objs += ../audio/spiceaudio.o

Hmm.  audio/audio.c will try to load audio-${backend}.so when you run
qemu -audiodev ${backend}, so I suspect this is not going to work ...

I guess we should better try to tackle modules depending on modules
problem.  Given that g_module_open() doesn't support a custom shared
object search path I suspect we can't simply link audio-spice.so against
ui-spice-core.so and let the dynamic linker handle this for us.
Probably qemu needs a list of dependencies it'll check on module load
...

take care,
  Gerd

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Michael S. Tsirkin]

On Fri, Jun 26, 2020 at 06:42:58PM +0200, Christophe de Dinechin wrote:
> In order to facilitate the move of large chunks of functionality to
> load modules, it is simpler to create a wrapper with the same name
> that simply relays the implementation. For efficiency, this is
> typically done using inline functions in the header for the
> corresponding functionality. In that case, we rename the actual
> implementation by appending _implementation to its name. This makes it
> easier to select which function you want to put a breakpoint on.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/qemu/module.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae76..1922a0293c 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>  }
>  #endif
>  
> +#ifdef CONFIG_MODULES
> +/* Identify which functions are replaced by a callback stub */
> +#ifdef MODULE_STUBS
> +#define MODIFACE(Ret,Name,Args)                                         \
> +    Ret (*Name)Args;                                                    \
> +    extern Ret Name##_implementation Args
> +#else /* !MODULE_STUBS */
> +#define MODIFACE(Ret,Name,Args)                                         \
> +    extern Ret (*Name)Args;                                             \
> +    extern Ret Name##_implementation Args
> +#endif /* MODULE_STUBS */
> +
> +#define MODIMPL(Ret,Name,Args)                                          \
> +    static void __attribute__((constructor)) Name##_register(void)      \
> +    {                                                                   \
> +        Name = Name##_implementation;                                   \
> +    }                                                                   \
> +    Ret Name##_implementation Args
> +#else /* !CONFIG_MODULES */
> +/* When not using a module, such functions are called directly */
> +#define MODIFACE(Ret,Name,Args)         Ret Name Args
> +#define MODIMPL(Ret,Name,Args)          Ret Name Args
> +#endif /* CONFIG_MODULES */
> +
>  typedef enum {
>      MODULE_INIT_MIGRATION,
>      MODULE_INIT_BLOCK,

Hmm that's quite a bit of overhead for each call across modules.
Can't code patching be used somehow?


> -- 
> 2.26.2

Re: [PATCH 10/10] REMOVE: Instrumentation to show the module functions being replaced

[Christophe de Dinechin]

On 2020-06-26 at 19:29 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:07PM +0200, Christophe de Dinechin wrote:
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  include/qemu/module.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 1922a0293c..8d6e10ba81 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -14,10 +14,13 @@
>>  #ifndef QEMU_MODULE_H
>>  #define QEMU_MODULE_H
>>
>> +#include "trace/recorder.h"
>>
>>  #define DSO_STAMP_FUN         glue(qemu_stamp, CONFIG_STAMP)
>>  #define DSO_STAMP_FUN_STR     stringify(DSO_STAMP_FUN)
>>
>> +RECORDER_DECLARE(modules);
>> +
>>  #ifdef BUILD_DSO
>>  void DSO_STAMP_FUN(void);
>>  /* This is a dummy symbol to identify a loaded DSO as a QEMU module, so we can
>> @@ -55,6 +58,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>>      static void __attribute__((constructor)) Name##_register(void)      \
>>      {                                                                   \
>>          Name = Name##_implementation;                                   \
>> +        record(modules, "Setting " #Name " to %p", Name);               \
>>      }                                                                   \
>>      Ret Name##_implementation Args
>>  #else /* !CONFIG_MODULES */
>
> Contrary to the commit $SUBJECT, I think you should keep this, not remove
> it. It should use QEMU's trace backend though.

OK. Will add a trace backend version in next iteration.

>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

On 2020-06-30 at 01:00 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>> > If so the more normal approach would be to have a struct defining
>> > a set of callbacks, that can be registered. Or if there's a natural
>> > fit with QOM, then a QOM interface that can then have a QOM object
>> > impl registered as a singleton.
>>
>> That was my second attempt (after the weak symbols). I cleaned it up a bit
>> and put it here: https://github.com/c3d/qemu/commits/spice-vtable.
>
> I think this is the direction we should take.
>
>> What made me switch to the approach in this series is the following
>> considerations:
>>
>> - A vtable is useful if there can be multiple values for a method, e.g. to
>>   implement inheritance, or if you have multiple instances. This is not the
>>   case here.
>
> Well, we'll have two.  The normal functions.  And the stubs.
>
> The stubs are inline functions right now, in include/ui/qemu-spice.h, in
> the !CONFIG_SPICE section.  We can turn them into normal functions, move
> them to some C file.  Let QemuSpiceOpts function pointers point to the
> stubs initially.  When spice initializes (no matter whenever modular or
> not) it'll set QemuSpiceOpts to the normal implementation.

Good idea. I'll do that in the next round.

>
> That way we'll also remove some spice #ifdefs as part of the spice
> modularization effort.
>
> Things like the "using_spice" variable which don't depend on the spice
> shared libraries can also be moved to the new C file with the spice
> stubs.

OK.

>
> I don't think we need to hide QemuSpiceOpts with inline functions like
> qemu_spice_migrate_info().  I would simply use ...
>
> 	struct QemuSpiceOps {
> 		[ ... ]
> 		int (*migrate_info)(...);
> 		[ ... ]
> 	} qemu_spice;
>
> ... then change the ...
>
> 	qemu_spice_migrate_info(...)
>
> .. callsites into ...
>
> 	qemu_spice.migrate_info(...)
>

OK.

>> - Overloading QOM for that purpose looked more confusing than anything else.
>>   It looked like I was mixing unrelated concepts. Maybe that's just me.
>
> Hmm?  Not sure what you mean.  There is no need for QOM here (and I
> can't see anything like that in your spice-vtable branch either).

I was responding to Daniels's earlier comment:

> Or if there's a natural fit with QOM, then a QOM interface that can then
> have a QOM object impl registered as a singleton.

>
>> - The required change with a vtable ends up being more extensive. Instead of
>>   changing a single line to put an entry point in a DSO, you need to create
>>   the vtable, add functions to it, add a register function, etc. I was
>>   looking for an easier and more scalable way.
>
> IMHO it isn't too much overhead, and I find the code is more readable
> that way.

There is certainly very little overhead in that case, since none of the
functions is performance critical.

>
>> - In particular, with a vtable, you cannot take advantage of the syntactic
>>   trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
>>   So for a vtable, you need to manually write wrappers.
>
> See above, I don't think we need wrappers.

Well, so far that's two for two for the vtable approach. So unless someone
else agrees with my arguments for pointer patching, that will be my next
iteration of that series :-)

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

On 2020-06-30 at 11:38 CEST, Michael S. Tsirkin wrote...
> On Fri, Jun 26, 2020 at 06:42:58PM +0200, Christophe de Dinechin wrote:
>> In order to facilitate the move of large chunks of functionality to
>> load modules, it is simpler to create a wrapper with the same name
>> that simply relays the implementation. For efficiency, this is
>> typically done using inline functions in the header for the
>> corresponding functionality. In that case, we rename the actual
>> implementation by appending _implementation to its name. This makes it
>> easier to select which function you want to put a breakpoint on.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  include/qemu/module.h | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae76..1922a0293c 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MODULES
>> +/* Identify which functions are replaced by a callback stub */
>> +#ifdef MODULE_STUBS
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    Ret (*Name)Args;                                                    \
>> +    extern Ret Name##_implementation Args
>> +#else /* !MODULE_STUBS */
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    extern Ret (*Name)Args;                                             \
>> +    extern Ret Name##_implementation Args
>> +#endif /* MODULE_STUBS */
>> +
>> +#define MODIMPL(Ret,Name,Args)                                          \
>> +    static void __attribute__((constructor)) Name##_register(void)      \
>> +    {                                                                   \
>> +        Name = Name##_implementation;                                   \
>> +    }                                                                   \
>> +    Ret Name##_implementation Args
>> +#else /* !CONFIG_MODULES */
>> +/* When not using a module, such functions are called directly */
>> +#define MODIFACE(Ret,Name,Args)         Ret Name Args
>> +#define MODIMPL(Ret,Name,Args)          Ret Name Args
>> +#endif /* CONFIG_MODULES */
>> +
>>  typedef enum {
>>      MODULE_INIT_MIGRATION,
>>      MODULE_INIT_BLOCK,
>
> Hmm that's quite a bit of overhead for each call across modules.

Do you mean runtime overhead, i.e. the difference between
foo(x) and (*foo)(x)?


> Can't code patching be used somehow?

I've considered it, but this seems like a bit of a hammer for half a dozen
calls that are mostly init, so not performance sensitive.

In the old times, ld.so used to do that for you. All you had to do was to
mark one symbol as weak. Apparently for security reasons, that feature was
dropped several years ago. At least, that put call patching where it
belonged, i.e. in the loader.

>
>
>> --
>> 2.26.2


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin]

On 2020-06-29 at 12:13 CEST, Claudio Fontana wrote...
> Hello Christophe,
>
> On 6/26/20 6:42 PM, Christophe de Dinechin wrote:
>> In order to facilitate the move of large chunks of functionality to
>> load modules, it is simpler to create a wrapper with the same name
>> that simply relays the implementation. For efficiency, this is
>> typically done using inline functions in the header for the
>> corresponding functionality. In that case, we rename the actual
>> implementation by appending _implementation to its name. This makes it
>> easier to select which function you want to put a breakpoint on.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  include/qemu/module.h | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae76..1922a0293c 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MODULES
>> +/* Identify which functions are replaced by a callback stub */
>> +#ifdef MODULE_STUBS
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    Ret (*Name)Args;                                                    \
>> +    extern Ret Name##_implementation Args
>> +#else /* !MODULE_STUBS */
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    extern Ret (*Name)Args;                                             \
>> +    extern Ret Name##_implementation Args
>> +#endif /* MODULE_STUBS */
>> +
>> +#define MODIMPL(Ret,Name,Args)                                          \
>> +    static void __attribute__((constructor)) Name##_register(void)      \
>> +    {                                                                   \
>> +        Name = Name##_implementation;                                   \
>> +    }                                                                   \
>> +    Ret Name##_implementation Args
>> +#else /* !CONFIG_MODULES */
>> +/* When not using a module, such functions are called directly */
>> +#define MODIFACE(Ret,Name,Args)         Ret Name Args
>> +#define MODIMPL(Ret,Name,Args)          Ret Name Args
>> +#endif /* CONFIG_MODULES */
>> +
>>  typedef enum {
>>      MODULE_INIT_MIGRATION,
>>      MODULE_INIT_BLOCK,
>>
>
> Just as background, I am interested in all modules-related work, because of my long term plan to have target-specific modules as well:
>
>  https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>
> I am not 100% clear on what is the goal and expected usage of this
> preprocessor code, despite the commit message, maybe you could clarify a
> bit with more verbosity?

Well, so far, the preference seems to be to go through a more verbose
approach with an explicit table of functions.

What the preprocessor code did was:

- If you build without modules, nothing changes, you get a direct call

- If you build with modules:

 + In the DSO, foo is replaced with foo_implementation
 + Elsewhere, foo is replaced with a function pointer also called foo.
 + The implementation adds constructor code that sets foo to point to foo_implementation


>
> Additionally if you happen to be interested, maybe you know already or
> could think about what this could mean for target-specific modules, which
> will require some improvements to the modules "subsystem"(?) as well.

So far, I've only integrated Gerd's workaround for target-specific
modules. Some additional mechanics is needed to name target-specific
modules, e.g. put them in some target directory.

>
> In my experimentation I didn't have to do this preprocessor work, instead
> I had to fine tune a bit the makefile support (rules.mak and makefiles) to
> be able to accomodate for (even large) modules in target/ as well.

It's probably because the modules you were dealing with already had the
required indirection and module_init calls, i.e. they were only invoked
using QOM already.

The mechanism I was proposing is to quickly add the indirection for
qemu functionality that does not have such indirect calls yet.

The consensus so far seems to be that the syntax I proposed is not nice, and
that it's better to make it more explicit through a table and indirect
calls, even if that means changing the call sites.

>
> Thanks!
>
> CLaudio


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

On 2020-06-30 at 01:08 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>>  obj-$(CONFIG_PC) += pc.o pc_sysfw.o
>> +pc.o-cflags += $(SPICE_CFLAGS)
>
> Hmm, looks strange.  Why does pc.c need spice?

It includes ui/qemu-spice.h, and I did not check why.
Turns out this is not needed. So I'll remove it.

>
>> +qmp-cmds.o-cflags += $(SPICE_CFLAGS)
>> +hmp-cmds.o-cflags += $(SPICE_CFLAGS)
>
> spice monitor commands need this I guess?

Yes.

>
>> +misc.o-cflags += $(SPICE_CFLAGS)
>
> Why this?

qemu_using_spice and qemu_spice_migrate_info

>
>> +vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SPICE_CFLAGS)
>
> spice init probably

Yes.
.
>
>> -stub-obj-y += vmgenid.o
>>  stub-obj-y += sysbus.o
>>  stub-obj-y += tpm.o
>>  stub-obj-y += trace-control.o
>> +stub-obj-y += vmgenid.o
>
> Huh?

I sent it separately as a trivial patch. Wrong alphabetical order, and where
that change was placed was causing a conflict on each rebase with a spice.c
stub I had at some point "at the right spot" ;-)

>
>> -spice-app.mo-cflags := $(GIO_CFLAGS)
>> -spice-app.mo-libs := $(GIO_LIBS)
>> +spice-app.mo-cflags := $(GIO_CFLAGS) $(SPICE_CFLAGS)
>> +spice-app.mo-libs := $(GIO_LIBS) $(SPICE_LIBS)
>
> Good.
>
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,11 +22,11 @@
>>  #ifdef CONFIG_MODULE_UPGRADES
>>  #include "qemu-version.h"
>>  #endif
>> -#ifdef CONFIG_TRACE_RECORDER
>>  #include "trace/recorder.h"
>> -#endif
>>
>>
>> +RECORDER(modules, 16, "QEMU load modules");
>> +
>>  typedef struct ModuleEntry
>>  {
>>      void (*init)(void);
>> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
>>  {
>>      ModuleEntry *e;
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +    static const char *name[] = {
>> +        "MIGRATION", "BLOCK", "OPTS", "QOM",
>> +        "TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
>> +        "MAX"
>> +    };
>> +#endif
>> +    record(modules, "Register DSO module init %p type %u %+s",
>> +           fn, type, name[type]);
>>      init_lists();
>>
>>      e = g_malloc0(sizeof(*e));
>
> Unrelated change.
>
> (the recorder stuff should probably integrate with qemu trace support,
> so you can record any trace point qemu has, but that'll be another patch
> series ...)

I sent it separately, and fixed the leftover patch.

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)