| Message ID | 1339977692-3196-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 18.06.2012 02:01, schrieb Anthony Liguori: > I think I understand enough of what's going on in these rules to ensure this is > right. But I could certainly use a second or third opinion... > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > v1 -> v2 > - Remove unnecessary includes (Andreas) > - Add a sub makefile for hw/kvm (Andreas) > --- > Makefile | 4 ++-- > Makefile.dis | 2 +- > Makefile.target | 2 +- > Makefile.user | 2 +- > hw/i386/Makefile.objs | 2 +- > hw/kvm/Makefile.objs | 1 + > rules.mak | 1 + > 7 files changed, 8 insertions(+), 6 deletions(-) > create mode 100644 hw/kvm/Makefile.objs > > diff --git a/Makefile b/Makefile > index cce45fb..593bd9b 100644 > --- a/Makefile > +++ b/Makefile > @@ -405,5 +405,5 @@ tar: > Makefile: $(GENERATED_HEADERS) > > # Include automatically generated dependency files > --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d) > --include $(wildcard qga/*.d hw/*.d hw/usb/*.d qom/*.d) > +# All subdir dependencies come automatically from our recursive subdir rules > +-include $(wildcard *.d) > diff --git a/Makefile.dis b/Makefile.dis > index 3e1fcaf..09060f0 100644 > --- a/Makefile.dis > +++ b/Makefile.dis > @@ -20,4 +20,4 @@ clean: > rm -f *.o *.d *.a *~ > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/Makefile.target b/Makefile.target > index 2907aad..550d889 100644 > --- a/Makefile.target > +++ b/Makefile.target > @@ -216,4 +216,4 @@ GENERATED_HEADERS += config-target.h > Makefile: $(GENERATED_HEADERS) > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/Makefile.user b/Makefile.user > index b717820..0ffefe8 100644 > --- a/Makefile.user > +++ b/Makefile.user > @@ -23,4 +23,4 @@ clean: > done > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index d43f1df..eb171b7 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -7,7 +7,7 @@ obj-y += debugcon.o multiboot.o > obj-y += pc_piix.o > obj-y += pc_sysfw.o > obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o > -obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o kvm/i8254.o > +obj-y += kvm/ This will work technically but I still feel this is wrong semantically. The pre-Paolo and current way is picking specific files from the hw/kvm/ directory. Your change above implies that in hw/kvm/ only x86 files can live, which I dislike. As suggested before, I would prefer if x86-only files were moved to an x86-specific location - the place for that existing since Paolo's refactoring would be hw/i386/. CC'ing Jan. That would match Paolo's reply in the unicore32 thread on future file placement. Alternatives would be hw/i386/kvm/ or hw/kvm/i386/; we're talking about a handful of files only though, so I don't think they require a new subdirectory. Andreas > obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o > > obj-y := $(addprefix ../,$(obj-y)) > diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs > new file mode 100644 > index 0000000..226497a > --- /dev/null > +++ b/hw/kvm/Makefile.objs > @@ -0,0 +1 @@ > +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o > diff --git a/rules.mak b/rules.mak > index 4bc5e52..60f3e96 100644 > --- a/rules.mak > +++ b/rules.mak > @@ -94,6 +94,7 @@ define unnest-dir > $(foreach var,$(nested-vars),$(call push-var,$(var),$1/)) > $(eval obj := $(obj)/$1) > $(eval include $(SRC_PATH)/$1/Makefile.objs) > +$(eval -include $(wildcard $1/*.d)) > $(eval obj := $(patsubst %/$1,%,$(obj))) > $(foreach var,$(nested-vars),$(call pop-var,$(var),$1/)) > endef
On 2012-06-18 02:32, Andreas Färber wrote: > Am 18.06.2012 02:01, schrieb Anthony Liguori: >> I think I understand enough of what's going on in these rules to ensure this is >> right. But I could certainly use a second or third opinion... >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> --- >> v1 -> v2 >> - Remove unnecessary includes (Andreas) >> - Add a sub makefile for hw/kvm (Andreas) >> --- >> Makefile | 4 ++-- >> Makefile.dis | 2 +- >> Makefile.target | 2 +- >> Makefile.user | 2 +- >> hw/i386/Makefile.objs | 2 +- >> hw/kvm/Makefile.objs | 1 + >> rules.mak | 1 + >> 7 files changed, 8 insertions(+), 6 deletions(-) >> create mode 100644 hw/kvm/Makefile.objs >> >> diff --git a/Makefile b/Makefile >> index cce45fb..593bd9b 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -405,5 +405,5 @@ tar: >> Makefile: $(GENERATED_HEADERS) >> >> # Include automatically generated dependency files >> --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d) >> --include $(wildcard qga/*.d hw/*.d hw/usb/*.d qom/*.d) >> +# All subdir dependencies come automatically from our recursive subdir rules >> +-include $(wildcard *.d) >> diff --git a/Makefile.dis b/Makefile.dis >> index 3e1fcaf..09060f0 100644 >> --- a/Makefile.dis >> +++ b/Makefile.dis >> @@ -20,4 +20,4 @@ clean: >> rm -f *.o *.d *.a *~ >> >> # Include automatically generated dependency files >> --include $(wildcard *.d */*.d) >> +-include $(wildcard *.d) >> diff --git a/Makefile.target b/Makefile.target >> index 2907aad..550d889 100644 >> --- a/Makefile.target >> +++ b/Makefile.target >> @@ -216,4 +216,4 @@ GENERATED_HEADERS += config-target.h >> Makefile: $(GENERATED_HEADERS) >> >> # Include automatically generated dependency files >> --include $(wildcard *.d */*.d) >> +-include $(wildcard *.d) >> diff --git a/Makefile.user b/Makefile.user >> index b717820..0ffefe8 100644 >> --- a/Makefile.user >> +++ b/Makefile.user >> @@ -23,4 +23,4 @@ clean: >> done >> >> # Include automatically generated dependency files >> --include $(wildcard *.d */*.d) >> +-include $(wildcard *.d) >> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs >> index d43f1df..eb171b7 100644 >> --- a/hw/i386/Makefile.objs >> +++ b/hw/i386/Makefile.objs >> @@ -7,7 +7,7 @@ obj-y += debugcon.o multiboot.o >> obj-y += pc_piix.o >> obj-y += pc_sysfw.o >> obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o >> -obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o kvm/i8254.o >> +obj-y += kvm/ > > This will work technically but I still feel this is wrong semantically. > The pre-Paolo and current way is picking specific files from the hw/kvm/ > directory. Your change above implies that in hw/kvm/ only x86 files can > live, which I dislike. As suggested before, I would prefer if x86-only > files were moved to an x86-specific location - the place for that > existing since Paolo's refactoring would be hw/i386/. CC'ing Jan. That > would match Paolo's reply in the unicore32 thread on future file > placement. Alternatives would be hw/i386/kvm/ or hw/kvm/i386/; we're > talking about a handful of files only though, so I don't think they > require a new subdirectory. Some per-arch separation is required, at least in the build process. We'll see power and arm stubs for in-kernel devices soon. Jan
On 18 June 2012 10:13, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-06-18 02:32, Andreas Färber wrote: >> This will work technically but I still feel this is wrong semantically. >> The pre-Paolo and current way is picking specific files from the hw/kvm/ >> directory. Your change above implies that in hw/kvm/ only x86 files can >> live, which I dislike. > Some per-arch separation is required, at least in the build process. > We'll see power and arm stubs for in-kernel devices soon. Indeed -- I have a hw/kvm/arm_gic.c in the qemu-linaro tree, so if you break building that I'll have to unbreak it :-) (Does architecture-specific separation make much sense in general? Not all devices are architecture-specific. I'd have thought that a functional split eg timer/serial/usb like the linux kernel layout would be better.) -- PMM
Am 18.06.2012 11:31, schrieb Peter Maydell: > On 18 June 2012 10:13, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2012-06-18 02:32, Andreas Färber wrote: >>> This will work technically but I still feel this is wrong semantically. >>> The pre-Paolo and current way is picking specific files from the hw/kvm/ >>> directory. Your change above implies that in hw/kvm/ only x86 files can >>> live, which I dislike. > >> Some per-arch separation is required, at least in the build process. >> We'll see power and arm stubs for in-kernel devices soon. > > Indeed -- I have a hw/kvm/arm_gic.c in the qemu-linaro tree, so > if you break building that I'll have to unbreak it :-) > > (Does architecture-specific separation make much sense in general? > Not all devices are architecture-specific. I'd have thought that > a functional split eg timer/serial/usb like the linux kernel layout > would be better.) Maybe you're misreading me? I was saying iff a device is specifically (not accidentally) for one target foo then it may/should be placed into hw/foo/ directory. We already have a hw/usb/ directory, and as long as there are no target dependencies and sufficient files I see nothing wrong with hw/timer/ or hw/serial/. Cheers, Andreas
On 18 June 2012 11:42, Andreas Färber <afaerber@suse.de> wrote: > Am 18.06.2012 11:31, schrieb Peter Maydell: >> (Does architecture-specific separation make much sense in general? >> Not all devices are architecture-specific. I'd have thought that >> a functional split eg timer/serial/usb like the linux kernel layout >> would be better.) > > Maybe you're misreading me? I was saying iff a device is specifically > (not accidentally) for one target foo then it may/should be placed into > hw/foo/ directory. Yes, I'm saying that seems like a confusing split, because a few devices for target foo will be in hw/foo and a number more in hw/, and there'll probably be cases where something in hw/foo has to move out into hw/ when a new target comes along that happens to reuse it. So rather than having hw/foo where foo == target-name, I'm suggesting hw/foo where foo == kind-of-device. As you say we've already moved a bit down this road with usb, for instance. -- PMM
Am 18.06.2012 12:56, schrieb Peter Maydell: > On 18 June 2012 11:42, Andreas Färber <afaerber@suse.de> wrote: >> Am 18.06.2012 11:31, schrieb Peter Maydell: >>> (Does architecture-specific separation make much sense in general? >>> Not all devices are architecture-specific. I'd have thought that >>> a functional split eg timer/serial/usb like the linux kernel layout >>> would be better.) >> >> Maybe you're misreading me? I was saying iff a device is specifically >> (not accidentally) for one target foo then it may/should be placed into >> hw/foo/ directory. > > Yes, I'm saying that seems like a confusing split, because a few > devices for target foo will be in hw/foo and a number more in hw/, > and there'll probably be cases where something in hw/foo has to > move out into hw/ when a new target comes along that happens to > reuse it. So rather than having hw/foo where foo == target-name, > I'm suggesting hw/foo where foo == kind-of-device. As you say > we've already moved a bit down this road with usb, for instance. But the point is that hw/foo/ is required for the new Makefile system, so we have the empty folders anyway, whereas putting target-specific stuff into, e.g., hw/apic/ will not solve the dependency issue that I tracked down here. If you do have an automated solution to that, please spill it out. :) Andreas
On 18 June 2012 12:35, Andreas Färber <afaerber@suse.de> wrote: > But the point is that hw/foo/ is required for the new Makefile system, > so we have the empty folders anyway, whereas putting target-specific > stuff into, e.g., hw/apic/ will not solve the dependency issue that I > tracked down here. Why should our makefile system mandate empty directories in the source tree hierarchy? That seems like a bug to me... -- PMM
On 06/18/2012 04:13 AM, Jan Kiszka wrote: > On 2012-06-18 02:32, Andreas Färber wrote: >> Am 18.06.2012 02:01, schrieb Anthony Liguori: >> This will work technically but I still feel this is wrong semantically. >> The pre-Paolo and current way is picking specific files from the hw/kvm/ >> directory. Your change above implies that in hw/kvm/ only x86 files can >> live, which I dislike. As suggested before, I would prefer if x86-only >> files were moved to an x86-specific location - the place for that >> existing since Paolo's refactoring would be hw/i386/. CC'ing Jan. That >> would match Paolo's reply in the unicore32 thread on future file >> placement. Alternatives would be hw/i386/kvm/ or hw/kvm/i386/; we're >> talking about a handful of files only though, so I don't think they >> require a new subdirectory. > > Some per-arch separation is required, at least in the build process. > We'll see power and arm stubs for in-kernel devices soon. i8259.o i8254.o ioapic.o don't need to be arch specific apic.o ought to be renamed to lapic.o and moved to target-i386/kvm/ I think clock.o also more than likely belongs in target-i386/kvm/. It would have to be implemented as part of the CPU core if it ever existed IRL. In general, if is logically part of a CPU core, it ought to be in target-$(ARCH). Otherwise, it shouldn't be built as a target specific object. Regards, Anthony Liguori > Jan >
On 2012-06-18 14:47, Anthony Liguori wrote: > On 06/18/2012 04:13 AM, Jan Kiszka wrote: >> On 2012-06-18 02:32, Andreas Färber wrote: >>> Am 18.06.2012 02:01, schrieb Anthony Liguori: >>> This will work technically but I still feel this is wrong semantically. >>> The pre-Paolo and current way is picking specific files from the hw/kvm/ >>> directory. Your change above implies that in hw/kvm/ only x86 files can >>> live, which I dislike. As suggested before, I would prefer if x86-only >>> files were moved to an x86-specific location - the place for that >>> existing since Paolo's refactoring would be hw/i386/. CC'ing Jan. That >>> would match Paolo's reply in the unicore32 thread on future file >>> placement. Alternatives would be hw/i386/kvm/ or hw/kvm/i386/; we're >>> talking about a handful of files only though, so I don't think they >>> require a new subdirectory. >> >> Some per-arch separation is required, at least in the build process. >> We'll see power and arm stubs for in-kernel devices soon. > > i8259.o i8254.o ioapic.o don't need to be arch specific In theory. In practice they carry quite a bit of the PC architecture (i8254: HPET and PC speaker port, i8259: ELCR). Maybe not the IOAPIC. It was once reused on IA64, but that arch is dead. > > apic.o ought to be renamed to lapic.o and moved to target-i386/kvm/ "apic" is fine as name as the code covers both cases. Should be move hw/apic* as well? > > I think clock.o also more than likely belongs in target-i386/kvm/. It would > have to be implemented as part of the CPU core if it ever existed IRL. > > In general, if is logically part of a CPU core, it ought to be in > target-$(ARCH). Otherwise, it shouldn't be built as a target specific object. There are some practical things like lacking types or defines in the KVM API that most probably prevent building certain KVM devices for all targets unconditionally. Jan
On 06/17/2012 07:01 PM, Anthony Liguori wrote: > I think I understand enough of what's going on in these rules to ensure this is > right. But I could certainly use a second or third opinion... > > Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> Applied. Regards, Anthony Liguori > --- > v1 -> v2 > - Remove unnecessary includes (Andreas) > - Add a sub makefile for hw/kvm (Andreas) > --- > Makefile | 4 ++-- > Makefile.dis | 2 +- > Makefile.target | 2 +- > Makefile.user | 2 +- > hw/i386/Makefile.objs | 2 +- > hw/kvm/Makefile.objs | 1 + > rules.mak | 1 + > 7 files changed, 8 insertions(+), 6 deletions(-) > create mode 100644 hw/kvm/Makefile.objs > > diff --git a/Makefile b/Makefile > index cce45fb..593bd9b 100644 > --- a/Makefile > +++ b/Makefile > @@ -405,5 +405,5 @@ tar: > Makefile: $(GENERATED_HEADERS) > > # Include automatically generated dependency files > --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d) > --include $(wildcard qga/*.d hw/*.d hw/usb/*.d qom/*.d) > +# All subdir dependencies come automatically from our recursive subdir rules > +-include $(wildcard *.d) > diff --git a/Makefile.dis b/Makefile.dis > index 3e1fcaf..09060f0 100644 > --- a/Makefile.dis > +++ b/Makefile.dis > @@ -20,4 +20,4 @@ clean: > rm -f *.o *.d *.a *~ > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/Makefile.target b/Makefile.target > index 2907aad..550d889 100644 > --- a/Makefile.target > +++ b/Makefile.target > @@ -216,4 +216,4 @@ GENERATED_HEADERS += config-target.h > Makefile: $(GENERATED_HEADERS) > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/Makefile.user b/Makefile.user > index b717820..0ffefe8 100644 > --- a/Makefile.user > +++ b/Makefile.user > @@ -23,4 +23,4 @@ clean: > done > > # Include automatically generated dependency files > --include $(wildcard *.d */*.d) > +-include $(wildcard *.d) > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index d43f1df..eb171b7 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -7,7 +7,7 @@ obj-y += debugcon.o multiboot.o > obj-y += pc_piix.o > obj-y += pc_sysfw.o > obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o > -obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o kvm/i8254.o > +obj-y += kvm/ > obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o > > obj-y := $(addprefix ../,$(obj-y)) > diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs > new file mode 100644 > index 0000000..226497a > --- /dev/null > +++ b/hw/kvm/Makefile.objs > @@ -0,0 +1 @@ > +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o > diff --git a/rules.mak b/rules.mak > index 4bc5e52..60f3e96 100644 > --- a/rules.mak > +++ b/rules.mak > @@ -94,6 +94,7 @@ define unnest-dir > $(foreach var,$(nested-vars),$(call push-var,$(var),$1/)) > $(eval obj := $(obj)/$1) > $(eval include $(SRC_PATH)/$1/Makefile.objs) > +$(eval -include $(wildcard $1/*.d)) > $(eval obj := $(patsubst %/$1,%,$(obj))) > $(foreach var,$(nested-vars),$(call pop-var,$(var),$1/)) > endef
diff --git a/Makefile b/Makefile index cce45fb..593bd9b 100644 --- a/Makefile +++ b/Makefile @@ -405,5 +405,5 @@ tar: Makefile: $(GENERATED_HEADERS) # Include automatically generated dependency files --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d) --include $(wildcard qga/*.d hw/*.d hw/usb/*.d qom/*.d) +# All subdir dependencies come automatically from our recursive subdir rules +-include $(wildcard *.d) diff --git a/Makefile.dis b/Makefile.dis index 3e1fcaf..09060f0 100644 --- a/Makefile.dis +++ b/Makefile.dis @@ -20,4 +20,4 @@ clean: rm -f *.o *.d *.a *~ # Include automatically generated dependency files --include $(wildcard *.d */*.d) +-include $(wildcard *.d) diff --git a/Makefile.target b/Makefile.target index 2907aad..550d889 100644 --- a/Makefile.target +++ b/Makefile.target @@ -216,4 +216,4 @@ GENERATED_HEADERS += config-target.h Makefile: $(GENERATED_HEADERS) # Include automatically generated dependency files --include $(wildcard *.d */*.d) +-include $(wildcard *.d) diff --git a/Makefile.user b/Makefile.user index b717820..0ffefe8 100644 --- a/Makefile.user +++ b/Makefile.user @@ -23,4 +23,4 @@ clean: done # Include automatically generated dependency files --include $(wildcard *.d */*.d) +-include $(wildcard *.d) diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index d43f1df..eb171b7 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -7,7 +7,7 @@ obj-y += debugcon.o multiboot.o obj-y += pc_piix.o obj-y += pc_sysfw.o obj-$(CONFIG_XEN) += xen_platform.o xen_apic.o -obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o kvm/i8259.o kvm/ioapic.o kvm/i8254.o +obj-y += kvm/ obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o obj-y := $(addprefix ../,$(obj-y)) diff --git a/hw/kvm/Makefile.objs b/hw/kvm/Makefile.objs new file mode 100644 index 0000000..226497a --- /dev/null +++ b/hw/kvm/Makefile.objs @@ -0,0 +1 @@ +obj-$(CONFIG_KVM) += clock.o apic.o i8259.o ioapic.o i8254.o diff --git a/rules.mak b/rules.mak index 4bc5e52..60f3e96 100644 --- a/rules.mak +++ b/rules.mak @@ -94,6 +94,7 @@ define unnest-dir $(foreach var,$(nested-vars),$(call push-var,$(var),$1/)) $(eval obj := $(obj)/$1) $(eval include $(SRC_PATH)/$1/Makefile.objs) +$(eval -include $(wildcard $1/*.d)) $(eval obj := $(patsubst %/$1,%,$(obj))) $(foreach var,$(nested-vars),$(call pop-var,$(var),$1/)) endef
I think I understand enough of what's going on in these rules to ensure this is right. But I could certainly use a second or third opinion... Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- v1 -> v2 - Remove unnecessary includes (Andreas) - Add a sub makefile for hw/kvm (Andreas) --- Makefile | 4 ++-- Makefile.dis | 2 +- Makefile.target | 2 +- Makefile.user | 2 +- hw/i386/Makefile.objs | 2 +- hw/kvm/Makefile.objs | 1 + rules.mak | 1 + 7 files changed, 8 insertions(+), 6 deletions(-) create mode 100644 hw/kvm/Makefile.objs