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

Jose R. Ziviani posted 2 patches 2 years, 7 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210927141201.21833-1-jziviani@suse.de
Maintainers: Cleber Rosa <crosa@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, David Hildenbrand <david@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Thomas Huth <thuth@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Cornelia Huck <cohuck@redhat.com>
There is a newer version of this series
hw/display/qxl.c                |  1 +
hw/display/vhost-user-gpu-pci.c |  1 +
hw/display/vhost-user-gpu.c     |  1 +
hw/display/vhost-user-vga.c     |  1 +
hw/display/virtio-gpu-base.c    |  1 +
hw/display/virtio-gpu-gl.c      |  1 +
hw/display/virtio-gpu-pci-gl.c  |  1 +
hw/display/virtio-gpu-pci.c     |  1 +
hw/display/virtio-gpu.c         |  1 +
hw/display/virtio-vga-gl.c      |  1 +
hw/display/virtio-vga.c         |  1 +
hw/s390x/virtio-ccw-gpu.c       |  1 +
hw/usb/ccid-card-emulated.c     |  1 +
hw/usb/ccid-card-passthru.c     |  1 +
hw/usb/host-libusb.c            |  1 +
hw/usb/redirect.c               |  1 +
include/qemu/module.h           | 10 +++++++++
meson.build                     | 25 ++++++++++++++-------
scripts/modinfo-generate.py     | 40 ++++++++++++++++++++-------------
19 files changed, 67 insertions(+), 24 deletions(-)
[PATCH v2 0/2] modules: Improve modinfo.c support
Posted by Jose R. Ziviani 2 years, 7 months ago
This patchset introduces the modinfo_need and changes
modinfo-generate.py/meson.build to generate/link one modinfo per target.

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

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

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

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

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

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

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

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

Thank you!

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

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

-- 
2.33.0


[PATCH v2 0/2] modules: Improve modinfo.c support
Posted by Jose R. Ziviani 2 years, 7 months ago
This patchset introduces the modinfo_need and changes
modinfo-generate.py/meson.build to generate/link one modinfo per target.

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

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

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

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

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

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

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

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

Thank you!

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

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

-- 
2.33.0


Re: [PATCH v2 0/2] modules: Improve modinfo.c support
Posted by Gerd Hoffmann 2 years, 7 months ago
On Mon, Sep 27, 2021 at 11:11:58AM -0300, Jose R. Ziviani wrote:
> This patchset introduces the modinfo_need and changes
> modinfo-generate.py/meson.build to generate/link one modinfo per target.
> 
> modinfo-generate.py will know, thanks to modinfo_need, which modules are
> currently enabled for a given target before adding it in the array of
> modules. It will give a hint about why some modules failed, so
> developers can have a clue about it:

The approach looks good to me.

>     /* hw-display-qxl.modinfo */
>     /* module  QXL is missing. */

You are using kconfig symbols here, so the comment should say so ;)

Renaming modinfo_need to modinfo_kconfig will probably also help
to make that clear.

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

Hmm?  Leftover from an older version of the series?

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

It's target-specific modules.  Although accelerators are the only
in-tree users right this is not limited to accelerators.

take care,
  Gerd


Re: [PATCH v2 0/2] modules: Improve modinfo.c support
Posted by Jose R. Ziviani 2 years, 7 months ago
Hello, Gerd!

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

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

Thank you!

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