| Message ID | 20200702122048.27798-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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(-) >
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
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
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, > >>> 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
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
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(-)