[PULL,00/10] Modules 20200702 patches
mbox

Message ID 20200702122048.27798-1-kraxel@redhat.com
State New
Headers show

Pull-request

git://git.kraxel.org/qemu tags/modules-20200702-pull-request

Message

Gerd Hoffmann July 2, 2020, 12:20 p.m. UTC
The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:

  hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/modules-20200702-pull-request

for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:

  chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)

----------------------------------------------------------------
qom: add support for qom objects in modules.
build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
build braille chardev as module.

note: qemu doesn't rebuild objects on cflags changes (specifically
      -fPIC being added when code is switched from builtin to module).
      Workaround for resulting build errors: "make clean", rebuild.

----------------------------------------------------------------

Gerd Hoffmann (10):
  module: qom module support
  object: qom module support
  qdev: device module support
  build: fix device module builds
  ccid: build smartcard as module
  usb: build usb-redir as module
  vga: build qxl as module
  vga: build virtio-gpu only once
  vga: build virtio-gpu as module
  chardev: enable modules, use for braille

 Makefile.objs            |  2 ++
 Makefile.target          |  7 +++++
 include/qemu/module.h    |  2 ++
 include/qom/object.h     | 12 +++++++
 chardev/char.c           |  2 +-
 hw/core/qdev.c           |  6 ++--
 qdev-monitor.c           |  5 +--
 qom/object.c             | 14 +++++++++
 qom/qom-qmp-cmds.c       |  3 +-
 softmmu/vl.c             |  4 +--
 util/module.c            | 67 ++++++++++++++++++++++++++++++++++++++++
 chardev/Makefile.objs    |  5 ++-
 hw/Makefile.objs         |  2 ++
 hw/display/Makefile.objs | 28 ++++++++++-------
 hw/usb/Makefile.objs     | 13 +++++---
 15 files changed, 148 insertions(+), 24 deletions(-)

Comments

Peter Maydell July 3, 2020, 8:54 a.m. UTC | #1
On Thu, 2 Jul 2020 at 13:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:
>
>   hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/modules-20200702-pull-request
>
> for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:
>
>   chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)
>
> ----------------------------------------------------------------
> qom: add support for qom objects in modules.
> build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
> build braille chardev as module.
>
> note: qemu doesn't rebuild objects on cflags changes (specifically
>       -fPIC being added when code is switched from builtin to module).
>       Workaround for resulting build errors: "make clean", rebuild.
>
> ----------------------------------------------------------------
>
> Gerd Hoffmann (10):
>   module: qom module support
>   object: qom module support
>   qdev: device module support
>   build: fix device module builds
>   ccid: build smartcard as module
>   usb: build usb-redir as module
>   vga: build qxl as module
>   vga: build virtio-gpu only once
>   vga: build virtio-gpu as module
>   chardev: enable modules, use for braille

No code review at all? :-(   In particular the "build: fix device module
builds" commit (as you note in your commit message) does not look at
all right. I would much prefer if we could get some code review
for these changes before applying them.

thanks
-- PMM
Claudio Fontana July 3, 2020, 9:14 a.m. UTC | #2
Hello Gerd,

I think in general the idea to make it easier to modularize things is great,
is this thought for devices only, or could I rework my changes to support modularizing per-target AccelClass types and all the related code on top of your design?

Thanks,

Claudio

On 7/2/20 2:20 PM, Gerd Hoffmann wrote:
> The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:
> 
>   hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)
> 
> are available in the Git repository at:
> 
>   git://git.kraxel.org/qemu tags/modules-20200702-pull-request
> 
> for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:
> 
>   chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)
> 
> ----------------------------------------------------------------
> qom: add support for qom objects in modules.
> build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
> build braille chardev as module.
> 
> note: qemu doesn't rebuild objects on cflags changes (specifically
>       -fPIC being added when code is switched from builtin to module).
>       Workaround for resulting build errors: "make clean", rebuild.
> 
> ----------------------------------------------------------------
> 
> Gerd Hoffmann (10):
>   module: qom module support
>   object: qom module support
>   qdev: device module support
>   build: fix device module builds
>   ccid: build smartcard as module
>   usb: build usb-redir as module
>   vga: build qxl as module
>   vga: build virtio-gpu only once
>   vga: build virtio-gpu as module
>   chardev: enable modules, use for braille
> 
>  Makefile.objs            |  2 ++
>  Makefile.target          |  7 +++++
>  include/qemu/module.h    |  2 ++
>  include/qom/object.h     | 12 +++++++
>  chardev/char.c           |  2 +-
>  hw/core/qdev.c           |  6 ++--
>  qdev-monitor.c           |  5 +--
>  qom/object.c             | 14 +++++++++
>  qom/qom-qmp-cmds.c       |  3 +-
>  softmmu/vl.c             |  4 +--
>  util/module.c            | 67 ++++++++++++++++++++++++++++++++++++++++
>  chardev/Makefile.objs    |  5 ++-
>  hw/Makefile.objs         |  2 ++
>  hw/display/Makefile.objs | 28 ++++++++++-------
>  hw/usb/Makefile.objs     | 13 +++++---
>  15 files changed, 148 insertions(+), 24 deletions(-)
>
Gerd Hoffmann July 3, 2020, 10:29 a.m. UTC | #3
On Fri, Jul 03, 2020 at 11:14:27AM +0200, Claudio Fontana wrote:
> Hello Gerd,
> 
> I think in general the idea to make it easier to modularize things is great,

