diff mbox series

[1/2] virtio-gpu: add virtio-gpu-pci module

Message ID 20201023064618.21409-2-kraxel@redhat.com
State New, archived
Headers show
Series virtio-gpu: build pci and vga bits modular too. | expand

Commit Message

Gerd Hoffmann Oct. 23, 2020, 6:46 a.m. UTC
Build virtio-gpu pci devices modular.  Must be a separate module because
not all qemu softmmu variants come with PCI support.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c          |  3 +++
 hw/display/meson.build | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Marc-André Lureau Oct. 23, 2020, 8:05 a.m. UTC | #1
On Fri, Oct 23, 2020 at 10:46 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Build virtio-gpu pci devices modular.  Must be a separate module because
> not all qemu softmmu variants come with PCI support.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

Hmm, I realize we have a different behaviour when building devices as
modules shared by different qemu system binaries.

It will attempt to load any kind of modules:

./qemu-system-m68k  -kernel ~/Downloads/vmlinux-5.8.0-3-m68k  -device
virtio-gpu-pci
Failed to open module:
/home/elmarco/src/qemu/buildnodoc/hw-display-virtio-gpu-pci.so: undefined
symbol: virtio_instance_init_common
qemu-system-m68k: -device virtio-gpu-pci: 'virtio-gpu-pci' is not a valid
device model name


And this is not a new problem, for example with qemu 5.1.0-5.fc33:

$ qemu-system-m68k -device help
Failed to open module: /usr/lib64/qemu/hw-usb-smartcard.so: undefined
symbol: ccid_card_send_apdu_to_guest
Failed to open module: /usr/lib64/qemu/hw-display-qxl.so: undefined symbol:
vga_ioport_read
...

What would be the solution? load modules from an arch-specific directory?
link to a common shared library location? Ex:
/usr/lib64/qemu/x86_64/hw-usb-smartcard.so ->
/usr/lib64/qemu/hw-usb-smartcard.so


