[PATCH v2 00/18] modules: add metadata database

Gerd Hoffmann posted 18 patches 2 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210610055755.538119-1-kraxel@redhat.com
Maintainers: Samuel Thibault <samuel.thibault@ens-lyon.org>, Markus Armbruster <armbru@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Lieven <pl@kamp.de>, "Michael S. Tsirkin" <mst@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Paolo Bonzini <pbonzini@redhat.com>, Eric Blake <eblake@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Max Reitz <mreitz@redhat.com>, Michael Roth <michael.roth@amd.com>, David Hildenbrand <david@redhat.com>, Thomas Huth <thuth@redhat.com>
include/qemu/module.h           |  23 +++
audio/spiceaudio.c              |   2 +
block/iscsi-opts.c              |   1 +
chardev/baum.c                  |   1 +
chardev/spice.c                 |   4 +
hw/display/qxl.c                |   4 +
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      |   3 +
hw/display/virtio-gpu-pci-gl.c  |   3 +
hw/display/virtio-gpu-pci.c     |   2 +
hw/display/virtio-gpu.c         |   1 +
hw/display/virtio-vga-gl.c      |   3 +
hw/display/virtio-vga.c         |   2 +
hw/s390x/virtio-ccw-gpu.c       |   3 +
hw/usb/ccid-card-emulated.c     |   1 +
hw/usb/ccid-card-passthru.c     |   1 +
hw/usb/redirect.c               |   1 +
qemu-modinfo.c                  | 270 ++++++++++++++++++++++++++++++
softmmu/vl.c                    |  20 +--
stubs/module-opts.c             |   4 -
ui/egl-headless.c               |   4 +
ui/gtk.c                        |   4 +
ui/sdl2.c                       |   4 +
ui/spice-app.c                  |   3 +
ui/spice-core.c                 |   5 +
util/module.c                   | 282 +++++++++++++++++++-------------
meson.build                     |  11 ++
qapi/meson.build                |   1 +
qapi/modules.json               |  36 ++++
qapi/qapi-schema.json           |   1 +
util/trace-events               |   3 +
34 files changed, 576 insertions(+), 131 deletions(-)
create mode 100644 qemu-modinfo.c
create mode 100644 qapi/modules.json
[PATCH v2 00/18] modules: add metadata database
Posted by Gerd Hoffmann 2 years, 9 months ago
This patch series adds support for module metadata.  Here are the pieces
of the puzzle:

  (1) Macros are added to store metadata in a .modinfo elf section
      (idea stolen from the linux kernel).
  (2) A utility to scan modules, collect metadata from the .modinfo
      sections, store it in a file (modinfo.json) for later consumption
      by qemu.  Can also be easily inspected using 'jq'.
  (3) Adding annotations to the modules we have.
  (4) Drop hard-coded lists from utils/module.c

take care,
  Gerd

Gerd Hoffmann (18):
  modules: add metadata macros, add qxl module annotations
  qapi: add ModuleInfo schema
  modules: add qemu-modinfo utility
  modules: add virtio-gpu module annotations
  modules: add chardev module annotations
  modules: add audio module annotations
  modules: add usb-redir module annotations
  modules: add ccid module annotations
  modules: add ui module annotations
  modules: add s390x module annotations
  modules: add block module annotations
  modules: add module_load_path_init helper
  modules: load modinfo.json
  modules: use modinfo for dependencies
  modules: use modinfo for qom load
  modules: use modinfo for qemu opts load
  modules: check arch and block load on mismatch
  [fixup] module_load_modinfo

 include/qemu/module.h           |  23 +++
 audio/spiceaudio.c              |   2 +
 block/iscsi-opts.c              |   1 +
 chardev/baum.c                  |   1 +
 chardev/spice.c                 |   4 +
 hw/display/qxl.c                |   4 +
 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      |   3 +
 hw/display/virtio-gpu-pci-gl.c  |   3 +
 hw/display/virtio-gpu-pci.c     |   2 +
 hw/display/virtio-gpu.c         |   1 +
 hw/display/virtio-vga-gl.c      |   3 +
 hw/display/virtio-vga.c         |   2 +
 hw/s390x/virtio-ccw-gpu.c       |   3 +
 hw/usb/ccid-card-emulated.c     |   1 +
 hw/usb/ccid-card-passthru.c     |   1 +
 hw/usb/redirect.c               |   1 +
 qemu-modinfo.c                  | 270 ++++++++++++++++++++++++++++++
 softmmu/vl.c                    |  20 +--
 stubs/module-opts.c             |   4 -
 ui/egl-headless.c               |   4 +
 ui/gtk.c                        |   4 +
 ui/sdl2.c                       |   4 +
 ui/spice-app.c                  |   3 +
 ui/spice-core.c                 |   5 +
 util/module.c                   | 282 +++++++++++++++++++-------------
 meson.build                     |  11 ++
 qapi/meson.build                |   1 +
 qapi/modules.json               |  36 ++++
 qapi/qapi-schema.json           |   1 +
 util/trace-events               |   3 +
 34 files changed, 576 insertions(+), 131 deletions(-)
 create mode 100644 qemu-modinfo.c
 create mode 100644 qapi/modules.json