> is this thought for devices only, or could I rework my changes to
> support modularizing per-target AccelClass types and all the related
> code on top of your design?

It should help for any QOM object (see patch 10/10 for a chardev).

With AccelClass being per-target modules most likely we'll need
something better for 4/10 though.

take care,
  Gerd
Gerd Hoffmann July 3, 2020, 10:39 a.m. UTC | #4
On Fri, Jul 03, 2020 at 09:54:13AM +0100, Peter Maydell wrote:
> On Thu, 2 Jul 2020 at 13:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:
> >
> >   hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)
> >
> > are available in the Git repository at:
> >
> >   git://git.kraxel.org/qemu tags/modules-20200702-pull-request
> >
> > for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:
> >
> >   chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)
> >
> > ----------------------------------------------------------------
> > qom: add support for qom objects in modules.
> > build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
> > build braille chardev as module.
> >
> > note: qemu doesn't rebuild objects on cflags changes (specifically
> >       -fPIC being added when code is switched from builtin to module).
> >       Workaround for resulting build errors: "make clean", rebuild.
> >
> > ----------------------------------------------------------------
> >
> > Gerd Hoffmann (10):
> >   module: qom module support
> >   object: qom module support
> >   qdev: device module support
> >   build: fix device module builds
> >   ccid: build smartcard as module
> >   usb: build usb-redir as module
> >   vga: build qxl as module
> >   vga: build virtio-gpu only once
> >   vga: build virtio-gpu as module
> >   chardev: enable modules, use for braille
> 
> No code review at all? :-(

Well, there have been 5 revisions on the list, partly due to bugs being
fixed, partly with changes as response to review comments.  So it got
some review (not much though) even though the v5 series (posted on June
22th, so there was more than a week time) didn't got any acks so far.

> In particular the "build: fix device module
> builds" commit (as you note in your commit message) does not look at
> all right.

I think this stop-gap will do fine as long as we don't have any
target-specific modules.

take care,
  Gerd
Paolo Bonzini July 3, 2020, 12:05 p.m. UTC | #5
On 03/07/20 12:39, Gerd Hoffmann wrote:
> On Fri, Jul 03, 2020 at 09:54:13AM +0100, Peter Maydell wrote:
>> On Thu, 2 Jul 2020 at 13:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>
>>> The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:
>>>
>>>   hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)
>>>
>>> are available in the Git repository at:
>>>
>>>   git://git.kraxel.org/qemu tags/modules-20200702-pull-request
>>>
>>> for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:
>>>
>>>   chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)
>>>
>>> ----------------------------------------------------------------
>>> qom: add support for qom objects in modules.
>>> build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
>>> build braille chardev as module.
>>>
>>> note: qemu doesn't rebuild objects on cflags changes (specifically
>>>       -fPIC being added when code is switched from builtin to module).
>>>       Workaround for resulting build errors: "make clean", rebuild.
>>>
>>> ----------------------------------------------------------------
>>>
>>> Gerd Hoffmann (10):
>>>   module: qom module support
>>>   object: qom module support
>>>   qdev: device module support
>>>   build: fix device module builds
>>>   ccid: build smartcard as module
>>>   usb: build usb-redir as module
>>>   vga: build qxl as module
>>>   vga: build virtio-gpu only once
>>>   vga: build virtio-gpu as module
>>>   chardev: enable modules, use for braille
>>
>> No code review at all? :-(
> 
> Well, there have been 5 revisions on the list, partly due to bugs being
> fixed, partly with changes as response to review comments.  So it got
> some review (not much though) even though the v5 series (posted on June
> 22th, so there was more than a week time) didn't got any acks so far.
> 
>> In particular the "build: fix device module
>> builds" commit (as you note in your commit message) does not look at
>> all right.
> 
> I think this stop-gap will do fine as long as we don't have any
> target-specific modules.

Yeah, it's hackish but target-specific modules would be a huge
complication so I don't think we'd be having them anytime soon.  With
Meson removing the unnest-vars logic, the hack would also go away on its
own.  So I guess it's okay.

Paolo
Gerd Hoffmann July 6, 2020, 1:20 p.m. UTC | #6
Hi,

> >>>   build: fix device module builds

> >> No code review at all? :-(
> > 
> > Well, there have been 5 revisions on the list, partly due to bugs being
> > fixed, partly with changes as response to review comments.  So it got
> > some review (not much though) even though the v5 series (posted on June
> > 22th, so there was more than a week time) didn't got any acks so far.
> > 
> >> In particular the "build: fix device module
> >> builds" commit (as you note in your commit message) does not look at
> >> all right.
> > 
> > I think this stop-gap will do fine as long as we don't have any
> > target-specific modules.
> 
> Yeah, it's hackish but target-specific modules would be a huge
> complication so I don't think we'd be having them anytime soon.  With
> Meson removing the unnest-vars logic, the hack would also go away on its
> own.  So I guess it's okay.

OK, good.  So how to go forward now?

  (1) Merge pull req as-is?
  (2) Re-spin with discussion summary added to patch "4/10 build: fix
      device module builds".
  (3) Something else?

Any chance for an ack in case we go (2) or (3) ?

thanks & take care,
  Gerd
Claudio Fontana July 6, 2020, 2:30 p.m. UTC | #7
On 7/3/20 2:05 PM, Paolo Bonzini wrote:
> On 03/07/20 12:39, Gerd Hoffmann wrote:
>> On Fri, Jul 03, 2020 at 09:54:13AM +0100, Peter Maydell wrote:
>>> On Thu, 2 Jul 2020 at 13:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>>
>>>> The following changes since commit fc1bff958998910ec8d25db86cd2f53ff125f7ab:
>>>>
>>>>   hw/misc/pca9552: Add missing TypeInfo::class_size field (2020-06-29 21:16:10 +0100)
>>>>
>>>> are available in the Git repository at:
>>>>
>>>>   git://git.kraxel.org/qemu tags/modules-20200702-pull-request
>>>>
>>>> for you to fetch changes up to 474a5d66036d18ee5ccaa88364660d05bf32127b:
>>>>
>>>>   chardev: enable modules, use for braille (2020-07-01 21:08:11 +0200)
>>>>
>>>> ----------------------------------------------------------------
>>>> qom: add support for qom objects in modules.
>>>> build some devices (qxl, virtio-gpu, ccid, usb-redir) as modules.
>>>> build braille chardev as module.
>>>>
>>>> note: qemu doesn't rebuild objects on cflags changes (specifically
>>>>       -fPIC being added when code is switched from builtin to module).
>>>>       Workaround for resulting build errors: "make clean", rebuild.
>>>>
>>>> ----------------------------------------------------------------
>>>>
>>>> Gerd Hoffmann (10):
>>>>   module: qom module support
>>>>   object: qom module support
>>>>   qdev: device module support
>>>>   build: fix device module builds
>>>>   ccid: build smartcard as module
>>>>   usb: build usb-redir as module
>>>>   vga: build qxl as module
>>>>   vga: build virtio-gpu only once
>>>>   vga: build virtio-gpu as module
>>>>   chardev: enable modules, use for braille
>>>
>>> No code review at all? :-(
>>
>> Well, there have been 5 revisions on the list, partly due to bugs being
>> fixed, partly with changes as response to review comments.  So it got
>> some review (not much though) even though the v5 series (posted on June
>> 22th, so there was more than a week time) didn't got any acks so far.
>>
>>> In particular the "build: fix device module
>>> builds" commit (as you note in your commit message) does not look at
>>> all right.
>>
>> I think this stop-gap will do fine as long as we don't have any
>> target-specific modules.
> 
> Yeah, it's hackish but target-specific modules would be a huge
> complication so I don't think we'd be having them anytime soon.  With
> Meson removing the unnest-vars logic, the hack would also go away on its
> own.  So I guess it's okay.
> 
> Paolo
> 
> 

Hi Paolo,

well, since it is one of my main objectives behind lots of the refactoring work I have in progress,
and have been announcing this to the list since May:

https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04628.html

I would disagree with the fact that target-specific modules are a huge complication in by themselves, as I got some initial promising results in building them.

Will work on the series and contribute it as soon as the basic requisite initial refactoring series are in,
so that the usefulness of target-specific modules can be demonstrated.

In my view modules could have been implemented differently, instead of the current design, in a way that would have made them easier to extend.

Probably meson is an interesting tool, I think however that good build designs will still be necessary,
and that bad ones will still result in difficult to extend build features.

Thanks & ciao,

Claudio