---
>  util/module.c          |  3 +++
>  hw/display/meson.build | 11 +++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/util/module.c b/util/module.c
> index fe3b82dd4d37..9490f975d303 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -301,6 +301,9 @@ static struct {
>      { "qxl",                   "hw-", "display-qxl"           },
>      { "virtio-gpu-device",     "hw-", "display-virtio-gpu"    },
>      { "vhost-user-gpu",        "hw-", "display-virtio-gpu"    },
> +    { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
> +    { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
> +    { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
>      { "chardev-braille",       "chardev-", "baum"             },
>      { "chardev-spicevmc",      "chardev-", "spice"            },
>      { "chardev-spiceport",     "chardev-", "spice"            },
> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index 0d5ddecd6503..669935371335 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -62,8 +62,15 @@ if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
>    hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
>  endif
>
> -softmmu_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI'], if_true:
> files('virtio-gpu-pci.c'))
> -softmmu_ss.add(when: ['CONFIG_VHOST_USER_GPU', 'CONFIG_VIRTIO_PCI'],
> if_true: files('vhost-user-gpu-pci.c'))
> +if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
> +  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])
> +  virtio_gpu_pci_ss.add(when: ['CONFIG_VHOST_USER_GPU',
> 'CONFIG_VIRTIO_PCI'],
> +                        if_true: files('vhost-user-gpu-pci.c'))
> +  hw_display_modules += {'virtio-gpu-pci': virtio_gpu_pci_ss}
> +endif
> +
>  softmmu_ss.add(when: 'CONFIG_VIRTIO_VGA', if_true: files('virtio-vga.c'))
>  softmmu_ss.add(when: 'CONFIG_VHOST_USER_VGA', if_true:
> files('vhost-user-vga.c'))
>
> --
> 2.27.0
>
>
>
Otherwise patch works well:

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Gerd Hoffmann Oct. 23, 2020, 11:01 a.m. UTC | #2
Hi,

> Hmm, I realize we have a different behaviour when building devices as
> modules shared by different qemu system binaries.
> 
> It will attempt to load any kind of modules:
> 
> ./qemu-system-m68k  -kernel ~/Downloads/vmlinux-5.8.0-3-m68k  -device
> virtio-gpu-pci
> Failed to open module:
> /home/elmarco/src/qemu/buildnodoc/hw-display-virtio-gpu-pci.so: undefined
> symbol: virtio_instance_init_common
> qemu-system-m68k: -device virtio-gpu-pci: 'virtio-gpu-pci' is not a valid
> device model name

Yes.  The last line is printed for non-modular builds too.
The module load error can obviously only happen on modular builds.

> $ qemu-system-m68k -device help
> Failed to open module: /usr/lib64/qemu/hw-usb-smartcard.so: undefined
> symbol: ccid_card_send_apdu_to_guest
> Failed to open module: /usr/lib64/qemu/hw-display-qxl.so: undefined symbol:
> vga_ioport_read

That one is fixed meanwhile:

commit 501093207eb1ed4845e0a65ee1ce7db7b9676e0b
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Wed Sep 23 11:12:17 2020 +0200

    module: silence errors for module_load_qom_all().

take care,
  Gerd
Marc-André Lureau Oct. 23, 2020, 11:26 a.m. UTC | #3
HI

On Fri, Oct 23, 2020 at 3:01 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > Hmm, I realize we have a different behaviour when building devices as
> > modules shared by different qemu system binaries.
> >
> > It will attempt to load any kind of modules:
> >
> > ./qemu-system-m68k  -kernel ~/Downloads/vmlinux-5.8.0-3-m68k  -device
> > virtio-gpu-pci
> > Failed to open module:
> > /home/elmarco/src/qemu/buildnodoc/hw-display-virtio-gpu-pci.so: undefined
> > symbol: virtio_instance_init_common
> > qemu-system-m68k: -device virtio-gpu-pci: 'virtio-gpu-pci' is not a valid
> > device model name
>
> Yes.  The last line is printed for non-modular builds too.
> The module load error can obviously only happen on modular builds.
>
> > $ qemu-system-m68k -device help
> > Failed to open module: /usr/lib64/qemu/hw-usb-smartcard.so: undefined
> > symbol: ccid_card_send_apdu_to_guest
> > Failed to open module: /usr/lib64/qemu/hw-display-qxl.so: undefined
> symbol:
> > vga_ioport_read
>
> That one is fixed meanwhile:
>
> commit 501093207eb1ed4845e0a65ee1ce7db7b9676e0b
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Wed Sep 23 11:12:17 2020 +0200
>
>     module: silence errors for module_load_qom_all().
>
>
Ok, but that could hide real errors, couldn't it? What about the proposal
to have a subdir per arch with symlinks?
Gerd Hoffmann Oct. 26, 2020, 6:21 a.m. UTC | #4
Hi,

> > commit 501093207eb1ed4845e0a65ee1ce7db7b9676e0b
> > Author: Gerd Hoffmann <kraxel@redhat.com>
> > Date:   Wed Sep 23 11:12:17 2020 +0200
> >
> >     module: silence errors for module_load_qom_all().
> >
> Ok, but that could hide real errors, couldn't it?

It should not.  If you explicitly ask for an module and it doesn't load
you'll get an error no matter what.  This only skips the error message
in case loading all qom modules (for '-device help' & friends) was
requested.

> What about the proposal to have a subdir per arch with symlinks?

The modules are not per-arch.  They just depend on pci or vga or usb
being present in core qemu, and some qemu-system-$arch variants don't
have that.

So -- for example -- s390x has no vga support, therefore qxl doesn't
load.  qxl wasn't available before, so nothing fundamental changed.  The
only difference is that you get an additional error message line from
the attempt to load the qxl module.

Why is this a problem?

take care,
  Gerd
diff mbox series

Patch

diff --git a/util/module.c b/util/module.c
index fe3b82dd4d37..9490f975d303 100644
--- a/util/module.c
+++ b/util/module.c
@@ -301,6 +301,9 @@  static struct {
     { "qxl",                   "hw-", "display-qxl"           },
     { "virtio-gpu-device",     "hw-", "display-virtio-gpu"    },
     { "vhost-user-gpu",        "hw-", "display-virtio-gpu"    },
+    { "virtio-gpu-pci-base",   "hw-", "display-virtio-gpu-pci" },
+    { "virtio-gpu-pci",        "hw-", "display-virtio-gpu-pci" },
+    { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu-pci" },
     { "chardev-braille",       "chardev-", "baum"             },
     { "chardev-spicevmc",      "chardev-", "spice"            },
     { "chardev-spiceport",     "chardev-", "spice"            },
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 0d5ddecd6503..669935371335 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -62,8 +62,15 @@  if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   hw_display_modules += {'virtio-gpu': virtio_gpu_ss}
 endif
 
-softmmu_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_PCI'], if_true: files('virtio-gpu-pci.c'))
-softmmu_ss.add(when: ['CONFIG_VHOST_USER_GPU', 'CONFIG_VIRTIO_PCI'], if_true: files('vhost-user-gpu-pci.c'))
+if config_all_devices.has_key('CONFIG_VIRTIO_PCI')
+  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])
+  virtio_gpu_pci_ss.add(when: ['CONFIG_VHOST_USER_GPU', 'CONFIG_VIRTIO_PCI'],
+                        if_true: files('vhost-user-gpu-pci.c'))
+  hw_display_modules += {'virtio-gpu-pci': virtio_gpu_pci_ss}
+endif
+
 softmmu_ss.add(when: 'CONFIG_VIRTIO_VGA', if_true: files('virtio-vga.c'))
 softmmu_ss.add(when: 'CONFIG_VHOST_USER_VGA', if_true: files('vhost-user-vga.c'))