-- 
2.31.1



Re: [PATCH v2 00/18] modules: add metadata database
Posted by Claudio Fontana 2 years, 9 months ago
On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
> This patch series adds support for module metadata.  Here are the pieces
> of the puzzle:
> 
>   (1) Macros are added to store metadata in a .modinfo elf section
>       (idea stolen from the linux kernel).
>   (2) A utility to scan modules, collect metadata from the .modinfo
>       sections, store it in a file (modinfo.json) for later consumption
>       by qemu.  Can also be easily inspected using 'jq'.
>   (3) Adding annotations to the modules we have.
>   (4) Drop hard-coded lists from utils/module.c
> 
> take care,
>   Gerd

The background has disappeared compared with V1.

V1 says:

"Background is that the hard-coded lists in util/module.c are somewhat
ugly and also wouldn't work very well with a large number of modules,
so I'm looking for something else."

Can you write more about what the actual high level goals of this series are?

We are in general making QEMU more and more difficult to get into,
requiring more and more investment for new contributors to get productive.

Is the additional complexity justified? What is the benefit, and is simplification a goal of the series as well?


> 
> Gerd Hoffmann (18):
>   modules: add metadata macros, add qxl module annotations
>   qapi: add ModuleInfo schema
>   modules: add qemu-modinfo utility
>   modules: add virtio-gpu module annotations
>   modules: add chardev module annotations
>   modules: add audio module annotations
>   modules: add usb-redir module annotations
>   modules: add ccid module annotations
>   modules: add ui module annotations
>   modules: add s390x module annotations
>   modules: add block module annotations
>   modules: add module_load_path_init helper
>   modules: load modinfo.json
>   modules: use modinfo for dependencies
>   modules: use modinfo for qom load
>   modules: use modinfo for qemu opts load
>   modules: check arch and block load on mismatch
>   [fixup] module_load_modinfo
> 
>  include/qemu/module.h           |  23 +++
>  audio/spiceaudio.c              |   2 +
>  block/iscsi-opts.c              |   1 +
>  chardev/baum.c                  |   1 +
>  chardev/spice.c                 |   4 +
>  hw/display/qxl.c                |   4 +
>  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      |   3 +
>  hw/display/virtio-gpu-pci-gl.c  |   3 +
>  hw/display/virtio-gpu-pci.c     |   2 +
>  hw/display/virtio-gpu.c         |   1 +
>  hw/display/virtio-vga-gl.c      |   3 +
>  hw/display/virtio-vga.c         |   2 +
>  hw/s390x/virtio-ccw-gpu.c       |   3 +
>  hw/usb/ccid-card-emulated.c     |   1 +
>  hw/usb/ccid-card-passthru.c     |   1 +
>  hw/usb/redirect.c               |   1 +
>  qemu-modinfo.c                  | 270 ++++++++++++++++++++++++++++++
>  softmmu/vl.c                    |  20 +--
>  stubs/module-opts.c             |   4 -
>  ui/egl-headless.c               |   4 +
>  ui/gtk.c                        |   4 +
>  ui/sdl2.c                       |   4 +
>  ui/spice-app.c                  |   3 +
>  ui/spice-core.c                 |   5 +
>  util/module.c                   | 282 +++++++++++++++++++-------------
>  meson.build                     |  11 ++
>  qapi/meson.build                |   1 +
>  qapi/modules.json               |  36 ++++
>  qapi/qapi-schema.json           |   1 +
>  util/trace-events               |   3 +
>  34 files changed, 576 insertions(+), 131 deletions(-)
>  create mode 100644 qemu-modinfo.c
>  create mode 100644 qapi/modules.json
> 


Re: [PATCH v2 00/18] modules: add metadata database
Posted by Gerd Hoffmann 2 years, 9 months ago
On Thu, Jun 10, 2021 at 10:32:39AM +0200, Claudio Fontana wrote:
> On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
> > This patch series adds support for module metadata.  Here are the pieces
> > of the puzzle:
> > 
> >   (1) Macros are added to store metadata in a .modinfo elf section
> >       (idea stolen from the linux kernel).
> >   (2) A utility to scan modules, collect metadata from the .modinfo
> >       sections, store it in a file (modinfo.json) for later consumption
> >       by qemu.  Can also be easily inspected using 'jq'.
> >   (3) Adding annotations to the modules we have.
> >   (4) Drop hard-coded lists from utils/module.c
> > 
> > take care,
> >   Gerd
> 
> The background has disappeared compared with V1.
> 
> V1 says:
> 
> "Background is that the hard-coded lists in util/module.c are somewhat
> ugly and also wouldn't work very well with a large number of modules,
> so I'm looking for something else."

Well, it's point (4) now (a bit short indeed ...).

> Can you write more about what the actual high level goals of this series are?

Right now we have information about modules hard-coded in various places
in qemu.  Most obvious ones are module_deps[] and qom_modules[] (both in
util/module.c), but also qemu_load_module_for_opts() (in softmmu/vl.c)
and maybe more I missed.

