mbox series

[0/2] virtio-gpu: build pci and vga bits modular too.

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

Message

Gerd Hoffmann Oct. 23, 2020, 6:46 a.m. UTC 
Gerd Hoffmann (2):
  virtio-gpu: add virtio-gpu-pci module
  virtio-gpu: add virtio-vga module

 util/module.c          |  6 ++++++
 hw/display/meson.build | 21 +++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
2.27.0

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?

-- 
Marc-André Lureau
<div dir="ltr"><div dir="ltr">HI<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 23, 2020 at 3:01 PM Gerd Hoffmann &lt;<a href="mailto:kraxel@redhat.com">kraxel@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">  Hi,<br>
<br>
&gt; Hmm, I realize we have a different behaviour when building devices as<br>
&gt; modules shared by different qemu system binaries.<br>
&gt; <br>
&gt; It will attempt to load any kind of modules:<br>
&gt; <br>
&gt; ./qemu-system-m68k  -kernel ~/Downloads/vmlinux-5.8.0-3-m68k  -device<br>
&gt; virtio-gpu-pci<br>
&gt; Failed to open module:<br>
&gt; /home/elmarco/src/qemu/buildnodoc/hw-display-virtio-gpu-pci.so: undefined<br>
&gt; symbol: virtio_instance_init_common<br>
&gt; qemu-system-m68k: -device virtio-gpu-pci: &#39;virtio-gpu-pci&#39; is not a valid<br>
&gt; device model name<br>
<br>
Yes.  The last line is printed for non-modular builds too.<br>
The module load error can obviously only happen on modular builds.<br>
<br>
&gt; $ qemu-system-m68k -device help<br>
&gt; Failed to open module: /usr/lib64/qemu/hw-usb-smartcard.so: undefined<br>
&gt; symbol: ccid_card_send_apdu_to_guest<br>
&gt; Failed to open module: /usr/lib64/qemu/hw-display-qxl.so: undefined symbol:<br>
&gt; vga_ioport_read<br>
<br>
That one is fixed meanwhile:<br>
<br>
commit 501093207eb1ed4845e0a65ee1ce7db7b9676e0b<br>
Author: Gerd Hoffmann &lt;<a href="mailto:kraxel@redhat.com" target="_blank">kraxel@redhat.com</a>&gt;<br>
Date:   Wed Sep 23 11:12:17 2020 +0200<br>
<br>
    module: silence errors for module_load_qom_all().<br><br></blockquote><div><br></div><div>Ok, but that could hide real errors, couldn&#39;t it? What about the proposal to have a subdir per arch with symlinks?<br></div><div> </div></div>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
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