| Message ID | 1539954845-26716-5-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add more CONFIG switches to make the build more modular | expand |
On 19 October 2018 at 14:14, Thomas Huth <thuth@redhat.com> wrote: > The "unimplemented-device" is currently only used for one arm board. ? It's used in all the MPS boards, several of the imx SoCs, the nrf51 SoC used by the microbit, and by the stellaris boards. > Let's add a CONFIG switch to make sure that we only compile it when > it is really necessary. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > default-configs/arm-softmmu.mak | 1 + > hw/misc/Makefile.objs | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak > index 6f2ffc1..dc9730f 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -7,6 +7,7 @@ CONFIG_NAND=y > CONFIG_OR_IRQ=y > CONFIG_SPLIT_IRQ=y > CONFIG_REGISTER=y > +CONFIG_UNIMP=y > CONFIG_ECC=y > CONFIG_SERIAL=y > CONFIG_SERIAL_ISA=y This seems awkward to me. The 'unimplemented' device is supposed to be an entirely generic thing usable in any board model. If we only turn it on in the arm-softmmu.mak then it means faffing around with the default-configs/ whenever it gets used in a different architecture. In particular, there's a pull request on the list that uses it in a sparc board, so this patch will break compile on sparc once that pullreq lands. thanks -- PMM
On 2018-10-19 15:57, Peter Maydell wrote: > On 19 October 2018 at 14:14, Thomas Huth <thuth@redhat.com> wrote: >> The "unimplemented-device" is currently only used for one arm board. > > ? It's used in all the MPS boards, several of the imx SoCs, > the nrf51 SoC used by the microbit, and by the stellaris boards. > >> Let's add a CONFIG switch to make sure that we only compile it when >> it is really necessary. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> default-configs/arm-softmmu.mak | 1 + >> hw/misc/Makefile.objs | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak >> index 6f2ffc1..dc9730f 100644 >> --- a/default-configs/arm-softmmu.mak >> +++ b/default-configs/arm-softmmu.mak >> @@ -7,6 +7,7 @@ CONFIG_NAND=y >> CONFIG_OR_IRQ=y >> CONFIG_SPLIT_IRQ=y >> CONFIG_REGISTER=y >> +CONFIG_UNIMP=y >> CONFIG_ECC=y >> CONFIG_SERIAL=y >> CONFIG_SERIAL_ISA=y > > This seems awkward to me. The 'unimplemented' device is supposed > to be an entirely generic thing usable in any board model. > If we only turn it on in the arm-softmmu.mak then it means > faffing around with the default-configs/ whenever it gets > used in a different architecture. The device has been added 1.5 years ago, and so far no other target is using it yet. So it's very seldom that you've got to expect any changes, I think. Anyway, it's also not only about speeding up the compilation process for people who don't want to use the corresponding targets, this is also very useful for downstream distributions of QEMU who want to make sure to not compile-in more devices than urgently needed. For example we disable this device here and the others in downstream RHEL version of QEMU, and I guess it might be interesting for nemu, too. So having more flexibility here in the Makefiles would be really great to decrease the burden with downstream-specific patches... > In particular, there's a pull request on the list that uses > it in a sparc board, so this patch will break compile on sparc > once that pullreq lands. Ok, then please disregard this patch here, but it would be great if you could still consider the other patches of this series. Thomas
On 19 October 2018 at 15:40, Thomas Huth <thuth@redhat.com> wrote: > Anyway, it's also not only about speeding up the compilation process for > people who don't want to use the corresponding targets, this is also > very useful for downstream distributions of QEMU who want to make sure > to not compile-in more devices than urgently needed. For example we > disable this device here and the others in downstream RHEL version of > QEMU, and I guess it might be interesting for nemu, too. So having more > flexibility here in the Makefiles would be really great to decrease the > burden with downstream-specific patches... I think if we want to support this for downstreams we need to look at something better than the default-configs/ mechanism for it. (Perhaps the kconfig-alike Paolo mentioned in a previous thread?) My dividing line for "should something go in a specific architecture's default-configs/ list" is "is this an SoC or real piece of hardware that is naturally limited to one or a few SoCs". thanks -- PMM
On 19/10/2018 16:43, Peter Maydell wrote: > I think if we want to support this for downstreams we need > to look at something better than the default-configs/ > mechanism for it. (Perhaps the kconfig-alike Paolo mentioned > in a previous thread?) True, but having more CONFIG_* symbols does not complicate the switch to a system like that one. > My dividing line for "should something go in a specific > architecture's default-configs/ list" is "is this an SoC > or real piece of hardware that is naturally limited to > one or a few SoCs". I tend to agree, but I'd rather have a separate CONFIG_* symbol as soon as two different *targets* (aka default-configs/*.mak files) use a device. So the SPARC change that you mention would actually be a good reason to introduce CONFIG_UNIMP, for example. Paolo
On Fri, Oct 19, 2018 at 4:40 PM Thomas Huth <thuth@redhat.com> wrote: > > On 2018-10-19 15:57, Peter Maydell wrote: > > On 19 October 2018 at 14:14, Thomas Huth <thuth@redhat.com> wrote: > >> The "unimplemented-device" is currently only used for one arm board. > > > > ? It's used in all the MPS boards, several of the imx SoCs, > > the nrf51 SoC used by the microbit, and by the stellaris boards. > > > >> Let's add a CONFIG switch to make sure that we only compile it when > >> it is really necessary. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> default-configs/arm-softmmu.mak | 1 + > >> hw/misc/Makefile.objs | 2 +- > >> 2 files changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak > >> index 6f2ffc1..dc9730f 100644 > >> --- a/default-configs/arm-softmmu.mak > >> +++ b/default-configs/arm-softmmu.mak > >> @@ -7,6 +7,7 @@ CONFIG_NAND=y > >> CONFIG_OR_IRQ=y > >> CONFIG_SPLIT_IRQ=y > >> CONFIG_REGISTER=y > >> +CONFIG_UNIMP=y > >> CONFIG_ECC=y > >> CONFIG_SERIAL=y > >> CONFIG_SERIAL_ISA=y > > > > This seems awkward to me. The 'unimplemented' device is supposed > > to be an entirely generic thing usable in any board model. > > If we only turn it on in the arm-softmmu.mak then it means > > faffing around with the default-configs/ whenever it gets > > used in a different architecture. > > The device has been added 1.5 years ago, and so far no other target is > using it yet. So it's very seldom that you've got to expect any changes, > I think. I disagree with this (and this patch). First because I'm using it heavily on MIPS and PPC boards, when no datashits are available. I'll submit that during the next merge window, although the MIPS tree seems now reluctant to that kind of hobbyist work. Second, because it is very clean when you implement a SoC to first start with an empty UNIMP region and a cpu core, then declare the mmio regions for each device (still UNIMP), then you can slowly add devices one at a time. This let you (me, so far) push at most a dozen of tiny patches in a working series, instead of a small series of a dozen of huge patches, or a series of 100 tiny patches. See https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06094.html about this approach. Regards, Phil. > Anyway, it's also not only about speeding up the compilation process for > people who don't want to use the corresponding targets, this is also > very useful for downstream distributions of QEMU who want to make sure > to not compile-in more devices than urgently needed. For example we > disable this device here and the others in downstream RHEL version of > QEMU, and I guess it might be interesting for nemu, too. So having more > flexibility here in the Makefiles would be really great to decrease the > burden with downstream-specific patches... > > > In particular, there's a pull request on the list that uses > > it in a sparc board, so this patch will break compile on sparc > > once that pullreq lands. > > Ok, then please disregard this patch here, but it would be great if you > could still consider the other patches of this series. > > Thomas
On 19/10/2018 18:25, Philippe Mathieu-Daudé wrote: > > First because I'm using it heavily on MIPS and PPC boards, when no > datashits are available. > I'll submit that during the next merge window, although the MIPS tree > seems now reluctant to that kind of hobbyist work. > > Second, because it is very clean when you implement a SoC to first > start with an empty UNIMP region and a cpu core, > then declare the mmio regions for each device (still UNIMP), then you > can slowly add devices one at a time. > This let you (me, so far) push at most a dozen of tiny patches in a > working series, instead of a small series of a dozen of huge patches, > or a series of 100 tiny patches. I don't see however why it's a problem to add/remove CONFIG_UNIMP to default-configs though (in the Kconfig world the board would "select UNIMP"). Paolo
On 19 October 2018 at 17:44, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 19/10/2018 18:25, Philippe Mathieu-Daudé wrote: >> >> First because I'm using it heavily on MIPS and PPC boards, when no >> datashits are available. >> I'll submit that during the next merge window, although the MIPS tree >> seems now reluctant to that kind of hobbyist work. >> >> Second, because it is very clean when you implement a SoC to first >> start with an empty UNIMP region and a cpu core, >> then declare the mmio regions for each device (still UNIMP), then you >> can slowly add devices one at a time. >> This let you (me, so far) push at most a dozen of tiny patches in a >> working series, instead of a small series of a dozen of huge patches, >> or a series of 100 tiny patches. > > I don't see however why it's a problem to add/remove CONFIG_UNIMP to > default-configs though (in the Kconfig world the board would "select > UNIMP"). Well, it's extra friction for people writing new board models, and the benefit of having CONFIG_UNIMP is all to downstream redistributors, not to upstream... I'm not vetoing this patchset, but I do think that if RedHat wants to distribute significantly-cut-down binaries then it would be worth working on a mechanism for making that more conveniently doable, rather than just hacking up the very creaky default-configs/ stuff. thanks -- PMM
On 19/10/2018 18:54, Peter Maydell wrote: > On 19 October 2018 at 17:44, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 19/10/2018 18:25, Philippe Mathieu-Daudé wrote: >>> >>> First because I'm using it heavily on MIPS and PPC boards, when no >>> datashits are available. >>> I'll submit that during the next merge window, although the MIPS tree >>> seems now reluctant to that kind of hobbyist work. >>> >>> Second, because it is very clean when you implement a SoC to first >>> start with an empty UNIMP region and a cpu core, >>> then declare the mmio regions for each device (still UNIMP), then you >>> can slowly add devices one at a time. >>> This let you (me, so far) push at most a dozen of tiny patches in a >>> working series, instead of a small series of a dozen of huge patches, >>> or a series of 100 tiny patches. >> >> I don't see however why it's a problem to add/remove CONFIG_UNIMP to >> default-configs though (in the Kconfig world the board would "select >> UNIMP"). > > Well, it's extra friction for people writing new board models, > and the benefit of having CONFIG_UNIMP is all to downstream > redistributors, not to upstream... > > I'm not vetoing this patchset, but I do think that if RedHat > wants to distribute significantly-cut-down binaries then > it would be worth working on a mechanism for making that > more conveniently doable, rather than just hacking up > the very creaky default-configs/ stuff. Yes, we are going to meet the NEMU guys at KVM Forum and talk about the "mini-Kconfig" stuff; in the meanwhile, default-configs/ is what we have, it was enough while we were caring basically only about x86 and thus we only needed to disable PCI devices, but now it's showing its age. Paolo
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index 6f2ffc1..dc9730f 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -7,6 +7,7 @@ CONFIG_NAND=y CONFIG_OR_IRQ=y CONFIG_SPLIT_IRQ=y CONFIG_REGISTER=y +CONFIG_UNIMP=y CONFIG_ECC=y CONFIG_SERIAL=y CONFIG_SERIAL_ISA=y diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index 6d50b03..c604859 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -9,7 +9,7 @@ common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o common-obj-$(CONFIG_EDU) += edu.o common-obj-$(CONFIG_PCA9552) += pca9552.o -common-obj-y += unimp.o +common-obj-$(CONFIG_UNIMP) += unimp.o common-obj-$(CONFIG_FW_CFG_DMA) += vmcoreinfo.o # ARM devices
The "unimplemented-device" is currently only used for one arm board. Let's add a CONFIG switch to make sure that we only compile it when it is really necessary. Signed-off-by: Thomas Huth <thuth@redhat.com> --- default-configs/arm-softmmu.mak | 1 + hw/misc/Makefile.objs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)