So, when you go build some qom object modular today you'll have to go
add that to the qom_modules[] list.  With this patch series applied
you'll go add a 'module_obj("typename");' to your source file instead.

Same goes for other metadata, see the "add $foo module annotations"
patches for examples.

> We are in general making QEMU more and more difficult to get into,
> requiring more and more investment for new contributors to get
> productive.
> 
> Is the additional complexity justified? What is the benefit, and is
> simplification a goal of the series as well?

IMHO it is a simplification for developers.  Modules are more
self-contained with this in place.  You just add the annotation macros
and you are done.  No need to edit manually maintained lists at some
non-obvious place elsewhere in the tree.  Also no patch conflicts in
those lists.  We have type_init() + friends for simliar reasons.

The price for that simplification is the new utility needed which
collects and stores the metadata.  But that is something you only need
to worry about when actually working on module support.  The build
system keeps the database automatically up-to-date and most developers
shouldn't even notice that it is there.

take care,
  Gerd


Re: [PATCH v2 00/18] modules: add metadata database
Posted by Claudio Fontana 2 years, 9 months ago
On 6/10/21 11:54 AM, Gerd Hoffmann wrote:
> On Thu, Jun 10, 2021 at 10:32:39AM +0200, Claudio Fontana wrote:
>> On 6/10/21 7:57 AM, Gerd Hoffmann wrote:
>>> This patch series adds support for module metadata.  Here are the pieces
>>> of the puzzle:
>>>
>>>   (1) Macros are added to store metadata in a .modinfo elf section
>>>       (idea stolen from the linux kernel).
>>>   (2) A utility to scan modules, collect metadata from the .modinfo
>>>       sections, store it in a file (modinfo.json) for later consumption
>>>       by qemu.  Can also be easily inspected using 'jq'.
>>>   (3) Adding annotations to the modules we have.
>>>   (4) Drop hard-coded lists from utils/module.c
>>>
>>> take care,
>>>   Gerd
>>
>> The background has disappeared compared with V1.
>>
>> V1 says:
>>
>> "Background is that the hard-coded lists in util/module.c are somewhat
>> ugly and also wouldn't work very well with a large number of modules,
>> so I'm looking for something else."
> 
> Well, it's point (4) now (a bit short indeed ...).
> 
>> Can you write more about what the actual high level goals of this series are?
> 
> Right now we have information about modules hard-coded in various places
> in qemu.  Most obvious ones are module_deps[] and qom_modules[] (both in
> util/module.c), but also qemu_load_module_for_opts() (in softmmu/vl.c)
> and maybe more I missed.

Maybe a good idea to find out.

> 
> So, when you go build some qom object modular today you'll have to go
> add that to the qom_modules[] list.  With this patch series applied
> you'll go add a 'module_obj("typename");' to your source file instead.
> 
> Same goes for other metadata, see the "add $foo module annotations"
> patches for examples.
> 
>> We are in general making QEMU more and more difficult to get into,
>> requiring more and more investment for new contributors to get
>> productive.
>>
>> Is the additional complexity justified? What is the benefit, and is
>> simplification a goal of the series as well?
> 
> IMHO it is a simplification for developers.  Modules are more

So the whole endevour here is to remove the need to update modules in two/three places?

It might simplify life for developers adding a module,
but it is at a cost for the developers trying to keep the plumbing to work in my opinion.

Based on the information you provided, the reason this whole series exists seems to be to remove the need to update modules in multiple places.

Should the design focus be on that and that only?

Is there a real need to copy over the mechanism from the kernel, or are the requirements actually different/simpler here?



> self-contained with this in place.  You just add the annotation macros
> and you are done.  No need to edit manually maintained lists at some
> non-obvious place elsewhere in the tree.  Also no patch conflicts in
> those lists.  We have type_init() + friends for simliar reasons.
> 
> The price for that simplification is the new utility needed which
> collects and stores the metadata.  But that is something you only need
> to worry about when actually working on module support.  The build
> system keeps the database automatically up-to-date and most developers
> shouldn't even notice that it is there.
> 
> take care,
>   Gerd
> 
> 


Re: [PATCH v2 00/18] modules: add metadata database
Posted by Gerd Hoffmann 2 years, 9 months ago
 Hi,

> Based on the information you provided, the reason this whole series
> exists seems to be to remove the need to update modules in multiple
> places.

Well, I'm trying to improve the way we handle module meta-data (see
other mail just send for details).

> Is there a real need to copy over the mechanism from the kernel, or
> are the requirements actually different/simpler here?

Even though the exact kind of meta-data is different (qemu wants know
qom types implemented by module, kernel wants know what hardware a
module can drive) the fundamental problem is the same:  We need some
meta-data about modules for lookup.  The workflow is simliar too:
The meta-data is stored in a .modinfo elf section.  Then a utility
collects that data and stores them in a database.

For the kernel 'depmod' stores the data in
/lib/modules/$version/modules.$name (with modules.alias being the
largest database).

I'm trying to have qemu-modinfo store this in modinfo.json.

take care,
  Gerd