1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/display: avoid creating empty loadable modules

Daniel P. Berrangé posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221219125830.2369169-1-berrange@redhat.com
hw/display/meson.build | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
[PATCH] hw/display: avoid creating empty loadable modules
Posted by Daniel P. Berrangé 1 year, 4 months ago 
When using --disable-virglrenderer, QEMU still creates

  hw-display-virtio-gpu-gl.so
  hw-display-virtio-vga-gl.so
  hw-display-virtio-gpu-pci-gl.so

but when these are loaded, they provide no functionality as the code
which registers types is not compiled in. Funtionally this is
relatively harmless, because QEMU is fine loading a module with no
types.

This is rather confusing for users and OS distro maintainers though,
as they think they have the GL functionality built, but in fact the
module they are looking at provides nothing of value.

The root cause is the use of 'when/if_true' rules when adding sources
to the module source set. If all the rules evaluate to false, then we
have declared the module, but not added anything to it.  We need to
put declaration of the entire module inside a condition based on
existance of the 3rd party library deps that are mandatory.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1352
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/meson.build | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index 7a725ed80e..20a53d24a8 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -64,7 +64,7 @@ softmmu_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 'ati_2d.c', 'ati_dbg.c'))
 
 
-if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
+if config_all_devices.has_key('CONFIG_VIRTIO_GPU') and pixman.found()
   virtio_gpu_ss = ss.source_set()
   virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU',
                     if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c'), pixman])
@@ -73,13 +73,15 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_ss.add(when: 'CONFIG_VHOST_USER_GPU', if_true: files('vhost-user-gpu.c'))
   hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
 
-  virtio_gpu_gl_ss = ss.source_set()
-  virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
-                       if_true: [files('virtio-gpu-gl.c', 'virtio-gpu-virgl.c'), pixman, virgl])
-  hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+  if virgl.found() and opengl.found()
+    virtio_gpu_gl_ss = ss.source_set()
+    virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
+                         if_true: [files('virtio-gpu-gl.c', 'virtio-gpu-virgl.c'), pixman, virgl])
+    hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
+  endif
 endif
 
-if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
+if config_all_devices.has_key('CONFIG_VIRTIO_PCI') and pixman.found()
   virtio_gpu_pci_ss = ss.source_set()
   virtio_gpu_pci_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI'],
                         if_true: [files('virtio-gpu-pci.c'), pixman])
@@ -87,10 +89,12 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
                         if_true: files('vhost-user-gpu-pci.c'))
   hw_display_modules += {'virtio-gpu-pci': virtio_gpu_pci_ss}
 
-  virtio_gpu_pci_gl_ss = ss.source_set()
-  virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', virgl, opengl],
-                           if_true: [files('virtio-gpu-pci-gl.c'), pixman])
-  hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+  if virgl.found() and opengl.found()
+    virtio_gpu_pci_gl_ss = ss.source_set()
+    virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', virgl, opengl],
+                             if_true: [files('virtio-gpu-pci-gl.c'), pixman])
+    hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
+  endif
 endif
 
 if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
-- 
2.38.1
Re: [PATCH] hw/display: avoid creating empty loadable modules
Posted by Paolo Bonzini 1 year, 4 months ago 
On 12/19/22 13:58, Daniel P. Berrangé wrote:
> When using --disable-virglrenderer, QEMU still creates
> 
>    hw-display-virtio-gpu-gl.so
>    hw-display-virtio-vga-gl.so
>    hw-display-virtio-gpu-pci-gl.so
> 
> but when these are loaded, they provide no functionality as the code
> which registers types is not compiled in. Funtionally this is
> relatively harmless, because QEMU is fine loading a module with no
> types.
> 
> This is rather confusing for users and OS distro maintainers though,
> as they think they have the GL functionality built, but in fact the
> module they are looking at provides nothing of value.
> 
> The root cause is the use of 'when/if_true' rules when adding sources
> to the module source set. If all the rules evaluate to false, then we
> have declared the module, but not added anything to it.  We need to
> put declaration of the entire module inside a condition based on
> existance of the 3rd party library deps that are mandatory.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1352
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   hw/display/meson.build | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index 7a725ed80e..20a53d24a8 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -64,7 +64,7 @@ softmmu_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
>   softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 'ati_2d.c', 'ati_dbg.c'))
>   
>   
> -if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
> +if config_all_devices.has_key('CONFIG_VIRTIO_GPU') and pixman.found()

pixman is mandatory if have_system.

Can you instead change hw/meson.build to skip system-only directories? 
I think this should do:

subdir('core')
if not have_system
   subdir_done()
endif

# everything else...

The rest is good, so I have applied it.

Paolo

>     virtio_gpu_ss = ss.source_set()
>     virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU',
>                       if_true: [files('virtio-gpu-base.c', 'virtio-gpu.c'), pixman])
> @@ -73,13 +73,15 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
>     virtio_gpu_ss.add(when: 'CONFIG_VHOST_USER_GPU', if_true: files('vhost-user-gpu.c'))
>     hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
>   
> -  virtio_gpu_gl_ss = ss.source_set()
> -  virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
> -                       if_true: [files('virtio-gpu-gl.c', 'virtio-gpu-virgl.c'), pixman, virgl])
> -  hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
> +  if virgl.found() and opengl.found()
> +    virtio_gpu_gl_ss = ss.source_set()
> +    virtio_gpu_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', virgl, opengl],
> +                         if_true: [files('virtio-gpu-gl.c', 'virtio-gpu-virgl.c'), pixman, virgl])
> +    hw_display_modules += {'virtio-gpu-gl': virtio_gpu_gl_ss}
> +  endif
>   endif
>   
> -if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI') and pixman.found()
>     virtio_gpu_pci_ss = ss.source_set()
>     virtio_gpu_pci_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI'],
>                           if_true: [files('virtio-gpu-pci.c'), pixman])
> @@ -87,10 +89,12 @@ if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
>                           if_true: files('vhost-user-gpu-pci.c'))
>     hw_display_modules += {'virtio-gpu-pci': virtio_gpu_pci_ss}
>   
> -  virtio_gpu_pci_gl_ss = ss.source_set()
> -  virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', virgl, opengl],
> -                           if_true: [files('virtio-gpu-pci-gl.c'), pixman])
> -  hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
> +  if virgl.found() and opengl.found()
> +    virtio_gpu_pci_gl_ss = ss.source_set()
> +    virtio_gpu_pci_gl_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI', virgl, opengl],
> +                             if_true: [files('virtio-gpu-pci-gl.c'), pixman])
> +    hw_display_modules += {'virtio-gpu-pci-gl': virtio_gpu_pci_gl_ss}
> +  endif
>   endif
>   
>   if config_all_devices.has_key('CONFIG_VIRTIO_VGA')

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.